diff --git a/cmd/crw/main.go b/cmd/crw/main.go index d38df6aba..838cb2d2c 100644 --- a/cmd/crw/main.go +++ b/cmd/crw/main.go @@ -178,7 +178,9 @@ func modes() []mode { }}, {"review", true, func(c invocation) int { return command.Run(c.ctx, c.args, c.stdout, c.stderr) }}, {"recall", false, func(c invocation) int { return recall.Run(c.args, c.stdout, c.stderr, recallNow()) }}, - {"pabcd", false, func(c invocation) int { return harness.Pabcd(c.args, os.Stdin, c.stdout, c.stderr, harness.Verbs()) }}, + {"pabcd", false, func(c invocation) int { + return harness.PabcdContext(c.ctx, c.args, os.Stdin, c.stdout, c.stderr, harness.Verbs()) + }}, {"role", false, func(c invocation) int { return role.CLI(c.args, os.Stdin, c.stdout, c.stderr, os.LookupEnv) }}, {"provider", false, func(c invocation) int { return provider.Run(c.ctx, c.stdout) }}, {"map", false, runRepoMap}, diff --git a/cmd/crw/pabcd_receipt_signal_test.go b/cmd/crw/pabcd_receipt_signal_test.go new file mode 100644 index 000000000..ebc5b7aab --- /dev/null +++ b/cmd/crw/pabcd_receipt_signal_test.go @@ -0,0 +1,163 @@ +package main + +import ( + "bytes" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/thisisjun786/codex-relay-workflow/internal/pabcd/state" + "github.com/thisisjun786/codex-relay-workflow/internal/testsupport" +) + +// An interrupted receipt test stops the check command at once and certifies nothing: the first +// SIGINT to the crw process reaches the command through the invocation's context, so the run ends +// with exit 1 and the "terminated by signal" refusal instead of waiting for the command and writing +// a receipt. While the pabcd row dropped that context the run ignored the interrupt, waited out the +// command and published a success receipt (CRW-582). +func TestPabcdReceiptTestStopsOnFirstInterrupt(t *testing.T) { + crw := testsupport.CRW(t) + home := t.TempDir() + root := filepath.Join(home, "work") + receiptSignalRepo(t, root) + epoch := "check-epoch" + s := state.DefaultState("s1", "") + s.Phase, s.OrchestrationActive, s.CheckEpoch = state.PhaseC, true, &epoch + if err := state.WriteState(root, s); err != nil { + t.Fatal(err) + } + before := receiptSignalHomeListings(t, home) + marker := filepath.Join(t.TempDir(), "started") + cmd := exec.Command(crw, "pabcd", "receipt", "test", "--session", "s1", "--", "/bin/sh", "-c", `: > "$1"; exec sleep 30`, "sh", marker) + cmd.Dir = root + cmd.Env = []string{ + "HOME=" + home, + "CODEX_HOME=" + filepath.Join(home, "codex"), + "CRW_HOME=" + filepath.Join(home, "crw"), + "PATH=" + os.Getenv("PATH"), + testsupport.RefuseLiveStateEnv + "=1", + } + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + cmd.WaitDelay = time.Second + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + waited := false + t.Cleanup(func() { + if !waited { + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + } + }) + deadline := time.Now().Add(10 * time.Second) + for { + if _, err := os.Stat(marker); err == nil { + break + } + if time.Now().After(deadline) { + t.Fatalf("the check command did not start within 10 s\nstdout:\n%s\nstderr:\n%s", stdout.String(), stderr.String()) + } + time.Sleep(20 * time.Millisecond) + } + if err := cmd.Process.Signal(syscall.SIGINT); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + select { + case <-done: + waited = true + case <-time.After(10 * time.Second): + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + <-done + waited = true + t.Fatalf("the command was still running 10 s after the SIGINT\nstdout:\n%s\nstderr:\n%s", stdout.String(), stderr.String()) + } + if code := cmd.ProcessState.ExitCode(); code != 1 { + t.Fatalf("exit code %d, want 1\nstdout:\n%s\nstderr:\n%s", code, stdout.String(), stderr.String()) + } + for _, want := range []string{"terminated by signal", "no receipt written"} { + if !strings.Contains(stdout.String(), want) { + t.Fatalf("stdout does not hold %q\nstdout:\n%s\nstderr:\n%s", want, stdout.String(), stderr.String()) + } + } + if _, err := os.Stat(filepath.Join(root, ".crw", "evidence", "s1", "test-receipt.json")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("an interrupted check left a receipt: %v", err) + } + if after := receiptSignalHomeListings(t, home); after != before { + t.Fatalf("the run changed the child's home listings\nbefore:\n%s\nafter:\n%s", before, after) + } +} + +// receiptSignalRepo makes root a git repository with one committed file, which the receipt's source +// capture needs; the test's own git calls get the fixture identity and no user configuration. +func receiptSignalRepo(t *testing.T, root string) { + t.Helper() + for _, entry := range os.Environ() { + key, _, _ := strings.Cut(entry, "=") + if strings.HasPrefix(key, "GIT_") { + t.Setenv(key, "") + if err := os.Unsetenv(key); err != nil { + t.Fatal(err) + } + } + } + for key, value := range map[string]string{ + "GIT_CONFIG_GLOBAL": os.DevNull, + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_CEILING_DIRECTORIES": filepath.Dir(root), + "GIT_AUTHOR_NAME": "fixture", + "GIT_AUTHOR_EMAIL": "fixture@example.invalid", + "GIT_COMMITTER_NAME": "fixture", + "GIT_COMMITTER_EMAIL": "fixture@example.invalid", + } { + t.Setenv(key, value) + } + if err := os.MkdirAll(root, 0o755); err != nil { + t.Fatal(err) + } + git := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = root + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + } + git("init", "-q", "-b", "main") + if err := os.WriteFile(filepath.Join(root, "file.txt"), []byte("one\n"), 0o644); err != nil { + t.Fatal(err) + } + git("add", "-A") + git("commit", "-qm", "initial") +} + +// receiptSignalHomeListings lists the home state roots the child could reach, so a run that wrote +// into a live-home path changes this string. +func receiptSignalHomeListings(t *testing.T, home string) string { + t.Helper() + var b strings.Builder + for _, name := range []string{".codex", ".crw", "codex", "crw"} { + entries, err := os.ReadDir(filepath.Join(home, name)) + if errors.Is(err, os.ErrNotExist) { + fmt.Fprintf(&b, "%s: absent\n", name) + continue + } + if err != nil { + t.Fatal(err) + } + names := make([]string, 0, len(entries)) + for _, entry := range entries { + names = append(names, entry.Name()) + } + fmt.Fprintf(&b, "%s: %s\n", name, strings.Join(names, " ")) + } + return b.String() +} diff --git a/contract/notes/cxc/CRW-582.json b/contract/notes/cxc/CRW-582.json new file mode 100644 index 000000000..f07354b12 --- /dev/null +++ b/contract/notes/cxc/CRW-582.json @@ -0,0 +1,6 @@ +{ + "issue": "CRW-582", + "pending": [], + "identical": [], + "intentionally-changed": [] +} diff --git a/docs/port-cxc/known-defects.md b/docs/port-cxc/known-defects.md index 977f2b0f0..7d4032d85 100644 --- a/docs/port-cxc/known-defects.md +++ b/docs/port-cxc/known-defects.md @@ -1013,3 +1013,9 @@ The same fail-closed rule as the CRW-517 section applies: a destination the orac - A created report could name any real child of the session, so with archived children accepted a later dispatch could report the id of a finished child as the agent it had just spawned and skip the spawn the check exists to prove; the archive refusal had blocked that for archived children only, and nothing blocked it for unarchived ones (source `internal/role/created_check.go` before generation 2, where `createdCheckRead` takes the agent id alone; probed on the base with two dispatches of one session that both accepted the same unarchived child); port: fixed by CRW-574 generation 2 (before a created report is published and before the host is asked, `createdArchivedReplay` reads the session's dispatch records and refuses an agent id that another attempt holds, of another dispatch or an earlier attempt of the same one; a record that cannot be read as a regular dispatch file fails the check; `TestCreatedArchivedReplayAcrossDispatches`, `TestCreatedArchivedReplayAcrossAttempts`, `TestCreatedArchivedReplayMakesNoHostCall` and `TestCreatedArchivedSiblingRecords` pin it, and the completion dispatch of `TestDispatchCommandHostReportsAcrossProcesses`, which reported a child the first dispatch had recorded, was given a child of its own). - The created check proves a real child of this session that no other attempt of the session holds, not that the child was spawned for this attempt: a child spawned outside any dispatch and never recorded is accepted as the created agent of the first dispatch that reports it (source `internal/role/created_check.go` `createdArchivedReplay` and `createdCheckNative`, which read only the ledger and the spawn marker's parent); port: kept (binding a report to its own spawn needs the child's creation time or an attempt marker in the host row or the ledger, outside this issue's edit region; follow-up direction: compare the host row's creation time with the moment the attempt was claimed, which the ledger would have to record). - The replay guard holds only the per-dispatch lock (`internal/role/dispatch_ledger.go:440`), so two dispatches of one session that report the same new id at the same instant can both pass; and an id recorded by a wrong report that was closed as stopped, left by an earlier crash, or reused by the host stays held by its record, so no other dispatch of the session can report it, and `failed` with `not_created` is a way out only when no child was really created, otherwise the operator repairs the ledger; and a record swapped for a named pipe between the directory listing and its read blocks that read while the dispatch lock is held, because the shared reader checks no file type on its own descriptor (source `internal/role/created_check.go` `createdArchivedReplay` and `internal/role/dispatch_ledger.go:262`; it needs write access to the directory); port: kept (a session-wide lock would add a stale-lock artifact to the directory the next dispatch change pins, and a descriptor-based read belongs to that change). + +## CRW-582 — the receipt test's first interrupt (port-introduced defect, fixed) + +- The oracle runs a receipt test's command through `spawnSync`, which blocks until the child ends and defers the signal: the recorded Node run died by the deferred SIGINT (rc -2) only after the child had completed, so it wrote no receipt (source `pabcd-state/src/receipt-cli.ts:120-137` at v0.2.40). The same signal against this port used to keep waiting for the child and publish a success receipt — a port-introduced defect, not an upstream one (the invocation context was dropped before `exec.CommandContext`); port: fixed by CRW-582, with the timing difference tagged intentionally-changed (the port stops the child at once instead of after it completes) and the same observable outcome: exit 1, "terminated by signal ... no receipt written", no receipt written. No corpus fixture covers this case, so no recorded case is claimed for it. +- A SIGINT that lands after the check command has already exited (for example during the second Git source capture, just before publication) used to still publish the receipt, where the oracle's deferred signal ends the process before it writes; port: fixed (generation 2 of CRW-582): `RunReceiptCLI` (`internal/pabcd/cli/receipt.go`) now checks the caller's context at the last moment publication can be skipped - after the command returns and after the second capture, before the receipt directory is created - and takes the refusal path with `receipt test: the command did not run to completion (interrupted); no receipt written`, exit code 1, writing nothing; `internal/pabcd/cli/receipt_late_cancel_test.go` proves it through the unexported `receiptLateCancelHook` seam. A cancellation that lands after that check still publishes, because the check precedes the directory work. +- An interrupted check kills only the command process (`exec.CommandContext`'s default cancellation), so a shell-launched descendant can outlive the refusal; port: kept (parity: the oracle never kills the check at all on SIGINT, and `ReceiptRunOptions` documents that cancellation kills only the process the call started); a process-group kill is a follow-up proposal with its own test. diff --git a/internal/harness/hook_test.go b/internal/harness/hook_test.go index 35fbf0784..37b988b19 100644 --- a/internal/harness/hook_test.go +++ b/internal/harness/hook_test.go @@ -389,11 +389,11 @@ func TestLegsMatchTheDeclaredRegistrations(t *testing.T) { func TestPabcdVerbs(t *testing.T) { var got []string - verbs := []Verb{{"orchestrate", func(args []string, _ io.Reader, out, _ io.Writer) int { + verbs := []Verb{{Name: "orchestrate", Run: func(args []string, _ io.Reader, out, _ io.Writer) int { got = args io.WriteString(out, "ran\n") return 7 - }}, {"freeze", nil}} + }}, {Name: "freeze"}} for _, c := range []struct { args []string code int diff --git a/internal/harness/pabcd.go b/internal/harness/pabcd.go index b7a1ec894..b46c0ac8c 100644 --- a/internal/harness/pabcd.go +++ b/internal/harness/pabcd.go @@ -1,15 +1,19 @@ package harness import ( + "context" "fmt" "io" "strings" ) -// Verb is one crw pabcd command: the issue that ports it adds its row to Verbs. +// Verb is one crw pabcd command: the issue that ports it adds its row to Verbs. A row that needs +// the invocation's context sets RunContext and leaves Run nil; the dispatch passes ctx to +// RunContext when it is set, and a row with neither function is not dispatched. type Verb struct { - Name string - Run func(args []string, in io.Reader, stdout, stderr io.Writer) int + Name string + Run func(args []string, in io.Reader, stdout, stderr io.Writer) int + RunContext func(ctx context.Context, args []string, in io.Reader, stdout, stderr io.Writer) int } // Verbs is the table of crw pabcd's commands, a row for each verb that is ported. @@ -17,7 +21,7 @@ func Verbs() []Verb { return []Verb{ {Name: "freeze", Run: freezeVerb}, {Name: "plan", Run: planVerb}, - {Name: "receipt", Run: receiptVerb}, + {Name: "receipt", RunContext: receiptVerb}, {Name: "evidence", Run: evidenceVerb}, {Name: "memory", Run: memoryVerb}, {Name: "reset", Run: resetVerb}, @@ -26,9 +30,16 @@ func Verbs() []Verb { } } -// Pabcd is crw pabcd [args]: it hands the arguments after the verb to its row, and answers an -// absent or unknown verb as the other crw modes do, with the usage and exit status 2. +// Pabcd is crw pabcd [args] without an invocation context: a caller that holds one calls +// PabcdContext. func Pabcd(args []string, in io.Reader, stdout, stderr io.Writer, verbs []Verb) int { + return PabcdContext(context.Background(), args, in, stdout, stderr, verbs) +} + +// PabcdContext is crw pabcd [args] under the invocation's context: it hands the arguments +// after the verb to its row, passing ctx when the row sets RunContext, and answers an absent or +// unknown verb as the other crw modes do, with the usage and exit status 2. +func PabcdContext(ctx context.Context, args []string, in io.Reader, stdout, stderr io.Writer, verbs []Verb) int { var names []string for _, v := range verbs { names = append(names, v.Name) @@ -39,7 +50,10 @@ func Pabcd(args []string, in io.Reader, stdout, stderr io.Writer, verbs []Verb) return 0 } for _, v := range verbs { - if len(args) > 0 && v.Name == args[0] && v.Run != nil { + if len(args) > 0 && v.Name == args[0] && (v.RunContext != nil || v.Run != nil) { + if v.RunContext != nil { + return v.RunContext(ctx, args[1:], in, stdout, stderr) + } return v.Run(args[1:], in, stdout, stderr) } } diff --git a/internal/harness/pabcd_cli.go b/internal/harness/pabcd_cli.go index 2a32bd8a6..307005305 100644 --- a/internal/harness/pabcd_cli.go +++ b/internal/harness/pabcd_cli.go @@ -1,6 +1,7 @@ package harness import ( + "context" "fmt" "io" "syscall" @@ -26,7 +27,7 @@ func planVerb(args []string, _ io.Reader, stdout, stderr io.Writer) int { return result.Code } -func receiptVerb(args []string, in io.Reader, stdout, stderr io.Writer) int { +func receiptVerb(ctx context.Context, args []string, in io.Reader, stdout, stderr io.Writer) int { cwd, err := syscall.Getwd() if err != nil { fmt.Fprintln(stderr, "crw cli failed: "+err.Error()) @@ -37,7 +38,7 @@ func receiptVerb(args []string, in io.Reader, stdout, stderr io.Writer) int { fmt.Fprintln(stderr, "receipt: "+err.Error()) return 1 } - result, err := cli.RunReceiptCLI(parsed, cli.ReceiptRunOptions{Stdin: in, Stdout: stdout, Stderr: stderr}) + result, err := cli.RunReceiptCLI(parsed, cli.ReceiptRunOptions{Context: ctx, Stdin: in, Stdout: stdout, Stderr: stderr}) if err != nil { fmt.Fprintln(stderr, "crw cli failed: "+err.Error()) return 1 diff --git a/internal/pabcd/cli/receipt.go b/internal/pabcd/cli/receipt.go index a1aaf77fe..9da3b22b0 100644 --- a/internal/pabcd/cli/receipt.go +++ b/internal/pabcd/cli/receipt.go @@ -131,6 +131,13 @@ Notes: Example: crw pabcd receipt test --session -- npm test` +// receiptLateCancelHook, when non-nil, runs immediately before the late-cancellation check in a +// receipt test. It is nil in production (an uninitialized variable, no package-level work at +// start); receipt_late_cancel_test.go sets it to cancel the context at the one point where the +// window the check closes is deterministic, after the command has returned and the source has been +// captured again. +var receiptLateCancelHook func() + // RunReceiptCLI ports receipt-cli.ts:75-185: guard, unlink stale receipt, capture, execute argv without a shell, capture again // and publish only a successful unchanged-tree result. The receipt stays native while a bound command runs in its source. // A non-nil error models the oracle's thrown remove/before-capture/publication errors. Atomic publication intentionally fixes @@ -193,6 +200,14 @@ func RunReceiptCLI(args ReceiptCLIArgs, options ReceiptRunOptions) (ReceiptCLIRe case source.ComparisonUnavailable: return refuse("receipt test: git could not resolve the source identity (" + cmp.Reason + "); no receipt written") } + // The last moment the publication can still be skipped: a cancellation that landed after the + // command returned refuses the receipt here, as the oracle's deferred signal refuses it. + if receiptLateCancelHook != nil { + receiptLateCancelHook() + } + if options.Context != nil && options.Context.Err() != nil { + return refuse("receipt test: the command did not run to completion (interrupted); no receipt written") + } record := receiptRecord{Kind: "test", SourceIdentity: after, Command: strings.Join(args.Command, " "), ExitCode: 0, CreatedAt: time.Now().UTC().Format("2006-01-02T15:04:05.000Z"), OwnerSessionID: sid, CheckEpoch: *st.CheckEpoch, GeneratedPaths: args.Generated} if _, err = crwdir.EnsureDir(args.Cwd); err != nil { return ReceiptCLIResult{}, err diff --git a/internal/pabcd/cli/receipt_late_cancel_test.go b/internal/pabcd/cli/receipt_late_cancel_test.go new file mode 100644 index 000000000..f25d4a6fd --- /dev/null +++ b/internal/pabcd/cli/receipt_late_cancel_test.go @@ -0,0 +1,24 @@ +package cli + +import ( + "context" + "testing" +) + +// A cancellation that lands after the check command has returned and the source has been captured +// again still refuses publication, as the oracle's deferred signal refuses it: the runner checks the +// context at the last moment publication can still be skipped. The seam (receiptLateCancelHook) +// exists only to reach that window deterministically - the real one is a race. +func TestReceiptLateCancellationRefusesPublication(t *testing.T) { + root := receiptRepo(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + receiptLateCancelHook = cancel + defer func() { receiptLateCancelHook = nil }() + a := ReceiptCLIArgs{Verb: "test", Cwd: root, Session: "s1", Command: receiptCommand(t, "exit", "0")} + got := receiptRun(t, a, ReceiptRunOptions{Context: ctx}) + if got.Code != 1 || got.Output != "receipt test: the command did not run to completion (interrupted); no receipt written" { + t.Fatalf("late cancellation: %#v", got) + } + receiptAbsent(t, root) +}