Accept any openable path for reads and add native environment support - #2
Conversation
envctl redact <(env) failed with "no such file: /proc/self/fd/N": the
regular-file gate rejected FIFOs even though every read path handles
them fine. Split the predicate - read commands (get, list, redact's
env file) now accept any existing path, while the mutating commands
keep requiring a regular file and report "not a regular file" when
handed anything else, keeping "no such file" truthful.
Add the process environment as a native source, so no env binary is
needed:
- get --env KEY and list --env [--values] read the environment under
the existing redact rules, so scripts stay composable
- redact --env uses the environment's values as the literal mask set
when filtering stdin
- envctl env prints the whole environment with redaction always on,
the safe one-word replacement for env | envctl redact --no-env
Environ entries are atomic KEY=VALUE pairs: env-file span logic never
applies, and a value containing newlines takes the segment-masking
path that multi-line file values already use.
Test harness: a fifo-file section serves a fixture through a FIFO with
{FIFO} argv expansion, standing in for process substitution; posix-env
mode gates environ-order expectations off Windows, where the runtime
reorders the environment block.
Breaking: env is now a command word; a key literally named env needs
the explicit form envctl get env.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used🔍 Remote MCP GitHub GrepRelevant review context
🔇 Additional comments (1)
📝 WalkthroughAdd support for non-regular read paths and process-environment sources Read commands now accept FIFOs, Add Expand the test harness with FIFO fixtures, POSIX environment-order handling and 19 new test cases. WalkthroughAdded process-environment retrieval, listing, dumping, and redaction through new Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant envctl
participant envsrc
participant maskset
User->>envctl: run env or get --env
envctl->>envsrc: enumerate or retrieve environment entries
envsrc-->>envctl: entries or requested value
envctl-->>User: print environment output
User->>envctl: run redact --env
envctl->>envsrc: obtain environment entries
envsrc-->>envctl: KEY=VALUE entries
envctl->>maskset: load environment values
maskset-->>envctl: configured masks
envctl-->>User: redact input and print output
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (5 passed)
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: 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 `@src/help.c`:
- Line 38: Update the redact command synopsis in the help text to include the
supported --no-env option alongside the existing file and --env choices, while
preserving the current masking description.
In `@tests/cases/get-env-missing.case`:
- Around line 1-4: Update the get-env-missing fixture’s --env input to use the
harness’s unset facility or a guaranteed-unique variable name, ensuring the
variable is deterministically absent regardless of the developer or CI
environment.
🪄 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: 2db8f715-07ee-4621-9d2b-2e3ca2b16897
⛔ Files ignored due to path filters (2)
tests/fixtures/expected/help-agent.outis excluded by!**/*.outtests/fixtures/expected/help-long.outis excluded by!**/*.out
📒 Files selected for processing (32)
CONTRIBUTING.mdMakefileREADME.mdsrc/envsrc.csrc/envsrc.hsrc/filter.csrc/filter.hsrc/help.csrc/main.csrc/mask.csrc/mask.htests/cases/bare-env-get.casetests/cases/bare-env-set-rejected.casetests/cases/env-cmd-basic.casetests/cases/env-cmd-rejects-positional.casetests/cases/env-cmd-rejects-raw.casetests/cases/error-set-fifo-not-regular.casetests/cases/filter-fifo-envfile.casetests/cases/get-env-basic.casetests/cases/get-env-missing.casetests/cases/get-env-redacted.casetests/cases/get-fifo-bare.casetests/cases/help-short.casetests/cases/list-env-keys.casetests/cases/list-env-values-redacted.casetests/cases/list-fifo-values.casetests/cases/redact-env-literals.casetests/cases/redact-env-noenv-conflict.casetests/cases/redact-env-with-file.casetests/cases/set-env-rejected.casetests/cases/tty-agent-env-get.casetests/run.sh
📜 Review details
🧰 Additional context used
🪛 Cppcheck (2.21.0)
src/mask.c
[style] 320-320: The function 'maskset_load_env' is never used.
(unusedFunction)
src/filter.c
[style] 13-13: The function 'act_redact' is never used.
(unusedFunction)
src/envsrc.c
[style] 27-27: The function 'act_env_get' is never used.
(unusedFunction)
[style] 60-60: The function 'act_env_dump' is never used.
(unusedFunction)
[style] 24-24: The function 'env_entries' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
[style] 35-35: The function 'act_env_list' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
🪛 Shellcheck (0.11.0)
tests/run.sh
[info] 48-50: Consider adding a default *) case, even if it just exits with error.
(SC2249)
[info] 56-58: Consider adding a default *) case, even if it just exits with error.
(SC2249)
🔍 Remote MCP GitHub Grep
Relevant external context
- GNU Coreutils’
printenviteratesenviron, skips arguments containing=, and matches variable names against entries—useful comparison forenv_entries()lookup and malformed-entry handling. - H2O counts and scans
environviaextern char **environ, confirming the conventional POSIX integration pattern used by the new environment source. - Ruby and mruby enumerate environment entries by locating the first
=withstrchr, consistent with treating the remainder—including additional=characters—as the value. - util-linux FIFO tests create a FIFO, launch writers asynchronously, read synchronization data from the FIFO, and explicitly terminate/reap writers during cleanup.
- FreeBSD tests exercise reader/writer lifecycle behavior and verify that closing a FIFO can terminate the writer, providing a useful comparison for the PR’s
epipe-openand FIFO cleanup handling. - No public repository containing the exact
envctl,maskset_load_env, oract_env_listsymbols was found, so target-specific implementation validation was not possible from GitHub Grep.
🔇 Additional comments (31)
src/envsrc.h (1)
1-9: LGTM!src/envsrc.c (1)
1-60: LGTM!tests/cases/get-env-redacted.case (1)
1-11: LGTM!tests/cases/help-short.case (1)
7-12: LGTM!tests/cases/list-env-keys.case (1)
1-13: LGTM!tests/cases/list-env-values-redacted.case (1)
1-17: LGTM!tests/cases/redact-env-literals.case (1)
1-16: LGTM!tests/cases/redact-env-noenv-conflict.case (1)
1-12: LGTM!tests/cases/redact-env-with-file.case (1)
1-15: LGTM!tests/cases/set-env-rejected.case (1)
1-13: LGTM!src/mask.h (1)
32-32: LGTM!src/mask.c (1)
314-338: LGTM!src/filter.h (1)
4-4: LGTM!src/filter.c (1)
4-4: LGTM!Also applies to: 13-13, 25-27
tests/cases/tty-agent-env-get.case (1)
1-15: LGTM!CONTRIBUTING.md (1)
23-23: LGTM!Also applies to: 50-53, 63-74
tests/run.sh (1)
42-59: LGTM!Also applies to: 81-81, 221-228, 248-278, 378-384
tests/cases/error-set-fifo-not-regular.case (1)
1-17: LGTM!tests/cases/filter-fifo-envfile.case (1)
1-19: LGTM!tests/cases/get-fifo-bare.case (1)
1-10: LGTM!tests/cases/list-fifo-values.case (1)
1-13: LGTM!Makefile (1)
11-12: LGTM!src/main.c (1)
8-8: LGTM!Also applies to: 30-30, 44-52, 64-64, 100-101, 120-121, 156-169, 179-236, 245-260, 276-280
src/help.c (1)
11-16: LGTM!Also applies to: 41-45, 55-58, 83-85
README.md (1)
73-97: LGTM!Also applies to: 108-109, 118-124, 155-197, 258-259
tests/cases/bare-env-get.case (1)
1-10: LGTM!tests/cases/bare-env-set-rejected.case (1)
1-13: LGTM!tests/cases/env-cmd-basic.case (1)
1-15: LGTM!tests/cases/env-cmd-rejects-positional.case (1)
1-12: LGTM!tests/cases/env-cmd-rejects-raw.case (1)
1-12: LGTM!tests/cases/get-env-basic.case (1)
1-11: LGTM!
1 participant
Problem
envctl redact <(env)failed withenvctl: no such file: /proc/self/fd/14. Process-substitution paths are FIFOs, and the file gate requiredS_ISREGbefore ever opening anything — then blamed the rejection on a missing file. The reader itself is plainfopenand handles FIFOs fine; only the mutating commands genuinely need a regular file (atomic temp + rename).There was also no way to read the process environment natively — an env dump had to be piped in through the
envbinary.Fix
Split the file predicate:
get,list,redact's env file, bare-form get) accept any existing openable path: FIFOs from<(...),/dev/fd/N, character devices.set,disable,enable,delete) keep requiring a regular file and now fail withnot a regular file: PATH;no such fileis reserved for paths that actually don't exist../.envdefault still requires a regular file, per the documented contract.Native environment support
envctl env— prints the whole environment asKEY=VALUEwith redaction always on (rejects--raw, like filter mode). One-word replacement forenv | envctl redact --no-env.envctl get --env KEY— exit 1 if unset; follows the existing get redact rules, so it stays composable in scripts.envctl list --env [--values]— same rules as filelist.envctl redact --env— filter stdin using the environment's values (plus base64/hex/URL/JSON-escaped encodings) as the literal mask set.Environ entries are treated as atomic KEY=VALUE pairs in a dedicated
envsrcmodule: env-file span logic never applies (a value with an unclosed quote can't swallow the next variable), and values with embedded newlines take the same segment-masking path as multi-line file values.Breaking:
envis now a command word. A key literally namedenvneeds the explicit formenvctl get env(consistent with the existing "command name wins" rule).Tests
19 new cases, all verified to fail against the unfixed binary. Harness additions:
fifo-filesection: serves a fixture through a FIFO with{FIFO}argv expansion, standing in for process substitution; skipped wheremkfifois unusable (incl. MSYS).posix-envmode: gates environ-order expectations off Windows, where the runtime reorders the environment block.filter-epipe-reports-write-failurefails in my sandbox both before and after this change (container pipe-timing); unrelated.Docs
README (usage, default-file rules, flags table, filter mode, new Process environment section),
-h/--helptext and regenerated fixtures, CONTRIBUTING (envsrc module,fifo-filesection, mode table).