feat(rip): ship hand-written zsh completion alongside wrapper#184
feat(rip): ship hand-written zsh completion alongside wrapper#184
Conversation
Stdout and stderr are duplicated into
${XDG_STATE_HOME:-~/.local/state}/nixos-build/build-<timestamp>-<pid>.log
with ANSI codes stripped, so failed runs leave a diagnosable record.
The EXIT trap closes the redirected fds and waits for the tee
subprocesses so trailing output flushes before the shell returns. The
PID suffix prevents collisions when two runs start within the same
second.
Also swap the bootstrap substituter preference from mirror.sjtu.edu.cn
to mirrors.ustc.edu.cn.
…idance The Registering in the Overlay section pointed to modules/system76/custom-packages-overlay.nix, but in-tree custom packages live in the shared customPackages overlay at modules/base/custom-packages-overlay.nix and hosts opt in via nixpkgs.overlays. The per-host files now carry only host-specific patches. Hash-fetching workflow now uses nix build .#nixosConfigurations.<host>.pkgs.<name>, since overlay-backed packages are not exposed as top-level flake outputs and the previous nix build .#<name> invocation always failed. When to Create an App Module now lists host-visible tooling as an explicit reason to add an app module (sss-pass-gpg-bootstrap and sss-nix-repair are both wired this way), and documents the apps-catalog-sync pre-commit hook contract that requires every app module to have a matching entry in each host's apps-enable.nix. Validation checklist updated to cite the correct registration file, the host-aware build command, and the apps-catalog-sync enforcement.
Ship a _rip completion file with the rip wrapper so zsh's compinit indexes a stable definition tied to the wrapper's actual flag set. Eliminates the stale-cache shadowing window where a previously installed rip2 _rip lingered in ~/.cache/zsh/zcompdump after the package left the closure. The completion mirrors the wrapper's mode_count check: --empty-trash, -s/--seance, -u/--unbury, and positional FILES are mutually exclusive, while --graveyard and --force are universal modifiers. -i/--inspect is gated to FILES mode. -r/-R/-d are exposed with an "rm compat, no-op" description for muscle-memory invocations like 'rip -rfd path/'. Verification: - nix build .#nixosConfigurations.system76.config.system.build.toplevel - find -L result/sw/share/zsh/site-functions -name _rip resolves to the rip-zsh-completion derivation output - zsh -n parses the script cleanly; compinit autoloads _rip from the shipped path and registers _comps[rip]=_rip
Bad3r
added
type(enhancement)
|
Claude finished @Bad3r's task in 7m 31s —— View job Code ReviewHeadline change ( Correctness —
|
| wrapper flag | completion spec | verdict |
|---|---|---|
-h/--help |
(- *){-h,--help} |
✓ excludes everything (standard) |
--empty-trash |
(--empty-trash --seance -s --unbury -u *)--empty-trash |
✓ correctly excludes other modes and positional * |
-s/--seance |
same exclusion list | ✓ |
-u/--unbury |
excludes other modes, NOT *; uses :*::path:_rip_files |
✓ — the rest-pattern is consumed by the option itself, so omitting * from the exclusion list is intentional |
-i/--inspect |
excludes modes, allows positional | ✓ matches wrapper |
-f/--force, -r/-R/-d |
no exclusions | ✓ legitimately combinable with anything |
--graveyard (both --graveyard PATH and --graveyard=PATH) |
--graveyard=[…]:trash dir:_files -/ |
✓ verified via the zsh manual: trailing = (not =-) in _arguments accepts both joined and separate-word forms (this is the convention used in _git, _curl, etc.) |
-- end-of-options |
implicit via _arguments -S |
✓ |
stacked short flags (e.g. rip -rfd path/) |
_arguments -s |
✓ |
| positional FILES | *:files to trash:_rip_files |
✓ |
The _rip_files shim's metacharacter-escape pattern is correct and matches the borrowed source. Two minor comments left inline.
Other commits in this PR
The branch contains four additional commits beyond the rip completion: feat(build): tee each run…, docs(custom-packages): correct overlay registration…, chore: drop notes.md scratch file, and chore: update inputs. The PR title and body only document the rip completion, so a reviewer reading "feat(rip): ship hand-written zsh completion" wouldn't know to look at build.sh or the docs guide. Per CLAUDE.md ("Keep one logical concern per commit") the commits themselves are clean, but bundling them here makes the PR mis-scoped — recommend splitting feat(build): tee each run… and docs(custom-packages): … into their own PRs (or at minimum amending the description to mention them).
build.sh— log-tee approach is sound; minor concurrency note inline. Also flags an unrelated substituter list change (sjtu/ustc swap) bundled into the tee commit.docs/guides/custom-packages-style-guide.md— solid corrections; aligns prose with the actualmodules/base/custom-packages-overlay.nixshape and updates the validationnix buildinvocation to go throughnixosConfigurations.<host>.pkgs.<name>. Looks good as-is.flake.lock/notes.md— routine.
Verdict
The rip completion change is good to merge once the post-switch TAB tests in the PR's checklist clear. Inline comments left for two minor observations on _rip_files and one on build.sh log fidelity. No blocking issues.
Tasks
- Read changed files
- Verify completion contract against wrapper flag set
- Verify
--graveyard=zsh semantics (accepts both--graveyard /pathand--graveyard=/path) - Cross-check
_rip_filesshim against the borrowed_trash_filespattern - Review
build.shand docs changes - Post inline comments + summary review
· Branch
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces automated logging to build.sh with ANSI color stripping and improved process cleanup, updates the custom packages style guide to use a shared overlay and correct build paths, and adds a Zsh completion script for the rip utility. Review feedback focused on the Zsh completion, specifically recommending the use of extendedglob for pattern matching, correcting the manipulation of completion arrays, and refining the _arguments specification to properly handle flag exclusivity and the --graveyard option.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| '(- *)'{-h,--help}'[print help and exit]' \ | ||
| '(--empty-trash --seance -s --unbury -u *)--empty-trash[permanently empty the trash]' \ | ||
| '(--empty-trash --seance -s --unbury -u *)'{-s,--seance}'[list files trashed from cwd]' \ | ||
| '(--empty-trash --seance -s --unbury -u)'{-u,--unbury}'[restore trashed files (interactive picker if no PATH)]:*::path to restore:_rip_files' \ |
There was a problem hiding this comment.
-u/--unbury consumes paths via :*::path to restore:_rip_files, which forwards to _files. That offers paths from the current filesystem, but trash-restore actually needs an original_location recorded in trash metadata — which by definition is a path that no longer exists on disk. So tab-completion here is mostly cosmetic; matches won't be discoverable. Not actionable for this PR (it mirrors trash-cli's _trash-restore behaviour and the PR explicitly aligns with it), but worth flagging for a future enhancement that parses trash-list output for restore candidates — gated on --graveyard since the trash dir is configurable.
There was a problem hiding this comment.
@claude Acknowledged and deferred. Implementing trash-list-backed restore-target completion changes scope significantly — output parsing, caching to keep TAB latency low, handling whitespace/special chars in recorded paths, and propagating --graveyard through both trash-list and trash-restore — and you flagged it as "not actionable for this PR" yourself. The existing :*::path to restore:_rip_files spec stays as-is so the form mirrors trash-cli's _trash-restore and remains predictable; tracking the upgrade as a follow-up enhancement.
There was a problem hiding this comment.
Claude finished @Bad3r's task in 25s —— View job
Deferral acknowledged
Agreed — keeping :*::path to restore:_rip_files for this PR is the right call. The scope you outlined (parsing trash-list, caching for TAB latency, escaping whitespace/metacharacters in recorded original_location paths, threading --graveyard through both trash-list and trash-restore) is a separate piece of work, and the current spec preserves shape-parity with trash-cli's _trash-restore so muscle memory carries across.
For when you pick this up, two notes worth pinning so they aren't rediscovered:
trash-listoutput format is line-oriented but not stable across versions — the rip wrapper currently shells out totrash-list/trash-restorefrom trash-cli, so the parser will need to track that upstream. A safer path isfind "$TRASH_DIR/info" -name '*.trashinfo'and read thePath=line directly (URL-decoded), which is what trash-cli itself does internally.--graveyardonly matters for the current completion invocation; the cache key for the restore-target list should include the resolved trash dir (default${XDG_DATA_HOME:-$HOME/.local/share}/Trashor$RIP_GRAVEYARDor the--graveyardvalue already on the command line), otherwise tabbing across two terminals with different graveyards will serve stale entries.
No code change for this thread. Closing out.
· Branch
Force `extendedglob` locally so the `(#m)` glob qualifier and `$MATCH` substitution inside the metachar escape pass keep working when the caller's shell disables the option, and switch the working buffer from `line` to a copy of `words`. `line` only holds the positional args parsed so far; `CURRENT` is the index of the active token in `words`, so `line[CURRENT]=()` was a silent no-op and the in-progress half-typed path stayed in the exclusion list. Operating on `tmp_words` makes `[CURRENT]=()` actually remove the active word, and the dead `(( CURRENT > 0 ))` guard goes with it (CURRENT is always >= 1 inside a completion function).
Drop the trailing `=` from `--graveyard` so `_arguments` matches both `--graveyard PATH` and `--graveyard=PATH`. The wrapper accepts both forms; the previous spec only completed for the equals form, leaving `--graveyard <TAB>` with no directory action. Add `-i --inspect` to the exclusion lists of the three other mode flags so the symmetry of the wrapper's `mode_count` rejection is visible in completion: `-i` is a trash-mode modifier and is no-op (or worse, misleading) when paired with `--empty-trash`, `-s/--seance`, or `-u/--unbury`. Add `*` to `-u/--unbury`'s exclusion list so the global `*:files to trash:_rip_files` spec stops firing once `-u` has been typed; the local `:*::path to restore:_rip_files` spec already provides positional completion for the restore mode and the wrapper forbids mixing restore targets with files to trash.
Two `sed -u | tee` chains were appending to ${LOG_FILE} concurrently,
one per fd. POSIX guarantees atomic O_APPEND writes only up to
PIPE_BUF (4096 bytes on Linux); a verbose nix build can emit longer
single lines (long store-path arrays, derivation dumps) that
interleave at byte boundaries between the two appenders.
Redirect stderr into stdout before the tee so a single sed process
owns the log writer. The outer tee still preserves ANSI on the
terminal; only the file copy goes through sed. Stream distinction in
the log was already absent, so nothing of value is lost; the trade-off
is that terminal output now lands on a single fd, which keeps colored
status lines but loses the stdout/stderr split that previously held on
the controlling terminal.
Review feedback addressed@gemini-code-assist @claude — thanks for the careful read. Here is what landed and why, broken out per thread: Accepted and implemented
Acknowledged, deferred
Bundling the substituter swap into
Validation
Pushing the new commits now and re-running the automated review. |
|
Claude finished @Bad3r's task in 1m 47s —— View job Re-review verdict — LGTMWalked the three follow-up commits against the code and the prior review threads.
|
|
Thanks for the detailed breakdown, @Bad3r. The adjustments to |
|
Claude finished @Bad3r's task in 5m 46s —— View job Re-review of fix-up commitsVerified the three fix-up commits against the working tree at
|
Reverting the trailing-`=` removal from `7b2c5f91`. Empirical PTY test
against the rebuilt completion confirmed claude's nuance was correct:
Spec form | `--graveyard PATH` | `--graveyard=PATH`
--- | --- | ---
`--graveyard=` | completes (`dir`) | completes (`=dir`)
`--graveyard` | completes (`dir`) | does NOT complete
Per `zshcompsys(1)` ("`-optname=`: argument may appear as the next
word, or in same word as the option name provided that it is
separated from it by an equals sign"), the trailing `=` in the spec
is what enables both call forms; without it, the same-word
`--graveyard=PATH` form silently fails to complete.
The wrapper accepts both forms (modules/apps/rip.nix:125-135), so the
completion needs to do the same. The original spec was already
correct; gemini's claim that the `=` form excluded the space form was
inverted from how the spec parser actually behaves.
|
@claude Empirically verified — your nuance was load-bearing, not just version-dependent. Reverted the PTY test (driven via
So the @gemini-code-assist The |
|
Claude finished @Bad3r's task in 52s —— View job Revert verified — empirical correction standsConfirmed The exclusion-list fixes from One correction to my own prior commentMy earlier hedge ("most modern zsh versions auto-split on For posterity, the rule the spec parser is enforcing:
The Tasks
No further changes needed from this thread. PR remains ready to merge once the post-switch TAB checklist on |
|
Thanks for the empirical verification, @Bad3r. That nuance regarding the |
|
Claude finished @Bad3r's task in 1m 24s —— View job Re-review verdict — LGTMThe single new commit since the last review is
|
| Spec | --graveyard <TAB> |
--graveyard=<TAB> |
|---|---|---|
--graveyard= (now restored) |
✓ completes | ✓ completes |
--graveyard (the regression) |
✓ completes | ✗ no completion |
This matches the rationale I flagged in the prior review pass and inverts the rationale gemini's earlier comment had argued. Net result: the spec is back to the form afea165 originally shipped, with the symmetric -i/--inspect exclusion fix and the * exclusion on -u/--unbury from 7b2c5f9 preserved on top.
Other invariants (re-confirmed)
_rip_files(modules/apps/rip.nix:259-275) —localoptions extendedglob+tmp_wordsbuffer swap fromcbd951cis intact; the active token still gets correctly removed from the exclude set passed to_files -F._argumentsexclusion lists (modules/apps/rip.nix:288-297) — three mode flags (--empty-trash,-s/--seance,-u/--unbury) symmetrically exclude-i/--inspect, and-u/--unburyexcludes*so the global positional spec stops firing in restore mode. Unchanged by6205d6a.build.sh:109— single-tee redirect fromef91f0fis unchanged.build.shis no longer in the diff againstorigin/mainfor this PR (onlymodules/apps/rip.nixis), so the build-script changes will need to ride on a separate PR if not already merged via another path.
Verdict
The rip completion change is ready to merge once the post-switch TAB checklist clears on system76 and tpnix. No further changes requested. Nice catch on the empirical verification — that's the kind of "trust the parser, not the rationale" check that prevents a regression slipping in under a confident-sounding review comment.
Tasks
- Verify
6205d6ais on the branch and inspect the restored--graveyard=spec - Cross-check against the wrapper's
--graveyardparsing (both--graveyard PATHand--graveyard=PATHaccepted) - Confirm the rest of the
_argumentsinvariants from the prior review still hold - Post follow-up review
· Branch
1 participant
Summary
_ripzsh completion file with the rip wrapper (modules/apps/rip.nix) socompinitindexes a stable definition tied to the wrapper's actual flag set.rip2_riplingered in~/.cache/zsh/zcompdumpafter the package left the closure.mode_countexclusivity (--empty-trash,-s/--seance,-u/--unbury, positional FILES are mutually exclusive) and exposes universal modifiers (--graveyard,--force) without exclusion.Implementation notes
ripCompletion = pkgs.writeTextFile { destination = "/share/zsh/site-functions/_rip"; ... }derivation alongside the existingripWrapperin the sameletblock; added toenvironment.systemPackagesunder the samecfg.enablegate (no new module options).modules/apps/build-sh-completion.nix. fpath wiring is automatic viaprograms.zsh.enableCompletion = trueinmodules/base/shell-config.nix._rip_filesborrows the metacharacter-escaping shim from trash-cli's_trash_files(see_trash-put/_trash-restoreon the same fpath).--graveyard=argument completes directories only (_files -/);-u/--unburyconsumes following non-flag args as restore targets via:*::path:_rip_files.-r/-R/-dexposed with[rm compat, no-op]description so muscle-memory invocations likerip -rfd path/stay discoverable.Test plan
nix fmt -- modules/apps/rip.nixnix develop -c pre-commit run --files modules/apps/rip.nix(deadnix, nix-parse, statix, treefmt, typos pass)nix build .#nixosConfigurations.system76.config.system.build.toplevelfind -L result/sw/share/zsh/site-functions -maxdepth 1 -name '_rip*'resolves to/nix/store/<hash>-rip-zsh-completion/share/zsh/site-functions/_ripzsh -nparses the shipped script cleanlycompinitautoloads_ripfrom the new path and registers_comps[rip]=_ripcommand rm -f ~/.cache/zsh/zcompdump* && exec zsh -l, then verify each TAB scenariorip <TAB>lists files in cwdrip --<TAB>offers--help --empty-trash --seance --unbury --inspect --force --graveyardwith descriptionsrip -<TAB>offers short forms with descriptions including-r/-R/-dtagged "rm compat, no-op"rip --graveyard <TAB>completes directories onlyrip --empty-trash <TAB>andrip -s <TAB>offer no further completionsrip -u <TAB>completes files (paths to restore)rip -- <TAB>andrip -rfd <TAB>complete filesOne-shot user step after switch
compinit -Cin ez-compinit will not pick up the new_ripuntil its 20-hour cache TTL expires. Bust the cache once after activation: