Fix hush-file-write: pr to a file stream must not be silenced under -q#26
Conversation
pyrex41
left a comment
There was a problem hiding this comment.
Summary
The PR implements a targeted native override for the kernel pr (defined in klambda/writer.kl as an unconditional (if (value *hush*) S (write...)) gate) inside install_native_stdlib. The wrapper consults *hush* only for the exact *stoutput* stream object (using Lua table identity); for all other streams (including files opened via (open ... out) and used by write-to-file), it temporarily clears *hush*, delegates to the original compiled-KL pr (so the write-chars/write-byte path and return value are byte-identical), and restores state. Updated CLI subprocess tests in cli_spec.lua now assert both that file payloads are written under -q and that pr to (stoutput) remains silenced. The change aligns shen-lua with shen-cl/shen-go/ShenScript for file writes while preserving the documented quieting behavior for console output. The implementation is careful about state, call-time lookups, and APP/PARTIAL currying paths; the dominant remaining risk areas are untested *sterror* interactions and any future stream-redirection scenarios.
Issue counts by severity
- bugs: 0
- suggestions: 1
- nits: 3
| -- so the write path stays byte-identical. | ||
| local orig_pr = F["pr"] | ||
| local function pr(s, st) | ||
| if GLOBALS["*hush*"] and st == GLOBALS["*stoutput*"] then return s end |
There was a problem hiding this comment.
[suggestion] The silencing condition uses a strict identity test st == GLOBALS["*stoutput*"]. This means pr writes to *sterror* (and any user-redirected or other streams) are no longer silenced under *hush* (the second if GLOBALS["*hush*"] branch forces a write). The kernel pr previously silenced all streams; the PR description and comment focus exclusively on "standard output" vs. files, but *sterror* behavior changed as a side-effect. No test exercises (pr ... (sterror)) (or equivalent) under -q.
Suggestion: Either extend the exemption (or the "do not hush" policy) to also cover *sterror* if errors/diagnostics should remain visible under quiet, or add an explicit or st == GLOBALS["*sterror*"] + comment if stderr should also be hushed. Add a cli_spec or io_spec case for stderr under -q to lock in the chosen policy. Update the long comment at 741-751 to explicitly state the *sterror* rule.
| -- restore it so we don't perturb global state. | ||
| local saved = GLOBALS["*hush*"] | ||
| GLOBALS["*hush*"] = false | ||
| local ok, res = pcall(orig_pr, s, st) |
There was a problem hiding this comment.
[nit] The force-write path for non-stdout streams under hush uses pcall + manual error(res, 0) re-raise after restore. While this correctly restores *hush* even on error (and matches patterns elsewhere e.g. boot.lua:703), the level-0 error call can alter the reported source location / traceback relative to what a direct call to the original orig_pr would have produced. In normal success paths this is invisible.
Suggestion: Consider a local function force_pr(...) local saved=...; GLOBALS["*hush*"]=false; local res = orig_pr(...); GLOBALS["*hush*"]=saved; return res end and pcall only the wrapper (or just let errors propagate if the restore can be done in an xpcall finally-style). Add a comment explaining why pcall is required here vs. other native overrides.
| -- Return a DISTINCT value (99) so the only way the marker can appear is the | ||
| -- pr write itself — which must be silenced under -q. | ||
| local marker = "NOISE_22_MARKER" | ||
| local expr = '(do (pr "' .. marker .. '" (stoutput)) 99)' |
There was a problem hiding this comment.
[nit] The new "other half" test constructs an expression that calls pr to (stoutput) but relies on -e result printing (the launcher io.write(shen.call("shen.app", ...)) at bin/shen:96) to produce observable stdout for the non-hushed case. The test correctly uses a distinct return value (99) so the marker can only come from the pr itself, and it captures combined stdout+stderr. However the local outn = run(...) discards the exit code (unlike the -q case) and the assertions only check marker presence/absence rather than also asserting that the result echo still appears.
Suggestion: Capture and assert the exit code for the non--q case too (for symmetry), and/or assert that the "99" result echo is present in outn (to document that -e value printing is intentionally outside the hush gate). Minor robustness improvement only.
| end | ||
|
|
||
| local function install(name, fn, arity) F[name] = fn; FA[fn] = arity end | ||
| install("pr", pr, 2) |
There was a problem hiding this comment.
[nit] The install("pr", pr, 2) call (and the preceding local function install...) was inserted before the install("variable?", ...) line that was previously first. While install order is irrelevant (pure F/FA table writes with no side effects or dependencies among these overrides), the diff moves a pre-existing line.
Suggestion: No functional change needed; purely cosmetic. If desired, keep the original install order and place the pr install at the end of the block for minimal diff.
Review follow-up. The native pr override exempts only *stoutput* from *hush*, so *sterror* (and file streams) write under -q as a side-effect. That is the intended policy — quiet mode silences standard-output chatter only, not diagnostics — but it was implicit and untested. - prims.lua: spell out in the comment that file streams AND *sterror* are not silenced by *hush*; only *stoutput* is. - cli_spec.lua: add a regression asserting `pr` to (value *sterror*) under -q still writes (distinct return value so the marker can only come from the pr). make test: 453 pass, 0 fail. bifrost: 30/30 across 6 impls. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d194d78 to
7491fcb
Compare
Under
-q(*hush*true),prto a file stream wrote a zero-byte file: the canonical kernelprgates all output on*hush*. An explicit write to a file is user intent, not the stdout chatter*hush*exists to silence.Adds a native
proverride inprims.luathat silences only when*hush*is set and the target stream is*stoutput*; for any other stream it temporarily clears*hush*, delegates to the original compiled-KLpr(byte-identical write path), and restores*hush*. Canonical kernel sources untouched.(Note: the earlier
fix/hush-pr-file-22was never actually merged tomain; this re-applies the fix cleanly on the current line.)Verification
hush-file-write: green for shen-lua; full corpus 30/30 across 6 impls.make test: 451 pass, 0 fail.make certify: 100%.🤖 Generated with Claude Code