Repository navigation
refactor(executor): hand the hook-log listener to runStart only - #14392
Merged
ndeloof merged 1 commit intoOct 26, 2026
Merged
Conversation
Epic docker#14081 moved the start phase into the plan engine. Since docker#14302 and docker#14305, a caller that streams pre_start/post_start hook logs into an attached session hands its listener to planRun.runStart when it enters the Start phase, because its printer only exists once the Create phase is done. As a result every production caller builds the executor with a nil listener, and only a few tests still passed one at construction; runStart kept a "nil keeps the construction-time listener" branch just for them. Drop the listener parameter of newPlanExecutor and make runStart the only way to give the executor a listener (nil means hook logs are not streamed). The doc comments that described a construction-time listener now describe that single path. Production behavior is unchanged. In tests, the one that fed a listener at construction sets it on the executor it exercises, the two that only pinned "construction-time listener kept or replaced" are removed, and a test pins that a nil listener leaves hooks running without one. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
24 of 25 tasks
dock-tor
approved these changes
Oct 26, 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.
Follows #14302 and #14305 (merged).
What this PR does, in one sentence
It makes handing the Start phase its hook-log listener the only way to give the plan executor one, by removing the now-unused listener parameter from its constructor.
Context
Epic #14081 moves the foreground
compose uponto the plan engine. Since #14302 and #14305, a caller that streamspre_start/post_starthook logs into an attached session passes its listener torunStartwhen it enters the Start phase, because its printer only exists once the Create phase is done. Every production caller therefore builds the executor with anillistener; only a few tests still passed one at construction, andrunStartkept a "nil keeps the construction-time listener" branch just for them. Two ways to do the same thing, one of them dead in production, and doc comments describing both.What the PR brings
No observable effect: production behavior is unchanged. The executor is built without a listener, and
runStartis the single place that gives it one (nil still means hook logs are not streamed). The comments that described a construction-time listener now describe that single path.On the test side, the one test that fed a listener at construction now sets it directly on the executor it exercises. The two tests that only pinned "construction-time listener kept or replaced" are gone, and a test now pins the remaining truth instead: a nil listener leaves hooks running silently, while a given one receives their logs.
Why this is the right next brick
It is a small follow-up of the epic that leaves one clear way to wire a listener before the cleanup of Lot 3, rather than carrying a dual path into it.
Part of #14081.
🤖 Generated with Claude Code