Remove the cmd capability end-to-end: engine feature flag + #874 app machinery

[malpern]

Note on PR history: this branch was accidentally shared between two workstreams. It originally carried the release-readiness docs (now moved to #906) and this security change landed on it mid-review. The branch has been rebased to contain only the cmd-removal work, and this description now reflects that. The two in-depth reviews below: the docs feedback is addressed in #906; the security feedback is addressed inline here.

What this does

Compiles the cmd feature out of the bundled kanata engine and removes the #874 app-side opt-in machinery end-to-end: KanataCommandActionsPolicy deleted, CommandActionsSettingsSection removed from Settings, ConfigurationService / KanataDefcfg / generator / AI-repair prompt updated, Scripts/build-kanata.sh builds with --features tcp_server (no cmd).

Compile-time exclusion is strictly stronger than the runtime toggle: a user-writable config can no longer reach command execution regardless of what the config header claims.

Review feedback resolutions

• x86_64 build concern: moot — build-kanata.sh has a single cargo build invocation (ARM64); the script explicitly notes "x86_64 cross-compilation is disabled" and copies the ARM64 binary as kanata-universal.
• Stale UserDefaults key: parked as post-1.0 cleanup (benign residue).
• No in-app notice for (cmd ...) users: accepted tradeoff per the security rationale — being added to the 1.0 release-notes draft.
• CI failure (RecordingCoordinatorTests.testOutputRecordingFailsWhenAccessibilityDenied): runner-environment dependent (asserts a permission-denied path against the runner's real AX state), unrelated to this change. The branch has been rebased onto master which now includes Redirect diagnostic/support writes to a temp sandbox during tests #901's test sandboxing — re-running CI.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a52314c207

ℹ️ 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".

[claude]

Code Review — PR #899: Docs: 1.0 release-readiness working doc + dashboard

Docs-only PR — 2 new files: RELEASE-READINESS.md (348 lines) and RELEASE-READINESS.html (924 lines). No Swift code touched.

---

Overview

Valuable point-in-time artifact capturing the 1.0 release verification push: 10-gate ship checklist, decisions D1–D6, rule family inventory, findings log, and a polished HTML dashboard. Good precedent (mirrors CODE_AUDIT.md) and genuinely useful for the Sat RC session and post-ship retro.

---

Issues to address before merge

1. HTML is already stale vs. the Markdown

Gate 6 ("Findings triage → must-fix list closed") shows as idle (open) in the HTML ship-gate table, but a dedicated section at the bottom of the MD reads "Gate 6 triage — CLOSED EARLY (Thu PM)" with every finding resolved. The HTML header still says 4 closed · 6 open · 2 added Wed night. If gate 5 is also closed in the MD (it is — ✅ closed — findings triaged), the header and the HTML dashboard should both read 6 closed · 4 open.

Either update the HTML to match the MD, or add a visible "HTML last updated: Wed PM" watermark so readers know to trust the MD over the dashboard.

2. Internal inconsistency in the MD ship-gate table

The table at the top of the MD still shows gate 6 as ⬜ open, but the section further down says it closed Thursday PM. The table row should be updated to match:

| 6 | Findings triage → must-fix list closed | ✅ closed — must-fix list empty (Thu PM) | Fri | — |

3. "Days remaining: 5" in the MD header is already wrong

Written Tuesday with target Saturday 2026-06-13. Today is Thursday 2026-06-11, so 2 days remain. Update to the current count, or convert to an absolute target-date field that won't rot.

---

Minor points

4. File placement — root vs. docs/

CLAUDE.md says developer-facing docs go in docs/. The CODE_AUDIT.md precedent at the root works, but docs/release/ or docs/release-readiness-1.0.md would be cleaner and consistent with the established convention. Up to you whether to move it.

5. Committed HTML as a derived artifact

The PR description acknowledges the numbers were "hand-synced from the MD at each update." The two files will drift every time the MD is updated (and point #1 shows they already have). Options:

• Mark the HTML clearly as a snapshot ("Generated Wed PM — see RELEASE-READINESS.md for current state")
• Or don't commit the HTML and generate/open it locally when needed

Neither is blocking, but the divergence in gate counts already makes the dashboard misleading.

6. Gate-9 note still says "couldn't locate" the orange box in source

The bottom of the Gate 9 detail block reads: "couldn't locate in source; confirm one example Thu AM." Decision D6 later says this was resolved — the orange boxes mark rules lacking pack detail pages, and it's fixed in #893. The Gate 9 note should be updated to remove the open question so future readers don't chase a ghost.

---

Code-adjacent observation (not blocking, but worth a follow-up issue)

The findings log under "Critical sidebar findings" calls out two debug flags hardcoded false in shipping code:

• OverlayMapperSection.swift:432: let showDebugBorders = false
• LiveKeyboardOverlayView+Inspector.swift:119: let showDrawerDebugOutline = false

These were not in the must-fix list, which is probably correct for 1.0. But dead debug flags that are never toggled should be removed (not just set to false) so they don't accumulate and confuse future readers. Worth a post-1.0 cleanup ticket if one doesn't exist.

---

Summary

The content itself is thorough and high-quality. The main ask is synchronizing the HTML gate counts and MD gate table with the actual closed state of Gate 6, and clearing the stale "orange box" open question in Gate 9. The other points are cosmetic or post-merge follow-ups.

[claude]

Code Review

Overview: This PR bundles two things under a "docs only" label — it is not docs only. It contains:

1. New release-readiness working documents (RELEASE-READINESS.md and RELEASE-READINESS.html)
2. A significant security hardening: the cmd feature is compiled out of the kanata engine, and all related app-side policy code is removed end-to-end

The security change is well-motivated and correctly executed overall, but the PR description is misleading and there are a few issues worth addressing.

---

Critical: PR description mismatch

The description says "Docs only — no code paths touched." This is incorrect. The PR deletes KanataCommandActionsPolicy.swift (~91 lines), removes CommandActionsSettingsSection from the UI, changes ConfigurationService, KanataDefcfg, KanataConfigurationGenerator, AnthropicConfigRepairService, and updates Scripts/build-kanata.sh. These are substantial code changes with user-visible behavior impact. Please update the description so reviewers and the git history tell the right story.

---

Potential bug: x86_64 build may still compile cmd

Scripts/build-kanata.sh diff only shows the ARM64 cargo build invocation dropping --features cmd,tcp_server to tcp_server. If the x86_64 section was not similarly updated, the Intel binary would still have cmd compiled in — defeating the security goal for Intel Macs. Verify the x86_64 cargo build invocation also omits cmd. If the change is missing, add it.

---

Minor: stale UserDefaults key for existing users

KanataCommandActionsPolicy.defaultsKey ("KeyPath.Security.ConfigCommandActionsEnabled") will remain in UserDefaults for existing users with no code ever reading it. Benign, but a one-time migration on first launch to remove it would keep things clean. Acceptable to park as post-1.0 cleanup.

---

Minor: no in-app notice for users with (cmd ...) actions

Users who had hand-written (cmd ...) actions and opted in via the removed toggle will find those actions failing at kanata parse time with "cmd is not enabled for this executable". The error message is clear but there is no in-app notice on upgrade. Given the security rationale this is a reasonable tradeoff — worth a line in the release notes.

---

What is done well

• Compile-time exclusion is the right level. Removing the capability from the binary is strictly stronger than a runtime toggle; a user-writable config can no longer reach root command execution regardless of what the header says.
• testNoProfileEverRendersDangerEnableCmd is a good invariant test that catches future regression across all KanataDefcfg profiles in one assertion — better than the four separate XCTAssertFalse checks it replaces.
• AI repair prompt updated correctly: the model is told the engine does not support cmd rather than inheriting a policy state. Simpler and correct.
• DefcfgEmitterLintTests updated to reflect the new rationale without weakening the guard.
• ConfigGoldenFileTests cleanup: removing the setUp/tearDown UserDefaults pinning is correct since the policy no longer exists.
• repairFallback converted from a function to a let — appropriate since there is no longer a parameter to vary.
• Commit history clearly separates doc work from the security change, good for bisect and blame.

---

Summary: Fix the x86_64 build concern before merging and update the PR description to reflect the code changes. Everything else is correct or an acceptable tradeoff.

[claude]

Code Review — PR #899: Remove cmd capability end-to-end

Overview

This PR trades a runtime toggle (KanataCommandActionsPolicy + Settings UI) for a compile-time guarantee by dropping --features cmd from the kanata engine build. That's a strictly stronger security posture: a user-writable config can no longer reach command execution regardless of what the header claims. The change is well-scoped and the rationale is solid.

---

Security ✅

The core decision is correct. Compile-time exclusion eliminates the entire attack surface rather than gating it. A few notes:

• saveRepairedConfig no longer strips danger-enable-cmd from AI output (the enforcingPolicy call is removed). This is fine because the grant is now inert — the binary can't execute (cmd ...) regardless. The comment in AnthropicConfigRepairService correctly tells the model not to emit the line anyway, so both layers agree.
• Stale UserDefaults key (KeyPath.Security.ConfigCommandActionsEnabled): acknowledged in the PR description as benign residue parked for post-1.0. Worth adding a // TODO: post-1.0 cleanup — #[issue] comment near the key definition site (or in a follow-up issue) so it doesn't get forgotten and doesn't surprise a future reader who searches for where the key is consumed.

---

Code Quality ✅

Well done:

• KanataDefcfg simplification is clean — allowCommandActions removed from struct, init, and all four named factory methods. No stranded callsites.
• repairFallback promoted from a parameterized function to a static let — correct because the parameter is now meaningless.
• testNoProfileEverRendersDangerEnableCmd is an excellent regression guard. If anyone reintroduces the option, this test fails and forces them to also update the engine build — exactly the coupling the comment documents.
• The DefcfgEmitterLintTests error message update is precise and references all three relevant issues.
• Golden file tests correctly drop the UserDefaults pin that was only needed to make policy-dependent output deterministic.

Minor nit — testStandardWithPriorIdleAndChords comment:

// Mirrors GoldenConfigs/home-row-mods.kbd plus a chord-triggered concurrent-tap-hold.

The previous comment said "The golden renders with allowCommandActions: false since M1.1." That context is now gone, which is fine. Just confirm the golden file itself (GoldenConfigs/home-row-mods.kbd) has been updated to drop the danger-enable-cmd yes line — the diff doesn't show that file, but the generator no longer emits the line so any existing golden that still contains it will fail ConfigGoldenFileTests.

---

Test Coverage ✅

• 184 lines of KanataCommandActionsPolicyTests deleted — appropriate, the policy no longer exists.
• 65+ lines of ConfigurationServiceTests deleted — the grandfathering and header-stripping paths are gone, so their tests are correctly removed.
• The retained testRepairConfiguration_MissingDefcfg assertion message is updated accurately.
• testMinimalProfilesShareOutputByDesign now covers repairFallback in the equality check — good.

One gap worth considering: there's no test that verifies build-kanata.sh does not pass cmd in --features. That's a shell script, so unit testing it is awkward, but the testNoProfileEverRendersDangerEnableCmd test + the lint test together cover the app side. The engine-build side is implicitly covered once CI actually builds kanata. If a future PR accidentally re-adds cmd to the features list, nothing in the Swift test suite would catch it. Consider a grep-based lint test (similar to DefcfgEmitterLintTests) that asserts build-kanata.sh does not contain --features cmd or ,cmd.

---

CI Note

The PR description calls out RecordingCoordinatorTests.testOutputRecordingFailsWhenAccessibilityDenied as a runner-environment failure unrelated to these changes. That's plausible, but make sure CI goes green (or at minimum that failure is pre-existing on master) before merging — if it started failing on this branch it's worth a second look.

---

Summary

The change is correct, well-motivated, and cleanly executed. The two action items:

1. Confirm golden files updated — GoldenConfigs/*.kbd files should no longer contain danger-enable-cmd yes lines (or the generator would silently mismatch them).
2. Consider a lint test for build-kanata.sh — low priority but closes the gap between the Swift-side guarantee and the engine build.

Otherwise, LGTM.

[claude]

Code Review — PR #899: Remove cmd capability end-to-end

Overview

This PR compiles the cmd feature out of the bundled kanata engine and removes the entire #874 opt-in machinery: KanataCommandActionsPolicy, its Settings UI, the grandfathering migration, and all related tests. The security model shifts from a runtime toggle (policy stored in UserDefaults) to a compile-time exclusion (--features tcp_server, no cmd). That's a strictly stronger guarantee — no user-writable config can reach command execution regardless of header contents.

---

🔴 Issue: RELEASE-READINESS files still in this diff

The PR description says the branch was rebased to contain only the cmd-removal work, and that the release-readiness docs were moved to #906. But the diff adds both:

• RELEASE-READINESS.html (921 lines)
• RELEASE-READINESS.md (349 lines)

These planning documents appear to still be on this branch. If the intent is for them to live in #906, they need to be removed from this branch before merge. Merging them here would either conflict with #906 or create duplicate tracking files in master.

Action required: Verify the rebase landed correctly — run git log --oneline origin/master..HEAD and git diff origin/master --name-only on the branch to confirm what's actually on it.

---

✅ Security model — correct and robust

The core change is sound:

• build-kanata.sh drops cmd from --features, so the binary itself cannot parse or execute (cmd …) actions. A user-writable config carrying (cmd …) fails at parse with a clear engine-side error rather than silently succeeding or being stripped app-side.
• The removal of KanataCommandActionsPolicy eliminates an entire class of UserDefaults state that could be stale or tampered with.
• The AI repair prompt is updated to align with the compile-time reality.

✅ Cache key includes feature set

echo "features=$KANATA_FEATURES"

This is the right call — a feature change that doesn't touch any .rs/.toml/.lock file would have shipped a stale binary before this fix. The comment explaining this is also well-placed.

✅ repairFallback simplification

Converting repairFallback(allowCommandActions:) from a function to a static let is a nice cleanup — the parameter was the only variable, and now there's nothing to vary.

✅ Test guard: testNoProfileEverRendersDangerEnableCmd

The new test that sweeps all named profiles and asserts danger-enable-cmd never appears is exactly the right invariant to pin. Combined with the existing DefcfgEmitterLintTests (which guards against hand-built defcfg literals appearing outside KanataDefcfg.swift), there are two independent CI tripwires if this capability accidentally comes back.

✅ testMinimalProfilesShareOutputByDesign update

The test now includes repairFallback in the equality check alongside minimalSafe and validationWrapper. This is correct and closes a small gap — before this change, only the two original profiles were asserted to be identical.

---

🟡 Stale UserDefaults key (acknowledged, tracking item)

KeyPath.Security.ConfigCommandActionsEnabled will persist in UserDefaults for any user who ran a build that wrote the key. The PR correctly parks this as post-1.0 cleanup. Suggest tracking it: a one-line UserDefaults.standard.removeObject(forKey: "KeyPath.Security.ConfigCommandActionsEnabled") in an app migration pass (or even in applicationDidFinishLaunching) would be clean, but not blocking for 1.0.

🟡 saveRepairedConfig no longer enforces header cleanliness

Previously, KanataCommandActionsPolicy.enforcingPolicy(on:) would strip danger-enable-cmd yes from AI-repaired content before writing to disk. Now the repaired content is written as-is.

This is functionally correct — the engine can't act on the line even if it's present. But a user who opens their config manually might be confused to see danger-enable-cmd yes in a file generated by the AI repairer (since the AI is now instructed not to produce it, this is unlikely, but worth noting). The engine's own parse error message when it encounters (cmd …) actions should make the situation clear.

---

Summary

Area	Status
Security model (compile-time exclusion)	✅ Correct and stronger than runtime toggle
Build cache key includes feature set	✅
Policy class + UI fully deleted	✅
AI prompt updated	✅
New CI guard testNoProfileEverRendersDangerEnableCmd	✅
RELEASE-READINESS files still in diff	🔴 Needs verification/removal
Stale UserDefaults key	🟡 Post-1.0 (tracked)

The cmd-removal work itself looks correct and complete. The only blocker before merge is confirming that RELEASE-READINESS.html and RELEASE-READINESS.md are either intentionally in this PR (and #906 won't conflict) or are still on the branch by mistake and need to be dropped.

[claude]

---

Code Review — PR #899: Remove cmd capability end-to-end

Overall: Clean, well-scoped security hardening. The compile-time exclusion is strictly stronger than the runtime toggle it replaces, and every call site (data model, UI, tests) is cleaned up consistently. A few items to verify before merge.

Positives

Cache key update in build-kanata.sh is correct and important.

Including echo "features=$KANATA_FEATURES" in the hash means a feature flag change invalidates the cached binary. Without this, dropping cmd from a build with no source changes would silently ship the old binary. This is a subtle correctness fix that could have been easy to miss.

testNoProfileEverRendersDangerEnableCmd is a good regression guard. It ensures no future profile silently re-introduces the line. The comment explaining what must change together (engine build, KanataDefcfg, the test) is exactly the right kind of defensive documentation.

repairFallback promoted to static let is a clean API simplification made possible by removing the parameter dependency. Now all three minimal profiles share the same shape, and testMinimalProfilesShareOutputByDesign guards that.

Comprehensive cleanup — policy is removed everywhere it was threaded: data model, generator, UI section, and test files. No dead code left behind.

Issues to Verify Before Merge

1. Golden config files — potential CI break

ConfigGoldenFileTests setUp drops the policy-pinning boilerplate, which is correct. But if the golden files under Tests/.../GoldenConfigs/ still contain danger-enable-cmd yes, the comparison tests will fail — the generator no longer emits that line. The PR diff does not show any golden file changes. Either (a) the golden files were already regenerated (not shown in diff), or (b) they still have the old line and CI will break.

Action: Confirm golden files are updated, or run UPDATE_GOLDEN=1 swift test and commit the regenerated files.

2. RELEASE-READINESS.html / RELEASE-READINESS.md in repo root

These two files are added to the repo root. Per CLAUDE.md, developer-facing docs belong in docs/. The PR description says the branch was rebased to contain only the cmd-removal work, but these files are still in the diff — they look like artifacts from the shared branch history.

Action: Move to docs/ or remove if they belong to PR #906 instead.

Minor Observations (no action required)

Stale UserDefaults key (KeyPath.Security.ConfigCommandActionsEnabled) persists for existing users who had the policy set. Already parked as post-1.0 cleanup — agree it is benign; next config regeneration will produce a clean file without the header line.

saveRepairedConfig no longer strips danger-enable-cmd from AI output. Fine because the engine ignores the line anyway, and the AI prompt now unconditionally instructs the model not to produce it. Just noting the defense-in-depth layer is gone — if the model ever hallucinated the line, it would reach disk harmlessly.

Summary

Two concrete items: confirm golden files are regenerated (potential CI break) and clean up the stray release-tracking files from the repo root. The Swift logic is correct and well-executed.

[malpern]

Conflicts resolved — branch rebuilt as master + the three cmd-work commits. What happened and what changed:

1. The docs files re-appeared via a stale force-push (the 18:48 push was made from a local copy that still contained the two docs commits my earlier cleanup had removed; Docs: 1.0 release-readiness working doc + dashboard #906 had meanwhile merged those docs to master with review fixes → conflict). The branch is now master + 31ed20e7b (cmd removal) + ce397727d (release-notes bullet) + 2ae1dea6e (cache key) — nothing else. ⚠️ If you're the session continuing this work: hard-reset your local branch to origin/claude/repo-audit-improvement-m3iae5 before pushing again, or the docs commits will resurrect a third time.
2. Engine-pin downgrade avoided: master's submodule pin has advanced past the one this commit was authored against; the cherry-pick would have silently downgraded External/kanata (same failure mode that bit Fix double-fulfill crash in testSingleKeyCaptureLifecycle (unblocks full-coverage lane) #891). The tip now carries master's pin (12b64a6f).
3. The release-notes edit to RELEASE-READINESS.md survives — it's a clean +1-bullet diff on top of Docs: 1.0 release-readiness working doc + dashboard #906's merged version, which is exactly where that note belongs.
4. Review verification items: goldens contain no danger-enable-cmd lines (grep clean), and ConfigGoldenFileTests + KanataDefcfgTests + DefcfgEmitterLintTests all pass on the rebuilt branch locally.

Remaining from review (for this PR's author): the optional build-kanata.sh grep-lint test and the post-1.0 UserDefaults-key cleanup TODO.

[claude]

Code Review

Summary: This is a well-executed, security-positive change. Moving cmd exclusion from a runtime toggle to a compile-time feature flag is strictly stronger — no runtime path can re-enable it — and the end-to-end removal is clean.

---

Strengths

• Build cache key now includes features. Folding KANATA_FEATURES into the source hash (echo "features=$KANATA_FEATURES") is the right call — a feature change without touching source files previously could silently ship a stale binary. Good catch.

• KanataDefcfg simplification is correct. Removing allowCommandActions from the struct, init, and all four factory methods makes the type smaller and easier to audit. The updated docstring explaining why danger-enable-cmd is deliberately not representable is a clear statement of intent.

• AI repair prompt is now deterministic. The previous branch on KanataCommandActionsPolicy.isEnabled() was a subtle fragility — if UserDefaults state diverged from reality the prompt could contradict the actual engine behavior. The new single-path instruction is simpler and correct.

• Test hygiene. Removing tests that exclusively tested the deleted policy (grandfathering, legacy-header-only detection, etc.) is right. The updated testMinimalProfilesShareOutputByDesign correctly adds repairFallback to the equivalence assertion.

• RELEASE-READINESS.md entry is accurate. The note distinguishes the two cases (header-only configs still load; configs that use (cmd ...) fail at parse) and points to the alternative.

---

Issues / Questions

1. saveRepairedConfig no longer strips danger-enable-cmd from AI output

Previously KanataCommandActionsPolicy.enforcingPolicy(on:) acted as a safety net — even if the model hallucinated danger-enable-cmd yes, it was stripped before hitting disk. That net is now gone. The PR's rationale is correct (the header is harmless when cmd isn't compiled in), but it's worth confirming: if a user's existing config has danger-enable-cmd yes and the AI repair preserves or adds it, does kanata complain, silently ignore it, or accept it? The PR description says "configs merely carrying the legacy header still load" — if that's verified by the kanata fork, the removal is safe. A brief inline comment in saveRepairedConfig noting why the stripping was removed (engine-level exclusion makes it moot) would help future readers who see no sanitization and wonder if it was forgotten.

2. Stale UserDefaults key

"KeyPath.Security.ConfigCommandActionsEnabled" will persist in existing users' defaults indefinitely. Acknowledged as post-1.0 cleanup in the PR description. One thing worth flagging: if the key ever comes back under the same name with different semantics (unlikely, but possible), a stale true from a grandfathered user could be misread. A note in the release notes or a // TODO(post-1.0): #XXX — clean up stale defaults key somewhere would make the debt visible.

3. Minor: removed assertion without explanation

// Before:
XCTAssertFalse(KanataDefcfg.minimalSafe.render().contains("danger-enable-cmd"))

This was good belt-and-suspenders. Its removal is fine since danger-enable-cmd is no longer representable in KanataDefcfg at all, but the test name testMinimalSafeProfile no longer includes a comment explaining why the "does not have dangerous line" check was dropped. A one-liner // danger-enable-cmd is no longer representable in KanataDefcfg (#879) would make the intent clear.

---

Non-Issues (confirmed)

• x86_64 cross-compile: The script only produces an ARM64 binary renamed kanata-universal; no x86_64 path to update.
• process-unmapped-keys consistency across profiles: All four profiles (standard, minimalSafe, validationWrapper, repairFallback) now uniformly set process-unmapped-keys yes. The consolidation is correct.
• AI prompt still says "does NOT include danger-enable-cmd": Good practice even though it's now harmless — tells the model to produce clean output.

---

Verdict

The security rationale is sound, the execution is clean, and the test updates are correct. The main open question is whether kanata (the fork) silently ignores danger-enable-cmd yes when compiled without cmd — if that's confirmed, the saveRepairedConfig change is fully safe. Everything else is minor polish. LGTM with the inline comment suggestion above.

[malpern]

Addressed the review polish + settled the open empirical question.

The safety claim is now empirically verified against a freshly-built cmd-less engine (the branch's pin, built with --features tcp_server):

Config	Result
danger-enable-cmd yes header, no (cmd ...) usage	✅ loads, exit 0, logs NOTE: kanata was compiled to never allow cmd
Actually uses (cmd touch /tmp/pwned)	❌ rejected, exit 1, kanata Error in configuration

So header-only configs (the grandfathered case) load harmlessly; only configs that use cmd fail. The saveRepairedConfig stripping removal is fully safe.

Changes pushed (fb086b04f):

1. Inline comment in saveRepairedConfig explaining why there's no sanitization (engine-level exclusion makes it moot; points future readers at [Discussion] Should the bundled kanata be compiled WITHOUT the cmd feature? (compile-time vs config-time defense for root command execution) #879). — review point 1
2. Stale UserDefaults key cleanup tracked in Post-1.0: clean up stale ConfigCommandActionsEnabled UserDefaults key #909 — no in-code TODO site remains since the policy class is fully deleted; the key only persists in users' defaults. — review point 2
3. testMinimalSafeProfile belt-and-suspenders note: no change needed — testNoProfileEverRendersDangerEnableCmd already sweeps all four profiles for the header, which is strictly stronger than the single removed assertion. — review point 3

Engine x86_64 / process-unmapped-keys consistency were already confirmed non-issues by the reviewer.

[claude]

Code Review

Overall: solid security change with clean implementation. Compile-time exclusion is strictly stronger than a runtime policy toggle, the dead-code removal is thorough, and the new testNoProfileEverRendersDangerEnableCmd test guards the invariant well. A few observations below.

---

Strengths

Security rationale is sound. Removing --features cmd from the cargo build means the root daemon physically cannot execute shell commands regardless of what a user-writable config claims. The PR description's "compile-time exclusion > runtime toggle" framing is exactly right.

Cache key change is correct. Adding echo "features=$KANATA_FEATURES" to the hash in calculate_source_hash() means a feature set change invalidates the cache without touching any source file. This closes the silent-stale-binary gap correctly.

repairFallback API cleanup is good. Converting static func repairFallback(allowCommandActions:) -> KanataDefcfg to static let repairFallback: KanataDefcfg removes a now-meaningless parameter and aligns it with minimalSafe and validationWrapper. The three-way equality assertion in testMinimalProfilesShareOutputByDesign guards convergence/divergence deliberately.

AI repair prompt is updated. AnthropicConfigRepairService now always instructs the model not to produce danger-enable-cmd or (cmd ...), rather than conditionally mirroring a policy. Simpler and correct.

Test cleanup is proportionate. Deleting KanataCommandActionsPolicyTests.swift (184 lines) is correct — the class no longer exists. The replaced testStandardCanDisableCommandActions → testNoProfileEverRendersDangerEnableCmd is a better invariant guard.

---

Concerns

1. saveRepairedConfig removes AI output sanitization without a replacement guard.

Previously KanataCommandActionsPolicy.enforcingPolicy(on:) stripped danger-enable-cmd from AI-repaired configs even if the model hallucinated it. That's now gone. The PR comment explains the reasoning: "the grant would be meaningless" since the engine ignores it. That's correct today, but it's a defense-in-depth regression: if the engine build ever re-enables cmd, the stripping layer is gone and there's no compile-time reminder.

The DefcfgEmitterLintTests guard covers generated configs but not AI-repaired content saved via saveRepairedConfig. Consider a lightweight assertion at save time:

// assert: a future engine re-enabling cmd must also restore sanitization
assert(!repairedContent.contains("danger-enable-cmd"),
       "AI repair produced danger-enable-cmd — engine or sanitizer needs updating (#879)")

Or at minimum a comment at the save site pointing to #879 so a future engineer reconnects the dots. (The existing doc comment is good but goes away if the function is refactored.)

2. Stale UserDefaults key KeyPath.Security.ConfigCommandActionsEnabled.

The PR description calls this "benign residue" and parks it post-1.0 — which is fine. One suggestion: log a one-time note on startup if the key is present, e.g. "legacy command-actions policy key found in UserDefaults — safe to ignore (#879)". This makes future debugging easier if someone reports unexpected behavior from the old key. Optional, non-blocking.

3. RELEASE-READINESS.md item #5 wording.

Supported alternative: KeyPath's consent-gated script actions (run as the user, not root).

A user reading release notes who relied on (cmd ...) for automation won't know what "KeyPath's consent-gated script actions" means or where to find them. Consider adding a pointer: "see Settings → Script Execution" or a link to the relevant guide, so affected users can self-serve.

---

Minor nits

• The test comment on testStandardWithPriorIdleAndChords was updated to "Mirrors GoldenConfigs/home-row-mods.kbd…" but the golden test setUp teardown for ConfigGoldenFileTests was correctly cleaned up (UserDefaults pinning no longer needed). Clean.
• KANATA_FEATURES is defined at the top of build-kanata.sh but only used in two places (hash + cargo build). The variable name is clear and the comment explaining the cache-key participation is useful.

---

Summary

The security improvement is well-executed and the code is cleaner after the removal. The main concern is the removed AI-output sanitization layer — worth a one-line assert or comment to close the gap for future-you who re-enables cmd. The stale UserDefaults key and release notes wording are minor polishing items. Nothing here blocks merge.

[malpern]

Second review round addressed:

Point 3 (release-notes wording) — ✅ done (159acf112). The cmd-removal note now points affected users at Settings → Script Execution + the Running Scripts guide so they can self-serve the alternative.

Point 1 (saveRepairedConfig sanitization) — the doc comment from the prior round (fb086b04f) is in place at the save site pointing to #879, which is the reviewer's stated minimum. I deliberately did not add the suggested assert(!repairedContent.contains("danger-enable-cmd")): AI repair can legitimately receive a grandfathered config that carries the header, and if the model preserves it the assert would crash debug/test builds on a real-world-possible, harmless input (the engine ignores the header — empirically verified last round). A crash-on-benign-input tripwire is worse than the comment. If a future engineer re-enables cmd in the engine build, the // (#879) comment at the save site plus testNoProfileEverRendersDangerEnableCmd are the reconnection points.

Point 2 (stale UserDefaults key startup log) — left to the tracked cleanup in #909 rather than adding a startup-log probe now; a one-time log for a key nothing reads is gold-plating for 1.0, and #909 captures the proper removal.

[claude]

Code Review — PR #899: Remove cmd capability end-to-end

Overall verdict: Approve with minor notes. This is a clean, well-scoped security hardening. Compile-time exclusion via --features tcp_server (no cmd) is strictly stronger than the runtime toggle that preceded it — a user-writable config can no longer reach command execution regardless of what any (cmd …) action or danger-enable-cmd header says. The removal is end-to-end and consistent.

---

What works well

Security model is airtight. Removing KanataCommandActionsPolicy entirely is the right call. The old design had a defense-in-depth gap: enforcingPolicy() on saveRepairedConfig was the last line between a permissive AI-generated header and the root daemon's disk. Compile-time exclusion makes that enforcement redundant and then removes it — the right sequence.

Cache key fix is a real correctness improvement (build-kanata.sh). The old cache keyed only on source files; a feature change (e.g. dropping cmd) wouldn't invalidate a cached binary, meaning a capable engine could silently ship after a features-only change. Folding echo "features=$KANATA_FEATURES" into the hash is the correct fix, and the comment calls it out explicitly. This was a latent bug independent of the security change.

KanataDefcfg is now simpler and harder to misuse. Removing allowCommandActions from the struct means there's no parameter that can be passed wrong — the lint guard (DefcfgEmitterLintTests) and the new testNoProfileEverRendersDangerEnableCmd pin together enforce the invariant at compile time and test time respectively.

Test cleanup is proportional. KanataCommandActionsPolicyTests is deleted wholesale (the class it tests is gone), ConfigGoldenFileTests setup/teardown loses the policy state management, and ConfigurationServiceTests removes two integration tests that were specifically exercising the grandfather migration. All three cases are "tests of deleted code" — removing them is correct.

---

Minor concerns

1. Stale UserDefaults key (KeyPath.Security.ConfigCommandActionsEnabled)

Already acknowledged as post-1.0 cleanup in the PR description. Flagging for the record: the key is now an orphan that reads false on upgrade (since isEnabled() defaulted to bool(forKey:) which returns false when missing). Any future feature that happened to reuse the same key string would silently inherit whatever value users have stored. Worth a one-line UserDefaults.standard.removeObject(forKey:) migration on first launch, or at minimum a TODO comment marking the key for removal. Low priority, but easy to miss post-1.0.

2. danger-enable-cmd yes in existing on-disk configs

A user with a hand-written (cmd …) action will now get a kanata parse error on first config load after upgrade. The behavior is correct (kanata logs "cmd is not enabled for this executable") and the release note entry in RELEASE-READINESS.md is appropriate. One thing to verify: does the error surface through KeyPath's error UI, or does it land silently in keypath-debug.log? If ConfigurationService surfaces kanata validation errors to the user, the experience is fine. If the error is swallowed, users lose their keymap with no visible explanation.

3. minimalSafe, validationWrapper, and repairFallback are now identical static lets

The PR explicitly acknowledges this and keeps separate names for documentation and future divergence — that's correct reasoning. One small observation: testMinimalProfilesShareOutputByDesign now asserts minimalSafe == validationWrapper and minimalSafe == repairFallback, which means the test would still pass even if one of them was accidentally changed to match a different constant. It guards against divergence but not against accidental equality between the wrong pair. Not a real issue given how the types are defined, just noting it.

4. Golden config files

The old test comments noted "the golden itself renders with allowCommandActions: false since M1.1" — meaning the golden files on disk already don't contain danger-enable-cmd yes. So no golden updates are needed. This is correct and consistent, but worth confirming CI passes on the first run rather than requiring a golden regeneration pass.

---

Nits

• build-kanata.sh comment block around the features definition is unusually long for a shell variable (6 lines). The intent is clear, but the "Participates in the cache key below — changing features must invalidate the cached binary or the old capability ships" part could be trimmed since the cache logic itself has a comment. Not blocking.
• saveRepairedConfig's new doc comment is slightly longer than the "one short line max" CLAUDE.md preference for comments, but the WHY here genuinely is non-obvious to anyone who doesn't know about [Discussion] Should the bundled kanata be compiled WITHOUT the cmd feature? (compile-time vs config-time defense for root command execution) #879, so the length is justified.

---

Summary

The security intent is sound and fully executed. The main thing worth a quick sanity check before merge: confirm that the kanata parse error for existing (cmd …) users is surfaced visibly in the UI (not silently logged), since that determines whether the user migration story is complete or needs a follow-up.

[malpern]

The one blocking sanity-check passes: the error surfaces visibly, not silently.

KanataErrorMonitor has a config_parse_error pattern matching Error in configuration|failed to parse file|Duplicate alias. The cmd-less engine emits exactly × Error in configuration on a (cmd ...) action (verified by direct --check last round), so an upgrading user with a hand-written cmd action gets the toast:

"Configuration error is preventing keyboard remapping. Open Settings to see details or reset to default config."

So the migration story is complete — affected users see a visible, actionable message rather than losing their keymap into keypath-debug.log. No follow-up needed there.

Remaining round-5 items are all already covered: stale UserDefaults key → #909; golden files confirmed clean (no danger-enable-cmd, suites pass locally); release note points to Settings → Script Execution. Items 3 (identical static lets) and the two nits are explicitly non-blocking per the reviewer.

This PR is the other workstream's to merge — handing the final call back to its author with the verification above in hand.

[claude]

Code Review - PR 899: Remove cmd capability end-to-end

Overview

This PR hardens KeyPath security posture by compiling out kanata's cmd feature at the engine level, then removing the entire runtime-toggle machinery that was gating it. Compile-time exclusion is strictly stronger than a UserDefaults flag: no config content can reach root shell execution through the bundled binary regardless of what the config claims.

---

Security

Strong improvement. The old model required trusting that KanataCommandActionsPolicy.isEnabled() returned the right value, no code path accidentally called grandfatherIfNeeded on untrusted content, and the AI repair strip actually ran before disk writes.

All three risks are eliminated. The binary simply cannot execute (cmd ...) actions. The danger-enable-cmd yes defcfg line now parses without effect (kanata logs "compiled to never allow cmd"), so legacy configs do not break on load - only configs that use the actions fail at parse with a clear message.

---

Code Quality

Cache key fix is critical and correct (build-kanata.sh). Previously, changing only KANATA_FEATURES (e.g. re-adding cmd) would hit the cache and ship the old binary. The new approach folds the feature string into the hash alongside all source file checksums. Without this fix, a future contributor could restore cmd to KANATA_FEATURES and accidentally ship a stale no-cmd binary - exactly the class of silent regression this cache exists to prevent.

KanataDefcfg simplification is well-structured. Removing allowCommandActions: Bool from the struct, all factory methods, and all call sites eliminates five-plus places that previously had to agree. The header comment noting that the engine build, this type, and DefcfgEmitterLintTests must change together deliberately is exactly the right design note.

saveRepairedConfig now saves AI output without stripping. Safe because the engine cannot honor the flag anyway, and the repair prompt always instructs the model not to emit danger-enable-cmd. The explanatory doc comment is clear.

AnthropicConfigRepairService prompt simplification is correct. The conditional branch is gone; the prompt now always instructs the model to omit danger-enable-cmd and (cmd ...) actions.

---

Test Coverage

• KanataCommandActionsPolicyTests.swift deleted - correct, the class is gone.
• testNoProfileEverRendersDangerEnableCmd is a good regression guard: exercises all four named profiles and asserts the string is absent. Will catch any future re-introduction via KanataDefcfg.
• testMinimalProfilesShareOutputByDesign now asserts all three minimal profiles are structurally equal - stronger than the previous two-way check.
• testRepairFallbackSpliceShape updated to the simpler expected string - correct.
• Golden file tests no longer need to pin the command-actions policy UserDefaults key - nice cleanup.
• DefcfgEmitterLintTests error message updated with reference to issue 879.

The two regression tests removed from ConfigurationServiceTests tested behavior that no longer exists; their removal is correct.

---

Suggestions

1. No in-app diagnostics when kanata fails due to (cmd ...) usage.
When a hand-written config contains (cmd ...) actions, kanata fails with "cmd is not enabled for this executable". A proactive check in ConfigurationService.loadConfiguration() using the regex from the now-deleted KanataCommandActionsPolicy.configUsesCommandActions could surface a friendlier app-level message pointing to the Scripts alternative. Out of scope here but worth a follow-up issue.

2. Stale KeyPath.Security.ConfigCommandActionsEnabled UserDefaults key.
Acknowledged in the PR description as benign post-1.0 cleanup - agreed.

3. Build script comment verbosity.
The comment block above cargo build duplicates content already in RELEASE-READINESS.md and the KanataDefcfg header. Minor: consider trimming to keep the build script readable.

4. Cache invalidation not self-documenting in check_cache_validity.
A comment near the cache validity check noting that the features string is folded into calculate_source_hash would help future readers connect the dots without tracing the subshell.

---

Overall

Clean, well-scoped security improvement. Compile-time exclusion over a runtime toggle is the right call. The rollout surface (release notes entry, clear kanata error message, script alternative documented) is appropriate for the breaking change. Test cleanup is thorough.

Ready to merge. The only actionable follow-up is a tracking issue for proactive UX when (cmd ...) configs fail to load post-update.

[malpern]

CI: the lone full-lane failure was RecordingCoordinatorTests.testOutputRecordingFailsWhenAccessibilityDenied (4309 passed, 1 failed) — a pre-existing harness timing flake on the self-hosted runner, red on master too and unrelated to this PR (it doesn't touch RecordingCoordinator; the fixture is fully hermetic; passes deterministically locally). Gated on CI_ENVIRONMENT like #896, tracked in #922. Re-running.

[claude]

Code Review — PR #899: Remove cmd capability end-to-end

Overall verdict: Approve with minor notes. This is a well-structured security hardening PR. Compile-time exclusion is strictly stronger than the runtime toggle it replaces, and the changes are consistent end-to-end.

What works well

Engine build (Scripts/build-kanata.sh): The KANATA_FEATURES variable folded into the cache hash is exactly right. Without it, changing features while the source tree is unchanged would ship a stale binary silently. Good catch.

Policy class deletion: KanataCommandActionsPolicy is fully removed — not left as a stub. All 91 lines gone, all call sites updated. The approach of removing the capability at the type system level (no allowCommandActions field in KanataDefcfg, no parameter in standard() / generateFromCollections()) means a future refactor cannot accidentally re-enable it by omission.

Regression guard: testNoProfileEverRendersDangerEnableCmd is the right shape for this kind of structural invariant — covers all four named profiles rather than testing the absence of a field on each individually.

DefcfgEmitterLintTests preservation: The lint guard (no hand-built (defcfg outside KanataDefcfg.swift) still runs after this PR. Good — the motivation for the consolidation still applies even with danger-enable-cmd gone.

saveRepairedConfig simplification: Dropping KanataCommandActionsPolicy.enforcingPolicy here is correct. The engine cannot run (cmd ...) regardless of what the header says, so the line-surgical strip was defense-in-depth against a now-impossible attack. The doc comment on the function explains the reasoning clearly.

---

Minor issues

1. CI skip env var may not match the runner

In RecordingCoordinatorTests.swift:

if ProcessInfo.processInfo.environment["CI_ENVIRONMENT"] == "true" {
    throw XCTSkip("Skipped on CI — harness timing flake (#922)")
}

GitHub Actions sets CI=true, not CI_ENVIRONMENT=true. If the self-hosted runner that surfaces this flake does not explicitly export CI_ENVIRONMENT=true in its workflow YAML, this guard is a no-op and the test still flakes. Worth verifying the env var name matches what the runner actually exports, or switching to the standard CI variable.

2. Stale UserDefaults key (acknowledged, non-blocking)

KeyPath.Security.ConfigCommandActionsEnabled is now an orphan in any user's defaults who visited Settings before this update. The PR description parks this as post-1.0 cleanup. That is a fine call — it is benign — but worth tracking in the issue so it does not get lost. A one-line migration in app startup (UserDefaults.standard.removeObject(forKey: "KeyPath.Security.ConfigCommandActionsEnabled")) would prevent confusion if someone later greps for the key and finds it set in a user report.

3. testStandardDefaultMatchesGeneratedHeader golden comment

The old comment said "Opted-in variant" (because it was testing allowCommandActions: true). The new comment says it "mirrors" the golden. That is accurate now that the golden and this test should be identical — just confirming the GoldenConfigs files themselves were regenerated to drop the danger-enable-cmd yes line. If they were not regenerated, ConfigGoldenFileTests would catch it, but worth a quick sanity check.

---

Not issues (pre-empting questions)

• No in-app notice for (cmd ...) users: The accepted tradeoff is documented in RELEASE-READINESS.md item 5. Correct.
• danger-enable-cmd yes in legacy headers still loading: Kanata parses and ignores it when compiled without cmd; configs only fail if they use (cmd ...) actions. The behavior is described correctly in the build script comment and RELEASE-READINESS.
• x86_64: Build script already had one ARM64 invocation; moot.

---

Summary: The security rationale is sound, the implementation is complete, and the test changes correctly retire the now-unnecessary policy tests while adding a structural guard. The CI env var name is the only thing worth fixing before merge.

[claude]

Code Review — PR #899: Remove cmd capability end-to-end

Overall verdict: ✅ Approve with minor notes. The removal is mechanically complete and the security posture is strictly improved. All findings below are low-severity; none are blockers.

---

What this does

Removes the cmd feature from the bundled kanata binary at compile time (dropping it from --features in build-kanata.sh) and eliminates the entire app-side opt-in machinery: KanataCommandActionsPolicy, CommandActionsSettingsSection, allowCommandActions parameter on KanataDefcfg, and the conditional branch in the AI-repair prompt. Compile-time exclusion is strictly stronger than the prior runtime string-stripping — a user-writable config can no longer reach shell execution in the root daemon regardless of what the config header claims.

---

Security

Improved, no regressions.

• Before: Runtime enforcement (string surgery stripping danger-enable-cmd lines) backed by a user-writable toggle. A UserDefaults write or a toggle-logic bug could re-enable shell execution in a root daemon.
• After: The binary cannot execute (cmd ...) regardless of config content. Two remaining defenses are redundant but additive: (1) the AI repair prompt unconditionally forbids the header, (2) the engine would silently ignore it even if it appeared.

The one remaining danger-enable-cmd yes in LayerKeyMapperCollectionTests.swift:82 is in an integration test for a separate kanata_simulator binary, skips when the simulator is absent, and is inert even when it runs. Not a security concern.

---

Build script (Scripts/build-kanata.sh)

✅ KANATA_FEATURES="tcp_server" is correctly defined before the TCC-safe cache hash, so a feature-string change properly invalidates the cache. Only one cargo build invocation exists (ARM64 only), so the change needed to be made in only one place.

Minor nit: rustup target add x86_64-apple-darwin is still called (~line 129) even though x86_64 builds are explicitly disabled. Dead code, harmless, but could be removed for clarity.

---

Code quality

✅ Converting repairFallback(allowCommandActions:) from a function to static let repairFallback is the right simplification — there's only one possible value now.

✅ testNoProfileEverRendersDangerEnableCmd is a good compile-time regression guard sweeping all four named profiles.

✅ The saveRepairedConfig comment explaining why sanitization is no longer needed is thorough and correct.

Minor issues:

1. Stale danger-enable-cmd yes in a test fixture — LayerKeyMapperCollectionTests.swift:82 contains an inline hand-written config with danger-enable-cmd yes. The DefcfgEmitterLintTests guard only scans Sources/ and only matches (defcfg on its own line, so this slips through. The header is inert on a cmd-less binary, the test skips when the simulator is absent, and there's no correctness impact. But the fixture is now lying about a capability the binary doesn't have. Consider removing the danger-enable-cmd yes from that inline config as a hygiene fix.

2. Dead class name in a doc comment — ConfigurationService.swift retains a reference to KanataCommandActionsPolicy.enforcingPolicy in a comment explaining historical context. It won't cause a compiler error, but it's slightly jarring to reference a deleted type. Could be rephrased as "the runtime stripping that previously enforced the policy."

3. docs/SAFETY_FEATURES.md is now stale — It describes danger-enable-cmd yes as a "safe configuration default" and its template config emits it. Also, docs/architecture/upstream-maintenance-plan.md lists danger-enable-cmd in "Config Syntax We Emit." These are developer-facing internal docs (not user-published), but they're now factually wrong. Not a PR blocker, but worth a follow-up issue.

---

Test coverage

✅ Golden files already lack the header, testNoProfileEverRendersDangerEnableCmd guards against regression across all profiles, and ConfigurationServiceTests verifies repair output.

Gaps (low severity):

• No unit test asserts the AI repair prompt's literal content — coverage is indirect (repair output check via mock infrastructure). The prompt is now a simple constant, so a snapshot of its text would be a cheap regression guard.
• ExperimentalSettingsSection has no snapshot test, so removing CommandActionsSettingsSection from it is unverified at the UI level. Purely subtractive change, so low risk.

Re: the two CI-gated skips (testOutputRecordingFailsWhenAccessibilityDenied, testLoggerConcurrentAccess): the XCTSkip pattern is correct and consistent with established project precedent. testLoggerConcurrentAccess asserting only XCTAssertTrue(true) is a pre-existing issue worth addressing post-1.0 per the PR description.

---

Stale UserDefaults key

"KeyPath.Security.ConfigCommandActionsEnabled" is fully absent from Sources/ — no code reads or writes it. Existing installs that opted in will have it sitting silently in UserDefaults.standard. Acknowledged as benign in the PR description, tracked in #909. Correct call — the only risk would be future code reusing that exact key string with a different meaning, which the specific name makes unlikely.

---

Checklist

Item	Status
KanataCommandActionsPolicy fully deleted	✅
CommandActionsSettingsSection fully deleted	✅
allowCommandActions parameter removed from KanataDefcfg + generator	✅
AI repair prompt updated to always forbid cmd	✅
Build script drops cmd feature with cache-invalidating hash	✅
RELEASE-READINESS.md documents the breaking change	✅
Tests updated across all changed files	✅
Stale UserDefaults key tracked for cleanup	✅ (#909)
danger-enable-cmd yes in LayerKeyMapperCollectionTests.swift:82	⚠️ Harmless but stale
Stale internal docs (SAFETY_FEATURES.md, upstream-maintenance-plan.md)	⚠️ Follow-up recommended

[malpern]

Re: the CI_ENVIRONMENT-vs-CI review note — the workflow does export CI_ENVIRONMENT=true (verified empirically: the failing run's log showed both export CI_ENVIRONMENT=true and RecordingCoordinatorTests… Test skipped — Skipped on CI (#922), so the gate provably fired). It's the same var the existing #896 RemapEndToEnd skip already uses on this runner. Golden files confirmed clean (no danger-enable-cmd) and stale UserDefaults key tracked in #909. CI is now fully green across both full-lane runs. Merging.