fix: hide KEYBOARD_HACK with PM_UNSET instead of removing it

[HaleTom]

Issue for this PR

Closes #9

Type of change

• Bug fix

What does this PR do?

Hides the KEYBOARD_HACK special parameter from typeset/echo listings when empty or unset, instead of removing it entirely. The approach uses PM_UNSET:

• IPDEF2("KEYBOARD_HACK", ...) registered with PM_DONTIMPORT | PM_UNSET — starts hidden by default
• keyboardhacksetfn toggles PM_UNSET based on value:
  • Non-empty ASCII value → pm->node.flags &= ~PM_UNSET (visible)
  • Empty string after unmetafy → pm->node.flags |= PM_UNSET (hidden)
  • NULL (param being unset via stdunsetfn) → pm->node.flags |= PM_UNSET (hidden)

The SUN_KEYBOARD_HACK option and keyboardhackchar variable continue working independently — the option handler sets the hack character directly without touching the parameter's visibility.

Also fixes a pre-existing memory leak: keyboardhacksetfn was missing free(x) on the ASCII validation error return path.

How did you verify your code works?

Code review and production-readiness adversarial review — see REVIEW-2026-05-25T18:54:46+07:00.md.

Changes in Src/params.c:

• IPDEF2 flags: PM_DONTIMPORT → PM_DONTIMPORT | PM_UNSET (1 insertion)
• keyboardhacksetfn: added PM_UNSET toggling + memory leak fix (9 insertions, 3 deletions)

Related

• Issue KEYBOARD_HACK special parameter is unnecessary legacy pollution #9: KEYBOARD_HACK special parameter is unnecessary legacy pollution

Checklist

• I have tested my changes locally
• I have not included unrelated changes in this PR

[HaleTom]

Production readiness review — diff to main

Branch: fix/keyboard-hack-parameter (4 commits, net diff: +10/-3 in Src/params.c)

---

Production-ready bar for this PR

1. The KEYBOARD_HACK parameter must be hidden from typeset/export output by default, while still functional when explicitly assigned.
2. The SUN_KEYBOARD_HACK option (setopt) must continue working independently — enabling/disabling the option must not depend on the special parameter's visibility.
3. The change must not introduce any memory leaks — all allocation paths must free x before returning.
4. The PM_UNSET toggling must follow existing zsh patterns (e.g., HOME at params.c:896-900).
5. The stdunsetfn unset path must remain correct — setting PM_UNSET after calling setfn(pm, NULL).
6. The change must not break the ASCII-only validation constraint on KEYBOARD_HACK values.
7. No unrelated refactors, drive-by changes, or scope creep beyond the single-file change.

---

Findings

🔶 NON-BLOCKING — getfn returns value from PM_UNSET parameter

• Type: Verified issue
• Evidence: Src/params.c:4738-4744, Src/params.c:4777
• Why it matters: When PM_UNSET is set (parameter hidden) and SUN_KEYBOARD_HACK option is enabled, keyboardhackgetfn returns the backtick character even though the parameter reports itself as unset. This means echo $KEYBOARD_HACK shows the character while typeset doesn't list it.
• Recommendation: This is intentional per the architecture decision — the option handler sets keyboardhackchar directly (options.c:827) and the special parameter's PM_UNSET state is independent. The same pattern exists for HOME and other PM_UNSET params in zsh. Accept as-is.
• Confidence: High

🔶 NON-BLOCKING — SUN_KEYBOARD_HACK option bypasses PM_UNSET toggle

• Type: Verified issue
• Evidence: Src/options.c:827
• Why it matters: Enabling SUN_KEYBOARD_HACK sets keyboardhackchar = '\'directly without touchingpm->node.flags. This means the hack becomes active but the parameter stays hidden. If a user later assigns KEYBOARD_HACK=x, the parameter becomes visible. If they then disable the option, keyboardhackchar` is cleared but PM_UNSET might remain cleared. This creates an asymmetric state.
• Recommendation: The current behavior is acceptable for the stated goal (hide from typeset/export) but could confuse users who toggle the option after assigning the parameter. Accept as-is.
• Confidence: Medium

💡 NIT — Memory leak fix is correct (pre-existing)

