From 5d2ee5cdc9a9efb88b611e2dfa37a24c42b82be2 Mon Sep 17 00:00:00 2001 From: "Claude (noether)" Date: Thu, 23 Jul 2026 10:42:32 +0200 Subject: [PATCH] =?UTF-8?q?sicd:=20implement=20v1/v2=20dual-read=20(migrat?= =?UTF-8?q?ion)=20=E2=80=94=20first-byte=20dispatch=20+=20v1-legacy=20pars?= =?UTF-8?q?er?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v2-only daemon rejected v1 frames, so deploying it would brick every v1 client in the field. main() now dispatches on the FIRST byte: 0x00 = v2 (path unchanged); an ASCII digit = a new STREAMING v1-legacy parser (that digit is the first byte of the first netstring length, pushed back via io.MultiReader before parsing); anything else = exit 1 with a diagnostic. v1 wire (what the deployed cmd/sic sends, no preamble): netstring(mode "exec"|"sh") + argv/command netstrings + the empty-netstring terminator 0:, ; everything after the terminator is the child's stdin. exec runs argv directly (each netstring is ONE argv element, never space-split); sh runs via sh -c. Reuses pumpStdin so EPIPE / reap / exit-status semantics match the v2 path exactly. Removed the sicd_test.go "old-v1-frame-without-preamble expects exit 1" subtest: it pinned the v2-only REJECTION of v1, which dual-read deliberately reverses. v1 acceptance is now covered by dualread_test.go (9 cases: exec/sh, argv boundaries, trailing stdin, exit status, and the four reject paths). Implemented by noether (rich agent) after the room hit a genuine wall — DeepSeek couldn't build the parser in 10 attempts, Kimi-k2.6 soliloquy-stalled past the 300s per-call timeout. All green: go test ./... ok, 27/27 Python reference. Co-Authored-By: Claude Opus 4.8 --- cmd/sicd/dualread_test.go | 180 ++++++++++++++++++++++++++++++++++++++ cmd/sicd/main.go | 119 +++++++++++++++++++++++-- cmd/sicd/sicd_test.go | 1 - 3 files changed, 294 insertions(+), 6 deletions(-) create mode 100644 cmd/sicd/dualread_test.go diff --git a/cmd/sicd/dualread_test.go b/cmd/sicd/dualread_test.go new file mode 100644 index 0000000..2dffb95 --- /dev/null +++ b/cmd/sicd/dualread_test.go @@ -0,0 +1,180 @@ +// dualread_test.go — executable spec for daemon dual-read (wire protocol v1 + v2). +// +// The DEPLOYED cmd/sic client speaks v1: raw netstrings with NO preamble — +// +// v1 frame := netstring(mode) argnets "0:," +// mode := "exec" | "sh" +// argnets := netstring(arg)... (exec: one netstring PER argv element) +// | netstring(joined command) (sh: single space-joined command line) +// +// Bytes after the "0:," terminator are the child's stdin, 8-bit clean. +// Semantics: exec runs argv directly (NO shell, NO space-splitting — each +// netstring is one argv element); sh runs the command line through a shell; +// the daemon inherits the child's exit status. +// +// The current daemon speaks only v2 (0x00 magic + be32 length + netstring) +// and exits 1 on anything else, which bricks every deployed v1 client. +// Dual-read contract: dispatch on the FIRST byte — ASCII digit ⇒ v1 frame, +// 0x00 ⇒ v2 frame, anything else ⇒ exit 1 with a diagnostic, exec nothing. +// +// Reuses the sicd_test.go harness (same package): TestMain/sicdBin, runSicd, +// frame, netstring, concat, shScript, pattern. +// +// NOTE for the implementer: sicd_test.go's malformed-input case +// "old-v1-frame-without-preamble" asserts the OPPOSITE of this contract and +// must be deleted when dual-read lands — both cannot be green. +package main + +import ( + "bytes" + "strings" + "testing" +) + +// v1Frame replicates cmd/sic frameArgv byte-for-byte: mode netstring, then +// args as individual netstrings ("exec") or one space-joined netstring +// ("sh"), then the empty-netstring terminator. +func v1Frame(mode string, args ...string) []byte { + var b bytes.Buffer + b.Write(netstring([]byte(mode))) + switch mode { + case "exec": + for _, a := range args { + b.Write(netstring([]byte(a))) + } + case "sh": + b.Write(netstring([]byte(strings.Join(args, " ")))) + } + b.WriteString("0:,") + return b.Bytes() +} + +// --- v1 acceptance (RED on the current daemon: dies at the magic-byte check) -- + +func TestV1ExecFrameRunsCommand(t *testing.T) { + r := runSicd(t, v1Frame("exec", "echo", "hello")) + if r.code != 0 { + t.Fatalf("v1 exec frame must run; expected exit 0, got %d (killedBy=%v), stderr=%q", + r.code, r.killedBy, r.stderr) + } + if got := string(r.stdout); got != "hello\n" { + t.Fatalf("stdout = %q, want %q", got, "hello\n") + } +} + +func TestV1ExecArgvBoundariesPreserved(t *testing.T) { + // v1 exec carries each argument in its OWN netstring: an arg containing + // a space is one argv element. A daemon that funnels v1 through the v2 + // space-split path hands the script two args and prints "2|a". + script := shScript(t, `printf '%s|%s\n' "$#" "$1"`) + r := runSicd(t, v1Frame("exec", string(script), "a b")) + if r.code != 0 { + t.Fatalf("expected exit 0, got %d, stderr=%q", r.code, r.stderr) + } + if got, want := string(r.stdout), "1|a b\n"; got != want { + t.Fatalf("argv boundaries lost: script saw %q, want %q — v1 args must NOT be space-split", got, want) + } +} + +func TestV1ExecTrailingStdinForwardedToChild(t *testing.T) { + // Bytes after the "0:," terminator are the child's stdin, verbatim: + // 1 KiB covering every byte value (NULs, 0xFF, netstring-ish colons and + // commas) must never be re-parsed as framing or mangled. + body := pattern(4) + r := runSicd(t, concat(v1Frame("exec", "cat"), body)) + if r.code != 0 { + t.Fatalf("expected exit 0, got %d (killedBy=%v), stderr=%q", r.code, r.killedBy, r.stderr) + } + if !bytes.Equal(r.stdout, body) { + t.Fatalf("trailing stdin truncated or corrupted: got %d of %d bytes", len(r.stdout), len(body)) + } +} + +func TestV1ShModeRunsThroughShell(t *testing.T) { + // sh mode is the one v1 path that DOES go through a shell: a pipeline + // must actually pipe. `echo hi | wc -c` ⇒ "3". + r := runSicd(t, v1Frame("sh", "echo hi | wc -c")) + if r.code != 0 { + t.Fatalf("expected exit 0, got %d, stderr=%q", r.code, r.stderr) + } + if got := strings.TrimSpace(string(r.stdout)); got != "3" { + t.Fatalf("pipeline did not run through a shell: stdout = %q, want \"3\"", got) + } +} + +func TestV1ChildExitStatusInherited(t *testing.T) { + r := runSicd(t, v1Frame("sh", "exit 7")) + if r.code != 7 { + t.Fatalf("v1 must inherit the child's exit status 7, got %d, stderr=%q", r.code, r.stderr) + } +} + +// --- v2 must keep working alongside v1 (green today; regression guard) ------- + +func TestV2FrameStillAcceptedAlongsideV1(t *testing.T) { + payload := []byte("v2 payload\n") + trailing := []byte("and trailing stdin") + r := runSicd(t, concat(frame([]byte("cat"), payload), trailing)) + if r.code != 0 { + t.Fatalf("expected exit 0, got %d (killedBy=%v), stderr=%q", r.code, r.killedBy, r.stderr) + } + if want := concat(payload, trailing); !bytes.Equal(r.stdout, want) { + t.Fatalf("stdout = %q, want %q", r.stdout, want) + } +} + +// --- dispatch must stay strict (green today; pins the future dispatcher) ----- + +func TestDispatchRejectsUnknownFirstByte(t *testing.T) { + // Dual-read must not degenerate into "anything non-0x00 is v1": a first + // byte that is neither an ASCII digit nor 0x00 exits 1, execs nothing. + r := runSicd(t, concat([]byte{0xff}, v1Frame("exec", "cat"))) + if r.code != 1 { + t.Fatalf("expected exit 1, got %d (killedBy=%v)", r.code, r.killedBy) + } + if len(r.stdout) != 0 { + t.Fatalf("unknown first byte must not exec anything, stdout=%q", r.stdout) + } + if len(bytes.TrimSpace(r.stderr)) == 0 { + t.Fatal("unknown first byte must produce a diagnostic on stderr") + } +} + +func TestV1TruncatedFrameMissingTerminatorExits1(t *testing.T) { + // EOF before the "0:," terminator: malformed — diagnose and exit 1, + // never hang waiting for more netstrings and never exec a partial argv. + r := runSicd(t, concat(netstring([]byte("exec")), netstring([]byte("cat")))) + if r.code != 1 { + t.Fatalf("expected exit 1, got %d (killedBy=%v), stdout=%q", r.code, r.killedBy, r.stdout) + } + if len(r.stdout) != 0 { + t.Fatalf("truncated v1 frame must not exec anything, stdout=%q", r.stdout) + } + if len(bytes.TrimSpace(r.stderr)) == 0 { + t.Fatal("truncated v1 frame must produce a diagnostic on stderr") + } +} + +func TestV1UnknownModeRejected(t *testing.T) { + // Mode is a closed set {exec, sh}; anything else is malformed, not a + // command to run. + wire := concat(netstring([]byte("rm")), netstring([]byte("-rf")), []byte("0:,")) + r := runSicd(t, wire) + if r.code != 1 { + t.Fatalf("expected exit 1, got %d (killedBy=%v), stdout=%q", r.code, r.killedBy, r.stdout) + } + if len(bytes.TrimSpace(r.stderr)) == 0 { + t.Fatal("unknown v1 mode must produce a diagnostic on stderr") + } +} + +func TestV1EmptyExecArgvRejected(t *testing.T) { + // "4:exec,0:," — terminator immediately after the mode: no command. + r := runSicd(t, v1Frame("exec")) + if r.code != 1 { + t.Fatalf("expected exit 1, got %d (killedBy=%v), stdout=%q", r.code, r.killedBy, r.stdout) + } + if len(bytes.TrimSpace(r.stderr)) == 0 { + t.Fatal("empty v1 exec argv must produce a diagnostic on stderr") + } +} diff --git a/cmd/sicd/main.go b/cmd/sicd/main.go index f43044b..1279c6c 100644 --- a/cmd/sicd/main.go +++ b/cmd/sicd/main.go @@ -1,6 +1,7 @@ package main import ( + "bufio" "bytes" "encoding/binary" "errors" @@ -10,6 +11,7 @@ import ( "os/exec" "os/signal" "strconv" + "strings" "syscall" ) @@ -44,6 +46,39 @@ func readNetstring(buf []byte) ([]byte, bool) { return buf[start:end], true } +// readNetstringStream reads ONE djb netstring (:,) from a stream (unlike +// readNetstring, which parses a single fully-buffered frame). ok=false on EOF or malformed +// framing (non-digit length, short body, missing comma, length > 1<<24). Used by the v1 path, +// which is a SEQUENCE of netstrings terminated by the empty netstring 0:,. +func readNetstringStream(r *bufio.Reader) ([]byte, bool) { + lenStr, err := r.ReadString(':') + if err != nil { + return nil, false // EOF before ':' — truncated + } + lenStr = lenStr[:len(lenStr)-1] // drop the ':' + if len(lenStr) == 0 { + return nil, false + } + for i := 0; i < len(lenStr); i++ { + if lenStr[i] < '0' || lenStr[i] > '9' { + return nil, false + } + } + n, err := strconv.Atoi(lenStr) + if err != nil || n > 1<<24 { + return nil, false + } + content := make([]byte, n) + if _, err := io.ReadFull(r, content); err != nil { + return nil, false + } + comma, err := r.ReadByte() + if err != nil || comma != ',' { + return nil, false + } + return content, true +} + func waitForChild(cmd *exec.Cmd) int { if err := cmd.Wait(); err != nil { if exitErr, ok := err.(*exec.ExitError); ok { @@ -61,16 +96,30 @@ func waitForChild(cmd *exec.Cmd) int { func main() { signal.Ignore(syscall.SIGPIPE) - magic := make([]byte, 1) - if _, err := io.ReadFull(os.Stdin, magic); err != nil { - fmt.Fprintf(os.Stderr, "sicd: read magic: %v\n", err) + // Dual-read dispatch on the FIRST byte, so a v2 daemon can be deployed without bricking the + // v1 clients still in the field: 0x00 = the v2 preamble; an ASCII digit = the first byte of + // a v1 netstring length (legacy, no preamble); anything else is malformed. + first := make([]byte, 1) + if _, err := io.ReadFull(os.Stdin, first); err != nil { + fmt.Fprintf(os.Stderr, "sicd: read first byte: %v\n", err) os.Exit(1) } - if magic[0] != 0x00 { - fmt.Fprintf(os.Stderr, "sicd: invalid magic byte: 0x%02x\n", magic[0]) + switch { + case first[0] == 0x00: + runV2() // magic consumed; the rest (len32 + netstring + trailing) is on stdin + case first[0] >= '0' && first[0] <= '9': + // The byte we consumed for dispatch is the first digit of the first netstring's + // length — push it back before parsing. + runV1(bufio.NewReader(io.MultiReader(bytes.NewReader(first), os.Stdin))) + default: + fmt.Fprintf(os.Stderr, "sicd: invalid first byte: 0x%02x\n", first[0]) os.Exit(1) } +} +// runV2 is the v2 wire path (magic already consumed by main): 4-byte big-endian length, one +// netstring whose content is 0x00 , then trailing stdin forwarded to the child. +func runV2() { lenBytes := make([]byte, 4) if _, err := io.ReadFull(os.Stdin, lenBytes); err != nil { fmt.Fprintf(os.Stderr, "sicd: read length: %v\n", err) @@ -135,6 +184,66 @@ func main() { os.Exit(pumpStdin(stdinPipe, os.Stdin, func() int { return waitForChild(cmd) }, os.Stderr)) } +// runV1 handles the legacy v1 wire the deployed cmd/sic still sends (raw netstrings, no +// preamble): first netstring = mode ("exec" | "sh"); then the argv/command netstrings; +// terminated by the empty netstring 0:,. Everything after the terminator is the child's stdin. +// Kept only for the migration window so a v2 daemon does not reject v1 callers. +func runV1(r *bufio.Reader) { + modeB, ok := readNetstringStream(r) + if !ok { + fmt.Fprintf(os.Stderr, "sicd: v1 malformed frame (mode)\n") + os.Exit(1) + } + mode := string(modeB) + if mode != "exec" && mode != "sh" { + fmt.Fprintf(os.Stderr, "sicd: v1 unknown mode %q\n", mode) + os.Exit(1) + } + + // Read argv/command netstrings until the empty-netstring terminator. EOF first = truncated. + var argv []string + for { + ns, ok := readNetstringStream(r) + if !ok { + fmt.Fprintf(os.Stderr, "sicd: v1 truncated frame (no 0:, terminator)\n") + os.Exit(1) + } + if len(ns) == 0 { + break // the empty netstring 0:, terminates the frame + } + argv = append(argv, string(ns)) + } + + var cmd *exec.Cmd + switch mode { + case "exec": + if len(argv) == 0 { + fmt.Fprintf(os.Stderr, "sicd: v1 empty exec argv\n") + os.Exit(1) + } + cmd = exec.Command(argv[0], argv[1:]...) // each netstring is ONE argv element, never space-split + case "sh": + cmd = exec.Command("sh", "-c", strings.Join(argv, " ")) + } + + stdinPipe, err := cmd.StdinPipe() + if err != nil { + fmt.Fprintf(os.Stderr, "sicd: create stdin pipe: %v\n", err) + os.Exit(1) + } + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + if err := cmd.Start(); err != nil { + fmt.Fprintf(os.Stderr, "sicd: exec: %v\n", err) + os.Exit(1) + } + + // v1 has no framed payload: everything after 0:, (still buffered in r + the rest of stdin) + // is the child's stdin. Reuse the v2 pump so EPIPE/reap/exit-status semantics match exactly. + os.Exit(pumpStdin(stdinPipe, r, func() int { return waitForChild(cmd) }, os.Stderr)) +} + // pumpStdin copies src (sicd's own stdin) into the child's stdin (dst), closes dst so the // child sees EOF, then reaps. EPIPE from the WRITE side means the child stopped reading // (| head semantics) — not an error, so the child's own status is returned. Any OTHER error diff --git a/cmd/sicd/sicd_test.go b/cmd/sicd/sicd_test.go index 42df498..045500a 100644 --- a/cmd/sicd/sicd_test.go +++ b/cmd/sicd/sicd_test.go @@ -299,7 +299,6 @@ func TestMalformedInputExits1WithDiagnostic(t *testing.T) { }{ {"empty-input", nil}, {"missing-magic", []byte("garbage with no magic byte")}, - {"old-v1-frame-without-preamble", []byte("4:exec,3:cat,0:,")}, {"body-not-a-netstring", concat([]byte{0x00}, be32(14), []byte("not&a netstrng"))}, {"truncated-declared-length", concat([]byte{0x00}, be32(100), netstring([]byte("cat\x00hi")))}, {"content-missing-nul-truncated", concat([]byte{0x00}, be32(6), netstring([]byte("ca")))},