feat(run): CLI-proxy integration for Terragrunt, plus an e2e tier and two credential fixes - #13
Open
AlexMKX wants to merge 142 commits into
Open
feat(run): CLI-proxy integration for Terragrunt, plus an e2e tier and two credential fixes#13AlexMKX wants to merge 142 commits into
AlexMKX wants to merge 142 commits into
Conversation
tunstrap writes structured errors to stdout, and Terragrunt inlines subprocess output into its own error messages, so anything echoed there reaches CI logs. Three sites serialised a pydantic ValidationError in a way that included the offending input values -- ssh_pkey, ssh_password and ssh_pkey_passphrase among them. - cli.py, cli_input.py: use errors(include_input=False, include_url=False, include_context=False); ValidationError.json() defaults include_input=True. - schemas.py: build the remote_targets message from the error msgs rather than str(ValidationError), which embeds input_value=. - exceptions.py: _scrub now recurses through nested dicts and lists; it previously stripped secret keys only at the top level of details. Blast radius was wider than the failing field: the validator over the whole nodes mapping meant one failure echoed every node's secrets, including nodes that were themselves valid. loc and msg are preserved, so errors still identify the offending field.
Replaces the run_cmd-from-inputs integration with a process wrapper around the OpenTofu binary, hooked via Terragrunt's terraform_binary. This removes the marker file, the after_hook, the jq, and the InputSchema from argv. Spec adds three generic capabilities to tunstrap, with no Terraform knowledge inside it: - --input-env VAR: read InputSchema from an environment variable. Forced by the shape of a foreground wrapper -- run cannot use stdin, which belongs to the child. - --output-var NAME: inject the full OutputSchema as JSON into the child's environment. - run's argument surface becomes a single variadic, because -- terminates only option parsing and an optional CONNECTION positional would swallow the child's executable. Also specified: run must not write to stdout (Terragrunt parses tofu's), and cleanup must own the whole post-spawn window. Measured, not assumed: dependency.* resolves inside extra_arguments.env_vars on Terragrunt 1.1.1 but not in locals; inputs travel as TF_VAR_* env vars; env_vars reach the listed command and its auto-init but not -version; a ~10KB payload with PEM material arrives byte-identical. An end-to-end run against a real kind cluster applied a Namespace and a helm_release through the tunnel and destroyed them again. The plan covers the core CLI changes only. The e2e tier and the docs deliverables are planned separately.
Scan before execution found five issues: - Tasks 0/6 assumed the ValidationError fix was uncommitted working-tree state; it is now committed (defd1cf) and merged (53f7832), so the baseline task verifies its presence instead of guarding a dirty tree. - Per-task ruff gates linted only the production file while also changing a test module. Measuring revealed the gates' 'all clean' expectation was already false: cli.py carries 3 pre-existing errors. All 17 gates now state an exact count against a recorded baseline. - _cleaning_teardown was duplicated verbatim in three test modules; extracted to tests/unit/conftest.py. - The 'complete envelope' test could not fail: its fixture omitted warnings, which pydantic defaults to [], so an implementation dropping the field passed. Rebuilt on a fixture where every value is non-default. - The multi-node scalar-suppression tests could pass while masked by an inherited KUBECONFIG. Rebuilt as two-nodes-in/one-connection-out, where an implementation reading the output node count leaks both TUNSTRAP_* and KUBECONFIG.
Three of the five tests the task specified were never written, and the two
that were are vacuous against the implementations they exist to catch.
- add the _RICH_KUBE/_RICH_PAYLOAD fixtures; every value is non-default, so
an envelope that drops a field cannot be refilled by a pydantic default
- rebuild test_output_var_carries_complete_envelope on _RICH_PAYLOAD with
restated per-field assertions; the old _conn fixture left fetch_files and
kube_targets at {}, so a lossy projection round-tripped equal
- give the survivor in test_multi_node_suppression_uses_input_count a real
kube target, so render_env reaches 'if kube_paths:' and the KUBECONFIG
assertion can actually fire under an output-count implementation
- write test_single_node_keeps_scalars_alongside_output_var: without it, an
output-var-as-replacement implementation passed 10 of 11 tests
- write test_multi_node_injects_output_var_and_no_scalars and
test_child_env_without_output_var_is_unchanged
Each is recorded failing against its target wrong implementation in
.superpowers/sdd/task-3.6-report.md. Tests only; tunstrap/ is untouched.
run learned its session directory from the daemon's success payload, so cleanup depended on parsing the very object whose validation can fail: a raising OutputSchema.model_validate left a live daemon whose location nothing knew. Minting the path before the spawn makes it a precondition of spawning rather than a fact discovered afterwards. - add _mint_session_dir and _discard_minted_root; drop resolved_session_dir = out.session_dir, so run never takes the session path from the payload again - _teardown_run/_teardown_run_inner take a keyword-only minted_root and remove the root run minted, never a caller-supplied --session-dir (a supplied path makes the worker's SessionDir non-generated, so the worker never removes the root) - discard the minted root on both pre-child failure paths, and retry the discard in _teardown_run's handler, which a raising stop primitive would otherwise short-circuit The brief's except click.UsageError clause is omitted: the next handler is except TunstrapError, which click.UsageError does not subclass, so it was dead code that added two pylint findings. The invariant it documented is kept as a comment at the minting site. Each test is recorded failing against the wrong implementation it exists to catch in .superpowers/sdd/task-4.1-report.md.
…teardown test_supplied_session_dir_is_never_minted_or_removed asserted that a caller-supplied --session-dir survives, but routed teardown through the teardowns fixture, whose cleaning_teardown removes only minted_root. The supplied directory therefore survived by construction and the assertion could not detect the regression it named: a _teardown_run_inner that removes session_dir unconditionally. - rename to test_supplied_session_dir_is_never_minted and drop the survival assertion, keeping only the wiring claim the stub can prove - add test_production_teardown_keeps_a_supplied_session_dir, which runs the real _teardown_run against a supplied root holding a caller-owned sentinel and a real tunnel-data/daemon.pid, and asserts tunnel-data is cleaned while the root and the sentinel survive All three assertions are recorded failing against a distinct wrong teardown in .superpowers/sdd/task-4.1-report.md; the pre-fix shape passes against the first of them, which is the finding reproduced directly. Tests only; tunstrap/ is untouched.
Everything between a successful spawn_daemon and the child's exit now runs inside cleanup ownership. Two holes carried forward from Phase 3/4.1 are closed: - OutputSchema.model_validate ran before the try opened, so a malformed success payload orphaned a live daemon. It now runs in _run_child, inside _supervise_child's teardown try. - _build_child_env ran after the except TunstrapError handler closed, so a raise there was neither mapped by the error guard (observed exit 1) nor covered by cleanup. Both consequences are gone. The envelope read needed more than the brief specified: message["kind"] and message["payload"] were indexed outside the window, and passing message["payload"] as an argument expression evaluates it in the caller, still outside the callee's try. Both keys are now read in a guarded step that tears down first, since an unreadable envelope leaves us unable to tell whether a worker is live. The kind != success branch still does not tear down: session_active means the recorded pid belongs to another live session, which we must not stop. Signal restoration is nested in its own try whose finally performs the teardown, and handlers are captured inside the outer try, so neither a failed capture nor a failed restore can skip the stop. Unexpected errors become a DaemonError envelope on stderr, never stdout, and exit 4. Phase 2 hardening in _teardown_run is untouched: BaseException, _warn, and silence for an already-exited daemon all remain. Each test is recorded failing against the specific wrong implementation it targets in .superpowers/sdd/task-4.2-report.md; the RED run also left 7 orphaned session roots on disk, which the fixed code reduces to zero.
… gate Three follow-ups from review of Task 4.2. 1. black --check . was red on tests/unit/test_cli_run_postspawn.py. Bisecting the file shows it was clean at 1afa980 and broke at 21949ef, so it came from the assertion added in Task 4.1, not from Task 2.4 -- my earlier reports called it pre-existing only because HEAD was my own prior commit. ruff format and black disagreed over an assert whose message sat in a hugged parenthesis; the construct is replaced by a named local plus a short assert, which both formatters accept and which names the leaked path on failure. Re-proved falsifiable. 2. The post-spawn guard was except Exception -> DaemonError -> 4, so a TunstrapError raised inside the window lost its mapping. A lone required:false node that fails yields a success envelope with no connections (manager.py:99-107), render_env raises MultiNodeEnvUnsupported, and the user saw exit 4 with an unexpected failure message instead of the documented exit 1. An except TunstrapError clause now precedes the generic guard; teardown already ran in _supervise_child's finally on both branches. 3. signal_guard is autouse: every test in the module that reaches _run_child installs _forward as this process's SIGINT/SIGTERM handler, and a failure while restoration is broken leaked it for the rest of the session. Phase 2 hardening in _teardown_run is untouched.
Five tests driving the installed console script against the docker rig: a real daemon, real forwards, a real child process. Until now the feature was covered only by unit tests with spawn_daemon and Popen faked. Both the single- and multi-node tests forward through sshd-bastion. It is the only service in the rig with AllowTcpForwarding enabled and a route to the internal target-1, so the multi-node case uses the bastion twice under two node keys rather than a second container. The tests pass on first run -- the feature already exists, so there is no natural red. They are proved load-bearing by two reverted mutations of _build_child_env, recorded verbatim in task-5.1-report.md. The second mutation also confirms end to end that a post-spawn TunstrapError keeps its own exit code (1, not the generic guard's 4). No assertion on KUBECONFIG absence: these nodes declare no kube_targets, so render_env would never set it and such a check could not fail. The popping of an inherited KUBECONFIG in the helper is hygiene, and says so; the falsifiable version of that guarantee is the unit test test_multi_node_suppression_uses_input_count. Tests only; tunstrap/ is untouched.
The tofu-proxy pattern rests on one claim: run emits nothing on fd 1 but the child's own bytes. The unit tests cannot prove it -- CliRunner swaps sys.stdout for an in-memory object, so an assertion there could not notice an os.write(1, ...) or a grandchild inheriting the descriptor. This is the first and only place the invariant is checked against a real process. The oracle is differential and byte-level, not a literal: each test compares the wrapped process's stdout with the bytes the same child script produces with no wrapper, captured as bytes rather than text so universal-newline translation cannot hide a rewritten \r\n. The child emits a CR, an LF and no trailing newline, so an appended diagnostic, a stripped terminator or a translated line ending all change the result. The baseline asserts on itself so a mis-built script cannot reduce the comparison to b"" == b"". Verified rather than assumed: with os.write(1, b"LEAK") injected into _teardown_run_inner the whole unit suite still passes (354) while these tests fail. The brief's sys.stdout.write mutation turns all four red. Both were reverted. Four scenarios: clean run, non-zero child, zero grace, and a tampered identity whose orphaned daemon the test stops itself via session.lock. Which teardown outcomes are and are not deterministically forceable is recorded in task-5.2-report.md -- notably the clean in-grace stop appears unreachable today, which is the pre-existing grace-loop defect from Task 5.1. Tests only; tunstrap/ is untouched.
This was referenced Aug 9, 2026
Owner
Author
Red-team review — corrections to the PR descriptionCross-model review of this branch @ 1. Code-health numbers are stale. The body claims Independent measurements at
All three independently confirm 2. The (Caveat: it is precisely that path-based delivery that the collision bug in the linked issue subverts.) 3. 4. For context, the gates themselves are all clean and were reproduced by all three reviewers: mypy |
Owner
Author
Filed issues from this review
#16 is the "linked issue" referenced in point 2 above — it is the path-based delivery that makes the collision a credential substitution rather than a cosmetic clash. Blocking for merge, in my read: #16 and #17. #18 and #19 should land with them — the first is inconsistent with this PR's own security thesis, the second means the primary public document is wrong in both directions. SSH host-key verification ( |
A corrupt or hostile `tunnel-data/daemon.pid` of `0` or `-1` is not a process. Under `kill(2)` those values select a process *group* -- `0` the caller's own, `-1` every process the caller may signal -- and under `waitpid(2)` they select a child group, or for `-1` any child. Letting such a value reach either syscall turns a targeted stop into a broadcast, and lets the reap steal the exit status of `run`'s foreground child. `read_identity` now rejects it at the reader, raising `SessionIdentityUnreadable` so `stop` and `run`'s teardown preserve the session rather than act on it, and `_process_exists` refuses it at the liveness probe, where `os.kill(pid, 0)` on a non-positive pid is a group/broadcast probe that answered True for any live host -- which is what let a hostile `session.lock` body of `-1` verify as `match`. Neither of those covers a caller that reaches the primitives directly, so two independent guards do, and neither leans on the gate above it: - `stop_session` re-asserts `pid > 0` at its entry, before `verify_session` and before any signal, so it also covers `verify_session`'s own signal-0 probe. It returns `identity check unavailable` -- an unresolved outcome, so the caller preserves rather than deletes, the same disposal `read_identity` demands for the identical value. `not found` would have been wrong in a way worth naming: `cli._stop_resolved` classifies it as resolved, so the guard would have suppressed the signal correctly and then destroyed the only handle on an unaddressable daemon. The reason string is one of the already-documented set, so `stop`'s JSON shapes are unchanged to the byte and no consumer sees a new value. - `_has_exited` keeps the same value off `waitpid`. It is module-level, so a future caller could reach it without passing `stop_session`'s entry guard; its guard is therefore pinned in its own right rather than by proxy. Each guard is pinned by its own test, verified by mutation rather than by inspection. Deleting `stop_session`'s entry guard fails only `test_stop_session_never_signals_a_non_positive_pid[zero|minus-one]`; relaxing `_has_exited`'s `pid > 0` to `if True` fails only `test_non_positive_pid_never_reaches_waitpid[zero|minus-one]`. That second mutation is why the test targets `_has_exited` directly: routed through `stop_session` it returned at the entry guard, so the documented `pid > 0` guard was completely unpinned -- the mutated tree passed all 503 tests. One test drives the composed path end to end: a real `daemon.pid` of `-1` through the real `stop` verb, asserting no `os.kill` is recorded, the session is preserved, and `tunnel-data` survives. Without it a regression that reintroduces the hole by a different route -- a new `stop_session` caller that skips `read_identity`, or a change to which exception `cli.py` catches -- would pass the whole suite.
… writes (#25) `acquire_session_lock` opened `session.lock` with `O_CREAT|O_RDWR` and then `ftruncate`d it. With `--session-dir` pointing somewhere a second uid can write -- a shared CI workspace, a group-writable scratch dir -- that uid plants `session.lock` as a symlink and tunstrap destroys whatever it points at. Reproduced end to end: `nobody` erased a 0600 file owned by the runner that it could not open directly, using tunstrap's privilege as the ladder. `session.py`'s own module docstring already declared `--session-dir` untrusted, and `_reclaim_data_slot` already rejected a symlinked or foreign-owned `tunnel-data` -- the posture existed and simply was not applied to the one path that truncates. The lock open now carries `O_NOFOLLOW|O_CLOEXEC` and, before any truncate, `fstat`s the descriptor for a regular file, owned by us, with exactly one link. Each clause covers what the others miss. `O_NOFOLLOW` stops a symlink but not a foreign-owned regular file planted in a writable root. Ownership stops that but not a hardlink -- a hardlink is not a symlink, so `O_NOFOLLOW` is silent, and it shares the victim's inode, so the file genuinely is a regular file genuinely owned by us. Only `st_nlink` tells the two names apart; without it a link planted before tunstrap ever ran leaves the victim at 8 bytes of daemon pid. The root itself is now secured too, because none of the above reaches an entry-level attack: directory write permission is authority over *entries*, independent of the mode and ownership of the files inside. A second uid with write on the root can unlink the live `session.lock` -- a fresh inode then passes every check above and wins flock, since flock is per-inode -- after which `_reclaim_data_slot` rmtrees a *running* session's kubeconfigs on the premise that holding the lock proves the session is dead. Or it renames `tunnel-data` aside (a rename within the parent needs write on the parent only) and substitutes a symlink, which `_validated_path` could not see: `path.resolve().parent != self._data.resolve()` resolves both sides through the attacker's link and compares equal, so the containment check was a no-op in exactly the case it existed for, and the patched kubeconfig -- with `client_key_data` -- landed in attacker space. `_validated_path` now rejects a symlinked `tunnel-data` outright. The root guard tightens rather than refuses. Refusing group-writable was tried first and is wrong: the mode cannot distinguish a user-private group (the Debian/Ubuntu default, zero cross-uid risk) from a genuinely shared one, so `mkdir d && tunstrap run --session-dir d` failed with a generic DaemonError on any stock umask-0002 account. Since ownership is already proven the runner may simply set the mode, so `_secure_supplied_root` clears `S_IWGRP|S_IWOTH` and leaves read/exec alone -- a legitimate 0755 root stays 0755 -- then re-`fstat`s through the same fd and refuses only if the bits survived, so an ACL mask or a filesystem that ignores `fchmod` cannot ship as "accept anything". It is fd-based throughout, so a rename-symlink swap cannot retarget the chmod. `_check_lock` gains `O_NOFOLLOW` on the same grounds: `status` and `stop` should not follow a hostile lock either. Three pre-existing tests had been quietly retargeted to `mkdir(mode=0o700)` to satisfy the refusing version, which is what kept the regression invisible; they are back to a bare `mkdir()`. `test_rejects_symlink_tunnel_data` was worse than retargeted -- under umask 0002 it had started failing on the root guard instead of the symlink guard it exists to pin, and stayed green because it only asserted `SessionError`. Assertions that pin one specific guard now carry `match=`, so that rot cannot recur silently. Every new guard was verified by deleting it and watching exactly one test go red.
`os.write` may return fewer bytes than it was handed. Both raw-fd writers in `session.py` ignored the count, so a short write silently truncated the file and reported success -- `daemon.pid`, materialized kubeconfigs and fetched files all ride these two paths. The project already knew this: `_worker`'s `_write_message` loops correctly. The loop now lives in one `_write_all` helper both writers share, including the `written <= 0` no-progress guard that stops a zero return spinning forever. `atomic_write` also orphaned its temp file. On any exception the pid-pinned `.name.<pid>.tmp` survived, and because `O_EXCL` rejects an existing name, every later write *from the same pid* failed permanently -- one transient error poisoned the writer for the life of the process. Cleanup now unlinks it and re-raises. `os.replace` moved inside the guarded block so a failing rename is cleaned up the same way; rename is atomic, so the destination is never left partial either way. Two docstring corrections fall out of this. The old text credited `O_EXCL` with guarding "against a colliding concurrent writer" -- it never did: the temp name embeds the pid, so two processes cannot collide on it, and concurrent writers are resolved at the destination by `os.replace`. What `O_EXCL` actually guards is same-pid re-entry, which is precisely what the orphaned temp broke. The parent `mkdir` gains `mode=0o700`, but it is defence-in-depth rather than a live hole and is documented as such: `tunnel-data` is always minted 0700 by `SessionDir.create` before either caller reaches `atomic_write`, so the mkdir is an unconditional no-op in production and only a direct caller could observe the umask-derived 0775. Noted in the docstring because `Path.mkdir(parents=True, mode=...)` applies the mode to the leaf only -- intermediate components are still created at `0o777 & ~umask`, so this call secures one directory, not a chain. Each guard is pinned by mutation, not inspection: reverting the loop fails three tests, removing the cleanup fails one, dropping `mode=0o700` fails one. Short writes are forced by a labelled `os.write` stand-in, since a real partial write to a local regular file cannot be provoked on demand.
…ts (#16) Both materializers wrote `tunnel-data/{node}-{name}`. Same directory, same key format, no separation -- and `_start_one` runs fetch first, kube second, so a kubeconfig silently overwrote a fetched file that happened to share a name. The unified output then advertised `fetch_files.<f>.size` and `.sha256` of the fetched file while `.path` pointed at the kubeconfig's bytes, private key included. A consumer reading that path got cluster credentials where it expected its own file, and nothing in the envelope said so. Reachable from the documented CLI, since `run` always materializes: tunstrap run host --fetch k=/etc/some/file --kube k=/etc/rancher/k3s/k3s.yaml -- tofu plan Leaves are now kind-prefixed: `fetch-<node>-<name>` and `kube-<node>-<name>`. That is deliberately a flat rename rather than the `kube/` and `fetch/` subdirectories the report suggested -- subdirectories would mean permitting `/` in a materialized name, and `_validated_path` rejects `/` precisely to block traversal. A prefix buys the same separation without touching that guard. Cross-kind collision is then impossible by construction: the two kind literals differ at their first character, so no kube leaf can equal a fetch leaf whatever the node and item are called -- including a node named `kube`, since the discriminator is position 0, not a substring. Within one kind the join is still ambiguous, and that half needs a validator: `node "a-b"` + `"c"` and `node "a"` + `"b-c"` both render `fetch-a-b-c`. `_validate_kube_identity_names_are_unique` already rejected this for kube identities and now also rejects it over the full materialized set, `nodes x (kube_targets u fetch_files)`. The two passes stay separate because they guard different namespaces -- the identity pass renders `tunstrap-<node>-<target>`, which is the kubeconfig *context* name and is not changed here. Neither the naming scheme nor the validator is sufficient alone, and the naming helper's docstring says so. Both materializers now use the atomic primitive. Keeping the kube write on the truncate-then-write path would leave a window where a provider reading `KUBE_CONFIG_PATH` observes a half-written kubeconfig, and the old docstring justified the split by a difference that no longer exists -- both writes happen in the same daemon process from the same `_start_one` path. That retires `SessionDir.materialize`, which had no remaining caller, so it is gone; the traversal and symlink guards it carried are re-pointed at `materialize_atomic`, and `_write_file`'s short-write loop is now pinned through `write_identity`, its only surviving caller. The two materializers had no test coverage at all. They now take the session explicitly instead of asserting it is not None -- the call sites were already guarded, so the assert could not fire and converting it to a raise would only have swapped one unreachable branch for another. That also retires two of the asserts owned by #26. README's claim that the kubeconfig lands at `tunnel-data/<node>-<target>` was left true by correcting it, and now adds that leaf names are an implementation detail: consumers read `path` from the envelope rather than construct it.
…to disk (#18) `start --output json` dumped the worker's payload raw, so stdout carried `client_key_data`, `client_certificate_data`, `certificate_authority_data` and the whole `content_b64` kubeconfig -- into `OUT=$(tunstrap start ...)`, into CI logs, into anything that captures a pipe. The `run` / `--output-var` path had already decided this was unacceptable and narrowed the identical data to `path` / `context` / `endpoint`. Two channels, one dataset, opposite postures. The report proposed projecting `start` unconditionally. That would have broken a supported mode. `daemon.materialize` defaults to **false**, and with nothing on disk `content_b64` is the only way to obtain the kubeconfig at all -- the README quickstart consumes exactly that. The leak is not the field; it is printing the field when the bytes are already sitting at a 0600 path. So the discriminator is `path is not None`, per entry. A materialized kube target renders through the same `UnifiedKubeRef` allow-list `run` uses; an unmaterialized one keeps the full envelope. Fetched files get the same treatment through `UnifiedFetchRef`, because `FetchedFile` carries `content_b64` too and #16 just gave it a `path` -- and a fetched file is whatever secret the operator pointed at, so half a fix would have been the more dangerous half. Errored fetch entries have no content and fall out naturally. `path is not None` rather than `daemon.materialize` because the render site only has the payload. The two are equivalent today -- the session is bound once per daemon -- and the docstring records that, so a future divergence fails safe toward projection. What this does not do is make the default safe. With `materialize` false, `start` still prints credentials to stdout, by design, because that mode exists to keep decoded content off disk. Flipping the default would silently write private keys on every invocation. That trade belongs to the operator, so it is now documented where an operator will actually meet it: next to the example that consumes `content_b64`, and in the Terragrunt recipe. Two README claims this change falsified are corrected with it: `start` is no longer "unaffected", and `content_b64` is no longer "always present". The remaining `--output-var` section is stale from #15/#16 in ways this change did not cause; #19 owns it.
…space (#20) `rename_identities` assigned `tunstrap-<node>-<target>` without checking whether the fetched file already contained it. When it did, the patched document ended up with two `clusters` entries, two `users` and two `contexts` under one name, and `_find_named` resolves first-match -- so which cluster the active context actually pointed at came down to ordering. The fetched kubeconfig is untrusted input, as this module's own docstring says, and the `InputSchema` collision validator only compares requested `(node, target)` pairs with each other; it cannot see upstream content. Rejected rather than uniquified. A `-2` suffix would resolve the duplicate and destroy the property the name exists for: `docs/recipe_terragrunt.md` tells consumers to write `tunstrap-<node>-<target>` as a literal, and a name that can silently shift is not a contract. Rejection also says the true thing -- an upstream file sitting in tunstrap's reserved namespace is either a misconfiguration or an attempt to shadow the identity we are about to create, and neither should be worked around quietly. The check runs before any mutation, so a refused document is left exactly as it was found. `run_kube_targets` contains the rejection per target, matching how the fetch and host-split failures are already handled. Without that it would escape `_start_one` -- `KubeParseError` is not in `_NODE_STARTUP_ERRORS` -- and be caught by the worker's broad guard, which reports a generic `daemon_error` and tears down every node over one bad target. The second half is a wording fix, not a behaviour fix. The module docstring claimed other contexts were "ignored and left byte-stable" and the warning said `ignored context '<x>'`, while the code rewrites their `cluster` and `user` references. The rewriting is right -- it is what keeps the document internally consistent when several contexts share an entry that was just renamed -- so the code stays and the words change, in kube.py and in the README paragraph that carried the same claim. The disclosure is emitted only after the rename has actually happened. In its first position it ran before the port split, the TLS resolution and the rename itself, so a target that failed any of those still announced a reference rewrite that never occurred -- the same defect, one door over. A test pins the position: a target failing the TLS check must not claim a rewrite.
…#26) The codebase argued against production asserts twice in its own comments -- an `AssertionError` is outside `TunstrapError`, so it escapes the CLI handler as a traceback, and `python -O` erases the check entirely -- and then carried twelve of them. `S101` was not in the effective ruff rule set, so nothing but those comments was holding the line. They were not all the same defect, so they did not all get the same fix. Seven in `rename_identities` guard structure in a document this module's own docstring calls untrusted; they become `KubeParseError`. Four were pure mypy narrowing, and converting those would only have relabelled dead code, so the narrowing is gone instead: `KubeconfigView.doc` is now `dict[str, object]` and the view carries the `cluster_body` that `parse_kubeconfig` already validated, which is the same live mapping `patch_view` used to re-find. That follows the precedent set in #16, where two asserts disappeared by passing `session` in rather than asserting it was not None. The last one, in `_worker`, guarded the manager's own typed return and is simply deleted -- mypy narrows the union after the error branch returns. What the seven raises are NOT is a production bug fix, and an earlier draft of this commit claimed otherwise. `run_kube_targets` always runs `parse_kubeconfig` on the same document first, and that dominates every structural case here: both functions narrow `contexts` with the same `isinstance` check and resolve names with the same first-match `_find_named`, and nothing between them mutates the entries in question. So for the only in-tree caller these are defence-in-depth; the sole raise reachable from `run_kube_targets` is #20's reserved-namespace collision. `rename_identities` is public and takes a raw dict, so the contract still has to hold for a direct caller -- but the docstring and the tests now say which of those two things they are, rather than asserting a failure path that cannot execute. That is the same defect this branch fixed in #20 one commit ago. The sweep over shared references moves into `_sweep_shared_refs`, which drops `rename_identities` from thirteen branches to eight and retires the `too-many-branches` suppression rather than carrying it. `S101` is now enabled for production and ignored under `tests/**`, which is built on `assert`. A rule that can be switched off in one line is not enforcement, so a coupling test pins the config: `S101` must stay in `extend-select`, and no per-file-ignores entry other than the tests one may list it. Both halves fail when mutated.
`_build_child_env` deletes `KUBECONFIG`, `KUBE_CONFIG_PATH` and `KUBE_CONFIG_PATHS` from the inherited environment unconditionally, before any injection and regardless of schema. `predicted_env_keys` reserved those names only when some node declared a kube target. Two lists that had to agree, kept separately, so they didn't: with no kube targets declared, `--output-var KUBECONFIG` passed the pre-spawn collision guard, and `run` then removed the operator's inherited value and wrote the unified JSON document under it. Reserving three more names in the guard would have papered over it. The defect is the second list, so there is now one: `KUBE_ENV_NAMES`, which the scrubber scrubs and the guard reserves. What each of them does with it stays different and that difference is now written down -- the scrub always covers all three, while `render_kube_env` sets a cardinality-dependent subset (one file exports `KUBE_CONFIG_PATH`, two or more export `KUBE_CONFIG_PATHS`, since the former shadows the latter in the providers). Reserved is not the same as set, and the constant is what stops that asymmetry drifting back into two lists. Making the reservation unconditional left `predicted_env_keys(schema)` never reading `schema`. The parameter is gone rather than silenced: a signature that claims the answer derives from the input, when it no longer does, is the exact framing that produced this bug, and leaving it would invite the next maintainer to make the reservation conditional again. If a future key really is input-dependent, the parameter comes back with the change that needs it. The scrub itself stays unconditional. Making it schema-conditional would mean an operator's ambient `~/.kube/config` silently becomes the target whenever a payload declares no kube target -- or whenever an optional kube node fails and cardinality shrinks to zero -- turning a missing tunnel into an apply against the wrong live cluster. Losing `KUBECONFIG` is a loud failure; inheriting it is a silent one. It is also what makes the reserved set a constant, which is what kills this defect class rather than this instance of it. It was documented only in the Terragrunt recipe, so README now says it too. The anti-drift test survives with an honest docstring. It can no longer catch a predictor that got its conservatism backwards, because a constant cannot have conservatism -- but `set(actual) <= predicted_env_keys()` still fails the moment anything injects a key the constant does not reserve, which is this bug from the other side. Its shrink fixture now pins the cardinality-correct export instead of re-asserting that a constant contains its own member, and three tests whose names claimed an input distinction the signature no longer has are folded into one.
…ed (#22) Everything `_emit_start_result` does -- validating the envelope, writing `output.json`, rendering the output -- happens after `spawn_daemon` has already returned success, which means after a detached worker is holding tunnels open. Any exception there fell through to the top-level guard, which replaced the success envelope with a generic `DaemonError` carrying nothing but the exception type. The daemon kept running and the operator had no way to name it. `run` was hardened against exactly this ("Nothing whatsoever between a successful spawn and the try that owns teardown"); `start` never was. Post-spawn failures are now handled apart from pre-spawn ones and carry the `session_dir` and `pid` out of the envelope the worker already returned, plus the `tunstrap stop --session-dir …` line `run` prints for the same situation. When the envelope itself is unusable -- malformed, or missing the scalars -- the caller-supplied `--session-dir` is used instead, because in that case the parent is holding the authoritative root already: it is the value it handed to `spawn_daemon`, and `SessionDir.create` uses it verbatim. `pid` is omitted there rather than guessed; `stop` does not need it. `start` does not pre-mint the session dir the way `run` does. `run` mints because it owns teardown and must know the path before spawning. For a success envelope the worker has already reported the authoritative root, so minting in the parent would add nothing here. This does not close the whole hazard class. `DaemonHandshakeError` is raised after `Popen` has already detached the worker, and `start` still routes it through the ordinary typed-error arm with no `session_dir` -- with a worker-generated root that daemon is unaddressable. Pre-minting is the fix there, and it is filed separately rather than smuggled in here. Also disclosed, because it should not travel silently a third time: `cli.py` carries `# pylint: disable=too-many-lines`, added undisclosed in 2a88d02 (#18), which reversed the 1000-line cap this project committed to in the #15 cycle. Its stated reason -- that command registrations must stay together -- was not true; Click registers commands on `main` from any module, as `cli_input.py` and `envrender.py` already show. The comment now says what it is: debt, with the instruction not to grow the file, to be paid by splitting the command bodies out.
Two unbounded waits, both reproduced. The parent read the worker's startup IPC frame with a bare `reader.read()` and no deadline, so a worker that retained the write fd without exiting hung the parent forever -- not a hypothetical: the harness showed the read still blocked with the writer held, and returning only when it was closed. And the generic fetched-file path awaited `stat`, `open` and `read` directly, so a stalled server hung the worker despite `connect_timeout`. The discipline already existed. `kube.py` wraps its fetches in `asyncio.wait_for`; it simply was never applied to these two. The generic fetch now reuses the node's `connect_timeout` the same way, as one budget per file rather than per operation. That is safe against a slow large transfer because `_MAX_FETCH_BYTES` already caps a fetch at 1 MiB on both paths -- the worst legitimate read needs about 17 KB/s to fit the default -- so the kube precedent transfers rather than being assumed. The startup deadline could not reuse `connect_timeout`, which is a per-node quantity spent serially up to thirty-odd times within a single node as `fetch_files` and `kube_targets` are fetched one at a time. The parent's wait spans nodes started concurrently, so it is a different quantity and gets its own `daemon.startup_timeout_seconds`, defaulting to 300 and floored at 1. There is deliberately no way to disable it: a null or zero would restore precisely the unbounded read this fixes. On expiry the parent terminates and reaps the worker, and both halves of that respect constraints this branch established earlier. The pid is taken from the `Popen` handle this process created and re-checked positive before both the terminate and the kill, because a non-positive pid signals a process group (#17). The error carries that pid, because `start` may have no session dir at all and a worker that survives the reap would otherwise be a live daemon with no handle -- the exact shape #22 forbids. The kill and its wait are contained so an `OSError` there cannot escape as a raw traceback outside the `TunstrapError` hierarchy. `exit_code_for` keys on the exact type, so `DaemonHandshakeTimeoutError`'s registry entry is load-bearing: without it every startup timeout would drop from exit 4 to exit 1. A test pins it -- deleting the entry fails that test and nothing else.
…s, pins (#28) Seven grouped items. Item (c), the drifted `file.py:LINE` cross-references, is deliberately left out: every commit on this branch invalidates it, so it is worth doing exactly once, last. **(a)** `stop` exited 0 no matter what it found, so `tunstrap stop … && echo done` printed "done" for a daemon it had just reported as still alive. It now exits 1 on every outcome that preserves the session -- identity mismatch, identity check unavailable, still alive, identity changed during grace, and the three identity-read failures -- and 0 only for the three that actually clean `tunnel-data`. The JSON body is untouched, so the byte-pinned envelopes still hold; only the status changed. Repeating the recovery command keeps returning 1 until the session is resolved by hand, which is the honest answer: an unresolved session was never recoverable by repetition. README and the Terragrunt recipe both promised that command was "safe to repeat" without saying what it returns, so both now say it. **(b)** `_kube_channel_keys`' `count == 0` branch was unreachable -- the only caller returns early on the empty case -- and is gone rather than reshaped into differently-shaped dead code. Its docstring now states `count >= 1` as a precondition so a future second caller cannot silently receive the `KUBE_CONFIG_PATHS` shape for an empty set. **(d)** 23 production callables had no docstring. They have rationale now, not restatement. **(e)** `mypy` and `pytest-cov` were the only unbounded dependencies; a review environment had already resolved `mypy 2.3.0`, a major this project has never been validated against. Bounded to match the house style the production deps already use. **(f)** `asyncssh` pointed at our fork through a git *tag*, which is mutable -- no integrity guarantee on a fork of the SSH security boundary. Repinned to the commit that tag resolves to, `938b88c`, with the divergence recorded. **(g)** `run_command` carried a `suppress_kubeconfig` parameter that was not a Click option and existed only so `tofu_proxy` could reach it through `run_command.callback` -- a second signature `--help` never showed. The shared body is now a plain function, with `tunstrap/run_invocation.py` as the programmatic entry, and the proxy no longer reaches through the callback. The "was --grace-seconds set" test moved out of the shared body too: it sniffed ambient Click state, which only worked because the proxy happens to run outside a Click context. It is now passed in explicitly, so the new module's claim to be free of Click internals is true rather than lucky. **(h)** Four `# renovate:` annotations in the test workflow were inert -- this repo is outside the org's autodiscovery scope -- so they promised an automation that could never run. Removed, with the coupling test that actually enforces those versions left as the real mechanism.
#19) README still published the environment contract #13 deleted. The healthz example interpolated `TUNSTRAP_API_ENDPOINT`, which is no longer exported, so the copy-pasteable line expanded to `curl http:///healthz`. The variable table listed `TUNSTRAP_<NAME>_PORT`, `_ENDPOINT` and `_KUBECONFIG`, none of which exist. `MultiNodeEnvUnsupported` and its pre-daemon exit-1 path were documented for a class that had been removed -- multi-node now succeeds and injects the session scalars regardless of node count. `TUNSTRAP_OUTPUT_FILE`, which #13 added, was absent, and `KUBE_CONFIG_PATH`/`KUBE_CONFIG_PATHS` appeared nowhere in README at all despite being the kube channel. The `--output-var` section was stale in the same way from #15/#16: it described `OutputSchema` on the wire rather than the unified `session`/`nodes` structure, listed kube fields the projection no longer emits, and carried two claims that were simply false -- that `FetchedFile` has no on-disk `path`, and that fetched content is exported verbatim rather than projected. Both are contradicted by `render_unified_output`, and the "do not --fetch a secret while using --output-var" advice built on top of them went with them. All of it is now written against `cli._session_scalars` and `envrender.render_kube_env`, including the cardinality rule the kube channel actually follows: one kube file exports `KUBE_CONFIG_PATH`, two or more export `KUBE_CONFIG_PATHS`, never both, because the former shadows the latter in the providers. Two things the first pass of this fix got wrong and this commit corrects, because a docs fix that introduces a fresh falsehood is worse than the drift it replaced. The output examples showed `cluster_name` and `context_name` as `"default"`; `rename_identities` runs on every success path, so both are always `tunstrap-<node>-<target>`, materialized or not. And the note that the materialized projection "renames `context_name` to `context`", sitting between those two examples, implied the value changes -- only the key does. Also corrected: `--output env` forces `daemon.materialize`, so it writes fetched files 0600 as well as kubeconfigs, not just kubeconfigs; and the unified `ports` value is the string `"127.0.0.1:<port>"`, which the deleted `TUNSTRAP_<NAME>_PORT` rows were the only place a reader could learn. The table is now pinned: a test extracts README's variable names and compares them with what `start --output env` actually emits. It pins names only, and says so rather than implying it also guards the mutual exclusion -- that invariant lives in the prose.
…27) Three committed documents carried the literal path `/home/<user>/Projects/.../worktrees/tunstrap-issue15-spike`, publishing a username and an unrelated sibling project from a public repository. The sweep went wider than the three files named in the report -- other absolute home paths were sitting in a recipe example and in four test fixtures -- and all of them are now placeholders. The second half mattered more. Three live citations pointed into `docs/artifacts/`, which `.gitignore` excludes, so no reader could resolve them. One of the three justifies a production decision: `envrender.py` cites the measured provider precedence to explain why `KUBE_CONFIG_PATH` and `KUBE_CONFIG_PATHS` are never exported together, the rule that otherwise silently hides every cluster but the first. The Terragrunt recipe cited the same file at consumers. That measurement is a result, not scratch work, so it is now a committed document under `docs/specs/` and both citations point at it. Its source-corroboration section is scoped as inference rather than presented as verified source reading, because that is what it is. The e2e baseline citation was a different case and is not promoted. Its claim -- plan, mutate only the kubeconfig, apply the saved plan, zero mismatch -- has no test behind it, so removing the citation as "self-evident from the assertions" would have erased the only provenance a live manual measurement had. The recipe now says plainly that finding #4 comes from the unpublished spike and is not automated. Edits to frozen plan and spec documents are redactions and repoints only. One of them initially had its `docs/artifacts/` prefix stripped rather than resolved, which silenced the guard while leaving the citation dangling and added a false "now committed" claim about files that are not in the repo; it names the real spec now. The guard test enforces the property rather than a spelling. Matching the literal string `docs/artifacts/` is exactly what prefix-stripping evades, so it resolves path-like references and fails on any that `git check-ignore` excludes, plus bare `*-findings.md` names that resolve to nothing. It carries a documented allowlist marker so the next author has a route other than rewording around a regex, and it builds its own message sentinels by concatenation so it does not flag itself once tracked. Verification note: the e2e tier could not be run green for this commit -- `tofu init` fails with `registry.opentofu.org requires authentication credentials` while resolving the kubernetes and helm providers. That reproduces on an unmodified HEAD, so it is an upstream registry problem rather than a regression here; this change is documentation plus one test, and the only production edit is two docstring citations.
Held back until last on purpose: every commit on this branch moved lines, so fixing these references earlier would only have re-broken them. The report found 5 of 6 sampled `file.py:LINE` citations already wrong at PR head, which is the argument against the form rather than against those six. Live citations now use the `path/to/file.py::SymbolName` form the shared rules mandate. Where a reference pointed at a statement with no symbol to name, it either moved up to the enclosing symbol or the sentence was reworded so the citation is unnecessary -- no symbol was invented to satisfy the shape. Six existing `::` citations were already broken and are fixed with them: three were unqualified, one used a wildcard that is not a symbol, and two named the Click verbs `status` and `stop` rather than the functions `status_command` and `stop_command`. `envrender.py`'s reference to an "Anti-drift guard" section that never existed in that file now points at the test that actually enforces it. Two guards keep it that way: every `::Symbol` citation must resolve through `ast` to a real module-level function, class or constant, or a method on a class; and the `file.py:LINE` form is rejected outright so it cannot come back. Both fail when mutated -- citing a nonexistent symbol, or reintroducing a line reference. Scope is deliberate and written into the guard rather than left implicit. The sweep found 282 line-form citations, 271 of them in `docs/superpowers/plans/` and `docs/specs/`. Those are frozen records of past design cycles, and their line numbers were accurate when written; rewriting them would not fix a stale reference, it would falsify a historical document -- the same principle #27 applied when it restricted edits there to redaction. The guard covers `tunstrap/`, `tests/`, README and the Terragrunt recipe, and says why it stops there.
Structural audit, run last on purpose: fourteen fix commits had just landed, so measuring before them would have measured the wrong tree. The lead indicator was the single-call-site helper -- the shape AI-authored volume produces most readily -- with the rule that one call site is a signal, not a verdict. A single-caller helper earns its keep when it is independently tested as a contract, names an invariant the caller would otherwise bury, or holds a module under a size cap. Most of them earned it. Roughly twenty helpers across `session`, `identity`, `kube`, `envrender` and `cli` are each called once and each stay, because they name security or parser invariants and have their own tests: `_secure_supplied_root`, `_reclaim_data_slot`, `_validated_path`, `_process_exists`, `_kube_channel_keys`, `_start_recovery_handles` and the rest. Four did not. `predicted_env_keys()` took no arguments and returned a constant expression -- indirection with no contract. It is `RUN_ENV_KEYS` now, and the anti-drift guard still means what it did, comparing the real `_build_child_env` output against the constant under a cardinality shrink. `_start_schema` and `_conn_flags_present` moved to `cli_input.py`, where every one of their collaborators already lived. That is what let `cli.py` fall from 1061 to 981 lines and the `too-many-lines` suppression -- carried as declared debt since #18 -- be deleted rather than re-justified. Nineteen lines of headroom is thin; splitting `stop`/`status` into their own module is the next honest cut, not another suppression. The one addition is a subtraction in disguise. `session._write_all` looped past short writes; `identity.acquire_session_lock` had the same raw `os.write` and could not reuse it, because `session` imports `identity` and the reverse would cycle. So the loop moved down to a stdlib-only leaf, `fdio.write_all`, and all three raw-fd writers -- `session`, `_worker`, `identity` -- now share one implementation of an invariant that is the worst possible thing to keep three copies of. That closes an actual unchecked write in the lock file, not just a duplication. It is pinned: replacing the call in `acquire_session_lock` with a bare `os.write` fails a test that previously did not exist. `_worker`'s short-write error reported `remaining` as the full payload once the loop moved out of it, since the caller no longer knew the count. The real unwritten count is carried out of `write_all` now rather than the key being left to lie. No behaviour changes here; the test count moves only by the one test added to pin the lock write.
…ask (#25) `Path.mkdir(parents=True, mode=0o700)` applies the mode to the leaf only. CPython creates the intermediate components with the default `0o777 & ~umask`, so `--session-dir /a/b/c` on a fresh tree produced `/a` and `/a/b` at 0775 on any ordinary account while `/a/b/c` was correctly 0700. That is not cosmetic here. `_secure_supplied_root` reasons at the level of directory *entries*: it proves the root is ours and clears its group and other write bits precisely because write permission on a directory is authority to create, unlink and rename what is inside it. A group-writable ancestor hands that same authority one level up -- another uid can rename our root aside and put its own there -- which defeats the premise the check is built on, with the root itself still passing every test. Every missing component is now created 0700 explicitly. Reverting the loop to `parents=True` fails the test that pins it. The rationale is recorded next to the loop, because the obvious "simplification" is exactly the thing that reopens the hole; and `_secure_supplied_root`'s docstring, which says the parent is never inspected, is scoped to *pre-existing* parents -- a parent this tool creates itself is a different question from one the operator handed us.
This was referenced Aug 10, 2026
Open
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.
What this changes
Replaces the Terragrunt integration model and adds an end-to-end test tier for it.
Before: Terragrunt reached tunstrap through
run_cmdinsideinputs.After: tunstrap ships a second console entry point,
tunstrap_tofu, which Terragrunt uses as itsterraform_binary. It brings the tunnel up andexecstofuwith the connection details in the environment.Nothing is copied into the consumer's repository.
terraform_binarypoints at the installed path; arun_cmdform is documented as an alternative for anyone who wants the bootstrap localized interragrunt.hcl.New CLI surface on
run:--input-env(read the input schema from a named environment variable) and--output-var(publish the connection envelope into a named variable for the child).The proxy bypasses
init,versionand anything with no subcommand, and tunnels everything else — so a consumer's owncommandslist stays authoritative. It parses argv structurally, which fixes a real gap the earlier shell shim had:tofu -chdir=DIR initused to miss the bypass and build a needless tunnel.Recipe:
docs/recipe_terragrunt.md. Design:docs/specs/2026-07-31-run-env-io-and-tofu-proxy-design.md.Security fixes
A cross-family review at branch scale found two credential exposures that per-task review had passed:
Critical —
--output-varpublished the entireOutputSchema, carryingclient_key_dataandcontent_b64(the full patched kubeconfig) into a non-sensitiveTerraform variable. OpenTofu persists root-module variable values into the plan file, which pipelines routinely archive.sensitive = truewould not have been sufficient: it suppresses rendering but leaves the value in the plan file.Fixed at the source with a
RunKubeTargetallow-list, so a field added to the schema later fails closed rather than open. Dropped:client_key_data,content_b64, andclient_certificate_data(it cannot authenticate alone, butCN/Oin an archived plan file is a free RBAC map). Keptcertificate_authority_data— a published trust anchor that token and exec-plugin auth need.runalready forces materialization, so the consumer readskube_targets[*].pathoff disk (mode0600inside a0700directory).Important — the variable named by
--input-envwas not scrubbed from the child environment. It holds the SSH private key PEM, sotofuand from there every provider plugin,externaldata source andlocal-execprovisioner inherited it.Important, carried on the branch — pydantic v2 embeds the offending value in its
ValidationErrorentries, so a malformed node put thessh_pkeyPEM straight into error output. Fixed byexc.errors(include_input=False, include_url=False, include_context=False)at three call sites plus a recursive scrub over dicts and lists (defd1cf).Deliberate, documented asymmetry:
fetch_files[*].content_b64still rides--output-var. Unlike the kube credentials — which tunstrap injected unasked and for which a lossless on-disk alternative already existed — fetched content is opt-in twice, is the operator's own data, andFetchedFilehas nopath, so dropping it would silently and unrecoverably break a legitimate consumer. Do not--fetcha secret while using--output-var. The end state is recorded in the spec's out-of-scope section.Test tier
35 end-to-end tests driving real OpenTofu with the real
kubernetesandhelmproviders against a real kind cluster through a real tunnel. Read-back uses an in-node oracle overdocker execthat never traverses the tunnel, so a broken tunnel and a broken assertion cannot cancel out.Proven: the decoded
config_pathis the module's only route to the cluster; a dead endpoint surfaces as a non-zero exit naming that port; exit codes propagate verbatim (not merely non-zero); the proxy adds no bytes totofu's stdout, compared byte-for-byte against an arm of the same binary where the tunnel is absent entirely.The recipe is executed, not just written. Its first shipped version put
terraform_binaryinside theterraform {}block, where Terragrunt rejects it — nobody caught it because the document had never been run. The tier now extracts the fenced HCL out ofdocs/recipe_terragrunt.mdand executes it, assembled the way a consumer assembles it: aroot.hclplus a unit thatincludes it, not a single concatenated file. A unit that fails to inherit the root renders an emptyterraform_binaryand falls silently back to plaintofu; that failure is now caught and is distinguishable from a forgottencommandsentry, which produces the same symptom at127.0.0.1:0.Real
terragrunt apply/destroy/outputrun against the cluster, andterragrunt output -jsonparses cleanly withtunstrap runin the pipeline for the parsed command. The four-row environment asymmetry (-version→ neither variable;init→ input only, no tunnel;apply/destroy→ tunnel with the input scrubbed;output→ per configuration) is asserted rather than narrated.Still not proven: anything about a remote cluster over a real network (everything is a local kind node); anything about TLS, auth, timeout or 5xx failures (only connection-refused against a dead endpoint); and stdout purity for
plan/applyunder real Terragrunt, whose stdout is the diff and is consumed differently.CI runs the tier on a cold runner. A skipped run cannot report success:
TUNSTRAP_E2E_REQUIRE_ALL=1turns a missing-tool skip into a failure, and anif: always()step assertsskipped == 0and a test-count floor, catching a stray@skip, a collection error or a deleted module even if pytest crashed.Measured, not assumed
Against Terragrunt v1.1.1:
before_hookcannot install the binary. Terragrunt probes<terraform_binary> -versionroughly 50 ms before any hook runs, and with a missing binary it fails outright.run_cmd(...)is accepted as the value ofterraform_binary, runs before that probe, is cached once perplan, and runs once per unit underrun --all. Its leading--terragrunt-quietmarker is load-bearing: without it the command's stdout is prepended toterragrunt output -jsonand the parse fails. (That marker is an argument to therun_cmdfunction; the identically-spelled CLI flag does not exist in v1.x.)tunstrap.clicosts 225 ms, and would have added ~0.7 s to everyterragrunt plan. The shipped entry point imports nothing heavy and costs 24.6 ms end-to-end — noise beside an 8-secondtofu init. Making__version__lazy droppedimport tunstrapfrom 67.3 ms to 17.5 ms, which speeds every invocation of both entry points.Code health
The organisation's quantitative RP/OP analysis (
code-health-check.md) was run against the package. It found the code healthy but mildly RP-heavy, and the top two ranked fixes were applied:parse_kubeconfigCC 23 to 4, split into an orchestrator plus five section parsers; packagemax_cc23 to 15.start_commandCC 18 to 5 andfetch_filesCC 18 to 7;cli.pywent 1000 to 987 lines, staying under pylint's module cap without raising it. The extracted schema-building helpers moved tocli_input.py, which already owned "CLI input to InputSchema" and now owns all three channels.RP 28.34 to 24.97, Score 54.43 to 49.38, each measured in an isolated worktree so the number is attributable to the change alone. Both refactors are behaviour-preserving, evidenced by characterization harnesses capturing exception type, message, cause and the raising line for every input, with each assertion mutation-proven.
The report's third proposed fix — table-driving the
schemas.pyvalidators — was measured and declined. It makes Score worse (+0.12): it adds more SLOC than it removes complexity, and the complexity it does remove is largely relocated into lambdas, which the metric does not count. Declining is recorded rather than silently skipped, along with the model's blind spots (lambda decision points, and OP being import-graph-only so intra-module indirection is invisible to it).Two properties the kube refactor pinned are now guarded permanently (
tests/unit/test_kube_parse_invariants.py): that thestr()coercions on cluster and user names are observable (ruamel returnsScalarString, so removing them silently changes field types), and that the same raise statement fires for each malformed input rather than merely the same exception type.Also cleaned: three stale lint suppressions removed, one corrected to exactly the messages that fire, and
vulture_whitelist.pydeleted — its entries sat belowmin_confidenceand its docstring described a finding class that did not exist.Verification
black --check ./ruff format --check .ruff check ./mypy --strict tunstrappylint tunstrap//vultureUnit suite also verified on 3.10.19 and 3.13.2, not only the local 3.14.4. The e2e tier runs in ~155 s locally after cluster setup.
Red-team remediation
The branch was then reviewed by a cross-model panel, and every finding was fixed. The two that mattered most were caught independently by two different models, and each remediation round found a further defect the previous one had left, so the batch went through four review passes before it stopped producing them:
os.execvpraises on failure rather than returning, so thesys.exit(127)after it was unreachable dead code behind a# pragma: no cover. A missingtofugave a raw traceback and exit 1 — on Terragrunt's very first-versionprobe. Now exits 127 with a stderr diagnostic.--input-envscrub coveredtofubut not the detached daemon:spawn_daemoncalledPopenwith noenv=, so the long-lived worker held the SSH key for the tunnel's lifetime whilerunpublished its PID to the scrubbed child — a same-uid/proc/<pid>/environrecovery path. The first fix then hardcoded the literalTUNSTRAP_INPUTwhile the option takes an arbitrary name, so it looked closed and was not; the regression test now uses a deliberately non-canonical name.tunstrap stop --force, an option that does not exist, and once that was fixedstopitself turned out to deletetunnel-dataunconditionally, eating the very handle it was called to recover. Both paths now share one_stop_resolvedpredicate rather than encoding the same rule twice.daemon.shutdown_grace_secondswas a schema field nothing read, while the README and the recipe told consumers to set it and--grace-secondswas the one daemon flag missing from the--input-envrejection matrix.runnow honours the payload value.Also fixed: a
re.searchin the kubectl/node-image coupling guard that would silently bind to the wrong URL if a second one appeared; a bareassertin production code contradicting this PR's own stated reason for removing one; anssh_private_keyexample that was a public-key line; recorder diagnostics written with default permissions; apytest.skipthat defeatedTUNSTRAP_E2E_REQUIRE_ALLinside the module defining that guard; and the e2e tier's non-parallel-safety, now documented as an explicit deviation.Known gaps carried as debt
_run_childdoes not check tunnel liveness while the child runs, so an SSH drop mid-apply surfaces as a provider error against half-applied state.tunnel-data/is0700.needs: unitmeans a unit failure skips e2e; a skipped required check reads as success under branch protection. Safe whileunitis itself required.kubectlagainstkindest/node— is enforced by a unit test instead, since the two are different Renovate datasources and could not be coupled by annotation anyway.terragrunt output -jsonassertion at the Terragrunt layer is stated as unfalsified: neither wrappingtunstrap(the proxy runs in-process) nor wrappingtofu(Terragrunt discards a stream it cannot parse) produces a break that falsifies it specifically. The falsifiable purity proof is the byte-equality test one layer down.Issues closed