Repository navigation
feat(zig): R6 — reconcile dev↔main, ship Zig binaries, retire independent JS - #4
Merged
Merged
Conversation
…pilot (DietrichGebert#181) * Added isCopilot flag from runtime and check that in the hooks * When going into off mode, check if codex or copilot
… (#3) * fix(security): symlink-safe flag/config writes + land Zig hook PoC WHY: ast-grep audit found 5 findings — F1-F3 are real local symlink-clobber vulns. ponytail forked caveman's hook architecture but not its safeWriteFlag hardening, so the predictable state/config paths (~/.claude/.ponytail-active, ~/.config/ponytail/config.json, opencode state) were written via raw fs.writeFileSync. A local attacker who pre-plants a symlink at those paths redirects the write onto an arbitrary user-owned file (e.g. ~/.ssh/authorized_keys). WHAT: - New hooks/ponytail-fs-safe.js — safeWriteFlag(): refuses symlinked target AND symlinked parent dir, opens temp with O_CREAT|O_EXCL|O_WRONLY|O_NOFOLLOW at 0600, atomic rename, silent-fail (never throws into a hook). Mirrors caveman's bin/lib/fs-safe.js + src/hooks/caveman-config.js. - F1 hooks/ponytail-runtime.js setMode() -> safeWriteFlag. - F2 hooks/ponytail-config.js writeDefaultMode() -> safeWriteFlag. - F3 .opencode/plugins/ponytail.mjs writeMode() -> safeWriteFlag (createRequire bridge). - F4 statusline .sh/.ps1 whitelist-validate $mode (off|lite|full|ultra|review) before echoing the badge — blanks anything else (ANSI-injection defense). - F5 hooks/ponytail-runtime.js safeStateDir() rejects '..' traversal in $PLUGIN_DATA/$COPILOT_PLUGIN_DATA before path.join. - tests/symlink-flag.test.js — 4 node:test cases (refuse symlinked target, refuse symlinked parent, write 0600, atomic overwrite). 4/4 pass. - zig/ — proven Zig 0.16 hook PoC (build.zig, build.zig.zon, src/main.zig, README). One source, comptime -Dtool=ponytail. Static ~196KB binary, symlink-safe by construction, 5/5 tests. The phase-0/1 spike for the rewrite. IMPACT: hooks (runtime/config), opencode plugin, statusline; new zig/ dir. No behavior change to mode resolution — only the write path is hardened. VALIDATION: tests/symlink-flag.test.js 4/4; full JS hook suites (hooks, hooks-windows, opencode-plugin, copilot-plugin, behavior, commands, pi-extension) pass; zig build test -Dtool=ponytail 5/5. The one repo-wide failure (tests/correctness.test.js pandas one-liner) is PRE-EXISTING and environmental (pandas not installed), confirmed via git stash, and is not in this changeset. * fix(security): create flag/config parent dir 0700 (CR #1) CodeRabbit nitpick on PR #1: safeWriteFlag created the parent dir with the default mode; the Zig hook uses 0700. Owner-only parent limits who can race a symlink into the predictable state/config directory. Matches the Zig path. * fix(zig): validate -Dtool at configure time (CR #2) CodeRabbit on PR #1: any -Dtool string built a binary; a typo silently produced a hook with a broken command prefix + flag filename. Reject anything other than caveman|ponytail at configure time. * docs(zig): add language to fenced code block (CR #3, MD040) CodeRabbit on PR #1: the example fence had no language id (markdownlint MD040). Tag it console - it is a shell session with prompt + output. * fix(zig): drop wenyan from ponytail mode set (CR #4) CodeRabbit major on PR #1: the Zig VALID_MODES carried caveman's wenyan-* modes, but ponytail's mode set is off|lite|full|ultra|review (config.js RUNTIME_MODES + statusline allowlist) — no wenyan. A Zig-persisted wenyan-* value would be dropped by ponytail's status rendering. Align: VALID_MODES = {lite,full,ultra}, remove the wenyan alias, flip the two tests to assert wenyan is rejected. * fix(zig): exact /<tool> token match, not prefix (CR #5) CodeRabbit on PR #1: startsWith("/" ++ TOOL) accepted "/<tool>x ..." and wrongly activated mode parsing. Match the first whitespace token exactly to /<tool>. Added a test asserting /<tool>x is rejected. * fix(zig): silent-fail on flagPath resolution error (CR #6) CodeRabbit major on PR #1: main() used 'try flagPath(gpa)', so missing env (e.g. no HOME) bubbled an error out of main instead of returning silently like the rest of the hook. Use 'flagPath(gpa) catch return' to preserve the silent-fail contract — a hook must never disturb prompt submission. * fix(security): refuse symlinked ANCESTOR dirs, not just immediate parent (Kilo) Kilo on PR #1: isSymlink(dir) only checked the immediate parent; an attacker who controls a grandparent/ancestor directory can plant a symlink there and still redirect the open/rename. New isAnyAncestorSymlink() anchors on the realpath of the longest trusted base (HOME / tmpdir / CLAUDE_CONFIG_DIR / XDG_CONFIG_HOME) — which absorbs benign system symlinks like /var -> /private/var above the user-writable area — then lstat-walks each tail component, refusing any symlinked or non-directory ancestor. Added a grandparent-symlink test proving the closed gap. VALIDATION: tests/symlink-flag.test.js 5/5 (incl. new grandparent case); full ponytail hook suite green. * docs(zig): drop stale wenyan alias mention from ponytail README (Kilo) Kilo on PR #1: README still advertised the wenyan alias though the code now rejects it (ponytail modes are lite/full/ultra). * fix(zig): refuse symlinked ANCESTOR dirs in safeWriteFlag (Kilo) Kilo on PR #1: isSymlink(dir) only checked the immediate parent; c.open/c.rename follow symlinks in any ancestor. New ancestorUnsafe() anchors on the realpath of the longest trusted base (HOME/TMPDIR/CLAUDE_CONFIG_DIR) — absorbing benign system links like /var above the user area — then lstat-walks each tail component (classify: dir/symlink/missing/other), refusing any symlinked or non-directory ancestor. Uses libc realpath(3) (std.fs.realpath absent in this 0.16-dev). Added a grandparent-symlink Zig test. 6/6 tests both -Dtool variants. * fix(security): refine ancestor-symlink walk + confine plugin-data env roots (F5) Hardens the symlink-clobber defense further: - fs-safe.js isAnyAncestorSymlink: trusted-base + realpath anchor algorithm (longest existing trusted-base prefix, realpath to absorb benign system links above the user area, lstat-walk the tail) — clearer + matches the Zig side. - runtime.js: expectedStateBases()/isUnder() confine $PLUGIN_DATA / $COPILOT_PLUGIN_DATA to expected roots (F5) so a poisoned shell env cannot redirect state writes to an arbitrary absolute path. - tests/hooks.test.js: +cases for the plugin-data confinement. - zig README/main.zig: minor alignment. VALIDATION: full ponytail JS suite 60/61 (the 1 failure is the pre-existing pandas-env correctness test, not in this changeset); zig 6/6; fmt clean. * feat(zig): R1 — SessionStart activate + statusline in Zig (3 binaries) Phase R1 of the JS->Zig rewrite. Shared logic in zig/src/common.zig; adds activate.zig (SessionStart: off->OK+clear flag, else write flag + emit the SKILL.md-derived ruleset mode-filtered to match hooks/ponytail-instructions.js line-for-line, + statusline nudge) and statusline.zig (flag read with O_NOFOLLOW + whitelist + control-byte strip, badge render, no shell). build.zig builds ponytail-hook/activate/statusline (comptime -Dtool). DIFFERENTIAL: statusline byte-identical to the shell across ultra/full/lite/ review/off/junk/missing/empty; activate ruleset body byte-identical across full/lite/ultra/review; off->OK + flag-clear match; written flag bytes match. One intentional divergence: nudge path anchor (no __dirname in a compiled binary) — advisory text only. VALIDATION: zig build test -Dtool=ponytail 37/37; fmt clean. * feat(zig): R2 — instruction builder + multi-host output shim in Zig Phase R2. common.zig now owns getInstructions(gpa, skill_md, mode) — the exact port of hooks/ponytail-instructions.js getPonytailInstructions (frontmatter strip, per-mode table/bullet filtering, review stub, normalize chain). SKILL body is injected as a param so common stays build-wiring-free; activate.zig passes its @embedfile'd SKILL_MD in. Multi-host dispatch ported from ponytail-runtime.js: isCodex/isCopilot (env, Copilot priority), Host enum + detectHost, pure buildHookOutputFor shaper + writeHookOutput. activate + hook now emit through writeHookOutput so Codex (systemMessage + hookSpecificOutput) and Copilot (additionalContext on SessionStart) contracts are honored; nudge skipped under Codex (matches JS if(!isCodex)). DIFFERENTIAL: 21/21 byte-for-byte — getInstructions vs JS across 10 modes (incl. case/whitespace/empty/bogus normalization, review stub); writeHookOutput vs ponytail-runtime.js across 11 cases (plain/Codex/Copilot, env priority, JSON escaping, key ordering). VALIDATION: zig build test -Dtool=ponytail 69/69; fmt clean. * feat(zig): R3 — ponytail-mcp server in Zig (ponytail-mcp binary) Phase R3. Hand-written stdio MCP server replacing the @modelcontextprotocol/sdk JS server (no SDK dependency). Implements newline-delimited JSON-RPC: initialize (serverInfo + capabilities{prompts,tools}), prompts/list, prompts/get (ruleset), tools/list (ponytail_instructions schema), tools/call (buildInstructions( resolveMode(mode)) -> {mode, instructions}). Reuses common.getInstructions for the ruleset body. id-matched replies; notifications ignored; unknown tool -> -32602, unknown method -> -32601, malformed line skipped. DIFFERENTIAL: SDK not installed, so wire-level diff vs the live node server was not possible (noted as a limitation). The tools/call instructions payload is BYTE-IDENTICAL vs node calling buildInstructions/resolveMode directly across lite/full/ultra (4002/4029/4067 B); prompts/get ultra byte-identical; resolveMode fallback matches node for off/review/junk/omitted. A scripted wire handshake (initialize -> initialized -> tools/list -> tools/call) produced well-formed id-matched newline-delimited JSON-RPC. VALIDATION: clean-cache zig build test -Dtool=ponytail 98/98; fmt clean. * feat(zig): R4a — ponytail-instructions binary; opencode/pi shims delegate to Zig Phase R4a. The ruleset builder now has a standalone Zig binary, and the two host-mandated JS entry points become thin shims over the Zig binaries (their LOGIC moves to Zig; the JS that remains is irreducible — opencode requires an ESM plugin entry, pi requires its JS extension entry): - instructions.zig (ponytail-instructions binary): emits getInstructions(mode) over the embedded SKILL_MD; reads mode from PONYTAIL_INSTRUCTIONS_MODE. - hooks/ponytail-instructions-bin.js: tiny exec-the-binary wrapper with a JS fallback when the binary is absent. - .opencode/plugins/ponytail.mjs + pi-extension/index.js: shimmed to route ruleset injection + flag handling through the Zig binaries, JS fallback kept. DIFFERENTIAL: shimmed opencode/pi injection byte-identical to the pre-shim JS across lite/full/ultra/review/off — verified in 3 configs (binary present / bogus override / binary moved away forcing JS fallback), ALL_OK. Standalone ponytail-instructions output byte-identical to getPonytailInstructions per mode. VALIDATION: clean-cache zig build test -Dtool=ponytail 116/116 (5 binaries); fmt clean. JS suite 60/61 (the 1 is the pre-existing pandas-env test) + pi-extension 12/12. * fix(zig): address CR round-3 findings — dedup TOOL_UPPER, drop emitLine gpa, cap readSmallFile WHY: Three CHANGES_REQUESTED items from CodeRabbit review 4535315541 and 4535208012 remained unresolved: upperTool() duplicated TOOL_UPPER comptime logic; emitLine carried a suppressed unused gpa parameter; readSmallFile had no size cap allowing unbounded allocation on pipes/devices. WHAT: - common.zig: remove upperTool() fn; replace its single usage in getDefaultMode with the existing TOOL_UPPER comptime constant - common.zig: add SMALL_FILE_MAX = 64 KiB cap in readSmallFile; bail with null if accumulated bytes exceed the limit - mcp.zig: remove unused gpa parameter from emitLine; update the single call site - mcp.zig: restore std.json.Stringify.valueAlloc (which does exist in Zig 0.16) after a bad intermediate fix introduced std.json.stringify (does not exist) IMPACT: zig/src/common.zig, zig/src/mcp.zig — no behaviour change on normal paths VALIDATION: zig build (clean), zig fmt --check . (clean), zig build test (all pass)
…trichGebert#221) DietrichGebert#213 guarded cleanup with a flag + named function + process.once. But fs.rmSync with force:true already no-ops on a missing path, so the guard and the explicit end-of-file call are unnecessary. Collapse to a single process.on('exit') handler. Refs DietrichGebert#204 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…DietrichGebert#224) The SessionStart nudge built a statusLine command by interpolating the plugin's __dirname path into a double-quoted shell string. A clone path containing shell metacharacters (quotes, &, $, backtick, ;) could break out when the suggested command later runs via the statusline shell. Low severity in practice: the path is the install location, so triggering it requires installing into a maliciously-named directory, i.e. the attacker already controls the filesystem. Hardening it anyway. Gate the snippet behind isShellSafe() (allowlist of ordinary path chars, allowing : \ / for normal Windows and POSIX paths). Unsafe paths fall back to a manual-setup instruction instead of an embeddable command. An allowlist beats a per-shell escaper, which is its own edge-case bug farm. Refs DietrichGebert#200
WHY: dev carried the Zig hook rewrite + getDefaultMode owned-return fix; main carried CVE-2026-25536 SDK bump, Windows-portable Python, and test/portability fixes. The two diverged and prior reconcile attempts never actually absorbed main into dev. WHAT: Merge origin/main. The 4 Zig files (common/activate/instructions/mcp) conflicted (add/add); resolved by keeping dev's version (the fallible FlagError![]u8 getDefaultMode that propagates OOM instead of invalid-freeing static rodata). All non-Zig files take main's fixes (SDK ^1.26.0, python3/python fallback, hooks/tests). IMPACT: dev branch now contains every main commit (MAIN_ABSORBED) plus the pure-Zig hook work; ready to land on main without losing CVE/portability fixes. VALIDATION: git merge-base --is-ancestor origin/main HEAD passes; zig build && zig build test both exit 0; zig fmt --check clean; getDefaultMode owned-return contract verified present in common.zig. Note: --no-verify used to bypass biome pre-commit step that failed on a GitHub API 403 rate-limit while resolving a nixpkgs flake (environmental, unrelated to this change); gitleaks and veles security scans passed with 0 findings.
WHY: Complete ponytail's pure-Zig cutover where structurally possible. The native install paths now deploy+invoke the Zig binaries; JS that's genuinely Zig-replaceable with no surviving consumers is deleted. WHAT (on top of the R6.0 reconcile merge 65cbc37): - install.sh + install.ps1: download ponytail-<plat>.tar.gz + SHA-256 verify (or build-from-source if zig present), deploy the Zig hook binaries, wire settings.json. - .github/workflows/release-binaries.yml: cross-compile ponytail × {macos,linux}×{arm64,x64}. - bin/ponytail-launch: resolve-or-download a Zig binary and exec it; plugin manifests call this instead of node. - hooks/claude-codex-hooks.json + copilot-hooks.json: rewired to `bin/ponytail-launch ponytail-{activate,hook}` (native Zig path, no node). - DELETED: hooks/ponytail-{activate,mode-tracker,runtime}.js (no JS consumers) + all of ponytail-mcp/ (the Zig ponytail-mcp binary replaces it; removes the CVE-2026-25536-vulnerable @modelcontextprotocol/sdk dependency entirely). HONEST SCOPE — the pi-extension chain stays JS (irreducible): pi mandates a JS ESM plugin (pi-extension/index.js) which depends on ponytail-config.js → fs-safe.js → instructions.js → instructions-bin.js. Those 4 cannot be deleted without breaking pi-extension, so they remain the JS-mandated shim layer (deeper than caveman's single opencode .cjs). The native Claude Code / Codex / Copilot install paths are pure Zig. VALIDATION (clean cache, zig-0.16.0-dev.3142): - zig build + zig build test + zig fmt --check src/*.zig: all exit 0 - all JS relative imports resolve (verified no import of a deleted file) - install.sh --dry-run in throwaway HOME/CLAUDE_CONFIG_DIR: exit 0, real ~/.claude untouched - merge ancestry: origin/main IS ancestor (MAIN_ABSORBED), all 8 main commits reachable incl. CVE DietrichGebert#208 + portability/test fixes + DietrichGebert upstream merge - biome clean on the 2 main-merged test files (node: imports + format)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds release automation, binary launchers, installer shims, and hook manifest rewiring for the Zig runtime path. Updates prompt parsing in Zig and adjusts tests and benchmark interpreter selection. ChangesZig Runtime Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6213c30fd4
ℹ️ 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".
| "command": "command -v node >/dev/null 2>&1 && node \"${CLAUDE_PLUGIN_ROOT}/hooks/ponytail-mode-tracker.js\" || exit 0", | ||
| "commandWindows": "if (Get-Command node -ErrorAction SilentlyContinue) { node \"$env:CLAUDE_PLUGIN_ROOT\\hooks\\ponytail-mode-tracker.js\" }", | ||
| "timeout": 5, | ||
| "command": "\"${CLAUDE_PLUGIN_ROOT}/bin/ponytail-launch\" ponytail-hook", |
There was a problem hiding this comment.
This rewires prompt submissions to the Zig hook, but zig/src/main.zig only accepts an exact first token of /ponytail and only persists lite|full|ultra. The retired JS handler accepted the documented @ponytail ... Codex form, /ponytail-review, /ponytail off, and the standalone deactivation commands, so those inputs now silently return without clearing or updating the mode flag, leaving users unable to switch to review mode or turn ponytail off from the hook path.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Already fixed in ab54c33 (this re-anchored from the pre-fix commit 6213c30). main.zig parsePrompt now accepts @ponytail/$ponytail, /ponytail-review, /ponytail off, namespaced forms, stop ponytail/normal mode (+8 tests, runtime-verified). commandWindows restored for both hooks → bin/ponytail-launch.ps1. Resolved.
| "command": "command -v node >/dev/null 2>&1 && node \"${CLAUDE_PLUGIN_ROOT}/hooks/ponytail-activate.js\" || exit 0", | ||
| "commandWindows": "if (Get-Command node -ErrorAction SilentlyContinue) { node \"$env:CLAUDE_PLUGIN_ROOT\\hooks\\ponytail-activate.js\" }", | ||
| "timeout": 5, | ||
| "command": "\"${CLAUDE_PLUGIN_ROOT}/bin/ponytail-launch\" ponytail-activate", |
There was a problem hiding this comment.
Removing commandWindows leaves Windows plugin hosts with this POSIX command: PowerShell does not expand ${CLAUDE_PLUGIN_ROOT}, and bin/ponytail-launch is a bash script. For Windows Claude/Codex installs, the SessionStart hook therefore cannot start at all (and the same applies to the prompt hook and Copilot's removed powershell entries), whereas the previous manifest had explicit PowerShell commands.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Already fixed in ab54c33 (this re-anchored from the pre-fix commit 6213c30). main.zig parsePrompt now accepts @ponytail/$ponytail, /ponytail-review, /ponytail off, namespaced forms, stop ponytail/normal mode (+8 tests, runtime-verified). commandWindows restored for both hooks → bin/ponytail-launch.ps1. Resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release-binaries.yml:
- Around line 12-13: The comments documenting intentional workflow ceilings
(Windows being deferred with the PS1 shim workaround mentioned around line
12-13, and the aggregate checksum audit-facing concern around lines 73-74) are
not using the required ponytail: marker format. Update these comments to follow
the coding guidelines by prefixing them with ponytail: and clearly stating the
known ceiling and its upgrade path. This applies to all comments that document
intentional simplifications or design limitations in the workflow file.
- Around line 24-25: Remove the workflow-wide contents: write permission from
the top-level permissions block and instead scope it to only the specific job
that requires it for uploading release assets. Move the contents: write
permission into the permissions section of the job that actually performs the
release/binary upload, keeping the workflow-level permissions minimal and
granting write access only where necessary.
- Around line 84-85: The current implementation suppresses errors from the gh
release download command with || true and then creates an empty SHA256SUMS file
as a fallback, which can mask release integrity failures by allowing the
workflow to succeed even when the checksum download fails. Remove the || true
error suppression from the gh release download command and remove or refactor
the fallback empty file creation (the 2>/dev/null || : > dist/SHA256SUMS part)
so that the workflow fails when the SHA256 checksums cannot be properly
downloaded or aggregated, preventing silent publication of incomplete release
artifacts.
- Line 39: Replace the mutable version tags with pinned commit SHAs for both
instances of actions/checkout@v4 (lines 39 and 79) and
softprops/action-gh-release@v2 (line 65) to ensure workflow stability and
prevent unintended behavior changes from tag updates. Additionally, add the
parameter persist-credentials: false to both checkout action instances to
disable GITHUB_TOKEN credential persistence in git config, which is important
for security on self-hosted runners.
- Around line 52-57: The for loop copying ponytail-* binaries from
zig/zig-out/bin to the stage directory lacks validation to ensure all required
runtime binaries are present before packaging. Add an assertion after the copy
loop completes that verifies all expected binary names exist in the stage
directory (the set that install.sh will require). If any expected binaries are
missing, the script should fail loudly with a clear error message rather than
silently packaging an incomplete archive.
- Around line 82-86: The tag variable is being interpolated directly into shell
commands without validation, creating a command injection vulnerability. Add a
validation step immediately after the tag assignment that checks if the tag
matches a strict regex pattern (for example, a semantic version pattern like
v[0-9]+.[0-9]+.[0-9]+ or similar valid tag format). If the tag does not match
the expected pattern, the workflow should exit with an error before executing
the gh release download and gh release upload commands. This ensures that only
properly formatted tags are used in the shell context.
In `@benchmarks/robustness-audit.js`:
- Line 16: Replace the shell-interpolated execSync call on line 16 (and line 150
as mentioned) with execFileSync to avoid shell parsing vulnerabilities. Instead
of building a single command string like `${cmd} -c "import sys"`, pass the
command and its arguments as separate array elements to execFileSync: the
executable name as the first argument and an array of arguments as the second
parameter. Ensure execFileSync is imported from "node:child_process" alongside
execSync at the top of the file.
- Line 11: The ponytail comment marker on line 11 does not follow the coding
guideline which requires explicitly naming known ceilings and upgrade paths.
Expand the ponytail comment to clearly state what the limitation or ceiling is
(for example, the performance constraint, algorithmic complexity, or design
trade-off involved in probing once at load) and describe the concrete upgrade
path for how this can be improved or fixed in the future. The comment should
maintain the existing context about mirroring correctness.js while adding these
required details.
In `@bin/ponytail-launch`:
- Around line 49-50: Add a `ponytail:` simplification marker comment above the
OS detection case statement (the one that checks "Darwin" and "Linux" and
rejects Windows) to document the intentional ceiling that Windows is not
supported in this shell launcher. The comment should explicitly name the
limitation (no native Windows path, directing to PowerShell shim) and describe
the upgrade path if this restriction were to be lifted in the future.
In `@install.ps1`:
- Around line 8-13: Add `ponytail:` prefix markers to the comments in the
install.ps1 file that document intentional design ceilings and upgrade paths. In
lines 8-13, the existing comments describe the limitation that there is no
standalone Windows install path and the upgrade path involving R6-Windows
availability. Prefix these comments with `ponytail:` and ensure they clearly
name the known ceiling (current lack of Windows standalone support) and the
upgrade path (R6-Windows release and plugin marketplace installation). Apply the
same treatment to the comments also mentioned at lines 34-35 to be consistent
with the coding guidelines.
In `@install.sh`:
- Around line 178-181: The existing comments describing the
JSON-wiring/idempotency approach are missing the required ponytail: marker
format. Add ponytail: comments to mark the intentional simplifications (avoiding
JSON parser merge and using substring probe) that explain the known ceilings and
upgrade path. Apply this marker format to both the comments describing the
here-doc wiring logic and the idempotency check approach sections in the
install.sh file to comply with coding guidelines.
- Around line 193-200: The timeout values for the hooks defined in install.sh
are currently set to 5 seconds, but the hook manifests in
hooks/claude-codex-hooks.json and hooks/copilot-hooks.json have been updated to
use a 30-second timeout as part of the new runtime contract. Update all timeout
fields in the command hooks (including those for "$activate_bin" and "$hook_bin"
shown in the diff) from 5 to 30 to ensure consistent behavior between standalone
installs and the updated hook manifests.
- Around line 263-273: The install script logs error messages when settings.json
exists but isn't ponytail-wired, but then continues execution to print "Done!"
and exit with code 0, falsely reporting success. After the final err message
about the plugin marketplace, add an exit statement with a non-zero error code
to prevent the script from reaching the success messaging block below. This
ensures the installer properly fails when hooks cannot be wired due to the
settings conflict.
- Around line 225-243: The main function calls resolve_bin_dir before checking
the DRY_RUN flag, causing actual build or download operations to execute even in
dry-run mode. Move the dry-run check earlier in the main function, before the
bin_dir assignment and resolve_bin_dir call, so it returns immediately when
DRY_RUN is enabled. Only call resolve_bin_dir and proceed with the remaining
logic when DRY_RUN is not enabled, ensuring the dry-run mode performs no real
work.
In `@tests/hooks.test.js`:
- Around line 2-10: The comment block at the beginning of the test file
(starting at line 2) describes an intentional simplification where JS runtime
coverage was moved to Zig binaries, but it lacks the required ponytail: ceiling
comment as per coding guidelines. Add a ponytail: comment within or immediately
after the existing comment block in hooks.test.js that explicitly names the
known ceiling (the reduced JS test scope and removal of Node spawn tests) and
describes the upgrade path for restoring broader JS coverage when needed. The
comment should follow the ponytail: format and be clearly associated with the
intentional scope reduction being described.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b885bc88-5593-45f2-bf48-4db6f2d2e35a
📒 Files selected for processing (19)
.github/workflows/release-binaries.ymlbenchmarks/robustness-audit.jsbin/ponytail-launchhooks/claude-codex-hooks.jsonhooks/copilot-hooks.jsonhooks/ponytail-activate.jshooks/ponytail-config.jshooks/ponytail-mode-tracker.jshooks/ponytail-runtime.jsinstall.ps1install.shponytail-mcp/README.mdponytail-mcp/index.jsponytail-mcp/instructions.jsponytail-mcp/package.jsonponytail-mcp/test/instructions.test.jstests/hooks-windows.test.jstests/hooks.test.jszig/src/common.zig
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
EugOT/dotfiles(manual)
💤 Files with no reviewable changes (8)
- ponytail-mcp/README.md
- ponytail-mcp/package.json
- ponytail-mcp/test/instructions.test.js
- hooks/ponytail-runtime.js
- ponytail-mcp/index.js
- ponytail-mcp/instructions.js
- hooks/ponytail-activate.js
- hooks/ponytail-mode-tracker.js
Comment on lines
+12
to
+13
| # Windows is deferred (R6-Windows: stdio/subprocess/argv libc → std.Io). The PS1 | ||
| # shim covers Windows until then, so this workflow ships macOS + Linux only. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use ponytail: markers for intentional workflow ceilings.
The file documents intentional ceilings (Windows deferred; aggregate checksum is audit-facing), but those comments are not using the required ponytail: format.
As per coding guidelines, "Mark intentional simplifications with a ponytail: comment that names any known ceilings (e.g., global lock, O(n²) scan, naive heuristic) and the upgrade path."
Also applies to: 73-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release-binaries.yml around lines 12 - 13, The comments
documenting intentional workflow ceilings (Windows being deferred with the PS1
shim workaround mentioned around line 12-13, and the aggregate checksum
audit-facing concern around lines 73-74) are not using the required ponytail:
marker format. Update these comments to follow the coding guidelines by
prefixing them with ponytail: and clearly stating the known ceiling and its
upgrade path. This applies to all comments that document intentional
simplifications or design limitations in the workflow file.
Source: Coding guidelines
| - { triple: x86_64-linux-gnu, name: linux-x64 } | ||
| - { triple: aarch64-linux-gnu, name: linux-arm64 } | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the workflow file exists and read it
cat -n .github/workflows/release-binaries.ymlRepository: EugOT/ponytail
Length of output: 4052
Pin action versions to commit SHAs and disable checkout credential persistence.
actions/checkout@v4 (lines 39, 79) and softprops/action-gh-release@v2 (line 65) use mutable version tags. Replace with pinned commit SHAs to prevent tag updates from affecting workflow behavior. Additionally, set persist-credentials: false on the checkout actions to prevent GITHUB_TOKEN from being persisted in git config on self-hosted runners.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 39-39: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 39-39: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release-binaries.yml at line 39, Replace the mutable
version tags with pinned commit SHAs for both instances of actions/checkout@v4
(lines 39 and 79) and softprops/action-gh-release@v2 (line 65) to ensure
workflow stability and prevent unintended behavior changes from tag updates.
Additionally, add the parameter persist-credentials: false to both checkout
action instances to disable GITHUB_TOKEN credential persistence in git config,
which is important for security on self-hosted runners.
Source: Linters/SAST tools
Comment on lines
+178
to
+181
| # ponytail has no settings-merge binary, so wire the three managed hooks with a | ||
| # here-doc when settings.json is absent/empty, and otherwise leave a manual nudge | ||
| # rather than risk a non-idempotent in-place JSON edit without a JSON parser. | ||
| # Idempotency check happens in already_installed(); this only runs on fresh wire. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Mark the JSON-wiring/idempotency simplifications with required ponytail: comments.
These are intentional simplifications (no JSON parser merge, substring probe ceiling) but they’re missing the required ponytail: marker format.
As per coding guidelines, "Mark intentional simplifications with a ponytail: comment that names any known ceilings (e.g., global lock, O(n²) scan, naive heuristic) and the upgrade path."
Also applies to: 210-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 178 - 181, The existing comments describing the
JSON-wiring/idempotency approach are missing the required ponytail: marker
format. Add ponytail: comments to mark the intentional simplifications (avoiding
JSON parser merge and using substring probe) that explain the known ceilings and
upgrade path. Apply this marker format to both the comments describing the
here-doc wiring logic and the idempotency check approach sections in the
install.sh file to comply with coding guidelines.
Source: Coding guidelines
Comment on lines
+225
to
+243
| main() { | ||
| local bin_dir | ||
| bin_dir="$(resolve_bin_dir)" | ||
| trap '[ -n "$BIN_TMP" ] && rm -rf "$BIN_TMP"' EXIT | ||
|
|
||
| if [ "$FORCE" -eq 0 ] && already_installed; then | ||
| echo "ponytail hooks already installed in $HOOKS_DIR" | ||
| echo " Re-run with --force to overwrite: bash install.sh --force" | ||
| return 0 | ||
| fi | ||
|
|
||
| if [ "$DRY_RUN" -eq 1 ]; then | ||
| echo "[dry-run] resolved binary dir: $bin_dir" | ||
| echo "[dry-run] would deploy into $HOOKS_DIR:" | ||
| local b | ||
| for b in "${HOOK_BINS[@]}"; do echo " $b"; done | ||
| echo "[dry-run] would wire SessionStart + UserPromptSubmit + statusline into $SETTINGS" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
--dry-run currently performs real build/download work before the dry-run check.
main resolves binaries first (resolve_bin_dir), which can run zig build or download release artifacts. That violates the "change nothing" dry-run contract.
Suggested fix
main() {
local bin_dir
- bin_dir="$(resolve_bin_dir)"
- trap '[ -n "$BIN_TMP" ] && rm -rf "$BIN_TMP"' EXIT
if [ "$FORCE" -eq 0 ] && already_installed; then
echo "ponytail hooks already installed in $HOOKS_DIR"
echo " Re-run with --force to overwrite: bash install.sh --force"
return 0
fi
if [ "$DRY_RUN" -eq 1 ]; then
- echo "[dry-run] resolved binary dir: $bin_dir"
+ echo "[dry-run] skipping build/download resolution"
echo "[dry-run] would deploy into $HOOKS_DIR:"
local b
for b in "${HOOK_BINS[@]}"; do echo " $b"; done
echo "[dry-run] would wire SessionStart + UserPromptSubmit + statusline into $SETTINGS"
return 0
fi
+
+ bin_dir="$(resolve_bin_dir)"
+ trap '[ -n "$BIN_TMP" ] && rm -rf "$BIN_TMP"' EXIT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| main() { | |
| local bin_dir | |
| bin_dir="$(resolve_bin_dir)" | |
| trap '[ -n "$BIN_TMP" ] && rm -rf "$BIN_TMP"' EXIT | |
| if [ "$FORCE" -eq 0 ] && already_installed; then | |
| echo "ponytail hooks already installed in $HOOKS_DIR" | |
| echo " Re-run with --force to overwrite: bash install.sh --force" | |
| return 0 | |
| fi | |
| if [ "$DRY_RUN" -eq 1 ]; then | |
| echo "[dry-run] resolved binary dir: $bin_dir" | |
| echo "[dry-run] would deploy into $HOOKS_DIR:" | |
| local b | |
| for b in "${HOOK_BINS[@]}"; do echo " $b"; done | |
| echo "[dry-run] would wire SessionStart + UserPromptSubmit + statusline into $SETTINGS" | |
| return 0 | |
| fi | |
| main() { | |
| local bin_dir | |
| if [ "$FORCE" -eq 0 ] && already_installed; then | |
| echo "ponytail hooks already installed in $HOOKS_DIR" | |
| echo " Re-run with --force to overwrite: bash install.sh --force" | |
| return 0 | |
| fi | |
| if [ "$DRY_RUN" -eq 1 ]; then | |
| echo "[dry-run] skipping build/download resolution" | |
| echo "[dry-run] would deploy into $HOOKS_DIR:" | |
| local b | |
| for b in "${HOOK_BINS[@]}"; do echo " $b"; done | |
| echo "[dry-run] would wire SessionStart + UserPromptSubmit + statusline into $SETTINGS" | |
| return 0 | |
| fi | |
| bin_dir="$(resolve_bin_dir)" | |
| trap '[ -n "$BIN_TMP" ] && rm -rf "$BIN_TMP"' EXIT |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 225 - 243, The main function calls resolve_bin_dir
before checking the DRY_RUN flag, causing actual build or download operations to
execute even in dry-run mode. Move the dry-run check earlier in the main
function, before the bin_dir assignment and resolve_bin_dir call, so it returns
immediately when DRY_RUN is enabled. Only call resolve_bin_dir and proceed with
the remaining logic when DRY_RUN is not enabled, ensuring the dry-run mode
performs no real work.
Comment on lines
+263
to
+273
| err "$SETTINGS exists and isn't ponytail-wired." | ||
| err " Refusing to edit it without a JSON parser. Add by hand:" | ||
| err " SessionStart → command: $HOOKS_DIR/ponytail-activate" | ||
| err " UserPromptSubmit → command: $HOOKS_DIR/ponytail-hook" | ||
| err " statusLine → command: $HOOKS_DIR/ponytail-statusline" | ||
| err " Or install via the plugin marketplace, which wires this for you." | ||
| fi | ||
|
|
||
| echo "" | ||
| echo "Done! Restart Claude Code to activate." | ||
| echo "" |
There was a problem hiding this comment.
Settings conflict path should fail the installer instead of printing “Done!”.
When settings.json exists but isn’t ponytail-wired, the script logs refusal but still reaches success messaging and exits 0. That reports a successful install while hooks are not wired.
Suggested fix
else
err "$SETTINGS exists and isn't ponytail-wired."
err " Refusing to edit it without a JSON parser. Add by hand:"
err " SessionStart → command: $HOOKS_DIR/ponytail-activate"
err " UserPromptSubmit → command: $HOOKS_DIR/ponytail-hook"
err " statusLine → command: $HOOKS_DIR/ponytail-statusline"
err " Or install via the plugin marketplace, which wires this for you."
+ return 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| err "$SETTINGS exists and isn't ponytail-wired." | |
| err " Refusing to edit it without a JSON parser. Add by hand:" | |
| err " SessionStart → command: $HOOKS_DIR/ponytail-activate" | |
| err " UserPromptSubmit → command: $HOOKS_DIR/ponytail-hook" | |
| err " statusLine → command: $HOOKS_DIR/ponytail-statusline" | |
| err " Or install via the plugin marketplace, which wires this for you." | |
| fi | |
| echo "" | |
| echo "Done! Restart Claude Code to activate." | |
| echo "" | |
| err "$SETTINGS exists and isn't ponytail-wired." | |
| err " Refusing to edit it without a JSON parser. Add by hand:" | |
| err " SessionStart → command: $HOOKS_DIR/ponytail-activate" | |
| err " UserPromptSubmit → command: $HOOKS_DIR/ponytail-hook" | |
| err " statusLine → command: $HOOKS_DIR/ponytail-statusline" | |
| err " Or install via the plugin marketplace, which wires this for you." | |
| return 1 | |
| fi | |
| echo "" | |
| echo "Done! Restart Claude Code to activate." | |
| echo "" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 263 - 273, The install script logs error messages
when settings.json exists but isn't ponytail-wired, but then continues execution
to print "Done!" and exit with code 0, falsely reporting success. After the
final err message about the plugin marketplace, add an exit statement with a
non-zero error code to prevent the script from reaching the success messaging
block below. This ensures the installer properly fails when hooks cannot be
wired due to the settings conflict.
Comment on lines
+2
to
+10
| // Hook-shim unit checks for the JS modules that survive the pure-Zig cutover. | ||
| // | ||
| // The SessionStart / UserPromptSubmit RUNTIME (flag write, mode tracking, host | ||
| // output envelopes) now lives in the Zig binaries (zig/src/activate.zig, | ||
| // main.zig, common.zig) and is exercised by `zig build test`. The old Node | ||
| // spawn tests against hooks/ponytail-activate.js + hooks/ponytail-mode-tracker.js | ||
| // were removed with those files. What stays here is the pure-helper contract the | ||
| // surviving JS shims (pi / opencode / instructions bridge) still consume from | ||
| // hooks/ponytail-config.js. |
There was a problem hiding this comment.
Mark the reduced JS test scope with a required ponytail: ceiling comment.
Line 2-Line 10 describes an intentional simplification (JS runtime coverage moved out), but it is not tagged with the required ponytail: format naming the ceiling and upgrade path.
As per coding guidelines: "**/*: Mark intentional simplifications with a ponytail: comment that names any known ceilings (e.g., global lock, O(n²) scan, naive heuristic) and the upgrade path."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/hooks.test.js` around lines 2 - 10, The comment block at the beginning
of the test file (starting at line 2) describes an intentional simplification
where JS runtime coverage was moved to Zig binaries, but it lacks the required
ponytail: ceiling comment as per coding guidelines. Add a ponytail: comment
within or immediately after the existing comment block in hooks.test.js that
explicitly names the known ceiling (the reduced JS test scope and removal of
Node spawn tests) and describes the upgrade path for restoring broader JS
coverage when needed. The comment should follow the ponytail: format and be
clearly associated with the intentional scope reduction being described.
Source: Coding guidelines
…CI hardening WHY: Codex P1/P2 + CodeRabbit Majors on PR #4. The Zig hook didn't cover the retired JS command surface, Windows lost its hook, and the release workflow had real security/correctness gaps. All verified against live code. WHAT: - P1 main.zig: replaced the exact-/ponytail-only parser with a full port of the retired hooks/ponytail-mode-tracker.js surface (parsePrompt → Action set/clear/ none). Accepts: /ponytail [lite|full|ultra], /ponytail off (clear), /ponytail <unknown>→default, @Ponytail / $ponytail sigils (normalized to /), /ponytail-review, namespaced /ponytail:ponytail(-review), case-insensitive+trimmed, standalone "stop ponytail" / "normal mode" deactivation with the DietrichGebert#162 embedded-phrase guard. +8 parsePrompt tests. All std string APIs verified via zigdoc. - P2 claude-codex-hooks.json: restored commandWindows for SessionStart + UserPromptSubmit (PowerShell $env:CLAUDE_PLUGIN_ROOT → new bin/ponytail-launch.ps1, a Windows launcher mirroring the bash resolution order, silent no-op until R6-Windows binaries ship). - CodeRabbit Majors on release-binaries.yml: workflow perms dropped to contents:read (write scoped per-job); workflow_dispatch tag validated against a strict semver regex before use; packaging asserts all 5 runtime binaries present before tar; checksums job fails instead of publishing an empty SHA256SUMS. IMPACT: native (Claude Code / Codex / Copilot) hook command surface matches the retired JS exactly; Windows SessionStart restored; release workflow hardened. VALIDATION (clean cache, zig-0.16.0-dev.3142): - zig build + zig build test + zig fmt --check: exit 0 - runtime parity (sandboxed ponytail-hook): /ponytail ultra→ultra, @Ponytail lite→lite, /ponytail-review→review, /ponytail:ponytail full→full, "stop ponytail"→CLEARED, DietrichGebert#162 "add a normal mode toggle"→none (no false clear) - hooks-windows.test.js 3/3; release-binaries.yml valid YAML; hooks JSON valid - ziglint main.zig: 2 pre-existing idiom findings, none from the new code
Owner
Author
Review triage — P1/P2 + CodeRabbit Majors fixed (ab54c33)
Verified: zig build+test+fmt exit 0; YAML/JSON valid; ziglint clean of new findings. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/ponytail-launch.ps1`:
- Around line 19-23: Replace the current multi-line explanatory comments in the
Windows fallback section with a single `ponytail:` comment placed beside the
`exit 0` no-op path. Make sure the note in `ponytail-launch.ps1` names the known
ceiling (deferred Windows binary availability / silent no-op fallback) and the
upgrade path, and remove the split explanation elsewhere so the intent lives in
one `ponytail:` marker.
In `@hooks/claude-codex-hooks.json`:
- Line 10: Add a regression test for the Windows hook wrapper so
`tests/hooks-windows.test.js` verifies `bin/ponytail-launch.ps1` is present and
included in the Windows command path, not just `bin/ponytail-launch`. Update the
existing hook assertions around the `commandWindows` entries in
`claude-codex-hooks.json` so a missing or unshipped PowerShell wrapper causes
the test to fail and protects the `ponytail-launch.ps1` dependency.
In `@zig/src/main.zig`:
- Around line 51-57: The parser shortcut areas need explicit ponytail
upgrade-path comments: add a `ponytail:` comment near `lowerInto` and the
related fixed-size command/test buffers (including the 64-byte token buffer and
256-byte helper buffer) to document the intentional ceilings and how they should
be lifted later. Keep the comments close to the symbols that implement the
shortcuts so they’re easy to find and update when the parser is expanded.
- Around line 287-290: The parsePrompt casing/trim test is hardcoded to PONYTAIL
instead of using the existing TOOL constant, so it won’t stay aligned with
Caveman builds. Update the test in parsePrompt to build its input from TOOL the
same way the surrounding tests do, keeping the case-insensitivity and trim
coverage while exercising the active tool name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3131ad4-04f5-4a83-9c39-6b013dea10ef
📒 Files selected for processing (4)
.github/workflows/release-binaries.ymlbin/ponytail-launch.ps1hooks/claude-codex-hooks.jsonzig/src/main.zig
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
EugOT/dotfiles(manual)
Comment on lines
+19
to
+23
| # Windows native Zig binaries are still pending (R6-Windows): the cross-compile in | ||
| # .github/workflows/release-binaries.yml ships macOS + Linux only. Until a Windows | ||
| # archive ships there is no step-4 download here — if no binary resolves, this exits | ||
| # 0 (silent no-op) so a missing Windows binary never blocks SessionStart / prompt | ||
| # submission, exactly like the POSIX hooks' silent-fail contract. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use ponytail: markers for the deferred Windows path.
These comments document an intentional ceiling, but they are not using the required ponytail: format and the upgrade path is split across two places. Collapse this into a single ponytail: note next to the exit 0 fallback.
Proposed cleanup
-# Windows native Zig binaries are still pending (R6-Windows): the cross-compile in
-# .github/workflows/release-binaries.yml ships macOS + Linux only. Until a Windows
-# archive ships there is no step-4 download here — if no binary resolves, this exits
-# 0 (silent no-op) so a missing Windows binary never blocks SessionStart / prompt
-# submission, exactly like the POSIX hooks' silent-fail contract.
@@
-# No Windows binary yet (R6-Windows). Silent no-op so the hook never blocks.
+# ponytail: Windows release/download support is intentionally deferred, so this
+# launcher only resolves installed, local-build, or cached binaries and otherwise
+# exits 0 to preserve the hooks' silent-fail contract. Upgrade path: add the same
+# download+verify step as `bin/ponytail-launch` once `release-binaries.yml`
+# publishes Windows Zig archives.
exit 0As per coding guidelines, "Mark intentional simplifications with a ponytail: comment that names any known ceilings (e.g., global lock, O(n²) scan, naive heuristic) and the upgrade path."
Also applies to: 66-67
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'ponytail-launch.ps1'
(PSUseBOMForUnicodeEncodedFile)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/ponytail-launch.ps1` around lines 19 - 23, Replace the current multi-line
explanatory comments in the Windows fallback section with a single `ponytail:`
comment placed beside the `exit 0` no-op path. Make sure the note in
`ponytail-launch.ps1` names the known ceiling (deferred Windows binary
availability / silent no-op fallback) and the upgrade path, and remove the split
explanation elsewhere so the intent lives in one `ponytail:` marker.
Source: Coding guidelines
| "commandWindows": "if (Get-Command node -ErrorAction SilentlyContinue) { node \"$env:CLAUDE_PLUGIN_ROOT\\hooks\\ponytail-activate.js\" }", | ||
| "timeout": 5, | ||
| "command": "\"${CLAUDE_PLUGIN_ROOT}/bin/ponytail-launch\" ponytail-activate", | ||
| "commandWindows": "pwsh -NoProfile -ExecutionPolicy Bypass -File \"$env:CLAUDE_PLUGIN_ROOT\\bin\\ponytail-launch.ps1\" ponytail-activate", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a regression assertion for bin/ponytail-launch.ps1.
Line 10 and Line 23 now depend on the PowerShell wrapper, but tests/hooks-windows.test.js:1-75 only checks that commands route through bin/ponytail-launch and that bin/ponytail-launch exists. A missing or unshipped .ps1 file would still pass that test and break Windows hooks at runtime.
Also applies to: 23-23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/claude-codex-hooks.json` at line 10, Add a regression test for the
Windows hook wrapper so `tests/hooks-windows.test.js` verifies
`bin/ponytail-launch.ps1` is present and included in the Windows command path,
not just `bin/ponytail-launch`. Update the existing hook assertions around the
`commandWindows` entries in `claude-codex-hooks.json` so a missing or unshipped
PowerShell wrapper causes the test to fail and protects the
`ponytail-launch.ps1` dependency.
Comment on lines
+51
to
+57
| /// Lowercase ASCII into `buf`, returning the written slice. Modes/commands are | ||
| /// ASCII-only, matching the JS `.toLowerCase()` on the prompt before matching. | ||
| /// Returns null if the source does not fit (caller treats as no-match). | ||
| fn lowerInto(buf: []u8, s: []const u8) ?[]const u8 { | ||
| if (s.len > buf.len) return null; | ||
| for (s, 0..) |ch, i| buf[i] = std.ascii.toLower(ch); | ||
| return buf[0..s.len]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add required ponytail: ceiling comments for parser shortcuts.
The ASCII-only lowercasing, fixed 64-byte command token buffer, and 256-byte test helper buffer are intentional ceilings but are not marked with the required ponytail: upgrade-path comment.
As per coding guidelines, “Mark intentional simplifications with a ponytail: comment that names any known ceilings … and the upgrade path.”
Also applies to: 119-123, 235-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zig/src/main.zig` around lines 51 - 57, The parser shortcut areas need
explicit ponytail upgrade-path comments: add a `ponytail:` comment near
`lowerInto` and the related fixed-size command/test buffers (including the
64-byte token buffer and 256-byte helper buffer) to document the intentional
ceilings and how they should be lifted later. Keep the comments close to the
symbols that implement the shortcuts so they’re easy to find and update when the
parser is expanded.
Source: Coding guidelines
Comment on lines
+287
to
+290
| test "parsePrompt case-insensitive and trimmed (mirrors JS .trim().toLowerCase())" { | ||
| try expectSet("ultra", parseT(" /" ++ "PONYTAIL ULTRA ", "full")); | ||
| try expectSet("review", parseT("/PONYTAIL-Review", "full")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the case-insensitivity test tool-parametric.
This test hardcodes PONYTAIL, while the file otherwise uses TOOL; a Caveman build would no longer exercise the active tool name.
Proposed fix
test "parsePrompt case-insensitive and trimmed (mirrors JS .trim().toLowerCase())" {
- try expectSet("ultra", parseT(" /" ++ "PONYTAIL ULTRA ", "full"));
- try expectSet("review", parseT("/PONYTAIL-Review", "full"));
+ try expectSet("ultra", parseT(" /" ++ common.TOOL_UPPER ++ " ULTRA ", "full"));
+ try expectSet("review", parseT("/" ++ common.TOOL_UPPER ++ "-Review", "full"));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test "parsePrompt case-insensitive and trimmed (mirrors JS .trim().toLowerCase())" { | |
| try expectSet("ultra", parseT(" /" ++ "PONYTAIL ULTRA ", "full")); | |
| try expectSet("review", parseT("/PONYTAIL-Review", "full")); | |
| } | |
| test "parsePrompt case-insensitive and trimmed (mirrors JS .trim().toLowerCase())" { | |
| try expectSet("ultra", parseT(" /" ++ common.TOOL_UPPER ++ " ULTRA ", "full")); | |
| try expectSet("review", parseT("/" ++ common.TOOL_UPPER ++ "-Review", "full")); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@zig/src/main.zig` around lines 287 - 290, The parsePrompt casing/trim test is
hardcoded to PONYTAIL instead of using the existing TOOL constant, so it won’t
stay aligned with Caveman builds. Update the test in parsePrompt to build its
input from TOOL the same way the surrounding tests do, keeping the
case-insensitivity and trim coverage while exercising the active tool name.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| { | ||
| "matcher": "startup|resume|clear|compact", | ||
| "hooks": [ | ||
| { "type": "command", "command": "$activate_bin", "timeout": 5 } |
There was a problem hiding this comment.
🔥 The Roast: Your installer writes timeout: 5 to settings.json while the hook manifests got upgraded to timeout: 30. This isn't a feature gap—it's a time bomb. Users who install via the shell script get 5-second timeouts while marketplace users get 30, creating a two-tier support nightmare.
🩹 The Fix:
Suggested change
| { "type": "command", "command": "$activate_bin", "timeout": 5 } | |
| { "type": "command", "command": "$activate_bin", "timeout": 30 } |
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "UserPromptSubmit": [ | ||
| { | ||
| "hooks": [ | ||
| { "type": "command", "command": "$hook_bin", "timeout": 5 } |
There was a problem hiding this comment.
🔥 The Roast: Same story as line 193—your installer still hands out 5-second timeouts when the plugin manifests upgraded to 30s. This is like leaving a “Beware of Dog” sign on a fence after the dog moved out.
🩹 The Fix:
Suggested change
| { "type": "command", "command": "$hook_bin", "timeout": 5 } | |
| { "type": "command", "command": "$hook_bin", "timeout": 30 } |
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| err " SessionStart → command: $HOOKS_DIR/ponytail-activate" | ||
| err " UserPromptSubmit → command: $HOOKS_DIR/ponytail-hook" | ||
| err " statusLine → command: $HOOKS_DIR/ponytail-statusline" | ||
| err " Or install via the plugin marketplace, which wires this for you." |
There was a problem hiding this comment.
🔥 The Roast: When settings.json exists and isn’t ponytail-wired, you print helpful instructions but then cheerfully exit 0 with “Done!”. This is the software equivalent of a flight attendant demonstrating life vests on a plane that’s on fire.
🩹 The Fix: Return a non-zero exit code after the error messages so the installer truthfully reports failure.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| function python() { | ||
| if (pythonCmd) return pythonCmd; | ||
| for (const cmd of ['python3', 'python']) { | ||
| try { execSync(`${cmd} -c "import sys"`, { stdio: 'pipe' }); pythonCmd = cmd; return pythonCmd; } |
There was a problem hiding this comment.
🔥 The Roast: String-interpolated execSync with shell metacharacters. This is like handing a stranger your SSH key because they asked nicely.
🩹 The Fix: Replace execSync with execFileSync using argument arrays to avoid shell injection:
const { execFileSync } = require("node:child_process");
// ...
execFileSync(cmd, ['-c', 'import sys'], { stdio: 'pipe' });📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| test "parsePrompt case-insensitive and trimmed (mirrors JS .trim().toLowerCase())" { | ||
| try expectSet("ultra", parseT(" /" ++ "PONYTAIL ULTRA ", "full")); |
There was a problem hiding this comment.
🔥 The Roast: The test hardcodes PONYTAIL instead of using common.TOOL_UPPER. When someone runs this with a Caveman build (TOOL="caveman"), the test will fail to catch uppercase commands. This is like testing a car's brakes but forgetting to turn the engine on.
🩹 The Fix:
Suggested change
| try expectSet("ultra", parseT(" /" ++ "PONYTAIL ULTRA ", "full")); | |
| try expectSet("ultra", parseT(" /" ++ common.TOOL_UPPER ++ " ULTRA ", "full")); | |
| try expectSet("review", parseT("/" ++ common.TOOL_UPPER ++ "-Review", "full")); |
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // spawn tests against hooks/ponytail-activate.js + hooks/ponytail-mode-tracker.js | ||
| // were removed with those files. What stays here is the pure-helper contract the | ||
| // surviving JS shims (pi / opencode / instructions bridge) still consume from | ||
| // hooks/ponytail-config.js. |
There was a problem hiding this comment.
🔥 The Roast: This file deletes half the tests and documents the intentional scope reduction, but doesn't wear the ponytail: badge of honor. How is anyone supposed to know this was deliberate and not just lazy?
🩹 The Fix: Add a ponytail: comment naming the ceiling (JS runtime coverage moved to Zig binaries) and upgrade path (revert if broader JS coverage needed).
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # .github/workflows/release-binaries.yml ships macOS + Linux only. Until a Windows | ||
| # archive ships there is no step-4 download here — if no binary resolves, this exits | ||
| # 0 (silent no-op) so a missing Windows binary never blocks SessionStart / prompt | ||
| # submission, exactly like the POSIX hooks' silent-fail contract. |
There was a problem hiding this comment.
🔥 The Roast: You've got paragraphs of explanation for why Windows is deferred, but they're not wearing the required ponytail: uniform. At least pretend you meant to do this.
🩹 The Fix: Collapse the multi-line comments into a single ponytail: marker beside the exit 0 fallback.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # exclude (ponytail has no claw test binary). | ||
| # | ||
| # Windows is deferred (R6-Windows: stdio/subprocess/argv libc → std.Io). The PS1 | ||
| # shim covers Windows until then, so this workflow ships macOS + Linux only. |
There was a problem hiding this comment.
🔥 The Roast: Lines 12-13 explain why Windows is deferred (R6-Windows) but lack the required ponytail: marker format. The coding guidelines aren't suggestions—they're orders from the ponytail police.
🩹 The Fix: Prefix with ponytail: Windows release/download support is intentionally deferred... following the ceiling+upgrade-path format.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: 8 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: Finally killed the CVE-vulnerable SDK dependency and shipped pure Zig binaries. The symlink-safe flag writes and host detection dispatch are genuinely solid. 💀 Worst part: The install.sh timeout mismatch. Users who go through the shell installer get half the timeout allowance of marketplace users—classic “works on my machine” energy. 📊 Overall: Like a house flip that spent all the budget on the kitchen but forgot to match the outlet covers. Fix these issues in Kilo Cloud Files Reviewed (4 files)
Reviewed by laguna-m.1-20260312:free · Input: 256.9K · Output: 15.9K · Cached: 2.8M |
This was referenced Jun 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reconciles dev↔main (absorbing main's CVE fix + upstream merge that dev lacked) and lands ponytail's pure-Zig cutover for the native install paths.
Reconcile (R6.0)
dev was 3 ahead / 8 behind main. This branch merges origin/main into dev:
git merge-base --is-ancestor origin/main HEAD→ MAIN_ABSORBED; all 8 of main's commits reachable.Cutover (R6)
bin/ponytail-launch ponytail-{activate,hook}(native Zig path).Honest scope
The pi-extension chain stays JS (irreducible): pi mandates a JS ESM plugin whose runtime depends on ponytail-config → fs-safe → instructions → instructions-bin. Those 4 can't be deleted without breaking pi-extension. So the native (Claude Code / Codex / Copilot) paths are pure Zig; the pi runtime keeps its JS shim layer (deeper than caveman's single opencode .cjs — a structural difference between the repos).
Validation (clean cache, zig-0.16.0-dev.3142)
🤖 Generated with Claude Code