Skip to content

Commit 26a1104

Browse files
David-Noble-at-workDavid Noble
David-Noble-at-work
and
David Noble
authored
fix(devlore-test): route output streams portably instead of via /dev paths (#380)
Issue #379. devlore-test run could not execute on Windows at all: every output stream defaulted to /dev/stdout, openDest was a plain os.OpenFile, and the open failed before the command produced anything. It aborted with "opening graph output: open /dev/null: The system cannot find the path specified." Per the ruling, the two sentinels are handled differently because they are different problems. Standard output is intercepted. stdoutSentinel is matched in openDest and never reaches the filesystem, which also fixes a latent bug the Unix path was hiding: the callers defer a Close on what openDest returns, so routing to stdout was closing the process's own standard output. openDest now returns an io.WriteCloser and the stdout case is a nopWriteCloser. Discard needs no interception. os.DevNull already names each platform's device — /dev/null on Unix, NUL on Windows — and opens correctly on both. So the fix there is spelling, not mechanism: help text and examples interpolate os.DevNull rather than hardcoding /dev/null, and the same applies to the tests that route streams away. Four further sites carried the same defect and are corrected together, being the identical mistake rather than separate scope: commands_test.go routed to /dev/null directly, and scenario_integration_test.go and repo_cmd_test.go both set GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM to /dev/null to suppress ambient git configuration. No /dev/null literal remains in non-prototype Go code. The root command's help also advertised /tmp/receipt.yaml as its example destination; that is now a bare relative filename, which is meaningful everywhere. This is roughly 16 of the 47 remaining Windows failures — the 12 exit-code-1 cases, the 5 missing JSON summaries, the 2 missing "Hello World!" assertions, and the 2 explicit /dev/null failures. Not addressed here, and left for its own change: data/test_compensation.star forces a failure with destination_path="/dev/null/impossible/path.txt". On Windows that path is apparently creatable, so the copy succeeds where the fixture expects an error. That is a different problem — the impossible-path idiom — not the discard sentinel. The plan's corrected phase 2 triage rides in this commit: the .exe fix cleared 16 failures rather than the ~37 estimated, because 14 TestCLI cases were failing for this second reason behind the first. Verified on Darwin: make vet, make build, full make test green — zero FAIL or panic lines by count; gofmt clean; run --help renders /dev/null here, which is what os.DevNull is on this platform. CI's windows leg is the real verification. Plan: docs/plans/platform-test-matrix.md (phase 3). Issue: #379 Co-authored-by: David Noble <[email protected]>
1 parent a2a904c commit 26a1104

7 files changed

Lines changed: 101 additions & 34 deletions

File tree

‎cmd/devlore-test/cli_test.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -133,19 +133,19 @@ func TestCLI_RunMissingFile(t *testing.T) {
133133
}
134134

135135
func TestCLI_ScriptFirst(t *testing.T) {
136-
stdout, _, code := run("run", scriptPath, "--output", "receipt=/dev/null", "--output", "graph=/dev/null")
136+
stdout, _, code := run("run", scriptPath, "--output", "receipt="+os.DevNull, "--output", "graph="+os.DevNull)
137137
assertExit(t, 0, code)
138138
assertValidSummary(t, stdout)
139139
}
140140

141141
func TestCLI_ScriptMiddle(t *testing.T) {
142-
stdout, _, code := run("run", "--output", "receipt=/dev/null", scriptPath, "--output", "graph=/dev/null")
142+
stdout, _, code := run("run", "--output", "receipt="+os.DevNull, scriptPath, "--output", "graph="+os.DevNull)
143143
assertExit(t, 0, code)
144144
assertValidSummary(t, stdout)
145145
}
146146

147147
func TestCLI_ScriptLast(t *testing.T) {
148-
stdout, _, code := run("run", "--output", "receipt=/dev/null", "--output", "graph=/dev/null", scriptPath)
148+
stdout, _, code := run("run", "--output", "receipt="+os.DevNull, "--output", "graph="+os.DevNull, scriptPath)
149149
assertExit(t, 0, code)
150150
assertValidSummary(t, stdout)
151151
}
@@ -161,23 +161,23 @@ func TestCLI_DefaultAllToStdout(t *testing.T) {
161161
}
162162

163163
func TestCLI_SummaryOnly(t *testing.T) {
164-
stdout, _, code := run("run", "--output", "graph=/dev/null", "--output", "receipt=/dev/null", scriptPath)
164+
stdout, _, code := run("run", "--output", "graph="+os.DevNull, "--output", "receipt="+os.DevNull, scriptPath)
165165
assertExit(t, 0, code)
166166
assertValidSummary(t, stdout)
167167
assertNotContains(t, stdout, "Hello World!")
168168
assertNotContains(t, stdout, "version:")
169169
}
170170

171171
func TestCLI_GraphOnly(t *testing.T) {
172-
stdout, _, code := run("run", "--output", "summary=/dev/null", "--output", "receipt=/dev/null", scriptPath)
172+
stdout, _, code := run("run", "--output", "summary="+os.DevNull, "--output", "receipt="+os.DevNull, scriptPath)
173173
assertExit(t, 0, code)
174174
assertContains(t, stdout, "Hello World!")
175175
assertNotContains(t, stdout, `"passed"`)
176176
assertNotContains(t, stdout, "version:")
177177
}
178178

179179
func TestCLI_ReceiptOnlyYAML(t *testing.T) {
180-
stdout, _, code := run("run", "--output", "graph=/dev/null", "--output", "summary=/dev/null", scriptPath)
180+
stdout, _, code := run("run", "--output", "graph="+os.DevNull, "--output", "summary="+os.DevNull, scriptPath)
181181
assertExit(t, 0, code)
182182
assertNotContains(t, stdout, `"passed"`)
183183
assertValidYAML(t, stdout)
@@ -186,7 +186,7 @@ func TestCLI_ReceiptOnlyYAML(t *testing.T) {
186186
}
187187

188188
func TestCLI_ReceiptOnlyJSON(t *testing.T) {
189-
stdout, _, code := run("run", "--output", "graph=/dev/null", "--output", "summary=/dev/null", "--receipt-format=json", scriptPath)
189+
stdout, _, code := run("run", "--output", "graph="+os.DevNull, "--output", "summary="+os.DevNull, "--receipt-format=json", scriptPath)
190190
assertExit(t, 0, code)
191191
assertValidJSON(t, stdout)
192192
assertContains(t, stdout, "shell.exec")
@@ -231,8 +231,8 @@ func TestCLI_JSONReceiptToFile(t *testing.T) {
231231

232232
_, _, code := run("run",
233233
"--output", "receipt="+receiptPath,
234-
"--output", "graph=/dev/null",
235-
"--output", "summary=/dev/null",
234+
"--output", "graph="+os.DevNull,
235+
"--output", "summary="+os.DevNull,
236236
"--receipt-format=json",
237237
scriptPath)
238238
assertExit(t, 0, code)
@@ -247,19 +247,19 @@ func TestCLI_JSONReceiptToFile(t *testing.T) {
247247
// --- Flags ---
248248

249249
func TestCLI_DryRun(t *testing.T) {
250-
stdout, _, code := run("run", "--dry-run", "--output", "graph=/dev/null", "--output", "receipt=/dev/null", scriptPath)
250+
stdout, _, code := run("run", "--dry-run", "--output", "graph="+os.DevNull, "--output", "receipt="+os.DevNull, scriptPath)
251251
assertExit(t, 0, code)
252252
assertValidSummary(t, stdout)
253253
}
254254

255255
func TestCLI_Trace(t *testing.T) {
256-
stdout, _, code := run("run", "--trace", "--output", "graph=/dev/null", "--output", "receipt=/dev/null", scriptPath)
256+
stdout, _, code := run("run", "--trace", "--output", "graph="+os.DevNull, "--output", "receipt="+os.DevNull, scriptPath)
257257
assertExit(t, 0, code)
258258
assertContains(t, stdout, `"trace"`)
259259
}
260260

261261
func TestCLI_Silent(t *testing.T) {
262-
_, stderr, code := run("--silent", "run", "--output", "graph=/dev/null", "--output", "receipt=/dev/null", scriptPath)
262+
_, stderr, code := run("--silent", "run", "--output", "graph="+os.DevNull, "--output", "receipt="+os.DevNull, scriptPath)
263263
assertExit(t, 0, code)
264264
if stderr != "" {
265265
t.Errorf("--silent should suppress stderr, got: %q", stderr)
@@ -269,7 +269,7 @@ func TestCLI_Silent(t *testing.T) {
269269
// --- Error cases ---
270270

271271
func TestCLI_InvalidOutputStream(t *testing.T) {
272-
_, _, code := run("run", "--output", "bogus=/dev/null", scriptPath)
272+
_, _, code := run("run", "--output", "bogus="+os.DevNull, scriptPath)
273273
assertExit(t, 1, code)
274274
}
275275

@@ -279,7 +279,7 @@ func TestCLI_MalformedOutput(t *testing.T) {
279279
}
280280

281281
func TestCLI_InvalidReceiptFormat(t *testing.T) {
282-
_, _, code := run("run", "--receipt-format=xml", "--output", "receipt=/dev/null", "--output", "graph=/dev/null", scriptPath)
282+
_, _, code := run("run", "--receipt-format=xml", "--output", "receipt="+os.DevNull, "--output", "graph="+os.DevNull, scriptPath)
283283
assertExit(t, 1, code)
284284
}
285285

‎cmd/devlore-test/devloretest/commands.go‎

Lines changed: 44 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package devloretest
66
import (
77
"encoding/json"
88
"fmt"
9+
"io"
910
"os"
1011
"strings"
1112

@@ -48,17 +49,23 @@ func (o *outputFlags) Type() string {
4849
return "stream=dest"
4950
}
5051

52+
// stdoutSentinel names the process's standard output in --output routing. [openDest] intercepts
53+
// it instead of opening it, so the name need not exist as a filesystem path — which it does not on
54+
// Windows. The discard destination needs no sentinel: os.DevNull already names the platform's own
55+
// device (/dev/null, or NUL on Windows) and opens correctly on each.
56+
const stdoutSentinel = "/dev/stdout"
57+
5158
func newRunCmd() *cobra.Command {
5259
outputs := &outputFlags{entries: map[string]string{
53-
"summary": "/dev/stdout",
54-
"receipt": "/dev/stdout",
55-
"graph": "/dev/stdout",
60+
"summary": stdoutSentinel,
61+
"receipt": stdoutSentinel,
62+
"graph": stdoutSentinel,
5663
}}
5764

5865
cmd := &cobra.Command{
5966
Use: "run [flags] <script.star>",
6067
Short: "Run a Starlark test script that plans and executes a graph",
61-
Long: `Run a Starlark test script through the graph execution engine.
68+
Long: fmt.Sprintf(`Run a Starlark test script through the graph execution engine.
6269
6370
The script uses plan.* bindings to build a graph and t.* assertions to
6471
verify expectations after execution.
@@ -68,14 +75,14 @@ Three output streams can be independently routed:
6875
summary JSON test result (passed, node_count, failures)
6976
receipt Full serialized execution graph
7077
71-
All streams default to /dev/stdout. Route to files or /dev/null:
78+
All streams default to %[1]s. Route to files or %[2]s:
7279
devlore-test run --output receipt=receipt.yaml test.star
73-
devlore-test run --output graph=/dev/null test.star`,
74-
Example: ` devlore-test run test.star
80+
devlore-test run --output graph=%[2]s test.star`, stdoutSentinel, os.DevNull),
81+
Example: fmt.Sprintf(` devlore-test run test.star
7582
devlore-test run --dry-run test.star
7683
devlore-test run --trace test.star
7784
devlore-test run --output receipt=receipt.yaml --receipt-format=json test.star
78-
devlore-test run --output graph=/dev/null --output receipt=/dev/null test.star`,
85+
devlore-test run --output graph=%[1]s --output receipt=%[1]s test.star`, os.DevNull),
7986
Args: cobra.ExactArgs(1),
8087
RunE: func(cmd *cobra.Command, args []string) error {
8188
return runTest(cmd, args[0], outputs)
@@ -144,11 +151,38 @@ func runTest(cmd *cobra.Command, script string, outputs *outputFlags) (err error
144151
return nil
145152
}
146153

147-
// openDest opens a destination path for writing. The caller must close the returned file.
148-
func openDest(path string) (*os.File, error) {
154+
// openDest opens a destination for writing.
155+
//
156+
// [stdoutSentinel] is intercepted rather than opened: it is not a filesystem path on Windows, and
157+
// the process's standard output must survive the caller's Close. Every other destination is a real
158+
// path — including the discard device, which os.DevNull names correctly on each platform.
159+
//
160+
// Parameters:
161+
// - `path`: the destination, either [stdoutSentinel] or a filesystem path.
162+
//
163+
// Returns:
164+
// - `io.WriteCloser`: the destination; the caller must close it, which is a no-op for standard
165+
// output.
166+
// - `error`: non-nil when a filesystem destination cannot be opened.
167+
func openDest(path string) (io.WriteCloser, error) {
168+
169+
if path == stdoutSentinel {
170+
return nopWriteCloser{os.Stdout}, nil
171+
}
172+
149173
return os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644) //nolint:gosec // G304: path from CLI flag
150174
}
151175

176+
// nopWriteCloser adapts a writer whose lifetime the caller does not own, so a deferred Close does
177+
// not shut the process's standard output.
178+
type nopWriteCloser struct{ io.Writer }
179+
180+
// Close discards the request, leaving the underlying writer open.
181+
//
182+
// Returns:
183+
// - `error`: always nil.
184+
func (nopWriteCloser) Close() error { return nil }
185+
152186
func writeSummary(dest string, result *Result) (err error) {
153187
f, err := openDest(dest)
154188
if err != nil {

‎cmd/devlore-test/devloretest/commands_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -160,9 +160,9 @@ t.expect_unit_count(1)
160160
cmd.SetArgs([]string{
161161
"--dry-run",
162162
"--receipt-format", "json",
163-
"--output", "summary=/dev/null",
163+
"--output", "summary=" + os.DevNull,
164164
"--output", "receipt=" + receiptFile,
165-
"--output", "graph=/dev/null",
165+
"--output", "graph=" + os.DevNull,
166166
script,
167167
})
168168
if err := cmd.Execute(); err != nil {
@@ -195,7 +195,7 @@ t.expect_unit_count(1)
195195
"--dry-run",
196196
"--output", "summary=" + summaryFile,
197197
"--output", "receipt=" + receiptFile,
198-
"--output", "graph=/dev/null",
198+
"--output", "graph=" + os.DevNull,
199199
script,
200200
})
201201
if err := cmd.Execute(); err != nil {

‎cmd/devlore-test/devloretest/root.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package devloretest
66

77
import (
88
"fmt"
9+
"os"
910

1011
"github.com/spf13/cobra"
1112
"github.com/spf13/viper"
@@ -30,7 +31,7 @@ func NewRootCmd() *cobra.Command {
3031
rootCmd := &cobra.Command{
3132
Use: "devlore-test",
3233
Short: "Graph test harness for Starlark plan + execute + verify",
33-
Long: `devlore-test is the graph test harness for the devlore execution engine.
34+
Long: fmt.Sprintf(`devlore-test is the graph test harness for the devlore execution engine.
3435
3536
It executes a Starlark test script that builds an execution graph, runs the
3637
graph through the engine, and verifies expectations against the results.
@@ -40,9 +41,9 @@ Output streams:
4041
summary Test result with pass/fail and expectation counts (default: stdout)
4142
receipt Full execution graph transaction log (default: stdout)
4243
43-
Use --output to route streams to files or /dev/null:
44-
devlore-test run --output receipt=/tmp/receipt.yaml test.star
45-
devlore-test run --output graph=/dev/null --output receipt=/dev/null test.star`,
44+
Use --output to route streams to files or %[1]s:
45+
devlore-test run --output receipt=receipt.yaml test.star
46+
devlore-test run --output graph=%[1]s --output receipt=%[1]s test.star`, os.DevNull),
4647
CompletionOptions: cobra.CompletionOptions{
4748
HiddenDefaultCmd: true,
4849
},

‎cmd/writ/scenario_integration_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ func initializeRepo(t *testing.T, dest string) {
144144
cmd := exec.CommandContext(context.Background(), "git", append([]string{"-C", dest}, args...)...)
145145
// The real user's global git config must not reach the sandbox repo (branch-protection
146146
// hooks would reject the baseline commit on main).
147-
cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null")
147+
cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL="+os.DevNull, "GIT_CONFIG_SYSTEM="+os.DevNull)
148148
if output, err := cmd.CombinedOutput(); err != nil {
149149
t.Fatalf("git %v: %v\n%s", args, err, output)
150150
}

‎cmd/writ/writ/repo_cmd_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ func sourceRepository(t *testing.T) string {
3535
{"-c", "user.name=t", "-c", "user.email=t@invalid", "commit", "--quiet", "--allow-empty", "-m", "seed"},
3636
} {
3737
command := exec.CommandContext(context.Background(), "git", append([]string{"-C", root}, args...)...)
38-
command.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null")
38+
command.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL="+os.DevNull, "GIT_CONFIG_SYSTEM="+os.DevNull)
3939
if output, err := command.CombinedOutput(); err != nil {
4040
t.Fatalf("git %v: %v\n%s", args, err, output)
4141
}

‎docs/plans/platform-test-matrix.md‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ packages, the first time it had ever run on Darwin. `test (windows-latest)` fail
167167

168168
| Cause | Failures | Bucket | Disposition |
169169
| --- | --- | --- | --- |
170-
| `binary` lacks `.exe`, so every exec returns -1 | ~37 | 2 | Fixed — `cli_test.go` |
170+
| `binary` lacks `.exe`, so every exec returns -1 | est. ~37 | 2 | Fixed — `cli_test.go` |
171171
| `Rel()` returns `\` separators | 6 | **1** | **Issue #377** |
172172
| Permission-bit assertions (`document_test.go`, `sops_integration_test.go`) | 5 | 2 | Pending |
173173
| `open /dev/null` | 2 | 1 or 2 | Pending — depends whether the product hardcodes it |
@@ -193,6 +193,38 @@ Clearing the `.exe` cause first is deliberate: ~37 masked failures may be concea
193193
the next run reveals what was behind them. The remaining counts above should be treated as a
194194
lower bound until that run reports.
195195

196+
#### Phase 2 results, corrected — second run, PR #378 / commit `762c61ec`
197+
198+
**63 → 47.** The `.exe` estimate was wrong: it cleared **16**, not ~37. The fix itself worked —
199+
exit codes became real numbers instead of `-1`, so the binary starts — but fourteen `TestCLI_*`
200+
tests were failing for a *second* reason sitting behind the first. The masking risk named above
201+
was real, which is why the table's counts were labelled a lower bound.
202+
203+
| Cause | Failures | Bucket | Disposition |
204+
| --- | --- | --- | --- |
205+
| Output streams default to `/dev/stdout`; `/dev/null` in examples | ~16 | **1** | **Issue #379** |
206+
| Separator form — `Rel`, `Abs`, `String`, `registry.FilePath` | 11 | mixed | **Issue #377**, needs splitting |
207+
| Permission-bit assertions | 5 | 2 | Pending |
208+
| `runner_test` file and compensation failures | 4 | ? | Pending — needs reading |
209+
| Windows path breaks Starlark escape parsing | 1 | **1** | **Issue #376**, correctly sized |
210+
211+
**#379 is the dominant remaining cause and a genuine product defect.**
212+
`cmd/devlore-test/devloretest/commands.go:53-55` defaults every stream to `/dev/stdout`, and
213+
`openDest` (`commands.go:148`) is a plain `os.OpenFile`. On Windows the open fails and
214+
`devlore-test run` aborts before emitting anything — the CLI cannot run there at all. The
215+
documented examples route to `/dev/null`, so the shipped guidance is Unix-only too.
216+
217+
**#376 was correctly sized** — `invalid escape sequence` appears exactly once in the log.
218+
219+
**#377 needs its classification split.** `Rel()` returning `\` is bucket 1: `Path` is serialized,
220+
so document bytes and checksums differ per platform. But `Abs()` and `String()` returning
221+
`\project\src\main.go` is *correct* on Windows — those three assertions feed Unix literals like
222+
`/project/src/main.go` and expect them back, making them bucket 2. The two groups must not be
223+
fixed the same way.
224+
225+
Three product defects have now been surfaced by one CI change — #376, #377, #379 — none of them
226+
reachable by the previous ubuntu-only gate.
227+
196228
### Phase 3: Fix — one branch per cluster
197229

198230
- [ ] Bucket 1 defects: fixed in the product, each with a regression test.

0 commit comments

Comments
 (0)