[REFACTOR] Extract a process-execution lifecycle port in internal/tools/workspace to close the fault-injection-required coverage class
- Date: 2026-08-30
- Subject: Architect (agent persona,
architect.yaml) - Griller: Griller persona (
griller.yaml),grillingskill loaded - Orchestrator: butler (relay-only; no content participation)
- Format: Subject's opening statement → 5 grill questions (one at a time, verbatim relay) → Griller verdict → Subject post-grill position
- Questions asked: 5 of 10 cap (Griller concluded early)
- Verdict: PROCEED WITH CHANGES
- Repo: gosharplite/tell-me-go · Issue: gosharplite/tell-me-go#1459
Diagnosis — verified, and slightly understated by the issue. The root cause holds in source: processExecutor constructs *exec.Cmd directly (exec.CommandContext at process_executor.go:135, pipeline.newPipelineCmd at pipeline.go:56) and calls the raw lifecycle (cmd.Start() :90, cmd.Wait() :97, StdoutPipe() :151, StderrPipe() :155, and pipeline.wirePipes/start/wait); shellTool hard-wires its executor — newshellTool(sm, eventBus, validator, translator, wrapper, fs) constructs newprocessExecutorWithFS(fs) internally, so no executor ever reaches shell.go's execution paths. The only existing port, tools.CommandExecutor (internal/domain/tools/executor.go), is consumer-facing (Output/CombinedOutput/LookPath) and never reaches the executor's internals — confirmed by the wiring: DI passes &exec.RealExecutor{} as CommandExecutor, registerGit uses it for git, and registerFiles's downcast (exec.(*processExecutor)) fails in production, leaving fileReader.executor nil behind the designed degrade at reader.go:305-307. Every Start/Wait/pipe/signal failure therefore needs OS-level faults to reach — the catalog's #1431 batch pins exactly 38 workspace-cluster sites (14+5+5+14, and I re-verified the pins against the live catalog section "Fault-injection-required branches (55 sites)"). Two corrections to the evidence base: the same package has two more sites the issue's table omits (reader.go:310-312, 344-345 — getFileDiff's LookPath+RunCommand via the internal commandExecutor downcast), so the cluster is ~40; and 2 of process_executor.go's 14 (:152-154, :156-158) are structurally-unreachable, not fault-injection-required, per the catalog's own StdoutPipe/StderrPipe entries — correct code cannot fail a pipe created before Start, and they must remain unreachable at the new seam by design.
Port shape and home — I approve the seam and placement, but the draft shape has three gaps I would fix before sign-off. Home: internal/domain/tools/, following ADR-060's three-home analysis verbatim — zero closure delta next to CommandExecutor, internal/domain/ports untouched so ADR-064's registry bijection and N≤12 bound stay green, and ADR-056 Decision 1 never fires (both layers are importable). Shape gaps: (1) Exit-code surface. RunCommand:105 and formatPipelineResult:284 both need *exec.ExitError-vs-other discrimination and the numeric code; the draft's Wait() error alone loses this. The ADR must give the port a domain-typed exit signal (my preference: an adapter-converted tools.ExitError{Code int} error, or ExitCode() int on the handle) rather than leaking os/exec types into the domain error position. (2) Stdin wiring. pipeline.wirePipes connects cmd[i]'s stdout reader to cmd[i+1]'s Stdin field; the draft handle has Stdout/Stderr/Wait but no stdin surface. Minimal consumer-driven fix: Stdin io.Reader on ProcessSpec (nil = today's behavior; pipeline wires handle[i].Stdout() into spec[i+1].Stdin). (3) Dead-member candidates. ProcessSpec.Dir has no consumer — neither setupCommand nor newPipelineCmd ever sets cmd.Dir — and LookPath on the runner has no tools-layer consumer that isn't already served by tools.CommandExecutor.LookPath; the Windows pwsh probe (shell.go:172, inside windowsShellWrapper) is shell-translation logic that stays put. Cut both or justify them under ADR-003 Rule #2's consumer-union analysis — the issue itself concedes the shape is draft. The ADR must also pin the adapter's behavioral contract that fakes cannot test: ctx cancellation kills the whole tree (POSIX setpgid + group-kill with the 200ms ESRCH retry window, proc_posix.go; Windows taskkill /F /T, proc_windows.go), WaitDelay=2s (:141), and env semantics = os.Environ() + per-key overlay — any silent regression there is a real behavior change to tool timeouts.
Adapter, construction, wiring — approved, with two conditions. Adapter internal/infrastructure/process/ owning exec.Cmd plus the relocated proc_posix.go/proc_windows.go (tools layer becomes OS-free for process code); single construction at internal/infrastructure/di/process_factory.go mirroring toolchain_factory.go's ToolchainRunner line; threaded via a new ToolRegistrationParams field → RegisterAll → workspace.Register → registerSystem → newshellTool → newprocessExecutorWithFS(fs, runner). On the fallback question I decide: full injection at registration, no default_runner.go fallback. ADR-055's defaultFS fallback exists because FileSystem has dozens of consumers across two packages; the runner has exactly one consumer chain, every construction site is DI-reachable, and per ADR-060 §5's tool-boundary nil-mode criterion the new field has no registration skip, no sentinel degrade, and no default substitution — so it is a guarded field and earns a TestRegisterAll_Errors row (like CommandExecutor/CommandValidator/EventBus/ToolchainRunner). Conditions: (a) the issue's test strategy over-claims when it cites verify-tools-infrastructure-import as the gate that stays green — that gate is blind to os/exec because it is stdlib, not infrastructure. Closing the class mechanically requires a new purpose-built grep gate (no exec.Command/exec.Cmd/configureProcAttrs in tools production files, ADR-055/060 gate pattern) wired into check/check-full, with one documented allowance for the pwsh probe at shell.go:172 (platform-specific class). (b) ADR-060 §7's per-destination wiring-probe contract carries over — a dropped assignment compiles (nil satisfies the interface), so newshellTool → executor.runner needs a behavioral probe, plus a DI seam-capture probe mirroring TestBuildRegistry_PopulatesToolchainRunner. The registration.go downcast wrinkle and fileReader's nil-degrade survive untouched (non-goals honored).
Test strategy and catalog coordination — approved with the ADR-060 anti-extension decision carried forward. FakeProcessRunner in internal/tools/toolstest/ (hand-rolled function-field mock per ADR-021, race-safe per ADR-036, call-log pattern mirroring FakeToolchainRunner); table-driven tests at the executor level driving Start-error, Wait-non-ExitError, signal, and ctx-cancel-during-Wait deterministically, which then flow up through the real processExecutor into shell.go's result/feedback branches (the two-layer fake is what makes the shell.go sites drivable — that part of the issue is sound). The surviving real-process tests (TestRunCommand_NonExitErrorWaitPath_SIGKILL, TestTimeoutKillsProcessTree) remain the real-adapter-over-mock verification surface, and no new gate may be extended to _test.go files (ADR-060 §9's anti-extension ruling, which also protects ~35 existing tools test files that legitimately import infrastructure). Catalog coordination obligations per the Drift Policy are non-negotiable and same-PR: re-anchor every affected file:line pin (the proc_posix.go/proc_windows.go moves and process_executor.go edits shift every subsequent line), update the real_nonfix_catalog_test.go partition rows in the same commit (coordination rule), re-triage shell.go's 14 sites post-seam with explicit re-anchoring of whatever remains, and note the coverage matcher has no name axis (ADR-054), so re-anchoring is the only mechanism. New port types will trip or escape modelith-drift advisory exactly as CoverageSummary/ToolchainRunner did — glossary-entry-vs-accept decided at ADR time per ADR-060 §6.
What I change in the issue's draft — the acceptance criteria as written cannot all be met. The criterion "the ~24 directly-attributable sites (process_executor 14, pipeline 5, proc_posix 5) covered by deterministic fake-based tests" is wrong in two of its three rows: proc_posix.go's 5 sites are the Cancel-closure internals — they are the port's implementation, not its consumers, and no fake can exercise syscall.Kill(-pid) ESRCH retry logic; their correct treatment is relocation into the adapter plus adapter-internal tests with an injected kill function (or continued real-process coverage), not fake-based tools-layer tests. pipeline.go's and process_executor.go's Start/Wait sites are genuinely fake-testable; the two pipe-error sites remain structurally-unreachable at the new seam (correctly — the adapter creates pipes before starting, preserving the os/exec ordering contract, so handle.Stdout()/Stderr() should not carry error returns that merely re-home an unreachable branch). Net effect: the deterministic-testable set is ~22 of the 24 + reader.go's 2 + whatever the shell.go re-triage yields, with proc_posix's 5 re-homed as adapter-internal verification. With those corrections — exit-code surface, Stdin wiring, dead-member cuts, full-injection fallback, the new stdlib-blind-spot gate, and the rescoped acceptance criteria — I grant conditional architect sign-off: the seam, the ADR-055/060 pattern application, the placement, and the wiring are right, and the ADR finalizing the shape above is the precondition for implementation. This is one missing abstraction being paid once instead of a per-gap catalog tax on a class that is still growing — the economics favor it, and triage-loop-closed is better served by closing the class than by cataloging its next 55 entries.
Bootstrap complete (README, Makefile gates, catalog §"Fault-injection-required branches (55 sites)" at INTENTIONAL_NON_FIXES.md:1774, the six cited workspace sources, registration.go, dev.go, issue #1459 full text via API). Most of the subject's evidence base held up under verification — the catalog pins (14+5+5+14, plus reader.go:310-312,344-345), the registration.go downcast, proc_posix.go's Cancel-closure contract, and the stdlib-blindness of verify-tools-infrastructure-import all check out against source. But one load-bearing condition of their sign-off fails against the live tree. Asking Q1 now.
Q1: Your condition (a) proposes a tools-wide grep gate — "no exec.Command/exec.Cmd/configureProcAttrs in tools production files" — with exactly one documented allowance, the pwsh probe (which you cite at shell.go:172; the probe is actually at shell.go:136). I verified the raw-exec inventory across internal/tools/** production files, and your plan leaves at least two more sites that the gate as specified would fail on day one:
internal/tools/developer/dev.go:58—realExecutor.Executecallsexec.CommandContext(ctx, name, args...).CombinedOutput()directly, andnewDevManagerhard-wires it (executor: &realExecutor{}). This is a raw-exec production site in the tools layer, outside the workspace cluster, absent from both the issue and your allowance list.processExecutor.LookPath(process_executor.go:497) —exec.LookPath, required by the internalcommandExecutorinterface (reader.go:15-20). Your own dead-member cut (noLookPathon the runner) leaves this production method with no sanctioned implementation under your gate — and note it is precisely the implementation your "reader.go:310-312 becomes fake-testable" claim depends on.
So the decision I need from you: what is the gate's predicate and exception inventory? Three coherent options:
- (A) Zero-allowance tools-wide gate — migrate
dev.go'srealExecutoronto the existingtools.CommandExecutor(itsCombinedOutputshape matches exactly) and wire the pwsh probe throughCommandExecutor.LookPath; scope grows tointernal/tools/developerand the shell-wrapper constructor. - (B) Workspace-scoped gate — predicate limited to
internal/tools/workspaceproduction files;dev.go:58andshell.go:136recorded as documented residue with named follow-up issues. - The one-allowance tools-wide gate as written is not a valid option — it fails to compile against the tree.
My recommendation: (B) for this issue — the ADR-062 triage-3 path your option (A) implicitly invokes (a sanctioned exception requires a per-edge ADR) is heavier than the issue intends, and (B) preserves the honest claim "the workspace class closes mechanically" rather than the overclaim "the tools class closes."
Which do you pick — and if (A), confirm the issue's Files-Affected list grows accordingly?
Q1 answer — I pick (B), the workspace-scoped gate, and I concede three things up front.
Concessions. (1) My pwsh-probe citation was wrong: the probe is at shell.go:136 (exec.LookPath("pwsh") inside windowsShellWrapper.Wrap), not :172 — verified against the live tree. (2) The one-allowance gate as I wrote it was invalid, and doubly so: dev.go:58 uses exec.CommandContext, which a grep for exec.Command matches as a substring, so the gate fails day one on a site absent from both the issue and my allowance list — and my single allowance was internally incoherent, because exec.LookPath at shell.go:136 does not match my own proposed predicate tokens (exec.Command/exec.Cmd/configureProcAttrs) in the first place. I specified a lifecycle-token gate and then carved an allowance for a token it doesn't catch. (3) My claim "the tools layer becomes OS-free for process code" is retracted as a tools-wide claim; under (B) it narrows to: the workspace package becomes OS-free for process-lifecycle code, gate-enforced.
The gate spec. New target verify-tools-process-import (wired into check/check-full), scoped to internal/tools/workspace production files, predicate = lifecycle tokens only: exec.Command (spelled in the ADR to also match exec.CommandContext), exec.Cmd, syscall.SysProcAttr, configureProcAttrs. Zero allowances. The full tools-wide raw-exec inventory (verified by grep just now) is: the workspace cluster (all eliminated by this refactor), developer/dev.go:58, and two exec.LookPath sites. The two LookPath sites are predicate-scope exclusions documented in the ADR, not gate allowances: process_executor.go:497 (delegation-wrapper class; it implements the internal commandExecutor interface at reader.go:20-23 and tools.CommandExecutor — cutting it would force port plumbing for zero catalog value) and shell.go:136 (platform-specific class, Windows-only, no cataloged sites). This predicate choice is deliberate against a third wrinkle the inventory surfaced: internal/tools/analysis/health.go:12 imports os/exec — apparently for exec.ExitError type references as a consumer of the existing CommandExecutor contract — which an import-ban predicate would trip on day one; a lifecycle-token predicate correctly leaves it alone. The ADR carries this inventory table so the predicate's narrowness reads as a decision, not an oversight.
dev.go:58 — documented residue with a named follow-up issue, not migrated here. Four grounds, verified: (1) I checked the entire catalog — internal/tools/developer has no fault-injection-required entries (the #1431 batch's 55-site list and the defensive-guard list contain no developer files), so migrating it closes no class; (2) its error path is already deterministically drivable today (CombinedOutput on a nonexistent binary returns the LookPath failure without any injection); (3) the migration would grow Files-Affected into internal/tools/developer — newDevManager's signature at dev.go:426 hard-wires executor: &realExecutor{} at :431, and developer/registration.go:18 passes only the runner, so wiring the already-threaded CommandExecutor through means signature churn, registration changes, and dev-test updates — unfunded adjacent work against the issue's own non-goals; (4) under (B) no gate exception exists to sanction, so ADR-062 triage-3's per-edge-ADR weight never engages — dev.go simply sits outside the gate's scoped paths, which is a scope decision the ADR records, not an exception it grants. The follow-up issue does option (A)'s one-line adapter swap (realExecutor.Execute's shape is exactly CommandExecutor.CombinedOutput) and extends the gate's scoped-path list to include internal/tools/developer in the same PR — the gate design supports scope growth precisely so that (B) today does not foreclose (A) tomorrow.
On the reader.go dependency the griller flagged. The griller is right that my "reader.go:310-312 becomes fake-testable" claim depends on processExecutor.LookPath keeping a sanctioned implementation — and under this spec it does: LookPath stays off the ProcessRunner port (my dead-member cut stands) but remains a method on processExecutor implemented directly with exec.LookPath, satisfying the internal commandExecutor interface unchanged. One precision note the griller's phrasing invites: in production fileReader.executor is nil — the downcast exec.(*processExecutor) at registration.go:48-51 fails against the DI-injected &exec.RealExecutor{}, so both sites sit behind the designed degrade at reader.go:305-307 and are reachable only from in-package tests. The runner port makes them trivially drivable (fake runner through a real processExecutor), but they were already drivable via a failing real command; the catalog delta from the port there is marginal, which is why I counted them separately from the ~22 core sites.
Files-Affected: no growth. Under (B) the issue's list stands as written, plus two additive items it already implied: the new Makefile gate target and the ADR (which now also records the dev.go:58 residue, the follow-up issue pointer, and the predicate-scope exclusions). The honest closure claim for the PR is exactly what the griller stated: the workspace class closes mechanically; the tools-wide raw-exec residue is one named site, tracked, not silently absorbed.
health.go verified: it imports os/exec solely for var exitErr *exec.ExitError + errors.As in runLint — the subject's predicate-scope argument holds, and (B) as amended is coherent. I also mapped pipeline.go's five pins against the real line numbers (start-error at 122-124, capture's stdout scanner-error at ~161, wait()'s ExitError discrimination at ~174-178) — all genuinely fake-drivable under the port, so no challenge there. But that verification surfaced a decisive technical fact for the subject's first shape gap. Asking Q2.
Q2: Your condition (1) demands a domain-typed exit signal, justified as "the draft's Wait() error alone loses this." That rationale is imprecise, and the real justification is stronger than the one you gave — but it forces a contract decision you haven't fully made. Three verified facts:
-
The ADR-060 precedent you invoke three times actually does what the draft does.
internal/tools/analysis/health.go:11-12importsos/execsolely forvar exitErr *exec.ExitError+errors.As(err, &exitErr)inrunLint, discriminating against the error returned through theToolchainRunnerport. The established in-repo pattern is: ports pass raw exec errors; consumerserrors.As. So the draft'sWait() errorloses nothing for the real adapter — what it loses is fake constructibility (fact 3), which is your actual argument. -
The raw-error convention's fake-fidelity cost is already observable.
runLinttreatserr != nil && !errors.As(err, &exitErr)as"ERROR"— so today a fakeToolchainRunnerreturning a plain error cannot emulate "linter ran and found issues" (real linter exit 1); it degrades to"ERROR"instead of"N Issues". That is precisely the bug class your port decision must avoid forProcessRunner. -
The constructibility wall makes your domain-typed type necessary, not stylistic.
os.ProcessStatecannot be fabricated with a chosen exit code outside theospackage — aFakeProcessRunnerunder the draft's raw-error shape can make the type-asserts atRunCommand:~105-109andformatPipelineResult:~256-260succeed only with a codeless&exec.ExitError{}, so the "exit 3 →executionResult.ExitCode == 3" branches are fake-untestable. Without the domain type, your own acceptance criteria are unmeetable by fakes.
The decision: confirm the exit-signal contract as domain-typed, errors.As-able, code-carrying — i.e., Wait() error where exit-status failures are returned as a domain error type carrying the numeric code — and rule on the two consequences: (a) signal identity is deliberately dropped at the port (code -1 convention), with signal semantics retained only in the real-adapter tests; (b) the ADR explicitly declares divergence from ADR-060's raw-error convention, naming health.go's errors.As dependency as the counter-precedent — accepting a two-convention tools layer, with ToolchainRunner alignment deferred to a follow-up.
My recommendation: yes to all of (contract + a + b) — dropping signal identity at the port is safe because no current consumer reads Sys().(syscall.WaitStatus); only the real-process tests do, and they stay. Confirm or amend.
Q2 answer — confirmed on all three: the contract, (a), and (b) — with one conversion-table sharpening the ADR must pin, and my original rationale retracted in its imprecise form.
Retraction and corrected justification. My claim that the draft's Wait() error "loses" the discrimination was imprecise, and the griller's reframing is correct and verified: health.go:11 imports os/exec and runLint does exactly var exitErr *exec.ExitError; if err != nil && !errors.As(err, &exitErr) { ... return "ERROR", err.Error() } — discriminating against an error returned through the ToolchainRunner port. Ports passing raw exec errors with consumers using errors.As is the established in-repo convention; the real adapter loses nothing under the draft shape. The actual loss is fake constructibility, and it is a hard wall, not a style preference: exec.ExitError embeds *os.ProcessState, whose wait status is unexported and only constructible by the os package from a real wait; a zero-value &exec.ExitError{} yields ExitCode() == -1 (a zero syscall.WaitStatus is not "exited"). So under the draft, the exit 3 → executionResult.ExitCode == 3 branches at the three extraction sites — verified live at process_executor.go:105-106 (assertion + ExitCode()), :284 (assertion-only, formatPipelineResult), and pipeline.go:177-178 (assertion + ExitCode() in pipeline.wait) — are fake-untestable, and my own acceptance criteria would be unmeetable by fakes. The domain type is necessary. QED on fact 3.
The contract, with the conversion table pinned. Confirmed: Wait() error where exit-status failures return a domain error type — my proposed shape is tools.ExitError{Code int} in internal/domain/tools/process.go (final name at ADR time), discriminated at the three sites via errors.As. The sharpening the ADR must pin: the adapter's conversion rule is "any *exec.ExitError from Wait() → tools.ExitError{Code: exitErr.ExitCode()}" — not "only when Exited()". Reason: a signal-killed child yields an *exec.ExitError whose ExitCode() is −1, and today the type assertion at :105 succeeds for it, so executionResult.ExitCode is −1 with no returned error. If the adapter converted only exited processes and passed signaled ExitErrors through raw, RunCommand's discrimination would flip from "result with code −1" to "returned error" — a behavior change to tool outputs, which the issue's non-goals forbid. The −1 convention isn't even new: it is today's observable behavior, merely made explicit and fake-constructible (tools.ExitError{Code: -1} for signal emulation).
Ruling (a): confirmed. My grep for Sys()/WaitStatus/ProcessState across workspace production files returned zero matches — no consumer reads signal identity; the only observers are the real-process tests (process_executor_stream_unix_test.go — TestRunCommand_NonExitErrorWaitPath_SIGKILL, TestTimeoutKillsProcessTree), which stay as the real-adapter verification surface per the ADR-060 §9 anti-extension carry-over. Signal semantics survive at the adapter (the tree-kill contract from my opening statement) and in those tests; the port carries only the numeric code, −1 for non-exit completions. Dropping identity at the port loses nothing anyone reads.
Ruling (b): confirmed, with the two-convention inventory spelled out and the follow-up scope bounded. The ADR's divergence clause must name the concrete counter-precedent — health.go's errors.As guard in runLint (the var exitErr *exec.ExitError + !errors.As(err, &exitErr) lines) discriminating through the ToolchainRunner port — plus the second raw-convention carrier: the CommandExecutor doc comment itself promises *exec.ExitError semantics. So the recorded state is: CommandExecutor and ToolchainRunner on the raw-stdlib convention (documented, not converted — consistent with ADR-060 §6's "couplings to stdlib wording are documented, not converted" ruling); ProcessRunner on the domain-typed convention from day one, where the sentinel-contract symmetry actually improves: couplings to our wording become errors.As-able against our own type immediately. The ToolchainRunner alignment follow-up is real work, not a rider: converting it touches runLint, the dev/release coverage paths (whose stdlib-wording couplings ADR-060 §6 explicitly chose to document rather than convert), and the 22-site real-adapter test surface — it needs its own ADR, and this PR accepting two conventions is the honest intermediate state, recorded as such.
Residual contract details for the ADR (so the griller sees the full surface I'm committing to): the p.cmds[i].Process == nil defensive guard in pipeline.wait stays executor-side (never-started is not a port failure class); the ctx.Err() pre-check at :101-103 is unchanged and orthogonal to the exit-signal design; non-ExitError wait failures pass through Wait() unconverted and keep today's handling (error returned, exit code 1 at RunCommand, formatPipelineResult's !ok propagation); and the new exported type in domain/tools trips the modelith-drift advisory exactly as CoverageSummary did — glossary-entry-or-accept decided at ADR time, per the precedent I cited in my opening statement.
ADR-060 read in full — the subject's §5 (guarded-field criterion), §6 (documented-not-converted), §7 (wiring probes), §9 (anti-extension), and §10 (seam-capture probe) citations are all verbatim-accurate; toolchain_factory.go confirms CommandExecutor: &exec.RealExecutor{} (so the production downcast failure/nil-degrade story holds), and notably ADR-060 §11 already records devManager.executor non-injectability as a residual — the subject's dev.go follow-up aligns with a decision that's already on the books. find_usages(newprocessExecutor) shows the zero-arg constructor is test-only (all ~70 hits in _test.go) — I'll bank that for the fallback question. Now Q3, on the last unresolved shape gap.
Q3: Your Stdin fix — "Stdin io.Reader on ProcessSpec (nil = today's behavior; pipeline wires handle[i].Stdout() into spec[i+1].Stdin)" — is a one-liner sitting on top of four unpinned contract decisions, three of which change observable behavior if gotten wrong. Verified mechanics first: today the pipeline runs wire-all → start-all → capture → wait → closePipes; wirePipes assigns p.cmds[i].Stdin = p.pipes[len(p.pipes)-2].(io.Reader) — an *os.File read-end, which trips os/exec's *os.File fast path (direct fd inheritance, no copy goroutine); and closePipes (executor-side, after the final wait, errors ignored) closes every stderr pipe plus all intermediate stdout read-ends. Under your port, handle[i] exists only after Start(spec[i]), so wiring spec[i+1].Stdin from handle[i].Stdout() forces an interleaved start sequence your statement never mentions. Pin the contract:
- (i)
Stdout()validity window + the interleave. Pin: valid only after the owning handle'sStartreturns (adapter creates pipes insideStartbeforecmd.Start, preserving the os/exec ordering that keeps pipe-creation failures structurally unreachable); and pin the wire/start interleave as behavior-preserving (same start order; today capture() reads only after all starts, so the pipe-buffer exposure is identical). - (ii) The fd fast path. Today's pipeline relies on zero-copy fd inheritance between stages. If the adapter treats
spec.Stdinas a genericio.Reader(copy goroutine + WaitDelay interplay), pipeline behavior subtly changes. Pin: the adapter type-asserts the read-end it handed out (it created the pipes — it knows) and preserves direct inheritance; generic readers get copy semantics. - (iii) Close ownership. Today
closePipescloses intermediates after the final wait; nobody else does, and a pipeline that leaks one fd per intermediate per invocation accumulates. Pin: handed-out readers are caller-closed (io.ReadCloser, executor mirrorsclosePipesafter the lastWait); the adapter never auto-closes handed-out readers; double-close remains tolerated as today. - (iv) The
Envhome contradiction. Your opening statement pinned "env semantics =os.Environ()+ per-key overlay" as an adapter behavioral contract — that requiresProcessSpec.Envto be the overlay (a map), and the draft'sEnv []string(final env) instead implies the overlay logic andos.Environ()stay in the executor. Both cannot hold, and your narrowed gate claim ("workspace becomes OS-free for process-lifecycle code") is only true under the adapter-side variant —os.Environ()is an OS read your lifecycle-token predicate deliberately doesn't catch. Pick one home and fix the draft shape to match.
My recommendation: pin (i)–(iii) as stated, and for (iv) go adapter-side — ProcessSpec.Env becomes the overlay map, nil = inherit, os.Environ() + overlay moves into the adapter — because it's the only variant that makes your own gate-claim honest, and env assembly is exactly the class of behavior your opening statement already demanded the ADR pin as adapter-tested. Confirm or amend.
Q3 answer — (i)–(iii) pinned as the griller states, with two precision amendments; (iv) conceded and resolved adapter-side. All mechanics claims verified against the live tree.
Concession on (iv) first, since it invalidates a piece of my opening statement. I pinned "env semantics = os.Environ() + per-key overlay" as an adapter behavioral contract while simultaneously accepting the draft's Env []string final-env shape — those are irreconcilable: with a final-env slice, the overlay assembly (os.Environ() read + append, setupCommand:143-149 and the identical block in newPipelineCmd) necessarily stays executor-side, and my gate claim then rests on the predicate's blindness to os.Environ() rather than on genuine relocation. The griller's read is correct. Ruling: adapter-side. ProcessSpec.Env becomes map[string]string — the overlay, nil = inherit — and os.Environ() + overlay assembly moves into the adapter. Three behavior-preservation details the ADR pins with it: (1) nil spec.Env must leave cmd.Env nil (pure inherit) — exactly today's behavior when config.Env is empty, which is in fact the pipeline path's constant state, since PipeCommands' params struct (shell.go:334-338) has no env field at all; only RunCommand ever carries an overlay; (2) the adapter preserves today's append-last-wins semantics verbatim — base os.Environ() first, overlay appended after, relying on exec.Cmd.Env's documented dedup-keeps-last so overlay keys win; random map iteration order is immaterial because the overlay map cannot contain duplicate keys and dedup is per-key; (3) the adapter gets an unexported environ hook for its own package tests (mirroring the osGetwd var precedent at process_executor.go:25-27) so "nil = inherit" and "overlay applied" are adapter-testable without process spawning. With this, the gate claim becomes cleanly true rather than predicate-tolerated: workspace production code retains exactly one documented os/exec touchpoint, exec.LookPath (Q1's exclusion), and zero process-setup assembly.
(i) Validity window + interleave — pinned, with the alternative recorded as rejected. Stdout()/Stderr() are valid only after the owning handle's Start returns; the adapter creates the pipes inside Start before cmd.Start (preserving the os/exec ordering contract that keeps pipe-creation failures structurally unreachable — so the getters carry no error returns, per my Q2/Q3 position that unreachable branches must not be re-homed at the seam). The wire/start interleave — Start(spec[0]) → wire spec[1].Stdin = handle[0].Stdout() → Start(spec[1]) → … — is pinned behavior-preserving on the verified mechanics: the read-end fd exists from the moment Start(spec[0]) returns (created before its process spawned), so writes by stage i before stage i+1 inherits land in the same OS pipe object in both orderings; start order remains 0..n−1; and capture() begins only after all starts in both designs (pipeline.go today, executor-side under the port), so pipe-buffer exposure is identical. Because the griller is right that this interleave was implicit in my shape, the ADR records the evaluated-and-rejected alternative explicitly: a second port method StartPipeline(ctx, []ProcessSpec) ([]ProcessHandle, error) performing wire-all→start-all adapter-internally. Rejected per ADR-003 Rule #2 — the consumer union has exactly one pipeline consumer, and a second lifecycle entry point doubles the fake surface and the consistency obligations to buy back an ordering difference that is provably unobservable. One port surface consequence I add while pinning: no Close() on ProcessHandle — reader-level ownership (iii) plus Wait-based reaping reproduces today's cleanup 1:1 (start-failure path at process_executor.go:256-259: closePipes() + p.wait() becomes close-handed-readers + Wait() each started handle), so the handle stays at exactly Stdout/Stderr/Wait.
(ii) fd fast path — confirmed as pinned. Today's zero-copy inheritance rests on p.cmds[i].Stdin = p.pipes[len(p.pipes)-2].(io.Reader) (pipeline.go:97) holding an *os.File read-end, which trips os/exec's *os.File fast path — direct fd inheritance, no copy goroutine. The adapter replicates this exactly: it created the pipes, so it type-asserts its own handed-out read-ends (*os.File) back out of spec.Stdin and preserves direct inheritance; a genuinely generic io.Reader gets documented copy semantics (os.Pipe + copy goroutine, subject to the adapter's WaitDelay). The ADR states the asymmetry openly rather than leaving it as adapter folklore: handed-out readers round-trip fd-exact, foreign readers copy — and the fake-runner tests assert spec-shape only, so the fast path is real-adapter-verified territory.
(iii) Close ownership — confirmed, with one precision the griller's phrasing needs. "The adapter never auto-closes handed-out readers" is achievable only modulo one documented exception: cmd.Wait() itself closes the pipe read-ends it created (os/exec's documented StdoutPipe contract). So the pinned rule is: adapter-owned closes = exactly cmd.Wait()'s documented close, nothing more; the caller owns the pre-Wait lifetime — early-exit cleanup (the start-failure path above) closes the handed-out readers it holds and reaps started handles; the happy-path defer p.closePipes() (:249, after the final Wait) is belt-and-suspenders double-close, tolerated, errors ignored exactly as today's _ = c.Close() (pipeline.go:188-192). This precision matters for the fake contract: FakeProcessRunner closes nothing the caller didn't ask it to, and the "leaked intermediate fd" scenario the griller cites is prevented by the caller-side close discipline, not by adapter magic. Handed-out readers are io.ReadCloser, so the executor's existing closePipes logic maps onto the port without new port surface.
Net spec shape after Q1–Q3 — this is what the ADR finalizes: ProcessSpec{Name string; Args []string; Stdin io.Reader; Env map[string]string}; ProcessHandle{Stdout() io.ReadCloser; Stderr() io.ReadCloser; Wait() error} with tools.ExitError{Code int} per Q2; Dir cut (dead member — no consumer sets cmd.Dir), LookPath cut from the port (Q1; stays a processExecutor method), Stdin added, Env retyped as the overlay map. Every member now traces to a verified consumer (RunCommand, RunPipeline, formatPipelineResult, pipeline.wait), zero dead members per ADR-003 Rule #2, and every behavior-preservation claim above is pinned either to a fake-testable executor assertion or to the real-adapter test surface. Confirm to the griller: (i)–(iii) as stated with the two amendments (no handle.Close; adapter-closes = Wait's documented behavior only), (iv) adapter-side as recommended.
Q3's amendments check out against source and docs: PipeCommands' params struct indeed has no env field (verified in my shell.go read — the pipeline path's constant nil-Env state is real), os/exec's dedup-keeps-last doc semantics match their append-last-wins ruling, the osGetwd hook precedent is at process_executor.go:25-27, and the cmd.Wait()-closes-its-own-pipes precision is the correct os/exec contract. The net spec shape is now fully consumer-traced. But their no-fallback ruling from the opening statement has an unexamined casualty I've now verified with find_usages. Asking Q4.
Q4: Your fallback ruling — "full injection at registration, no default_runner.go fallback" — leaves the zero-arg newprocessExecutor() homeless, and you've never accounted for it. Verified: it currently constructs via defaultFS (the ADR-055 sanctioned fallback file pattern you rejected for the runner), and find_usages shows every one of its ~80 call sites is in _test.go (process_executor_stream_test.go ×33, 24, _unit_test.go ×reader_test.go ×7, pipeline_test.go, _security_test.go, _stress_test.go, _posix_test.go, registration_test.go) — production callers: zero, since newshellTool constructs via newprocessExecutorWithFS(fs) directly. Your "every construction site is DI-reachable" claim is true for production (one site) but the test layer is where your ruling bites: under no-fallback there is no legal production body for the zero-arg constructor — it can't build a default runner (that's the rejected fallback), and it can't pass nil (your guarded-field criterion panics, mirroring the nil-fs panic).
So the decision: what happens to it?
- (a) Delete it and update ~80 test call sites to
newprocessExecutorWithFS(fs, runner)explicitly — at which point each site must choose a runner: the real adapter (legal in_test.goper ADR-060 §9) or a fake — a large, semantic-risk churn across the real-process test surface. - (b) Relocate the zero-arg convenience constructor into the test layer (a
_test.gohelper inworkspace, e.g. alongsidemain_test.go), defaulting to the real adapter — preserving today's semantics 1:1 (the ~80 sites exercise the real OS path viadefaultFStoday; defaulting them to a fake would silently gutTestTimeoutKillsProcessTreeand the stream/stress suites), keeping all ~80 call sites untouched, and sitting squarely inside ADR-060 §9's "direct adapter construction remains legitimate in tools_test.gofiles" ruling — the same shape as the 22 survivingtoolchain.NewGoRunnertest sites.
My recommendation: (b) — it's the only option that is simultaneously no-fallback-honest (production has exactly one DI-reachable construction), zero-churn, and semantically faithful to what those 80 tests verify today. Note the ADR must then state the asymmetry explicitly: production = newprocessExecutorWithFS(fs, runner) with the guarded-field panic; tests = the relocated real-adapter convenience constructor. Confirm (a) or (b) — or name a third shape if you have one.
Q4 answer — (b) confirmed, with the griller's asymmetry statement adopted and three additions that complete the residue accounting this ruling implies. And one concession: the accounting gap is real.
Concession. My "every construction site is DI-reachable" was true for production — newshellTool is the sole production construction path via newprocessExecutorWithFS(fs), and newprocessExecutor() has zero production callers (verified: only the declaration at process_executor.go:63 and its comment/panic references) — but I never accounted for where its test-layer callers land under no-fallback. Q4 is right that the ruling as stated leaves them homeless.
Census precision. 69 direct call sites by grep (stream_test ×27, unit_test ×19, reader_test ×7, pipeline ×5, security ×4, stress ×2, executor_test ×2, registration ×1, stream_unix ×1, posix ×1); the griller's ~80 via find_usages additionally counts declaration/comment references. Immaterial to the decision.
(a) rejected, for exactly the reason the griller names. Forcing ~69 sites to choose a runner yields either no-op churn (real adapter chosen, one line longer, zero information) or silent semantic corruption (a fake chosen by accident guts what the suite verifies — TestTimeoutKillsProcessTree and the SIGKILL path test tree-kill behavior a fake cannot exhibit). Churn with a corruption mode and no upside is the worst of both.
(b) confirmed, with three additions.
- Placement is forced and singular: an in-package
workspace_test.gohelper.processExecutoris unexported, so notoolstestor externalworkspace_testhome can construct it; all workspace executor tests are in-package (they callexecutor.setupCommandand friends directly). The helper — one file, defaulting to the real adapter per ADR-060 §9 ("direct adapter construction remains legitimate in tools_test.gofiles") — is the workspace analog of the 22 survivingtoolchain.NewGoRunnertest sites, centralized, which is strictly better for audit than 69 scattered constructions. The new fake-based tests do not use it: they constructnewprocessExecutorWithFS(fs, fakeRunner)explicitly, because choosing the fake is the test's assertion. - Retire
default_fs.goin the same PR. The verification chain is:defaultFS's only production referencer is the dying zero-arg constructor (process_executor.go:64), no test file referencesdefaultFSdirectly (verified — grep exit 1), anddefault_fs.go:13's own comment states the fallback exists precisely "so the zero-arg newprocessExecutor() keeps working." Its sole consumer was test-serving all along; a production file whose only referencer is a test helper is exactly the dev.go:58-class of residue this round has refused to carry. The helper constructs the OS filesystem inline; theverify-tools-adapter-importexclusion list shrinks to the analysisdefault_fs.goonly — a mechanical, same-PR Makefile edit recorded in the new ADR as an ADR-055 amendment (workspace fallback retired: sole consumer was test-only; analysis fallback untouched). The_test.goimport is already gate-exempt by that target's own predicate. - Update the nil-fs panic message at
process_executor.go:74— it currently instructs "use newprocessExecutor() for the default OS filesystem," which post-refactor points at a function that no longer exists in production. The new message directs to the injected seam, and the runner field's nil-panic mirrors it (guarded field per ADR-060 §5,TestRegisterAll_Errorsrow already committed in my Q-open plan).
Rejected third shapes, for the record: a production zero-arg that panics unconditionally is strictly worse than deletion (a production function that can never legally run); a zero-arg that builds the real adapter is the rejected fallback — a second production construction site, no-fallback-dishonest by construction; a toolstest home is impossible against an unexported type.
Gate non-conflicts, verified against the Q1 spec: the new verify-tools-process-import gate is production-file-scoped (the helper is a _test.go file — exempt), ADR-062's gate excludes _test.go by predicate, and ADR-060 §9's anti-extension decision covers the helper's direct adapter construction. Nothing in (b) requires touching any gate predicate beyond the ADR-055 exclusion-list shrink in (2).
ADR asymmetry statement, confirmed with the additions folded in: production = newprocessExecutorWithFS(fs, runner) with the nil-fs and nil-runner guarded panics, exactly one DI-reachable construction (internal/infrastructure/di/process_factory.go); tests = the relocated, singular, in-package real-adapter convenience constructor serving the 69 real-path sites, with explicit construction for the new fake-based tests; default_fs.go retired; the ADR-055 gate list amended; the panic messages updated. The ruling from my opening statement stands — full injection, no fallback — and Q4's gap in it is now closed: the no-fallback principle was never threatened by the test layer, because the test layer's legitimacy was already settled by ADR-060 §9; what was missing was saying so.
Verified: defaultFS's only referencer is the zero-arg constructor at process_executor.go:64, no test touches it, and default_fs.go's own comment confirms it exists solely to serve that constructor — the subject's retirement chain and ADR-055 gate-list shrink are sound. Q4 is closed. Now the last open correctness item before I can deliver a verdict: the arithmetic inside their sign-off's central correction.
Q5: Your headline correction — "the criterion 'the ~24 directly-attributable sites covered by deterministic fake-based tests' is wrong in two of its three rows" and "the deterministic-testable set is ~22 of the 24" — is itself arithmetically wrong, and I can prove it from the live catalog. The issue's 14 process_executor.go pins are exactly 90-92, 98-100, 107-109, 241-243, 246-248, 256-260, 265-267, 269-269, 472-474, 474-476, 477-479, 484-486, 486-488, 489-491 — the pipe sites :152-154/:156-158 are not among them; they are separate 2026-07 structurally-unreachable entries that live outside the 24. So your "2 of its 14" was a phantom correction — you subtracted two sites the issue never counted, producing "~22," a number that matches neither reality: fake-based at the tools layer is 19 (executor 14 + pipeline 5 — I mapped all five pipeline pins to start()-error 122-124, capture()'s stdout scanner-error ~161, and wait()'s Wait/ExitError branches ~174-185; all five are drivable via fake Start errors, an erroring reader from the fake handle, and fake Wait errors), and deterministic including adapter-internal is 24 (19 + proc_posix's 5 with the injected kill function). The issue's criterion is wrong in one row — proc_posix, fake-based → adapter-internal — not two. And pipeline's row, which your opening implicitly demoted, is fully fake-testable.
Two asks, one question: restate the acceptance-criteria correction as the exact per-site disposition table (14+5 fake-based; 5 adapter-internal; 2 pipe sites outside the 24 dissolving into the adapter; reader.go's 2 marginal real→fake; shell.go's 14 post-seam re-triage), retracting the phantom correction and the "two of three rows" claim — and confirm the re-anchor work for this PR will be done against the live tree with every pin re-verified at edit time, because your own line citations drifted repeatedly this round (shell.go:172→:136, newPipelineCmd:56→:52, formatPipelineResult:284→~257 per catalog pin 256-260, RunPipeline's start-failure path :256-259→~:235), and a re-anchor pass executed from memory is precisely the failure mode your own Drift Policy exists to prevent.
My recommendation: adopt the table above verbatim into the issue comment/ADR, and state the re-verification protocol explicitly (grep-anchored re-read of every touched file:line in the same commit as the edit, per the Drift Policy's "re-verify on every PR" clause). Confirm, and I'll deliver the verdict.
Q5 answer — retraction accepted, table adopted, protocol confirmed. Two of the griller's four drift-audit items are conceded; two are rebutted with receipts from my own greps this round, which does not weaken the protocol ask — two confirmed drifts are more than enough to mandate it.
Retractions. (1) The phantom correction: my claim "2 of process_executor.go's 14 (:152-154, :156-158) are structurally-unreachable" is false. The #1431 batch's 14 pins are exactly 90-92, 98-100, 107-109, 241-243, 246-248, 256-260, 265-267, 269, 472-474, 474-476, 477-479, 484-486, 486-488, 489-491 — I extracted that list myself in the opening round and then misquoted it. :152-154/:156-158 are the separate 2026-07 structurally-unreachable StdoutPipe/StderrPipe entries that live outside the 24; I conflated two catalog sections and subtracted sites the issue never counted. (2) The "~22 of the 24" arithmetic falls with it. (3) "Wrong in two of its three rows" — wrong in one row (proc_posix). My opening's own pipeline row ("pipeline.go's and process_executor.go's Start/Wait sites are genuinely fake-testable") was correct, making the "two rows" framing internally inconsistent with my own statement; the griller is right that pipeline's 5 are fully fake-testable, and its pin mapping is consistent with what I verified live (pipeline.go:177-178 = wait's ExitError branch, from my Q2 grep).
The per-site disposition table (adopted into the issue comment/ADR):
process_executor.go— 14 pins: fake-based, tools layer. Start error via fakeStartfailure; Wait/ctx branches via fakeWaiterrors andtools.ExitError{Code}; Output/CombinedOutput branches via canned results. One protocol input from my own live checks: pin:241-243currently lands on RunPipeline's arity guard (if len(pipedParts) < 2, verified live — deterministic today without any fault), and:246-248lands on thenewPipelinesetup-error propagation (the pipe-dissolution region). Row disposition unchanged — every pin that survives re-anchoring as a genuine lifecycle fault branch is fake-drivable — but these two pins' exact fate (re-anchor vs. remove vs. re-classify) is decided by the edit-time protocol, not by this table.pipeline.go— 5 pins (122-124start error,161-164capture scanner error,174-175/175-177wait branches,185last-error): fake-based, tools layer. Griller's mapping adopted: fakeStarterrors, an erroring reader from the fake handle, fakeWaiterrors. Pipeline's three pipe-creation sites (live:88-90,:101-103,:110-113) are not among the 5 and dissolve into the adapter.proc_posix.go— 5 pins: adapter-internal. Cancel-closure internals are the port's implementation; relocation tointernal/infrastructure/processwith adapter-internal tests over an injected kill function (deterministic ESRCH-retry simulation), real-process tests keeping end-to-end coverage. Net catalog reduction regardless of whether the entry moves or closes.- Outside the 24 — executor
:152-154/:156-158plus pipeline's three uncataloged pipe sites: dissolve into the adapter (handle getters carry no error returns per the Q2 ruling); the two entries re-anchor or retire as the adapter absorbs them. Counted in no headline number. reader.go— 2 pins (310-312,344-345): marginal real→fake. Already drivable today via real failing commands, behind the production nil-degrade (:305-307); fake path makes them trivial; no criterion weight.shell.go— 14 pins: post-seam re-triage. Fake-drivable through the two-layer fake where the fault semantics survive; whatever remains is re-anchored, not silently dropped.
Corrected criterion: "19 sites (executor 14 + pipeline 5) covered by fake-based tests at the tools layer; proc_posix's 5 covered by deterministic adapter-internal tests plus real-process coverage; net catalog reduction ≥ 24 pending the shell.go/reader.go re-triage." The issue's criterion is wrong in one row, and only in its adjective: 24 deterministic, 19 fake-based, 5 adapter-internal.
Drift audit — two conceded, two rebutted with receipts. Conceded: shell.go:172→:136 (conceded in Q1) and newPipelineCmd:56→:52 (the declaration is at :52, verified in my Q3 grep; my :56 was from memory). Rebutted: my formatPipelineResult:284 was live-verified in this round's own grep output (process_executor.go:284: if _, ok := waitErr.(*exec.ExitError); !ok {), and the catalog pin 256-260 live-matches the RunPipeline start-failure block (257: p.closePipes(), 259: return …failed to start, verified in Q3) — not formatPipelineResult; likewise my :256-259 for the start-failure path was grep-verified live. I state this not to contest the audit's conclusion but because the distinction matters for the protocol: some citations this round were grep-anchored and some were from memory, and only the former survived. That is the entire argument for the protocol.
Re-verification protocol — confirmed, stated explicitly for the ADR: every file:line pin on a touched file is re-read from the live tree with a grep-anchored lookup in the same commit as the edit that shifts it — never from memory, never from this round's citations, including mine. The pass records a per-pin verdict (unchanged / re-anchored / removed / re-classified) in the entry's rationale per the established drift-note pattern, updates the real_nonfix_catalog_test.go partition rows in the same commit (coordination rule), and applies symmetrically to the 24, the 2 pipe entries, reader's 2, shell's 14, proc_posix's 5 (moving), and the new pins the adapter itself introduces. The verify-nonfix-catalog matcher is the mechanical backstop and never a substitute for the re-read. Confirm — the table and protocol stand as above; deliver the verdict.
Verification complete — the subject's rebuttals hold on both counts, and I owe two retractions before the verdict. Line-numbered source confirms: process_executor.go:284 is exactly if _, ok := waitErr.(*exec.ExitError); !ok { (their cite was grep-accurate; my "257" claim was wrong), catalog pin :235" was wrong), pin 256-260 lands exactly on RunPipeline's start-failure block (:256-259 — their cite accurate; my "241-243 is indeed the deterministic arity guard, and pin 246-248 is the newPipeline setup-error propagation — all four exactly as the subject stated. I therefore retract two of my four drift-audit items: the confirmed drifts this round are shell.go:172→:136 and newPipelineCmd:56→:52 only, and the pattern is precisely as the subject framed it — grep-anchored citations (theirs, and my catalog/file reads) survived; memory-based citations (mine in that audit, theirs in the conceded pair) did not. That is the strongest possible evidence for the edit-time re-verification protocol they've committed to.
VERDICT: The subject's diagnosis is substantially verified and, where it was wrong, self-corrected under pressure — the root cause (hard-wired exec.Cmd in processExecutor/pipeline, consumer-only CommandExecutor port, the registration.go downcast/nil-degrade), the catalog evidence base (38 workspace pins plus the two reader.go sites the issue omitted, proc_posix's five pins being port internals), and every ADR-055/060/062/064 pattern citation checked out against source, while the round's corrections — the phantom pipe-site arithmetic ("~22 of the 24," retracted), the incoherent one-allowance gate (replaced by a workspace-scoped lifecycle-token gate with zero allowances and two documented predicate-scope exclusions), the Env-home contradiction (resolved adapter-side), the exit-signal contract (domain-typed, made necessary by the os.ProcessState constructibility wall with health.go as the named counter-precedent), the Stdin/pipeline lifecycle contract, and the no-fallback residue (test-layer constructor relocation, verified default_fs.go retirement) — leave a plan that is now strictly stronger than the issue's draft. Recommendation: proceed with changes — the subject's conditional sign-off stands upgraded to an actionable ADR mandate: finalize the port shape as converged in Q1–Q5 (spec {Name, Args, Stdin, Env map} overlay/nil-inherit; handle {Stdout, Stderr, Wait} with tools.ExitError{Code int} under the any-*exec.ExitError conversion rule; no Close, no Dir, no port LookPath), adopt the corrected acceptance criterion (19 fake-based + 5 adapter-internal; net catalog reduction ≥ 24 pending the shell/reader re-triage), execute the grep-anchored same-commit re-anchor protocol across all touched pins (including the discovered drifted pins 241-243/246-248), file the dev.go:58 follow-up consistent with ADR-060 §11's recorded residual, and amend ADR-055's gate list when workspace/default_fs.go retires — at which point this refactor closes the workspace fault-injection-required class mechanically and honestly.
What I stand by. Every load-bearing element of the opening statement survived pressure and is now verification-backed rather than assertion-backed: the root cause (no seam between command assembly and process execution — exec.Cmd constructed at process_executor.go:135/pipeline.go:59, raw lifecycle, shellTool hard-wiring newprocessExecutorWithFS, the consumer-only CommandExecutor port, and the registration.go:48-51 downcast that fails in production behind the reader.go:305-307 nil-degrade); the catalog evidence base (38 workspace pins, the two reader.go sites the issue omitted, proc_posix's five pins being port internals, not port consumers); the seam home (internal/domain/tools per ADR-060's three-home analysis, ADR-064's registry untouched); the adapter + single-DI-construction wiring; the no-fallback ruling; and the disposition of proc_posix's five sites as adapter-internal. Two of my line citations were challenged and held (:284, :256-259) — the griller retracted both of those audit items, which converts the round's lesson into a jointly-endorsed rule: grep-anchored citations survived on both sides; memory-based ones did not. That is the strongest possible grounding for the edit-time re-anchor protocol now in the mandate.
What I revised as a result of the grill. Seven substantive corrections, each making the plan strictly stronger than the issue's draft: (1) the gate collapsed from an incoherent one-allowance tools-wide predicate — which failed day one on dev.go:58 and allowed a site (shell.go:136, not :172 as I'd cited) my own tokens never matched — to a workspace-scoped lifecycle-token gate with zero allowances and two documented predicate-scope exclusions; (2) the "tools layer becomes OS-free" claim narrowed to the workspace package, with dev.go:58 recorded as named residue and a follow-up issue instead of unfunded scope growth; (3) the exit-signal rationale replaced — not "Wait() error loses discrimination" (false; health.go's errors.As proves the raw convention works for real adapters) but the os.ProcessState constructibility wall, which makes the domain-typed tools.ExitError{Code int} necessary for the fake-based acceptance criteria, under the any-*exec.ExitError conversion rule that preserves the −1 signal convention; (4) the Env-home contradiction conceded and resolved adapter-side (ProcessSpec.Env as overlay map, nil = inherit, os.Environ() leaving the tools layer); (5) the Stdin one-liner expanded into the four pinned lifecycle contracts, with my two amendments (no Close() on the handle; adapter-owned closes = exactly cmd.Wait()'s documented behavior); (6) the no-fallback accounting gap closed — the 69-call-site zero-arg constructor relocates to a singular in-package test helper over the real adapter, default_fs.go retires with the ADR-055 gate-list amendment, and the panic messages update; (7) the phantom correction retracted in full — the 14 executor pins never contained the pipe sites, "~22 of the 24" was wrong arithmetic, and the criterion is wrong in one row, not two: 19 fake-based + 5 adapter-internal = 24 deterministic, with the discovered drifted pins 241-243/246-248 flagged into the protocol.
Open questions. (1) Pin-level fates under the re-anchor protocol: 241-243 (live: the deterministic arity guard) and 246-248 (live: setup-error propagation in the pipe-dissolution region) get remove/re-classify/re-anchor verdicts at edit time, not by this table. (2) Whether proc_posix's five entries move to an adapter-owned record or close outright when the adapter-internal kill-function tests land — architect's call at ADR time; net catalog reduction either way. (3) Final naming of the domain exit type (tools.ExitError proposed, non-binding). (4) modelith-drift handling for the four new exported domain types — glossary entries or accepted advisory, per the CoverageSummary precedent. (5) The ToolchainRunner alignment follow-up that retires the two-convention tools layer — deliberately its own ADR, trigger and timing unset. (6) Adapter-internal determinism detail: the relocated ESRCH retry loop's 2ms sleep will need a clock hook or equivalent in the adapter's tests to stay ADR-036-clean. (7) Mechanical fold-back: the issue's acceptance criterion and Files-Affected list absorb the converged shape (gate target, ADR, test helper, retired file) before implementation dispatch.
The conditional sign-off I opened with is now unconditional in substance and mandated in form: finalize the port shape as converged in Q1–Q5, adopt the corrected criterion, execute the grep-anchored same-commit re-anchor protocol across every touched pin, file the dev.go:58 follow-up per ADR-060 §11's residual pattern, and amend ADR-055's gate list when the workspace fallback retires. The refactor closes the workspace fault-injection-required class mechanically and honestly — one seam paid once, instead of the next 55 catalog entries.
Transcript assembled by the butler (orchestrator) from verbatim relay captures; [MODEL] retrieval artifacts stripped. Section count verified: 1 opening + 5 Q + 5 A + verdict + post-grill position.