fix: clear inspect child environment by default - #271
Conversation
vyncint
left a comment
There was a problem hiding this comment.
Reviewed and verified. The behaviour is right and the PATH decision is the one I would have made.
What I checked beyond CI, by running the built example:
- Default clears the environment: a
FOO=leakedin the caller's shell reaches the child as unset, whilePATHsurvives. --envis genuinely repeatable (--env A=1 --env B=2sets both), splits on the first=so--env 'K=a=b'givesKthe valuea=b, and rejects both a missing=and an empty key in one line with exit 1, in the same shape--sizeuses.--inherit-envrestores the old behaviour exactly.- A bare program name still resolves, which is the whole point of keeping
PATH— and your new test exercises that path by spawningsh, which is neat. --helplists both new flags, and the usage text stays one source.
Keeping PATH is a documented exception rather than a hole, and the usage text says so, which is what the issue asked for.
Two more changes, alongside the shared one below
-
The new test's assertion has no failure message. Every other assertion in
crates/termlens/tests/inspect.rsprints the output it was reading —"terminal size missing from:\n{stdout}"and so on — so a failure shows what the screen actually held. As written, a regression here printsassertion failed: …contains("unset:yes")and nothing else. Please give it the same treatment. -
--inherit-envhas no test. It is the flag that restores the previous behaviour, so it is the one most likely to be quietly broken by a later change to the argument loop. One more case in the same test, asserting that a variable set on theCommanddoes reach the child under--inherit-env, would close it. That also makes the pair of cases read as one statement about the policy.
One thing to fix, and it is the same in all five of your PRs
The CHANGELOG entry landed inside the published ## [0.9.0] - 2026-09-05 section rather than under ## [Unreleased]. Easy to miss: [Unreleased] was empty when you branched, so the nearest heading with content under it belongs to 0.9.0.
It matters for two reasons. 0.9.0 is already on crates.io and its GitHub Release notes were generated from that section, so the entry now describes a release that does not contain the change. And docs/RELEASING.md cuts the next release by moving [Unreleased] down into a new version heading — an entry parked in [0.9.0] would never appear in any release notes at all.
Move it up under ## [Unreleased], adding the ### Added / ### Changed / ### Fixed sub-heading there if it does not exist yet. main has moved on by a commit since you branched, so a rebase is wanted anyway:
git fetch origin && git rebase origin/mainThanks for these — five well-scoped fixes with tests and docs, and the PR bodies say what you decided and why. That is exactly the shape this repository asks for.
Signed-off-by: nightcityblade <nightcityblade@gmail.com>
782d9e9 to
5d3aec5
Compare
|
Addressed all requested changes in 5d3aec5: rebased and moved the changelog entry under Validation: |
What & why
Make the
inspectexample clear the child environment by default so it matches hermetic termlens tests.PATHis deliberately retained so bare program names still resolve; repeatable--env KEY=VALUEadds selected variables, while--inherit-envrestores the previous behavior. End-to-end coverage verifies both clearing and selective injection.Closes #263.
Checklist
cargo fmt --allandcargo clippy --workspace --all-targets --all-featuresare cleangit commit -s) — see CONTRIBUTING.md §5CHANGELOG.mdupdated under[Unreleased](user-facing changes only)cargo insta review, not blind-accepted (N/A: no snapshots changed)