feat(tui): command history recall + tmux/stdout fire targets - #9
Merged
Conversation
Add internal/fire (stdout default, tmux send-keys -l prefill, $TMUX-gated) and internal/history (ctrl+r recall; stores template + var bindings, never the resolved string). --fire flag + per-command ctrl+t override. History file is owner-only on every OS: POSIX 0600 on Unix, owner-only DACL on Windows via x/sys/windows. Engine untouched; show-before-fire preserved.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesFire, History, and TUI Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@internal/fire/fire.go`:
- Around line 63-80: Emit currently falls through every non-Tmux value to
stdout, so unsupported Target values are silently accepted instead of failing
like Parse does. Update Emit to explicitly handle only the known targets and
return an error for any unknown/invalid Target, using the Target type and
Emit/Tmux symbols to locate the switch. This should also surface misconfigured
Options.DefaultTarget from internal/tui/tui.go instead of printing to Out by
mistake.
In `@internal/history/history.go`:
- Around line 14-22: The NOCK_HISTORY handling in history initialization is not
truly case-insensitive, so values like oFf fall through as a path instead of
disabling history. Update the switch logic in the history setup code (around the
NOCK_HISTORY parsing in history.go) to normalize the environment value before
matching, and ensure the disabling branch is triggered for any case variation of
off while preserving the existing behavior for other values.
In `@internal/history/open_unix.go`:
- Around line 13-18: The openAppend helper only sets 0600 for newly created
files, so existing history files can remain too permissive. Update openAppend to
harden any existing path before writing by checking the opened file’s
permissions and tightening them when needed, using the existing openAppend
symbol as the place to enforce this behavior. Keep the current append/create
flow, but ensure world-readable history files are corrected before Append writes
variable bindings.
In `@internal/history/open_windows.go`:
- Around line 19-21: Reject appends in openHistoryFile when the target history
file does not already have the owner-only DACL, since OPEN_ALWAYS can reuse a
permissive existing ACL. Update the history-file opening path to verify or
reapply the owner-only DACL before allowing writes, and fail closed if the
descriptor is missing or not restricted. Use the openHistoryFile logic in
internal/history/open_windows.go as the place to enforce this policy.
- Around line 34-40: The Windows history append handle in open_windows.go is too
restrictive because CreateFile in the history open path only allows
FILE_SHARE_READ, which can block concurrent writers. Update the sharing flags in
the CreateFile call used by the history append logic to include FILE_SHARE_WRITE
alongside FILE_SHARE_READ, so multiple nock processes can append safely without
sharing violations.
In `@internal/tui/tui.go`:
- Around line 316-322: History recall is rebuilding m.selected with only ID and
Command, which drops the Risk and RequiresAuth metadata used by viewConfirm().
Update recallEntry and the similar recall path near the other referenced lines
to rehydrate the full format.Command from engine.Get when the command still
exists, then overlay the stored template instead of replacing the whole struct.
Keep the existing selected sheet/template restore behavior intact while
preserving the safety badges.
- Around line 64-66: The DefaultTarget handling in TUI setup and confirmation
flow accepts tmux blindly even when fire.Tmux.Available() is false, so validate
the configured target before it becomes active or prevent Enter confirmation
when the target is unavailable. Update the logic around opts.DefaultTarget in
the TUI initializer and the confirmation path used by ctrl+t/Enter (for example
in the TUI methods that set and confirm the target) so tmux is only allowed when
the session is actually available, and fail or keep the dialog open early
instead of letting Run reach fire.Emit after leaving the alt-screen.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33220ebc-002a-405b-b220-c2cf1bd21983
📒 Files selected for processing (11)
ROADMAP.mddocs/cli.mdgo.modinternal/fire/fire.gointernal/fire/fire_test.gointernal/history/history.gointernal/history/history_test.gointernal/history/open_unix.gointernal/history/open_windows.gointernal/tui/tui.gointernal/tui/tui_test.go
- fire.Emit: unknown target errors instead of silently printing to stdout - tui.Run: validate fire target before alt-screen; fail fast outside tmux - tui recall: rehydrate full command via engine.Get so risk/auth badges survive - history: re-tighten existing files to owner-only (0600 / reapplied Windows DACL) - history: NOCK_HISTORY=off is case-insensitive (strings.EqualFold) Add .coderabbit.yaml (auto-review scoped to develop/main bases) and document the review workflow, its solo-project rationale, and git-flow branching in CONTRIBUTING and README.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/history/open_windows.go`:
- Line 42: Add WRITE_OWNER to the access mask used when reopening the history
file in open_windows.go so SetSecurityInfo can update OWNER and GROUP
successfully. Update the handle-open logic around the FILE_APPEND_DATA and
WRITE_DAC permissions to include WRITE_OWNER before the call path that applies
security changes, keeping the change localized to the history file
open/permission setup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 769a9dbf-9ef4-4269-9566-ae98464d7891
📒 Files selected for processing (11)
.coderabbit.yamlCONTRIBUTING.mdREADME.mdinternal/fire/fire.gointernal/fire/fire_test.gointernal/history/history.gointernal/history/history_test.gointernal/history/open_unix.gointernal/history/open_windows.gointernal/tui/tui.gointernal/tui/tui_test.go
✅ Files skipped from review due to trivial changes (2)
- CONTRIBUTING.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/history/open_unix.go
- internal/fire/fire_test.go
- internal/fire/fire.go
- internal/history/history_test.go
- internal/tui/tui_test.go
- internal/tui/tui.go
Add .coderabbit.yaml (auto-review scoped to develop/main) and document the review workflow + solo-project rationale. Lead README with the problem, an at-a-glance comparison, and a what-it-feels-like demo.
Complete the free security-gate stack on both git-flow hops; document it in SECURITY.md.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/gitleaks.yml:
- Around line 32-34: The gitleaks workflow uses actions/checkout with the
default persisted token, which exposes unnecessary credentials. Update the
checkout step in the workflow to disable persisted credentials by setting
persist-credentials to false while keeping the existing fetch-depth behavior for
the gitleaks scan.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ca2cefb-8e2e-4ca3-946b-3ea7b34b9c2c
📒 Files selected for processing (4)
.github/workflows/codeql.yml.github/workflows/gitleaks.ymlREADME.mdSECURITY.md
✅ Files skipped from review due to trivial changes (1)
- SECURITY.md
SetSecurityInfo with OWNER_SECURITY_INFORMATION needs SeRestorePrivilege, which ordinary processes (incl. CI runners) lack -> 'Access is denied'. The creator is already the owner; the protected owner-only DACL is what gates access, so set the DACL only. Fixes the failing windows-latest test job on #9.
Cross-builds and cross-lints every release target + coverage + govulncheck, so platform-specific breakage is caught locally before push (make check is host-only). A living gate that tracks CI through to v1.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/dev-check.sh`:
- Around line 76-87: The golangci-lint check in dev-check.sh is optional right
now, which allows verify to pass without any linting; make this step mandatory
by failing immediately when golangci-lint is unavailable, or install/bootstrap
the pinned version before continuing. Update the control flow around the
existing step "golangci-lint (host)" and the later GOOS=windows lint path so
missing tooling stops the script instead of only calling warn.
- Around line 80-84: The current cross-lint block in scripts/dev-check.sh only
runs golangci-lint once with GOOS=windows, so it does not cover the full
advertised platform matrix. Update the logic around the lint step to iterate the
same PLATFORMS matrix used by the build/verify flow, including linux, darwin,
and windows across amd64 and arm64, so platform-specific files and
arch-dependent code are linted consistently. Use the existing step/golangci-lint
run block as the entry point and preserve the FAST gating while expanding the
per-platform execution.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fecc828-ea36-4041-88e2-fa2866d18d7b
📒 Files selected for processing (3)
CONTRIBUTING.mdMakefilescripts/dev-check.sh
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
The Access-denied failure came from setting the file OWNER at runtime, which needs SeRestorePrivilege (denied on CI runners). New files now get the owner-only DACL atomically via SecurityAttributes at creation (no privileged call); existing files are re-tightened best-effort. Fixes the windows-latest test job on #9.
…target OSes Address CodeRabbit on dev-check.sh: a missing linter now fails the gate instead of passing green, and the cross-lint pass iterates every target OS, not just windows.
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
2 participants
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.
Changes
internal/fire— where a confirmed command goesTarget:stdout(default, every OS) ortmux.stdoutprints the command once after the UI tears down, for the shell tocapture. nock never runs it.
tmuxprefills the current pane viatmux send-keys -lwith no trailingEnter — the operator still fires it. Gated on
$TMUXviaAvailable(), so itis never offered or fired where it can't work (including all of Windows, which
has no tmux).
--fire=stdout|tmux(set once at launch) and a per-commandctrl+toverride on the confirm screen.internal/history— recall past fired commands (ctrl+r)variable bindings separately as JSONL — never the flattened resolved
string. Values stay redactable and recall re-
Resolves through the enginerather than trusting a baked-in line.
os.UserConfigDir()/nock/history.jsonl.NOCK_HISTORY=<path>relocates it;
NOCK_HISTORY=offdisables persistence (no-op store).Cross-platform owner-only history file
0600on Unix (open_unix.go).open_windows.go) viagolang.org/x/sys/windows— SDDLO:<sid>G:<sid>D:P(A;;FA;;;<sid>): protectedDACL, current-user SID only, no inheritance.
openAppendis the build-tagged seam.x/syswas already in the tree (indirectvia bubbletea); promoted to a direct dep — no new download, CGO-free.
TUI wiring (
internal/tui)Options{History, DefaultTarget}onRun; newstageHistoryrecall screen.engine.Vars()then re-enters the existing fill→confirm flow.Security — show-before-fire holds
emitted; pre-bound vars just skip straight to confirm — still displayed (ADR
009/015). No auto-run, displayed == emitted.
send-keys -lwith no Enter: a prefill, never an execution.Tests
internal/fire:Parsetable,$TMUX-driven availability, stdout emit capturesthe command verbatim, tmux-without-session errors.
internal/history: append/recent round-trip (newest-first, capped), disabledstore no-op, missing file empty, corrupt line skipped,
NOCK_HISTORYenv,0600perm assertion (skipped on Windows — ACL instead).internal/tui:ctrl+rrecall re-fills from saved vars, fired entry recordstemplate+bindings, default target, empty-history hint, esc-from-history.
make checkgreen (fmt, vet, lint 0 issues, race) on host andGOOS=windowsbuild + lint clean.Out of scope (still open in M2)
Distribution (Releases / Homebrew /
go install/ AUR). Clipboard target remainsa later opt-in (ADR 015).
Summary by CodeRabbit
ctrl+r, restoring saved variable bindings for re-run.ctrl+tto redirect a confirmed command into an active tmux session when available.NOCK_HISTORY, with an OS-specific default location.make verifylocal gate.