-
Notifications
You must be signed in to change notification settings - Fork 0
decisions
Short records of non-obvious choices — especially the ones that look wrong but are intentional, so they don't get re-litigated. Newest first.
Capability-aware CLI dispatch: gate on supports(), then cast to the rich driver union (#27)
Each subcommand declares the capability it needs; _client(args, cap) builds the
driver and rejects it with a clean message + exit 1 (not an AttributeError) when it
lacks the capability, deriving the command name from args.command. _rich_client
wraps it and casts to KVMClient | FakeDriver (the RichDriver alias) — which looks
like a type lie. It's sound: RedfishDriver is the only capability-partial driver and
it lacks exactly HID/Video/Events, so gating on one of those excludes it, leaving the
PiKVM-family/Fake surface that carries the convenience kwargs (slow=, quality=,
stream=) the minimal capability protocols don't declare. A future rich-but-partial
driver would need its own Protocol rather than this cast.
power-cycle gates on POWER, but hard_cycle isn't part of the Power protocol —
KVMClient/FakeDriver carried it as a convenience, RedfishDriver didn't, so
power-cycle --driver redfish would AttributeError despite a BMC plainly having
power. Added hard_cycle (force-off → on) so the invariant "advertises POWER ⇒ the
CLI power-cycle works" holds for every power driver. Its off_delay/on_delay
default to 0.0 (unlike KVMClient's ATX settle delays) because the two gated power
ops already block on the real PowerState transition.
It was, for a while, the third copy of the same power_off_hard → power_on
composition (KVMClient, FakeDriver, RedfishDriver). That was consolidated
in #63 into PowerMixin.hard_cycle (in drivers/base.py), composed from the
Power protocol methods with the settle delays as overridable class attributes
(_hard_cycle_off_delay/_hard_cycle_on_delay): the PiKVM ATX path keeps 5.0/3.0
because its power ops don't block on the state change, while Redfish (which blocks
on the PowerState transition) and Fake keep 0.0. hard_cycle(off_delay=, on_delay=)
still overrides per call — the public defaults are now None (meaning "use the
driver's class attribute"), a small alpha-era signature refinement.
Session auth is the BMC norm (and what real iDRAC/iLO recommend), so it stays the
default. But sushy-tools' --fake emulator exposes no SessionService, and a BMC can
administratively disable session or basic auth (cf.
#29) — an "unlocked" --driver redfish that could only do session auth couldn't authenticate to either. The
basic opt-in keeps the unlock honest. It's redfish-only and ignored by the PiKVM
family.
The pure-stdlib tests/redfish_emulator.py validates the full CLI → driver → HTTP
path in the default hermetic suite (test_cli_redfish.py). The opt-in integration
job (tests/integration/) runs the same surface against DMTF-conformant sushy-tools
sushy-emulator --fake — an independently authored implementation, so a spec
assumption shared by our driver and our own mock can't hide. sushy's fake driver has
no SessionService, hence the basic-auth path; it applies power transitions with a
short simulated delay that the driver's wait loop absorbs. The --fake backend needs
no libvirt/QEMU and no nested KVM, so it runs on a stock GitHub runner (pip-install +
self-started subprocess — no Docker, no services: container). The fixture also honors
KVM_PILOT_REDFISH_URL so the same tests can run against an already-running emulator —
the local-Docker fallback (quay.io/metal3-io/sushy-tools) when sushy-tools isn't on
PATH.
They landed with zero implementers, which reads like dead code. Kept on purpose:
they are the documented seam for BMC drivers, and they're no longer speculative —
FakeDriver and RedfishDriver implement BootProgress, RedfishDriver implements
Sensors. SerialConsole/Watchdog are the IPMI/SOL seam. Don't delete them as
"dead code."
The GL/Bli devices are API-compatible forks — a subclass that overrides only the
deltas is the natural shape, and there are ≥2 real subclasses. (General guidance
still favors composition; this is the case where inheritance genuinely fits.)
KVMClient/PiKVMClient stay as aliases of PiKVMDriver so no public API breaks.
Looks over-broad. It's intentional: GL firmware disables the whole /api/*
surface by default, so a 404 on any endpoint is overwhelmingly "API disabled," and
that's the dominant first-contact failure on a GL-RM1PE. The stock PiKVMDriver
sets no hint, so its 404s stay generic (see test_stock_pikvm_404_is_a_plain_error).
GLKVM_QUIRKS is seeded with the single documented quirk and grows from real
testing (source="observed"). Never invent firmware-version-specific data — the
project's honesty rule (alpha, untested on hardware) applies to the quirk DB too.
Reviewers flagged retrying non-idempotent POSTs (reset/insert). Kept: those ops are
effectively idempotent (resetting twice is still reset; inserting twice is still
inserted), retry only fires on transient 409/503/network, and it matches the
existing KVMClient behavior.
The driver-from-HostConfig dispatch is shape-aware (fake takes no credentials;
the PiKVM family builds via from_config). It lives in one place so cfg.driver
is honored identically by both consumers (used in ≥2 places → justified helper).
http.py is PiKVM-specific (X-KVMD-* auth, the ok/result envelope) and
discards status/headers, which Redfish needs (202, X-Auth-Token, Location,
ETag). Generalizing http.py is the separate Step 2 (#6).
For BootProgress=OSRunning, which means "OS handed off, running" — no existing
token fit, and the alternative (returning None) wrongly signals "can't report."
Cheap to add now that SYSTEM_PROMPT interpolates ALL_PHASES.
So analyzer paths resolved by a cheap gate (e.g. power_off) run offline with no
key — classify --driver fake needs no credentials. Mirrors the lazy model
resolution.
SafetyPolicy.guard returns True for any op id NOT in DESTRUCTIVE_OPS (so
adding a driver method doesn't accidentally gate a read) — but that means a
typo'd op id or a dropped guard() call silently un-gates a destructive method.
Because gating is the tool's one safety mechanism (and it's exposed to LLM
agents), a table-driven contract test exercises every gated method under
deny/dry-run/recording, and a source-scan invariant asserts every literal op id
passed to .guard() is registered. A dropped guard now fails CI (verified by
mutation); previously the suite stayed green.
SafetyPolicy.guard checks dry_run first and never invokes confirm for a
skipped call. The old order (confirm first) made --dry-run prompt — and in
non-interactive automation block with exit 3 — for calls that were never going
to be sent. Consequence: a denying confirm callback is not exercised in dry-run;
tests that want to see the callback fire must run live (dry_run=False).
type_text/press_key/send_shortcut/key_event/mouse_click guard through
hid.* ops: keystrokes land on a live console (rm -rf is one type_text
away), so "changes target state" clearly applies. Mouse moves stay ungated
(cursor position alone changes nothing). Known cost: under the CLI's interactive
confirm, key-spam helpers like enter_bios prompt per keystroke — --yes or a
session-scoped confirm is the escape hatch; refining that UX is follow-up work.
With no ATX board, kvmd reports enabled: false and the power LED is
meaningless. Reporting "off" there made the vision layer short-circuit every
classification to power_off on ATX-less devices, suppressing all snapshots.
Same fail-open rationale as has_video_signal. Caller-visible change:
wait_for_power_state(False) on an ATX-less device now times out (the device
cannot sense power) instead of returning instantly with a false success.
The stdlib's default opener copies every request header — including our
credentials (X-KVMD-Passwd, X-Auth-Token, Authorization: Basic, the
session Cookie) — to whatever host a 3xx Location names, with no
same-origin check and even across an https->http downgrade. That defeated the
Redfish _same_origin guard (which only covers URLs we construct, not
server-issued redirects) and left the PiKVM transport, which has no such guard,
fully exposed: a hostile or MITM'd device could exfiltrate the admin password.
Both transports now build their opener with a _NoRedirect handler that
refuses 3xx and surfaces it as a ConnectionError. Neither kvmd nor Redfish
needs transparent redirect following (Redfish async/Location is read
explicitly), so this costs nothing.
A read-phase reset/timeout means the device may have already executed the request; re-firing a power/HID/MSD POST could run it twice. Connect-phase failures (nothing was sent) stay retryable for every method. Retrying 409/503 stays safe for all methods: those are definitive "rejected" responses.
Real BMCs expose 100-400 Sensor resources; one GET per member (each a fresh
TCP+TLS handshake) is 10s of seconds to minutes, and the sensing hierarchy
polls it. read_sensors() now probes ServiceRoot.ProtocolFeaturesSupported. ExpandQuery and, when the service advertises it, fetches the Sensors collection
with ?$expand=*($levels=1) (or . for a Levels-only service) — one request
instead of N. It falls back to the per-member loop when $expand isn't advertised,
and remembers a 501 (the DSP0266 response for an unsupported $-query) so it
doesn't retry expansion per call. Deferred: HTTP keep-alive in the transport
(every request currently pays a fresh handshake) — a cross-cutting change to
both transports, tracked separately.
kvmd's /api/log?seek=N interprets N as seconds of history, so the shared
Logs protocol standardizes on that; the Redfish driver was interpreting seek
as an entry-skip index, so get_logs(seek=3600) returned "the last hour" on
PiKVM and "everything after entry 3600" (usually empty) on Redfish. Redfish now
filters LogEntry.Created to entries within the lookback, with three field
caveats: entries with a missing/unparseable timestamp are kept; unset-RTC epoch
stamps (~1970, common on fresh OpenBMC) are kept; and index skipping is never
used as a fallback, because LogEntry ordering varies by vendor (iDRAC
newest-first, OpenBMC oldest-first). seek=0 returns everything.
The Chassis and Managers collections have no defined ordering correspondence to
the Systems collection (DSP0266), so indexing all three with one system_index
could read sensors/logs/virtual-media from a different node than the one being
power-cycled on multi-node gear (blades, Supermicro twins). The driver now
resolves them from the chosen ComputerSystem's Links.Chassis /
Links.ManagedBy (DSP0268), falling back to the collection only when the System
advertises no such link. An out-of-range system_index is a hard CapabilityError
(never a silent fall-back to member 0 — that would target the wrong node with a
destructive op), the reset confirm prompt names the resolved system URI, and
system_index stays programmatic-only (not plumbed through config) until a
multi-node config surface is actually needed.
Inserted/WriteProtected are optional (DSP2046) and merely restate the insert
defaults, but strict firmware (Supermicro X11/X12, some Lenovo/older iDRAC)
rejects an InsertMedia body that carries them (the fix OpenStack sushy adopted
for Supermicro). So the driver POSTs {"Image": source} alone. For the inverse
quirk — a BMC that requires TransferProtocolType (sushy bug #2072805,
reported as 400 ActionParameterMissing) — it retries once with the type
derived from the URL scheme. Full @Redfish.ActionInfo-driven parameter
negotiation (as _reset_info does for Reset) was deliberately not built: the
omit-plus-targeted-retry pair covers the field-known cases with far less code
(CLAUDE.md: smallest change that works, no speculative generality).
request_json now carries the HTTP status on the VisionError (status_code)
and parses a 429's Retry-After seconds into retry_after. wait_for_any_state
keeps a separate error counter and, on each failed poll, sleeps
max(bounded_backoff, retry_after) (clamped to the remaining deadline) instead
of hammering a rate-limited API at the fixed interval — each failed poll
re-uploads the whole image, so this matters. The retryable set is effectively
{429, 500, 503, 529}: the loop already retries every VisionError to the
deadline, so no explicit gate is needed there; the single-shot classify/CLI path
keeps its one-attempt behavior (a jittered in-request retry was left as optional
future work).
Real BMCs terminate idle sessions (DSP0266 SessionService inactivity timeout,
~30 min default) and drop every token on reboot, and a token cleared by
close() leaves the (cached) discovery URIs valid but credential-less. The
transport now catches a session-mode 401, clears the token, calls login()
once, and retries the request a single time (a per-call guard, not a loop).
Safe even for destructive POSTs — a 401 is rejected before the action runs —
and non-recursive, because login() issues its own requests unauthenticated.
Skipped for 403 (a privilege failure re-login can't fix), PasswordChangeRequired
(re-login would leak a session slot), basic auth, and the Sessions collection
itself. This lives in the transport, not the driver, because the driver's
_root_doc/_system_doc caches mean close() + _ensure_login cannot recover
a live object on their own.
PushPowerButton pulses the power button (DSP0268) — a toggle, not an
absolute state — so choosing it from AllowableValues alone can invert a
safety-gated intent: on iDRAC8-class firmware (off set [ForceOff, PushPowerButton], no GracefulShutdown) power_off on an already-off host
powered it back on, then timed out. Both power methods now read the current
PowerState first: a host already at the target gets no reset at all, and
PushPowerButton is selected only when the pulse moves toward the target
(otherwise the preference falls through to ForceOff). A 400/409 that
nonetheless lands while the host is observed at target is treated as success —
a refinement of, not a change to, the "resetting twice is still reset" retry
rationale below (which holds only for absolute ResetTypes, not toggles).
PoweringOn/PoweringOff/Paused (DSP0268) are mid-transition; only a literal
Off becomes power_off. Conservative choice: a wait loop must not conclude a
host is down while it is coming up.
kvmd silently ignores preview_quality without preview=1, and the preview
path downscales to ~1/5 resolution (which would break OCR/vision) — there is no
full-resolution re-encode-at-quality endpoint. The parameter was a no-op lie;
deleted rather than deprecated while the API is alpha.
Most structural choices came from adversarial review passes (find → verify →
fix). The fixes are in the code; this file preserves the rejected findings and
tradeoff rationale. The Redfish driver's spec grounding lives in
redfish.md.
- Home
- Getting started
- Architecture
- CLI reference
- Configuration
- Design decisions
- Reflexes (RFC)
- Redfish reference
- Firmware registry
- Remote firmware update
- Unattended Linux installs
- Claude skill
- MCP server
- Contributing
- Security policy
- Analysis: 2026-07-01 deep review
- Analysis: 2026-07-03 RM1PE firmware + encoder
- Hardware compatibility