fix(ctl,api): probe genie-api and HA from [services.*] config - #105
fix(ctl,api): probe genie-api and HA from [services.*] config#105kiannidev wants to merge 2 commits into
Conversation
ai-hpc
left a comment
There was a problem hiding this comment.
Substance of the PR is good — this is exactly the right fix for #103. Some structural observations on the code, then two commit-metadata issues that have to be sorted before merge.
Code is on track
service_http_probe(url, default_port)ingenie-commonis the right primitive: parseshttp://...into(host:port, path), handles missing-port via thedefault_portarg, supports[ipv6]:portnotation, falls back to/when no path is present. Four unit tests cover the main shapes (explicit host+port+path, missing port, the canonical genie-api/api/status, and IPv6). Right scope — noreqwest/hyperdep added.[services.api]with#[serde(default = "defaults::api_service_endpoint")]keeps existinggeniepod.tomlfiles working without an explicit section. Theservices_config_deserializes_without_explicit_api_sectiontest pins that backward-compat invariant so a future refactor can't quietly drop it.api_service_endpointdefaults tohttp://127.0.0.1:3080/api/statuswhich matches the existing hardcoded value, so behavior is byte-identical when the section is absent. Right call.Config::api_service()accessor + alias map updates inmanages_service_alias/lookup_service_unit_for_aliasextend the existing pattern symmetrically. Adding"api" | "genie-api"next to the"core"/"llm"aliases is the kind of consistency that pays off when someone later doesgenie-ctl restart api.configured_http_probes(&config)ingenie-ctlreturnsVec<(&'static str, String, u16)>— Home Assistant is skipped whenhomeassistant_service()returnsNone. The previous code always probed127.0.0.1:8123even on installs without HA configured, which produced spurious[DOWN] Home Assistantlines ingenie-ctl healthand noise insupport-bundle. This PR fixes that too.- Dashboard side:
dashboard_service_targetsingenie-apinow readsconfig.services.api.urland.systemd_unitinstead of hardcoding. Same shape as howcore/llm/homeassistantalready worked. Clean.
Worth flagging, not blocking:
- The
default_portarg onservice_http_probeis only used when the URL omits a port. Operators who puthttp://homeassistant.local/api/in config get the right port via the arg. Operators who puthttp://nonsense.example/get:8123slapped on, which is correct for HA but weird-looking. Not a real problem since HA's port is the only realistic case for the default-port path. cmd_support_bundledoes its ownservice_http_probefor the API security URL separately from the probe-list — could be DRY'd, but the current shape keeps theapi_securityJSON field next to the existing actuation / connectivity sections, which is the right output structure. Fine.
Blocker 1: commit carries Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Same project norm that landed in #91 (Contribution / PR body checklist CI rule against AI-attribution footers in PR bodies) and CONTRIBUTING.md "Commit hygiene" (no Co-Authored-By: Claude / Copilot / other AI-assistant trailers on commits). The CI body check doesn't scan commit messages, but the norm explicitly covers commit trailers. Using Cursor or any other AI tooling to draft the work is welcome — credit in git log / git shortlog stays with the human contributor.
Blocker 2: commit author email is a personal Gmail, not a GitHub-noreply
Author: bohdansolovie <bohdansolovie@gmail.com>
That email goes into git log permanently on a public AGPL repo, and anyone cloning the project gets it indexed by every code-search / mirror site. The standard fix is to set git config user.email to a GitHub noreply form like <numeric-id>+<username>@users.noreply.github.com and re-amend. (You can find the noreply for any GitHub account at https://github.com/settings/emails.)
Separately, the commit author (bohdansolovie) and the PR submitter (kiannidev / kpdev) are different GitHub accounts. That's unusual — if you're collaborating with someone and want them credited as the commit author, that's fine, but the maintainer side has to know which identity actually pushed the code. If the bohdansolovie identity is a stale machine config that should have been kiannidev, the amend is the moment to fix it.
Suggested fix path
One amend covers both:
git config user.email "<your-noreply>" # if not already set
git commit --amend --reset-author
# remove the `Co-authored-by: Cursor <cursoragent@cursor.com>` line
# from the message in the editor that opens
git push --force-with-leaseOnce that pushes:
- The PR body checklist will re-run (passing — it didn't block before).
- The cargo / clippy / test / aarch64 / no-default-features jobs that needed first-time-contributor approval will now run on the new commit head — I just approved them on the current head, so the same approval should carry over (or I'll re-approve if needed).
- The substance of the review above stands, so once CI clears + the commit metadata is clean, this is a quick re-approve and merge.
Cross-references
- Same Cursor-trailer pattern previously flagged on @galuis116's PR #92 (norm note) and enforced via closure on PR #96 when it recurred. The contributor's clean follow-up on PR #98 is a good template for the amend+force-push path.
CONTRIBUTING.md"Commit hygiene" section covers both the AI-trailer rule and the implicit human-authored expectation.
Add [services.api] with a backward-compatible default, parse service URLs via service_http_probe(), and use configured endpoints in genie-ctl health, diag, support bundle, and genie-api dashboard targets. Fixes GeniePod#103
db5f64e to
8d9663d
Compare
|
Closing this one. The substance — config-driven probes for Original head: commit authored as Current head after force-push: account mismatch and Cursor trailer are resolved (good — you read the review). But the substantive commit ( This is the third PR in this stretch that ran into the same commit-author-email norm without resolution (#92 was the first cycle, @galuis116; #96 was the close-and-redo cycle; #98 was the clean resubmit; #102 was the soft-flag-pass for andriypolanski). For #105 the issue is single-vector now (just email), but it's also the third revision opportunity to land it cleanly, and the maintainer-side patience for repeat litigation on the same norm has a limit. PR #102 (@andriypolanski) merged the parallel
The |
Summary
[services.api]togeniepod.toml(defaulthttp://127.0.0.1:3080/api/status) with backward-compatible serde default for existing configsservice_http_probe()ingenie-commonto parsehttp://service URLs into(host:port, path)genie-ctl health,diag, andsupport-bundleto probe genie-api and Home Assistant from configured URLs (skip HA when not configured)genie-apidashboard service targets to use[services.api].urlinstead of a hardcoded latency URLFixes #103
Real Behavior Proof
What I ran
cargo fmt --all cargo test -p genie-common -p genie-ctl -p genie-apiWhat I observed
All 58 unit tests in the three touched crates passed on x86_64 Linux:
genie-common:service_http_probe,[services.api]default, TOML backward compatibility without[services.api]genie-ctl: existing tests pass after health/diag/support-bundle probe changesgenie-api: dashboard target tests pass with config-driven api latency URLNo Jetson hardware in this dev environment; changes are config/HTTP-probe plumbing only.
Test plan
cargo fmt --all -- --checkcargo test -p genie-common -p genie-ctl -p genie-apigenie-ctl healthwith custom[services.homeassistant].urlwhen HA is enabled