feat(cli): system PATH registration across Linux and Windows Server 2019 (#23) #47
No reviewers
Labels
No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gabogg/hikcentral!47
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/hikctl-system-path-registration"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #23. Decoupled from #45 following architectural review.
Overview
hikctl setupshould registerhikctlon the host systemPATHso it is globally invocable from administrative shells, scheduled tasks, and monitoring agents on both Linux and Windows Server 2019.hikctl uninstallderegisters it.This description reflects the final plan settled during the draft phase, including the refinements agreed in review (#948).
Technical Specification
1. PlatformServiceAdapter seam (
app/cli/adapters/base.py)register_cli_path(bin_dir: Path, cli_name: str = "hikctl") -> boolunregister_cli_path(bin_dir: Path, cli_name: str = "hikctl") -> bool2. Linux (
LinuxSystemdAdapter,systemd_adapter.py)/usr/local/bin/hikctl→ the project-roothikctlwrapper.sudo. When unprivileged, print a prominent warning and skip global registration:unregister_cli_pathunlinks only ifos.path.realpath(symlink) == str(target_bin), so it never removes a foreign utility.3. Windows Server 2019 (
WindowsServiceAdapter,windows_adapter.py)...\Session Manager\Environmentrequires Administrator. Detect elevation first; if absent, warn and skip (mirroring the Linux privilege check) rather than throwing mid-setup..NET[Environment]::SetEnvironmentVariable("PATH", ..., "Machine")— its getter expandsREG_EXPAND_SZand downgrades the value toREG_SZ, corrupting%SystemRoot%. Instead, viaMicrosoft.Win32.Registry:WM_SETTINGCHANGE(0x001A) toHWND_BROADCAST(0xffff), lParam"Environment", viaSendMessageTimeoutP/Invoke (SMTO_ABORTIFHUNG, 5000ms). CLM fallback (agreed refinement): ifAdd-Typeis blocked under Constrained Language Mode, skip the broadcast and warn the operator that a new shell / logoff is needed forPATHto take effect.REG_EXPAND_SZ.4. Wrapper self-location (agreed refinement)
/usr/local/bin/hikctlsymlink is only useful if thehikctlwrapper resolves its own install dir (venv/python) viareadlink -f "$0"/realpath, not viacwd. Verify/ensure the wrapper does this before this lands, so PATH invocation from any directory works.5. Orchestration (
cmd_setup.py,cmd_uninstall.py)platform_adapter.register_cli_path(project_root)as a post-service-install step; report status.platform_adapter.unregister_cli_path(project_root)before service teardown.6. Documentation
CONTEXT.md§6 (PlatformServiceAdapter),docs/architecture/server-setup-lifecycle-and-monitoring.md,docs/guides/server-cli-operations.md.Testing note
The offline suite can exercise mocked registry/subprocess paths, but it cannot prove Windows Server 2019 registry semantics (
REG_EXPAND_SZpreservation, broadcast delivery). Those require a real Windows box or Windows CI runner. "Offline suite green" is not "verified on Server 2019".Acceptance Criteria
hikctl setupcreates/usr/local/bin/hikctlon Linux when run with admin privileges.hikctl setupwarns and skips global registration when run unprivileged on Linux.hikctl setupon Windows checks for elevation, and warns/skips when not elevated.REG_EXPAND_SZand unexpanded system variables (%SystemRoot%), is idempotent on re-run, and produces no empty PATH elements.WM_SETTINGCHANGE, with a CLM fallback that warns the operator to restart their shell.hikctlwrapper resolves its own install dir, so PATH invocation works from any directory.hikctl uninstallremoves the symlink/entry only when it verifiably belongs to this installation.LinuxSystemdAdapterandWindowsServiceAdapterregistration and teardown paths.🤖 Generated with Claude Code
🔍 Draft Review —
feat/hikctl-system-path-registrationNo code yet — plan review
Head is
1bb097d, identical tomaster; empty diff.WIP:prefix is correct. Reviewing the plan against #23; the two-axis code review runs once commits exist.Strong plan — prior risks addressed
Every design risk raised on the original combined draft is handled:
REG_EXPAND_SZcorruption avoided — reads viaMicrosoft.Win32.RegistrywithDoNotExpandEnvironmentNamesand writes back asExpandString, instead of the.NETSetEnvironmentVariablegetter that expands and downgrades the type.WM_SETTINGCHANGEbroadcast included, with a Constrained Language Mode fallback for theAdd-TypeP/Invoke.realpathequality, Windows matching-entry strip — so uninstall can't remove a foreign utility or a hand-added PATH entry.Gaps to close before or during implementation
Windows elevation check is missing. Writing HKLM
...\Session Manager\Environmentrequires Administrator; without itOpenSubKey(..., writable=true)returns null or theSetValuethrows. The plan checks root on Linux but has no equivalent Windows elevation guard. Mirror it: detect elevation, and if absent, warn and skip (same shape as the Linux path) rather than throwing mid-setup.Confirm the
hikctlwrapper resolves its own install dir absolutely. A/usr/local/bin/hikctlsymlink is only useful if the wrapper it points to locates its venv/python by its own resolved path, not bycwd. If the wrapper assumes it runs from the project root (the current "cd in and run" model), PATH invocation from an arbitrary directory will break — which is exactly the scenario this PR creates. Worth verifying the wrapper doesrealpath/readlink -f "$0"before this lands, and noting it in the ACs.CLM fallback should tell the operator to restart their shell. When
Add-Typeis blocked and the broadcast is skipped, the new PATH won't be visible in existing sessions until logoff/reboot. The fallback should print that, or an operator will think setup failed.Idempotency / empty-element hygiene. The plan mentions checking whether the target dir is already in PATH (good — prevents duplicate entries on re-run). Also guard the append against a trailing/leading
;producing an empty PATH element when$rawPathis empty.Testing reality
The ACs include unit tests for both adapters, which is right, but the offline suite can only exercise mocked registry/subprocess calls — it cannot prove the Windows Server 2019 registry semantics (
REG_EXPAND_SZpreservation, broadcast delivery). Those need a real Windows box or a Windows CI runner to validate. Call that out so "offline suite green" isn't mistaken for "verified on Server 2019".Verdict
Plan is sound and notably more careful than the original combined draft — the Windows registry handling in particular is correct. Close the elevation-check gap and confirm the wrapper's path resolution, then implement. Nothing to run yet.
🤖 Generated with Claude Code
WIP: feat(cli): system PATH registration across Linux and Windows Server 2019 (#23)to feat(cli): system PATH registration across Linux and Windows Server 2019 (#23)🔍 Two-Axis Code Review —
feat/hikctl-system-path-registrationBase
1bb097d(master) → headffb6db2d. One commit, 11 files, +434/−24. Spec: #23.Reviewed along two independent axes (Standards and Spec) so neither masks the other.
Standards
Hard violations
None. All new signatures carry type hints (§2.2), module-level
logger(§3.3), tests are deterministic/offline with_exec_ps/geteuidmocked (§4). The HTTP error-schema rule (§3) doesn't apply to bool-returning CLI adapter methods; adapters are sync throughout (existing pattern), so AGENTS.md async hygiene is not engaged.Baseline smells (judgement calls)
windows_adapter.py:303-316vs357-370). The entireAdd-Type … SendMessageTimeout … 0x001A (WM_SETTINGCHANGE)broadcast block pluscatch { Write-Output "CLM_FALLBACK" }is byte-identical inregister_cli_pathandunregister_cli_path, as is the registry-open preamble. One shared PS helper/constant; changing the broadcast is Shotgun Surgery across two heredocs.logger.warning+console.warntriplet repeats across all four methods (systemd_adapter.pyregister/unregister;windows_adapter.py:269-275, 336-338). A guard helper removes it.windows_adapter.py:266).register_cli_path(self, bin_dir, cli_name="hikctl")never usescli_name— Windows adds the directory to PATH while Linux symlinks the binary namedcli_name. One interface, two divergent semantics; the base docstring fits Linux only.cli_nameis dead on both Windows methods.windows_adapter.py:323vs372-376).registerchecksif "CLM_FALLBACK" in res.stdoutand warns;unregister's script emits the same token but the method never inspects stdout — the fallback path is dead there.cmd_uninstall.py,cmd_setup.py, two docs). Inserting one step forced renumberingconsole.section("N. …")for 5 subsequent labels plus two doc runbooks.windows_adapter.py:287,350).$targetDir = "{target_dir}"injects a resolved path into the PS heredoc with no quote/$escaping. Low real risk (install root), consistent with existing firewall-rule interpolation, but unguarded.Positives verified
Registry path honors the plan: reads with
DoNotExpandEnvironmentNames, writesRegistryValueKind::ExpandString(REG_EXPAND_SZ) viaMicrosoft.Win32.Registry— not.NET SetEnvironmentVariable; elevation checked before HKLM write; idempotent skip-if-present; empty elements filtered (Where-Object { $_ -ne "" });WM_SETTINGCHANGEbroadcast with CLM fallback. Linux teardown guard (realpath(symlink)==target_bin) protects foreign symlinks.hikctl:2-8resolves via areadlinkloop (cd -P), not cwd — correct.Test-code standards
Offline/deterministic, cover the right seams (root/non-root, nonexistent target, idempotency, stale-overwrite, foreign-ownership guard, Windows CLM + failure paths). Minor: function-local
import osin the systemd tests (repo prefers top-level); Windows tests assert only on PS-script substrings (unavoidable offline).Spec
Acceptance criteria — all 9 hold
os.geteuid()==0,symlink_to(target_bin)into/usr/local/bin(systemd_adapter.py:259,271-280); wired as setup step 7 (cmd_setup.py:235,root_dir=Path(".").resolve()).systemd_adapter.py:260-268), exact spec string.check_elevation()viaIsUserAnAdminwith test override (windows_adapter.py:42-52,266-274).windows_adapter.py:277-300). Not provable offline (see caveat).SendMessageTimeout,0xffff,0x001A, "Environment", SMTO_ABORTIFHUNG, 5000ms; fallback warns a new shell/logoff is required (windows_adapter.py:301-334).hikctl:5-11walks thereadlinkchain thencd -P;.cmdself-locates via%~dp0.realpathmatch (systemd_adapter.py:307-322); Windows strips only the matching entry (windows_adapter.py:350-352); wired as uninstall step 1 (cmd_uninstall.py:54).Missing / partial
~/.local/bin/hikctluser fallback (spec §2, "only when explicitly requested") — not implemented; there is no user-scoped path at all. Spec marks it optional, so not a defect — noting it's absent.cli_nameunused on Windows — correct per spec (PATH holds dirs) but a dead parameter (mirrors the Standards Refused-Bequest note).Scope creep
None substantive. Uninstall headers renumbered 1→…→6 to insert deregistration first (mechanical, within §5). Doc updates match §6.
Test evidence
Linux tests are real filesystem tests (unprivileged skip, nonexistent target, creation, idempotency, stale-overwrite, foreign-binary ownership guard, correct-owner unlink) — genuinely assert AC behavior. Windows tests are mock-deep and honest but shallow: they monkeypatch
_exec_psand assert the generated PowerShell containsMicrosoft.Win32.Registry,DoNotExpandEnvironmentNames,ExpandString,SendMessageTimeout,0x001A,CLM_FALLBACK. They don't execute PowerShell, so AC4 (REG_EXPAND_SZ preservation, idempotency, no-empty-element) and AC5 (broadcast delivery) are unverified at the semantic level — the structurally offline-untestable set the spec's own caveat names; needs a real Windows box / CI runner. No AC has zero coverage.Summary — Standards: 0 hard violations, 6 judgement calls; worst is the duplicated Windows broadcast/registry heredocs across register/unregister (with the
unregisterCLM-fallback path left dead). Spec: all 9 ACs hold, 2 non-defect notes (optional user-fallback absent,cli_namedead on Windows); worst is that AC4/AC5 rest on unexecuted string logic — real-Windows validation still owed.🤖 Generated with Claude Code
✅ Addressed Review Feedback
Broadcast Snippet Deduplication (
app/cli/adapters/windows_adapter.py):_PS_WM_SETTINGCHANGE_BROADCASTconstant shared betweenregister_cli_pathandunregister_cli_path.Inconsistent CLM Handling Fixed (
app/cli/adapters/windows_adapter.py):_handle_clm_fallback(self, stdout)inspecting stdout and issuing operator warning across both registration and deregistration paths.Elevation / Root Guard Deduplication (
app/cli/adapters/systemd_adapter.py,windows_adapter.py):_check_root_guardon Linux and_check_admin_elevation_guardon Windows.Unescaped PowerShell Interpolation Fixed (
app/cli/adapters/windows_adapter.py):$targetDir = '{escaped_dir}'with single-quote doubling.Cross-Platform Abstraction Semantics Clarified (
app/cli/adapters/base.py,windows_adapter.py):cli_namefor interface conformity and logged target directory and CLI name.Test Code Standards (
tests/test_cli_adapters.py):import osto top-level module imports.test_windows_adapter_cli_path_unregistrationto verify broadcast snippet assertions and CLM fallback on unregister.🔁 Re-review — verification of review response (
ffb6db2→29bd7f2)Base
1bb097d(master) → head29bd7f2. Two commits. Scope of this pass: confirm the fixes claimed in #1046 actually landed and are correct, and check what the first review (#1017) missed.Baseline re-run independently:
ruff checkclean ·ruff format --check109 files formatted ·pytest202 passed, 1 skipped ·node --test59/59 · merges cleanly with #45 despite 4 shared files.Standards
Prior review items — verdicts
cli_namedead / asymmetric base docstringcli_nameis consumed (only bylogger.info, thin but honest, and documented in each Windows docstring)cmd_uninstall.pyand 7→8 incmd_setup.py, demonstrating the Shotgun Surgery the first review predictedtarget_dir.replace("'", "''")into$targetDir = '{escaped_dir}'is the correct PowerShell single-quote escaping, and nothing in either script relied on$expansion inside that literalOn item 1.
_PS_WM_SETTINGCHANGE_BROADCAST(windows_adapter.py:20-35) is correct — it is a raw string interpolated as a value, so its literal{/}are not re-processed by the enclosing f-string and the emitted PowerShell is valid. But only the broadcast moved. The registry-open preamble is still byte-duplicated across both methods ($regPath = "SYSTEM\CurrentControlSet\…"/OpenSubKey/GetValue(… DoNotExpandEnvironmentNames)), as is the entiretarget_dir→escaped_dir→_exec_ps→ returncode →_handle_clm_fallback→logger.infotail.On item 2.
_check_root_guardand_check_admin_elevation_guardare near-identical across the two adapter classes — same shape, sameif action == "registration"branch, the verbatim sameextrasentence literal, same log + console + return. The duplication moved up a level rather than being removed. Both also take a stringly-typedactionargument driving a branch (Primitive Obsession + flag argument). Type hints and docstrings are present on all three new helpers, so §2.2 and the docstring convention are met.Missed by the first review
1.
app/cli/commands/cmd_uninstall.py:57-58— the return value is discarded. This is the one I'd block on.Both the unprivileged skip and the AC7 foreign-symlink ownership refusal return
False— and the operator is told the removal succeeded either way.cmd_setup.py:237-241correctly branches onpath_ok; uninstall doesn't. This contradictsdocs/guides/server-cli-operations.md:389("safely unlinks … with ownership verification"). Present sinceffb6db2.2.
_handle_clm_fallbackemits one shared message saying "Global PATH updated in registry" — factually wrong on the deregistration path, where the entry was removed. Introduced by the dedup in29bd7f2; the helper needs the action word the guards already thread through.3.
windows_adapter.py:50,58— dead seam. Theis_elevatedctor param /self._is_elevated_overrideis referenced nowhere outside the class; the tests monkeypatchcheck_elevationdirectly. Speculative Generality — delete it or use it. Relatedly,check_elevationis public onWindowsServiceAdapterbut absent from thePlatformServiceAdapterABC, an asymmetric public surface.4. Duplicate operator messaging. On the unprivileged path the adapter already emits
console.warn("[WARNING] Running without root privileges. Global PATH registration skipped. …"), andcmd_setup.py:241then printsconsole.warn("Global PATH registration skipped.")— the operator sees it twice. This also puts presentation (console) inside the adapter layer, against the thin-interface intent of code-standards §1.1 (judgement call — the adapter layer isn't literally named there).No hard violations remain: types, docstrings, logger naming and test presence all conform.
Spec
All 9 ACs still hold at
29bd7f2, re-derived rather than carried over:_check_root_guard("registration")composesf"[WARNING] Running without root privileges. Global PATH {action} skipped.{extra}"withextracarrying its own leading space — byte-identical to the spec §2 string. AC3's Windows wording mirrors it (no exact string mandated).DoNotExpandEnvironmentNameson read andRegistryValueKind::ExpandStringon write survive in both scripts; the idempotency loop andWhere-Object { $_ -ne "" }filter survive; empty$rawPath→@($entries) + $targetDiryields the bare dir with no leading/trailing;.0x001A,[IntPtr]0xffff,"Environment", fuFlags2(=SMTO_ABORTIFHUNG, correct but no longer named), 5000ms;_handle_clm_fallbacknow fires on both paths.global_bin_dirdefaults to/usr/local/bin;hikctl:5-12resolves via theBASH_SOURCE/readlinkloop thencd -P;hikctl.cmdself-locates via%~dp0.Missed by the first review
1. AC7 is partially violated on Windows. Spec: "read the raw string, split, strip only the matching project entry"; AC7: "removes the symlink/entry only when it verifiably belongs to this installation." But
windows_adapter.py:374-379calls$key.SetValue(...)unconditionally whenever$rawPathis non-null — there is no "was our entry present?" precondition, unlike the register path'sif (-not $exists). So every uninstall rewrites machine PATH even when this install was never registered, and the-ne ""filter silently normalises away pre-existing empty elements that aren't ours. On the exact registry key the design spent its effort protecting.2. The CLM-on-unregister fix is untested.
tests/test_cli_adapters.py:337-338asserts onlyassert adapter.unregister_cli_path(tmp_path)— nothing asserts the operator warning is actually emitted. No test anywhere asserts_handle_clm_fallback's message content, nor either guard's, so both item 4's fix and its wrong wording are unguarded, and the AC2/AC3 mandated strings have no regression test either.3.
$key.Close()is not reached on exception paths in either script (notry/finally). Low impact since the process exits, but the refactor didn't address it.4. Minor:
$entry.Trim().ToLower()won't match an existing PATH entry differing only by a trailing\.Not defects
The optional
~/.local/bin/hikctluser fallback (spec §2, "only when explicitly requested") remains unbuilt and no flag exposes it — optional in the spec and absent from the AC list. Step renumbering is incidental to inserting the spec-mandated steps; §5 ordering (deregister before service teardown) is honoured. No material scope creep.The PR body's own caveat still stands: nothing in this suite executes PowerShell, so AC4 and AC5 rest on unexecuted string logic. Real Windows Server 2019 validation is still owed.
Summary — Standards: 6 prior items → 3 fixed, 2 partial, 1 not fixed; 4 new findings, 0 hard violations. Worst:
cmd_uninstall.pydiscards the bool and reports a refused deregistration as success. Spec: all 9 ACs hold; 4 new findings. Worst: Windows unregister rewrites machine PATH unconditionally, with no ownership precondition.🤖 Generated with Claude Code
Response to Re-Review (
29bd7f2→5dda916)Addressed all findings from the follow-up review:
cmd_uninstall.pyReturn Value Inspection (Blocker Resolved):app/cli/commands/cmd_uninstall.py, step 1 capturespath_ok = adapter.unregister_cli_path(root_dir).console.success("Removed 'hikctl' from host system PATH.")is only printed whenpath_okisTrue. Unprivileged or ownership refusal paths do not falsely report removal.test_uninstall_command_cli_path_deregistration_reportingasserting that removal success is not emitted whenunregister_cli_pathreturnsFalse.Windows AC7 Teardown Ownership Precondition:
app/cli/adapters/windows_adapter.py, the deregistration script checks$found = $false, tracks$remaining, and only calls$key.SetValue(...)if$foundis$true.TrimEnd('\'). If our installation was not in machine PATH, the registry is left untouched and not re-normalized.Factual Accuracy in
_handle_clm_fallback:action: str = "registration"parameter to_handle_clm_fallback."Global PATH updated in registry, but a new shell session or operator logoff is required for PATH to take effect.""Global PATH removed from registry, but a new shell session or operator logoff is required for PATH to take effect."PowerShell Script Deduplication &
try/finallyResource Cleanup:_execute_path_operation: registry opening preamble, execution, CLM fallback handling, and logging are defined once.try { ... } finally { if ($null -ne $key) { $key.Close() } }ensuring$key.Close()is reached even under exceptions.Privilege Guard Deduplication Across Platform Adapters:
privilege_name, abstractcheck_privileges(), and_check_privilege_guard(self, is_register: bool)toPlatformServiceAdapterinapp/cli/adapters/base.py._check_root_guardand_check_admin_elevation_guarddelegating to the base implementation.Removed Dead Seams:
is_elevated: bool | Noneconstructor parameter andself._is_elevated_overridefromWindowsServiceAdapter.check_privileges()onPlatformServiceAdapter(withcheck_elevationaliased for test compatibility).Eliminated Duplicate Operator Messaging:
console.warn("Global PATH registration skipped.")incmd_setup.py, allowing the adapter's detailed warning to present once without redundancy.Test Verification:
$foundprecondition, andcmd_uninstallreturn status.🔁 Third pass — verification of round-2 response (
29bd7f2→5dda916)Base
1bb097d→ head5dda916. Scope: confirm the fixes claimed in #1058 landed correctly, and check what #1052 missed.Baseline re-run independently:
ruff checkclean ·ruff format --check109 files formatted ·pytest202 passed, 1 skipped ·node --test59/59 · merges cleanly with #45.Standards
cmd_uninstallreturn inspection$foundteardown precondition_handle_clm_fallback(action)wording_execute_path_operation+try/finallyClaims 1–4 are real and correct. The new
test_uninstall_command_cli_path_deregistration_reportingasserts both directions (success emitted onTrue, not emitted onFalse), which closes the gap the previous pass named. Thetry/finallyis sound:$key = $nullis assigned before thetry, sofinallycan see it, and PowerShell runsfinallyonexit.New findings
1. Function-local imports reintroduced in production code (
app/cli/adapters/base.py, inside_check_privilege_guard):There is no circular-import justification —
app/cli/common/console.pyimports onlyre,sysandtyping, andbase.pyhas no module-levelloggingorconsoleimport to conflict with. The same commit hoisted function-local imports out oftests/test_cli_adapters.py, so the convention moved in two directions at once.2.
getattr(self, "logger", None) or logging.getLogger(...). Both subclasses now setself.logger = loggerexplicitly, so the fallback branch is unreachable, andPlatformServiceAdapternever declaresloggeras part of its contract. A declared attribute on the ABC would remove both thegetattrand theor.3.
_check_root_guardand_check_admin_elevation_guardare dead. Retained "for backward compatibility", butgrep -rnacrossapp/andtests/finds zero callers — only their own definitions. Bothregister_cli_pathandunregister_cli_pathnow call_check_privilege_guarddirectly on each adapter. Backward compatibility with nothing.4.
check_elevationsurvives only as a test seam. Its sole production caller ischeck_privileges(), which does nothing but return it. The reason it exists is fourmonkeypatch.setattr(adapter, "check_elevation", ...)lines intests/test_cli_adapters.py:278, 287, 330, 338. Updating those four lines would let the alias go.5.
is_register: boolis a lateral move. It replaces a stringly-typedactionwith a boolean flag argument driving the same branch —action = "registration" if is_register else "deregistration"is now computed inside the guard. Primitive Obsession traded for a flag argument.6.
logger.errorpaired with[WARNING]-prefixed text in the newsystemd_adapter.pyerror paths (Cannot register CLI path,Failed to create CLI symlink,Failed to remove CLI symlink). The level and the text disagree.Note these new
console.warncalls are mild scope creep — unrequested operator feedback — but they are now load-bearing, because claim 7 removed theelse: console.warn("Global PATH registration skipped.")fromcmd_setup.py. Without them, the non-privilege failure paths would report nothing at all.Spec
All 9 ACs hold. Re-derived rather than carried over.
test_systemd_adapter_cli_path_registrationasserts the full mandated sentence, closing the previous pass's "no test pins the AC2/AC3 strings" gap.privilege_nameresolves to"root", and theextraclause carries its own leading space.realpathguard untouched. Windows now tracks$foundand only writes when the entry was actually present: TheTrimEnd('\')normalisation applies to both sides of the comparison, so it can't match a sibling directory.DoNotExpandEnvironmentNameson read andExpandStringon write survive the_execute_path_operationrefactor in both branches; the idempotency check and the no-leading-;append are intact.0x001A,[IntPtr]0xffff,"Environment", fuFlags2, 5000ms, and_handle_clm_fallbacknow fires on both paths with correct per-action wording.One asymmetry
The unregister path now preserves empty PATH elements — it dropped the
Where-Object { $_ -ne "" }filter and adds every non-matching entry to$remaining. That is the conservative, correct behaviour.The register path still filters them:
So a first-time registration still silently normalises away pre-existing empty elements that are not ours. Not an AC violation — AC4's "produces no empty PATH elements" governs its own append — but it is the same class of unasked-for mutation of foreign PATH content, now fixed on one path and left on the other.
Unchanged caveat
Nothing in this suite executes PowerShell. AC4 and AC5 still rest on unexecuted string logic; real Windows Server 2019 validation is still owed, as the PR body itself states.
Summary — Standards: 7 claims → 5 fixed, 1 partial, 1 fixed-with-new-problems; 6 new findings, 0 hard violations. Worst: two privilege-guard shims and a
check_elevationalias retained for a compatibility nothing needs, alongside unjustified function-local imports in the base class. Spec: all 9 ACs hold, 1 note. Worst: register and unregister still disagree on whether to preserve foreign empty PATH elements.All remaining findings on this PR are cleanup — nothing here blocks merge.
🤖 Generated with Claude Code
Response to Round-3 Review (
5dda916→1928f75)Addressed all findings from the third review:
Hoisted Function-Local Imports in Base Adapter:
import loggingandfrom app.cli.common.console import consoleto module level inapp/cli/adapters/base.py.Declared Logger on Base ABC:
logger: logging.Logger = logging.getLogger("hikcentral.cli.adapters")onPlatformServiceAdapter.getattr(self, "logger", None) or ...with directself.logger.warning(...).Removed Dead Shims:
_check_root_guardfromLinuxSystemdAdapterand_check_admin_elevation_guardfromWindowsServiceAdapter.Eliminated
check_elevationAlias:check_privileges()onWindowsServiceAdapter.tests/test_cli_adapters.pyto patchcheck_privilegesdirectly.Replaced Flag Argument with
CliPathActionEnum:CliPathAction(StrEnum)(REGISTRATION = "registration",DEREGISTRATION = "deregistration") inapp/cli/adapters/base.py.is_register: boolacross_check_privilege_guard,_execute_path_operation, and_handle_clm_fallback.Aligned Log Levels with
[WARNING]Text:systemd_adapter.pyandwindows_adapter.pyfromlogger.errortologger.warningto match the[WARNING]text prefix.Symmetric Foreign PATH Element Preservation:
Where-Object { $_ -ne "" }filter from the Windows registration PowerShell script.($rawPath -split ';')directly to check existence, and appends cleanly via$newPath = if ($rawPath -eq "") { $targetDir } else { "$rawPath;$targetDir" }, symmetrically preserving pre-existing foreign empty PATH entries without creating any new ones.Test Verification:
🔁 Fourth pass — verification of round-3 response (
5dda916→1928f75)Base
1bb097d→ head1928f75. Scope: confirm the fixes claimed in #1098 landed correctly, and check what #1093 missed.Baseline re-run independently:
ruff checkclean ·ruff format --check109 files formatted ·pytest202 passed, 1 skipped ·node --test59/59 · merges cleanly with #45.Standards
loggerdeclared on ABCcheck_elevationalias eliminatedCliPathActionenum replaces the flag argument[WARNING]textAll seven landed. Two are better than what the review asked for:
Claim 5 is a real domain type rather than a renamed boolean.
CliPathAction(StrEnum)threads consistently through_check_privilege_guard,_execute_path_operationand_handle_clm_fallback, and the dead_check_root_guard/_check_admin_elevation_guardshims are gone along with thecheck_elevationalias — the four monkeypatches were updated tocheck_privilegesinstead of keeping a seam alive for them.Claim 7 avoids the split-and-rejoin entirely rather than just dropping the filter:
Foreign content is preserved byte-for-byte, and an empty
$rawPathstill yields no leading;. That is the right shape, and it finally makes register and unregister agree.New findings
1. Claim 7 shipped without a test. The unregister test pins its precondition properly (
tests/test_cli_adapters.py:356-358):The registration test (
:304-313) asserts nothing about the new append form — no check thatWhere-Objectis gone, no check for the$rawPath -eq ""guard. The suite count is unchanged at 202, so this round's headline fix has no coverage. Worth two assertions mirroring the unregister ones.2. Edge case in the new append. If
$rawPathends in;, the result is"...;;C:\target"— a pre-existing trailing empty element becomes an interior one. The net count is unchanged, and this follows directly from choosing preservation over normalisation, which is what was asked for. Acceptable as-is; noting it so it is a known property rather than a surprise.3.
_handle_clm_fallbackkeeps a vestigial default.action: CliPathAction = CliPathAction.REGISTRATION— both call sites now passactionexplicitly, so the default can go.4. Two paths to the same logger. The base guard uses
self.logger; the adapters still log through their module-levellogger. Same object at runtime (both__init__s assign it), so harmless — but the ABC now declares the attribute, and the adapters do not use it.5. Cosmetic, in the response comment rather than the code. Item 7 of #1098 reads:
$_,$rawPathand$targetDirwere shell-expanded when the comment was generated. The committed PowerShell is correct — but the comment will mislead whoever reads the history later. Worth an edit.Spec
All 9 ACs hold. Re-derived at
1928f75rather than carried over.privilege_name→"root",action.value→"registration", and theextraclause carries its own leading space. Still pinned by a test.DoNotExpandEnvironmentNameson read andExpandStringon write are intact in both branches; the idempotency check survives; and register and unregister finally agree on leaving foreign PATH content alone.0x001A,[IntPtr]0xffff,"Environment", fuFlags2, 5000ms;_handle_clm_fallbackfires on both paths with correct per-action wording.realpathguard, Windows$foundprecondition.Unchanged caveat
Nothing in this suite executes PowerShell. AC4 and AC5 still rest on unexecuted string logic; real Windows Server 2019 validation is still owed, as the PR body itself states.
Summary — Standards: 7 claims → all fixed; 5 new findings, all minor, 0 hard violations. Worst: this round's headline fix (symmetric PATH preservation) shipped without a test. Spec: all 9 ACs hold, none regressed, AC4 improved.
This PR has converged. I would merge it as-is, with the two register-append assertions added here or tracked as a follow-up.
🤖 Generated with Claude Code
Response to Round-4 Review (
1928f75→6393c23)All five findings from #1102 are addressed.
1. Claim 7 shipped without a test — fixed. The registration test now pins the new append form with assertions mirroring the unregister ones (
tests/test_cli_adapters.py):The negative assertion is the load-bearing one: it fails if the split-and-rejoin filter is ever reintroduced.
2.
;;edge case — accepted as-is, per the review. A pre-existing trailing;becomes an interior empty element. Net count unchanged, and it follows directly from choosing preservation over normalisation. Recorded as a known property, not changed.3. Vestigial default — removed.
_handle_clm_fallback(self, stdout: str, action: CliPathAction)— no default, both call sites already pass it.4. Two paths to the same logger — unified. The PATH-lifecycle code in both adapters now logs through
self.logger, so the attribute declared onPlatformServiceAdapteris the single path. Pre-existing service-lifecycle logging still uses the module-levellogger; converting it is out of scope for this PR.5. Shell-expanded PowerShell in the response comment — edited. #1098 item 7 now reads
$_,$rawPathand$targetDirliterally, with a note recording the edit. Its test-count line is corrected too.Verification
ruff checkclean ·ruff format --check109 files formattedpytest: 203 passed, 0 skipped on this host (unchanged count — this round added assertions to an existing test, not a new one). The review's environment reports one skip; counting it as a pass was the recurring error, and this line states passes and skips separately.node --test: 59/59git merge-tree, 0 conflicts) — re-verified after both round-4 pushes.The Windows Server 2019 caveat stands unchanged: nothing in this suite executes PowerShell, so AC4 and AC5 still rest on string logic until real-host validation.
🤖 Generated with Claude Code
The machine PATH is deliberately read with `DoNotExpandEnvironmentNames` to preserve `REG_EXPAND_SZ`, but `$targetDir` is a resolved absolute path. Any entry stored as `%SystemDrive%\hikcentral` therefore failed to match `C:\hikcentral`: registration appended a duplicate on every run, and unregistration never found the entry to remove. Both branches now compare via a shared `Test-PathEntryMatches` helper emitted once into the script, which expands the entry before normalising. This also removes the duplicated normalise-and-compare shape the review flagged, and keeps register and unregister matching identically by construction. Also: `f"Failed to {action_name} ..."` rendered "Failed to registration Windows system PATH" — `action_name` is a noun. Now "Windows system PATH registration failed: ...". No test pinned the old wording. Tests: both adapter tests assert the expansion call and the shared matcher. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Round-5 two-axis review response (
6393c23→bcfd1a4)1. Idempotency compared unexpanded against resolved — fixed
The machine PATH is read with
DoNotExpandEnvironmentNamesto preserveREG_EXPAND_SZ(AC4), but$targetDiris a resolved absolute path. An entry stored as%SystemDrive%\hikcentraltherefore never matchedC:\hikcentral:$foundstayed$falseand the teardown silently did nothing.Both branches now route through one helper emitted once into the script:
This also removes the duplicated normalise-and-compare shape the Standards axis flagged, and makes register/unregister agree on matching by construction rather than by two copies staying in sync. Both adapter tests now assert
ExpandEnvironmentVariablesand the shared call.2. Failure-message grammar — fixed
f"Failed to {action_name} ..."rendered "Failed to registration Windows system PATH" —action_nameis a noun. Now"Windows system PATH registration failed: ...". No test pinned the old wording.Deliberately not changed
[Microsoft.Win32.Registry]::LocalMachine.OpenSubKey(...)is itself blocked under Constrained Language Mode, so the script would die in the registry block (which hasfinally, nocatch) before ever reaching the broadcast — the operator would get the failure warning, never the CLM warning. I read the emitted script and agree it looks that way, but nothing here executes PowerShell, so this is reasoned, not proven. It belongs with the Windows Server 2019 validation this PR already defers. Worth a follow-up issue.;producing;;. Round 4 explicitly accepted this as a known property of preservation-over-normalisation. Not reversing that without a decision.code-standards.md:92says "Log using module-level loggers"; round 3 asked for the logger on the ABC and round 4 asked the adapters to use it. The written standard contradicts both. Left as-is pending a call on which wins.Verification
ruff checkclean ·ruff format --checkcleanpytest: 203 passed, 0 skippednode --test: 59/59git merge-tree, 0 conflicts)🤖 Generated with Claude Code
**Logger.** `docs/standards/code-standards.md:92` requires module-level loggers, and 47 modules across `app/` follow it — the three adapter files this PR touched were the only deviation. The class attribute on the ABC is gone; `_check_privilege_guard` logs through `base.py`'s own module logger, and both adapters log through theirs. Round 3 asked for the logger on the ABC and round 4 asked the adapters to use it; the written standard supersedes both. **Trailing ';'.** Registration now appends onto `$rawPath.TrimEnd(';')`, so a machine PATH ending in ';' can no longer have its trailing empty element turned into an interior one. An empty PATH element resolves as the current directory in some resolvers, which on a machine-wide PATH is a privilege-escalation surface, so removing one is strictly an improvement. This is deliberately asymmetric: unregistration does NOT trim. It is already rewriting the value it was asked to change, whereas trimming during teardown would mutate an element the operator never asked it to touch -- the exact footgun the ownership guards exist to prevent. Both behaviours are now pinned by assertions, including a negative one on the unregister path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Round-5 open items closed (
bcfd1a4→5240167)Both remaining items decided and applied; the third is tracked as follow-up issues.
1. Logger — conformed to the written standard
docs/standards/code-standards.md:92requires module-level loggers. Round 3 asked for the logger on the ABC; round 4 asked the adapters to use it. The standard contradicts both, and the repo backs the standard: 47 modules underapp/use module-level loggers, and the only three files deviating were the adapter files this PR touched.The ABC class attribute is removed.
_check_privilege_guardlogs throughbase.py's own module logger; both adapters log through theirs. Cost is that the privilege warning is attributed tohikcentral.cli.adaptersrather than the concrete platform — acceptable, since the message already namesrootvsadministrative.2. Trailing
;— register trims, unregister does notRegistration now appends onto
$rawPath.TrimEnd(';'). Round 4 accepted the;;behaviour as a property of preservation-over-normalisation, which was reasonable, but an empty PATH element resolves as the current directory in some resolvers — on a machine-wide PATH that is a privilege-escalation surface. Removing one is strictly an improvement, and it touches no foreign non-empty content.Deliberately asymmetric, flagging it before a reviewer does: unregistration does not trim. Registration is already rewriting the value it was asked to change, whereas trimming during teardown would mutate an element the operator never asked it to touch — the exact footgun the ownership guards exist to prevent. Both behaviours are pinned, including a negative assertion (
"TrimEnd(';')" not in script) on the unregister path.3. CLM fallback — tracked, not fixed here
Filed as #59 (
ready-for-human), proposing thereg.exerewrite:reg.exeis an external binary, so it is unaffected by Constrained Language Mode, andreg add /t REG_EXPAND_SZkeeps AC4's type preservation.Not fixed in this PR deliberately: nothing here executes PowerShell, so the fix could not be verified, and shipping an unverifiable rewrite of the load-bearing path is the pattern that produced this finding in the first place.
The broader gap — AC4 and AC5 resting entirely on string assertions, with CI on a single
runs-on: nativeLinux runner — is #60 (ready-for-human).Verification
ruff checkclean ·ruff format --checkcleanpytest: 203 passed, 0 skippednode --test: 59/59git merge-tree, 0 conflicts)All findings from every round are now fixed, or recorded as a deliberate decision, or tracked. The Windows Server 2019 caveat in the PR body still stands and is now issue-backed. Ready to merge from my side.
🤖 Generated with Claude Code