Skip to content

fix(cmd/morphic)!: stop a failed -o write masking exit 1 - #371

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/cli-output-write-exit-code
Open

fix(cmd/morphic)!: stop a failed -o write masking exit 1#371
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/cli-output-write-exit-code

Conversation

@OmarAlJarrah

@OmarAlJarrah OmarAlJarrah commented Aug 9, 2026

Copy link
Copy Markdown
Member

Summary

morphic compile let whichever failure came last decide its exit code. A run whose diagnostics
reached the --fail-on threshold returned 1 when -o could be written and 2 when it could
not, so the same spec with the same diagnostics reported a different code depending only on where
-o pointed:

$ morphic compile bad.yaml -o /dev/null; echo "EXIT=$?"
error openapi/unresolved-ref bad.yaml#: not found -- struct is nil at /components/schemas
error openapi/unresolved-ref bad.yaml#/paths/~1x/get/.../schema: unresolved $ref "#/components/schemas/Missing"
morphic: create output "/dev/null": open /dev/.null.tmpf332ba9d918baf53: operation not permitted
EXIT=2

$ morphic compile bad.yaml -o out.json >/dev/null; echo "EXIT=$?"
error openapi/unresolved-ref ...
EXIT=1

A caller reading the code alone could not tell a spec that failed the gate from a destination it
could not write.

The diagnostics' verdict now wins. A run that reached the threshold exits 1 wherever -o
pointed; 2 is left to mean a run that failed for a reason outside the spec. The write error still
prints to stderr either way, so nothing is hidden from a human. README's exit-code paragraph now
says which wins, and replaceFile's doc comment records why the write itself is left alone.

After:

### bad spec -> /dev/null
error openapi/unresolved-ref bad.yaml#: not found -- struct is nil at /components/schemas
error openapi/unresolved-ref bad.yaml#/paths/~1x/get/responses/200/content/application~1json/schema: unresolved $ref "#/components/schemas/Missing"
morphic: create output "/dev/null": open /dev/.null.tmp05de98371b7311c5: operation not permitted
EXIT=1

### bad spec -> out.json
error openapi/unresolved-ref bad.yaml#: not found -- struct is nil at /components/schemas
error openapi/unresolved-ref bad.yaml#/paths/~1x/get/responses/200/content/application~1json/schema: unresolved $ref "#/components/schemas/Missing"
EXIT=1

### bad spec -> /dev/stdout
morphic: create output "/dev/stdout": open /dev/.stdout.tmpc96056da8fd06b5d: operation not permitted
EXIT=1

### bad spec -> read-only dir holding a writable file
morphic: create output "ro/out.json": open ro/.out.json.tmp98bac84af5499a50: permission denied
EXIT=1

### clean spec -> /dev/null
morphic: create output "/dev/null": open /dev/.null.tmp3a88f253dbf467ce: operation not permitted
EXIT=2

### clean spec -> read-only dir
morphic: create output "ro/out.json": open ro/.out.json.tmpd122db8689c6614b: permission denied
EXIT=2

### clean spec -> good.ir.json
EXIT=0

Why the exit code and not the write