• Type: Verified issue
• Evidence: Src/params.c:4764-4767
• Why it matters: The pre-existing code returned on ASCII validation failure without freeing x. The fix adds free(x) before the early return. All other return paths handle it correctly.
• Recommendation: Fix verified as correct. No further action needed.
• Confidence: High

💡 NIT — PM_UNSET set twice on unset path (idempotent)

• Type: Verified issue
• Evidence: Src/params.c:3724, Src/params.c:4777
• Why it matters: stdunsetfn calls keyboardhacksetfn(pm, NULL), which sets pm->node.flags |= PM_UNSET (line 4777). Then stdunsetfn also sets pm->node.flags |= PM_UNSET (line 3724) after the setfn returns. Harmless (idempotent OR) but redundant.
• Recommendation: No change needed — the redundancy protects against future refactors.
• Confidence: High

---

What I could not fully verify

• Whether any external zsh plugin or script depends on detecting $KEYBOARD_HACK being set before the user explicitly assigns it. Since issue KEYBOARD_HACK special parameter is unnecessary legacy pollution #9 reports this as pollution, such a dependency would be a user error.
• Test suite execution — this repo has no TESTING.md or CI configuration visible in the diff context.

---

Final verdict

✅ Ready to merge — no blocking issues. 4 findings (0 blocking, 2 non-blocking, 2 nits).

---

Review generated from main..HEAD on fix/keyboard-hack-parameter. Full review artefact: REVIEW-2026-05-25T18:54:46+07:00.md.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the KEYBOARD_HACK special parameter handling so its unset/set state is tracked consistently via PM_UNSET.

Changes:

• Initialize KEYBOARD_HACK with PM_UNSET in the parameter definition.
• Update keyboardhacksetfn to toggle PM_UNSET based on whether the resulting value is empty/unset.
• Fix memory management in the non-ASCII validation path by freeing the incoming value before returning.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[HaleTom]

Issues found and addressed during development:

1. Options handler bug (commit 5f1d2a4)

• Issue: c88e600 changed keyboardhackchar = (value ? '\' : '\0')toif (value) keyboardhackchar = '`', which meant unsetting SUN_KEYBOARD_HACK` no longer cleared the hack character — the hack remained active after the option was disabled.
• Fix: Restored the original handler. With PM_UNSET, the IPDEF2 flags already handle hiding the parameter; the option handler should behave correctly (set on enable, clear on disable).

2. Memory leak (commit a8ec582)

• Issue: keyboardhacksetfn returned early on ASCII validation failure without freeing x.
• Fix: Added free(x) before return on the validation error path.

3. Design evolution (commits 65cdcfa → a8ec582)

• Issue: Fully deleting the KEYBOARD_HACK special parameter registration was unnecessarily destructive — broke backward compatibility for any user who inspects or assigns the parameter.
• Fix: Keep the parameter but start it hidden via PM_DONTIMPORT | PM_UNSET. The setter toggles PM_UNSET based on value: non-empty → visible, empty/unset → hidden. The SUN_KEYBOARD_HACK option works independently via the raw keyboardhackchar variable.

4. Code clarity (commit a8ec582)

• Issue: UNUSED(Param pm) was incorrect once we started accessing pm->node.flags — the parameter is now actively used.
• Fix: Removed UNUSED wrapper.

5. PM_UNSET double-set on unset path (NIT, no change needed)

• stdunsetfn calls keyboardhacksetfn(pm, NULL) which sets PM_UNSET, then stdunsetfn also sets it afterwards. Idempotent OR — harmless and protectively redundant.

Review verdict

✅ Ready to merge — no blocking issues. All findings addressed or acknowledged.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

Src/params.c:1

• keyboardhacksetfn silently ignores all but the first character (x[0]). If KEYBOARD_HACK is intended to be a single ASCII character, it would be clearer to validate len <= 1 and warn/reject when extra characters are provided (or explicitly document that only the first character is used). This avoids confusing behavior like KEYBOARD_HACK=abc being treated as a without any feedback.

/*

[HaleTom]

Re: Copilot review #4361918200 — suppressed low-confidence observation about multi-character values:

The concern is already addressed. At Src/params.c:4760:

if (len > 1) {
    len = 1;
    zwarn("Only one KEYBOARD_HACK character can be defined");
}

When KEYBOARD_HACK=abc is assigned, the setter warns and truncates to just the first character (a). The comment /* could be changed if needed */ marks the extension point if multi-character support were ever desired.