Choose redaction with --redact=WHEN, and test completions by running them - #10
Merged
Conversation
…them
`envctl env` opened with `# envctl VERSION (redacted)`. The suffix means
something else in this tool, since `src/diff.c` writes `+++ .env (redacted)`
where the thing before the parenthesis is the subject whose content is
hidden, so the banner read as though the version string had been masked.
It also took the first line of a `KEY=VALUE` stream, which `head -1` and
every other consumer then had to skip. The line is gone.
Masking now reads as one flag with five values instead of two opposing
booleans, following `--color=always|auto|never` in ls, grep, diff and git.
Three values would not cover it: `want_redact` evaluates two independent
signals, and agent detection is a module of its own with seventeen cases
behind it.
never never
auto an agent is detected and stdout is a terminal
agent an agent is detected, terminal or not
tty stdout is a terminal, agent or not
always always
`auto` stays the default, so `pipe-agent-stays-raw.case` still holds: an
agent reading through a pipe gets the real value, which is what keeps
`TOKEN=$(envctl get TOKEN)` working. `--redact=agent` is the setting for
anyone who wants that closed, and it had no spelling before.
`--raw` did two unrelated jobs. ls keeps them apart, `--color` for one and
`--show-control-chars` for the other, so control-byte escaping moves to a
flag of its own under GNU's name and `--raw` becomes the shorthand for
both. It also works on `env` now: refusing it there bought nothing with
`env(1)` sitting next to it.
The completion tests only compared the generator against a recorded copy
of its own output, which cannot catch an emitter that puts correct data
where the shell applies it too widely. That is exactly what shipped: the
zsh per-command arm was right the whole time while the top-level
`_arguments` kept its specs live past the command word, and the suite
stayed green. `tests/completions.sh` asks bash and fish for their
candidates and drives zsh through zpty, then asks the binary whether each
one is valid, in contexts including one where a flag is already typed. It
was checked against a copy of the script with the fault put back.
It found one on its first run: bash returned the shell list for
`completions` and `module` before looking at flags, so `envctl completions
-` offered nothing at all.
|
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:
📝 WalkthroughRemove the Replace redaction booleans with Add Improve Bash, Fish, Zsh, and PowerShell completion generation with flag values and command-specific contexts. Add an integration harness that validates generated completions, invalid scopes, invalid values, already-typed flags, and Improve Zsh completion tests with warm-up renders, rendered-output polling, timeouts, preserved stderr, and raw-byte diagnostics. Update CI, documentation, the README, the manual, and test fixtures. Add coverage for redaction modes, invalid values, incompatible options, raw environment output, and control-byte display. Extend ShellCheck coverage to all test scripts. Add PowerShell formatting support and register the PSScriptAnalyzer dprint plugin. WalkthroughThe CLI now supports selectable redaction modes, raw environment output, and independent control-character display. Environment output no longer includes a version header. Flag parsing accepts and validates Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as envctl
participant Policy as want_redact
participant Environment as act_env_dump
User->>CLI: Run env with redaction flags
CLI->>Policy: Resolve RedactWhen
Policy-->>CLI: Return masking decision
CLI->>Environment: Pass redaction and sort settings
Environment-->>User: Render environment entries
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (4 passed)
Comment |
The Makefile passes $(CURDIR)/$(BIN), which on Windows is a drive-letter path like D:/a/envctl/envctl/envctl.exe. The absolute-path guard only recognised a leading slash, so it prefixed $PWD again and the harness reported the doubled path as not executable. tests/run.sh has accepted both forms since it was written; its guard was copied here without the drive-letter half.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
kjanat
changed the title
--redact=WHEN, and test completions by running them
Review findings on the branch. `envctl env` masks unconditionally by default, but an explicit WHEN replaces that default rather than layering on it, so `--redact=agent` leaves the dump unmasked when no agent is present. The registry text and both docs claimed only `--redact=never` could change it. They also said `--redact=never` prints the environment exactly as it stands, which skips that it still escapes control bytes on a terminal; only `--raw` drops both. The value spellings reached the per-command flag lists but not the arm each generator uses before a command word, so `envctl --redact=` offered nothing in bash, fish and pwsh while the parser accepts `envctl --redact=never env`. Only zsh was right, because its no-command arm already went through the shared spec writer. The harness never probed after a `=`, which is the whole of the new behaviour, and it never probed an alias. It does both now, and the values come from the parser itself: a bogus value makes it name the valid ones. Checked against a script with the spellings stripped, where all five values fail in every context including the one before a command word. `env-cmd-raw.case` used ordinary text, so it would have passed if `--raw` lost its control-byte half. It carries an ESC byte now. `shellcheck -x -o all` is clean across the suite. An unused alias map in the new harness is gone because the aliases are now exercised, the shell dispatches carry a default arm, and four pipelines in run.sh that discarded the first command's status were split.
CodeRabbit flagged the skip on an unavailable shell. The CI logs show what that was hiding: ubuntu-26.04 and windows-2025-vs2026 both printed `fish (not installed)` and `zsh (not installed)` and then passed, while macos-15 runs bash 3.2, where `mapfile` and `declare -A` do not exist, so the script died before its first check and `make test` still exited 0. The bash arm was the only one that had ever run, and zsh is the shell whose defect the harness was written to catch. The workflow installs zsh and fish on Linux, and fish on macOS where zsh is already present. It installs bash there too: Apple ships 3.2 for licensing reasons and the harness needs 4 for `mapfile` and associative arrays. A version check at the top now says so, rather than the script failing four times in a row and returning success. A missing shell is a failure rather than a skip. `COMPLETIONS_SHELLS` narrows the set for Windows, which has no native fish and reaches zsh only through MSYS2, and an empty value is rejected so the override cannot quietly check nothing. The closing line names the shells it drove, so a green run states its own coverage. CONTRIBUTING.md gains a section on the harness, which it had never mentioned, and its shellcheck line covers tests/*.sh instead of run.sh alone.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
macos-15 reported 57 mismatches, every one of them zsh, while bash and fish were clean and Windows was green. The driver wrote a TAB into the zpty, slept a second, then read whatever had arrived. That is a race, and a slower runner loses it. Dropping the sleep to 0.02s locally reproduces the failure exactly: the same 57 lines in the same order. The read now collects until the stream has been quiet for a second, with a ten second ceiling. zsh loads `_arguments` and its helpers from disk on the first completion in a fresh shell, which outruns that deadline on a cold cache, so each shell gets one discarded warm-up render first. Without it the first context failed and the rest passed, which is worse than a consistent failure. The harness also keeps its evidence now. stderr from the pty goes to a file rather than /dev/null, so a failed zpty setup is visible, and a mismatch dumps the raw bytes of the first failing context through `od -c`. The first cut printed the last render, which had usually succeeded. The zsh arm costs a second per render, taking `make test` from about 15s to 1m14s.
kjanat
added
enhancement
The label set had no name for the four shell scripts and the pwsh module, which is what this branch and issue #9 are mostly about.
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
1 participant
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.
The banner is gone
envctl envopened with# envctl v0.5.0 (redacted). That suffix already meanssomething in this tool:
src/diff.cwrites+++ .env (redacted), where thething before the parenthesis is the subject whose content is hidden. In the
banner the thing before it was the version string, so the line read as though
the version had been masked. Nothing in it ever was.
It also took the first line of what is otherwise a pure
KEY=VALUEstream, soenvctl env | head -1returned a comment instead of a variable, and everyconsumer had to know to skip a leading
#.crontab -lmade the same mistakeand broke
crontab -l | crontab -.Four
env-cmd-*cases only shelled out togit describebecause of the versionin that line. They are byte-literal again.
--redacttakes a WHENMasking was a tri-state written as two opposing booleans. Every tool with
terminal-conditional output uses one flag with values instead:
--color=always|auto|neverinls,grep,diff,git,rg,ip,dmesg.Three values would not have covered this one.
want_redactevaluates twoindependent signals, and agent detection is a module of its own with seventeen
cases behind it, so
autoalone would bury that axis:--redact=neverautoagentttyalwaysautostays the default, sopipe-agent-stays-raw.casestill holds: an agentreading through a pipe gets the real value, which is what keeps
TOKEN=$(envctl get TOKEN)working. That is a deliberate decision, and--redact=agentis the setting for anyone who wants it closed. It had nospelling before this.
A bare
--redactstill meansalways, so nothing existing changes.--rawstops doing two jobsIt meant "never mask" and "never escape control bytes".
lskeeps those apart,--colorfor one and-N,-b,-q,--show-control-charsfor the other.Escaping moves to
--show-control-charsunder GNU's name, and--rawbecomesthe shorthand for both.
--rawworks onenvnow. Refusing it there bought no safety withenv(1)sitting next to it; it only cost the caller envctl's key filtering, sorting and
escaping.
Completions are tested by running them
The old coverage compared the generator against a recorded copy of its own
output. That cannot catch an emitter that puts correct data somewhere the shell
applies too widely, which is precisely what shipped: the zsh per-command arm
listed each command's flags correctly the whole time, while the top-level
_argumentskept its specs live past the command word, soenvctl env --redact --sort -offered--raw. The suite stayed greenthroughout.
tests/completions.shruns inmake test. It asks bash and fish for theircandidate lists directly, drives zsh through
zpty, and asks the binary whethereach flag is valid for that command, probing contexts including one where a flag
is already typed, which is where the fault lived. It was checked against a copy
of the script with the fault put back, and fails there.
On its first run it found another one: bash returned the shell list for
completionsandmodulebefore it ever looked at flags, soenvctl completions -offered nothing at all.Verification
make test: 250 cases plus the completion harnesszsh -n,bash -n,fish --no-executeon every regenerated script--redact=od -c, sincecat -vrenders an escapedcaret and a real ESC identically
dprint fmtcleanNot in this change
The ten
c->id == CMD_*branches insrc/complete.cstill encode facts theregistry does not carry. That refactor is #9. The harness above is what keeps
them honest until then.