The other direction on the table was to recognise destinations that cannot be published by rename
and write through to them directly, making -o /dev/null work. That is deliberately not done here,
for three reasons:

  1. It would not fix this bug. The masking is not a property of the destination's file type. A
    read-only directory holding a writable regular file masks the exit code identically (captured
    above), and that case is pinned as deliberate by TestWriteParsed_ReadOnlyDirFails. Recognising
    non-regular destinations cannot reach it. Fixing the precedence fixes every shape at once,
    including the ones nobody has enumerated yet — a full disk, a name too long for a temp suffix.
  2. It would reverse a trade that was made on purpose. Probed on main: a FIFO in a writable
    directory does not fail today — the temp file is created beside it and the rename replaces the
    FIFO with a regular file, the same class as the symlink and hard-link cases replaceFile
    documents and three tests pin ("each is a way to reach the destination's bytes other than
    through its own name"). Writing through to it is a reversal of that contract, not a bug fix.
  3. It costs the CLI a bounded-time guarantee. Probed: open(fifo, O_WRONLY) on a reader-less
    FIFO does not return. -o some.fifo would hang instead of failing. Doing it safely needs
    O_NONBLOCK plus an EAGAIN retry loop — a bounded-write problem, not an exit-code one.

The issue also notes that direction interacts with the exit-code taxonomy #58 is still deciding
(spec problems become diagnostics at 1; 2 stays for I/O and programmer errors). This change leaves both codes' documented meanings exactly as written and only decides
which wins when both fire, so it settles nothing on that thread's behalf.

-o /dev/null therefore still fails. That limitation is now recorded in replaceFile's doc
comment (where a reader asking "why can't I write there?" lands) and in README's CLI section
(where a user asking the same question lands), rather than only here.

Test plan

  • TestRunParse_WriteFailureDoesNotMaskDiagnostics — four flat rows over two destination shapes
    (a missing parent directory; a writable file inside a read-only directory) crossed with two
    specs (one reaching the threshold, one clean), asserting 1 and 2 respectively.

  • Every row also asserts stderr carries create output, so a row cannot pass because the run died
    earlier. The fixture spec is chosen so it still lowers to a non-nil document: a spec that lowers
    to nil returns 1 before the write is attempted and would exercise nothing.

  • /dev/null is covered by the captured runs above rather than by a test, on purpose: under a root
    /dev the temp file would be created and the rename would replace /dev/null itself.

  • Mutation check. Reverting compileSpec to return 2 and re-running reddens exactly the two
    threshold rows, on both destination shapes:

    --- FAIL: TestRunParse_WriteFailureDoesNotMaskDiagnostics/threshold_reached,_missing_parent_dir
            Error:      Not equal:
                        expected: 1
                        actual  : 2
    --- FAIL: TestRunParse_WriteFailureDoesNotMaskDiagnostics/threshold_reached,_read-only_dir
            Error:      Not equal:
                        expected: 1
                        actual  : 2
    

    The two clean-spec rows stay green, which is correct — they were always 2.

  • Existing pins still pass unchanged: TestRunParse_OutputCreateError (clean spec, unwritable
    destination, 2) and TestWriteParsed_ReadOnlyDirFails.

  • Full gate green: gofmt, go vet ./..., golangci-lint run (0 issues), go build ./...,
    ./scripts/check-coverage.sh (100% of 4946 statements).

Breaking

morphic compile over a spec that reaches the --fail-on threshold now exits 1, not 2, when
the -o destination cannot be written. A caller that read 2 as "no output was produced" must
read stderr for the write error instead. 2 remains the code when the spec had nothing to report
at the threshold and the write failed.

Closes #308

compile let whichever failure came last decide its exit code, so a run whose
diagnostics reached the --fail-on threshold returned 1 when -o could be written
and 2 when it could not. Whether a destination can be written is a property of
the destination, not of the spec: -o publishes by rename, and both /dev/null and
a read-only directory holding a writable file refuse the temp file that needs.
The same spec with the same diagnostics therefore reported a different code
depending on where -o pointed, and a caller reading the code alone could not
tell a spec that failed the gate from a destination it could not write.

The diagnostics' verdict now wins. A run that reached the threshold exits 1
wherever -o pointed, and 2 is left to mean a run that failed for a reason
outside the spec. The write error still prints to stderr either way.

Writing through to a destination that cannot be published by rename, so that
-o /dev/null works at all, is deliberately not done here: it would honour what a
name points at rather than replacing the name, which is the trade replaceFile
already declines for symlinks and hard links; opening a reader-less FIFO for
writing blocks indefinitely; and it reaches into the exit-code taxonomy that is
still being settled. It would also not fix this bug, which is not specific to
those destinations — a read-only directory holding a writable regular file masks
the exit code identically.

BREAKING CHANGE: morphic compile over a spec that reaches the --fail-on
threshold now exits 1, not 2, when the -o destination cannot be written. A
caller that read 2 as "no output was produced" must read stderr for the write
error instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cli: -o /dev/null fails, and its exit 2 masks the diagnostics' exit 1

1 participant