fix: make the masking registry per-invocation - #511
Merged
Conversation
The set of values docket must not print was process-wide, and the API that filled it *replaced* the set rather than adding to it. Each command populated it at the top of `Run` and deferred a clear, so two runs sharing a process had the first one's teardown blank the second one's secrets while it was still writing output. That is fail-open, and it is the last of the three globals #423 named. It becomes a `subprocess.Masker` the run owns. The layers that mask text but have no other reason to know about a run - the subprocess transports, a task registering a secret it just read back off the server - reach it through the context they already receive. The layers that render but have no context - the human formatter, the JSON emitter, `docket validate` - are handed it at construction, which is where they already take the Ui. There is no `Set`. Nothing needs one: a run's secrets only accumulate, and a masker goes out of scope with the run that owns it, so there is no teardown left to get wrong. A nil `*Masker` masks nothing and is safe to call, which is what lets every masking site drop its nil check - a caller that registered no secrets has nothing to hide. Three places could not take either route. `unknownPlayError.Error()` has a fixed signature and formats lazily on purpose, because the message must name secrets registered after the error was built (#477); it carries the masker, and since that is a pointer to the run's own, the later registrations still reach it. `docket validate` has no context at all, being offline by contract, so its masker rides on the command struct. And `maskedStrings` straddles the emitter boundary, serving both the JSON stream and `--list-tasks`, so it takes the masker as an argument rather than becoming a method. Two things from #507 are deleted rather than migrated. The `TestMain` canary that failed the `tasks` run when the registry was dirty, and the `isolateMaskRegistry` helper its soundness depended on, both existed to police a global that no longer exists. The three `tasks` test files that #502 had to leave serial for this reason are unblocked; turning them parallel belongs with the rest of that migration. Fixes #501.
This was referenced Sep 2, 2026
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
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 set of values docket must not print was process-wide, and the API that filled it replaced the set rather than adding to it. Each command populated it at the top of
Runand deferred a clear, so two runs sharing a process had the first one's teardown blank the second one's secrets while it was still writing output. That is fail-open, and it is the last of the three globals #423 named.It becomes a
subprocess.Maskerthe run owns. The layers that mask text but have no other reason to know about a run - the subprocess transports, a task registering a secret it just read back off the server - reach it through the context they already receive. The layers that render but have no context - the human formatter, the JSON emitter,docket validate- are handed it at construction, which is where they already take the Ui.There is no
Set. Nothing needs one: a run's secrets only accumulate, and a masker goes out of scope with the run that owns it, so there is no teardown left to get wrong. A nil*Maskermasks nothing and is safe to call, which is what lets every masking site drop its nil check - a caller that registered no secrets has nothing to hide.Three places could not take either route.
unknownPlayError.Error()has a fixed signature and formats lazily on purpose, because the message must name secrets registered after the error was built (#477); it carries the masker, and since that is a pointer to the run's own, the later registrations still reach it.docket validatehas no context at all, being offline by contract, so its masker rides on the command struct. AndmaskedStringsstraddles the emitter boundary, serving both the JSON stream and--list-tasks, so it takes the masker as an argument rather than becoming a method.Two things from #507 are deleted rather than migrated. The
TestMaincanary that failed thetasksrun when the registry was dirty, and theisolateMaskRegistryhelper its soundness depended on, both existed to police a global that no longer exists. The threetaskstest files that #502 had to leave serial for this reason are unblocked; turning them parallel belongs with the rest of that migration.Fixes #501.
Unblocks the
commandshalf of #502, alongside #505 and #506.