diff --git a/cmd/proof/proxy.go b/cmd/proof/proxy.go index 492eb16..f166319 100644 --- a/cmd/proof/proxy.go +++ b/cmd/proof/proxy.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "flag" "fmt" "io" @@ -23,12 +24,14 @@ import ( type clientOptions struct { listen string json string + record string verbose bool } func (o *clientOptions) register(fs *flag.FlagSet) { fs.StringVar(&o.listen, "listen", "127.0.0.1:0", "accept clients on `host:port` (port 0 picks a free one)") fs.StringVar(&o.json, "json", "", "also write a JSON report to `file`") + fs.StringVar(&o.record, "record", "", "also write the requests the client sent to `file`, as the start of a client profile") fs.BoolVar(&o.verbose, "v", false, "show every message the client and the server exchanged") } @@ -143,11 +146,18 @@ func gradeClients(stdout, stderr io.Writer, r report.Run, traffic []check.Traffi } } showDeath(stdout, srv) + // A recording can't be made again, so it's written even when the + // report can't be. + var errs []error if o.json != "" { - if err := writeJSON(o.json, r); err != nil { - fmt.Fprintln(stderr, "proof:", err) - return 2 - } + errs = append(errs, writeJSON(o.json, r)) + } + if o.record != "" { + errs = append(errs, os.WriteFile(o.record, []byte(checks.RecordedProfile(traffic)), 0o666)) + } + if err := errors.Join(errs...); err != nil { + fmt.Fprintln(stderr, "proof:", err) + return 2 } if r.Counts()[check.Failed] > 0 { return 1 diff --git a/cmd/proof/proxy_test.go b/cmd/proof/proxy_test.go index c68c134..0727462 100644 --- a/cmd/proof/proxy_test.go +++ b/cmd/proof/proxy_test.go @@ -132,6 +132,24 @@ func TestProxyWritesJSONAndTranscripts(t *testing.T) { } } +// The recording is written even when the report can't be, as the client +// would have to do it all again. +func TestProxyRecordsTheClient(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "client.toml") + code, stdout, stderr := proxyRun(t, []string{"d2:id1:12:op8:describee"}, "-record", path, "-json", filepath.Join(dir, "missing", "client.json")) + if code != 2 || !strings.Contains(stderr, "missing") { + t.Errorf("exit status %d, want 2 with the JSON report's error:\n%s%s", code, stdout, stderr) + } + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(b), `send = { op = "describe" }`) { + t.Errorf("the recording doesn't have the request:\n%s", b) + } +} + func TestProxyNeedsAServer(t *testing.T) { var stderr bytes.Buffer if code := proxyUntil(context.Background(), nil, io.Discard, &stderr, nil); code != 2 { diff --git a/doc/design.md b/doc/design.md index f836a19..f2e3e54 100644 --- a/doc/design.md +++ b/doc/design.md @@ -134,6 +134,10 @@ use. Replies that only bother the client (e.g. an error the client just shows to the user) get a note. Right now there's a profile for CIDER, built from what CIDER sends with its default settings. +A new profile starts with a recording, as `proof proxy -record` saves +what a client sent as a profile with a check for each connection. What +the client needs from the replies comes from reading its code. + ## Strict About the Wire, Relaxed About the Rest proof has its own bencode implementation, as the popular Go libraries @@ -308,8 +312,7 @@ what's planned next: - robustness checks (malformed messages, fields of the wrong type, clients disconnecting in the middle of an evaluation) - client profiles for more clients (e.g. Calva, Conjure and - vim-fireplace), and a way to record them with `proof proxy`, which - already sees the traffic but doesn't save it yet + vim-fireplace) - more servers in the compatibility matrix and a proper home for the matrix itself - incorporating [Spec Changes](spec-changes.md) into the spec, so that diff --git a/doc/hacking.md b/doc/hacking.md index 61518dc..9fe20e7 100644 --- a/doc/hacking.md +++ b/doc/hacking.md @@ -261,9 +261,14 @@ like the links in `refs.go`). A check that sends code in some language should list the capability for it in `needs`, unless the client sends that code to any server (like CIDER's startup code). -To find out what a client sends, run it through `proof proxy` with `-v`, -which shows every request. Then read the client's code to see what it -does with each reply, and keep only what the client really needs. +To find out what a client sends, run it through `proof proxy -record +client.toml`. That gives you a profile with a check for each connection, +whose steps send what the client sent (and `-v` shows the replies as +well). Then read the client's code to see what it does with each reply, +keep only the steps the client really needs and fill in the rest. Keep +in mind that each step waits for its reply before the next one goes out, +so requests sent while another one is still running (e.g. `stdin` and +`interrupt`) won't work in a profile. `TestClientProfiles` makes sure a profile hangs together, and the fake server should get a quirk for anything a profile catches that the other checks don't. diff --git a/doc/usage.md b/doc/usage.md index bec8bb9..f361cba 100644 --- a/doc/usage.md +++ b/doc/usage.md @@ -277,6 +277,7 @@ Here are the options supported by `proof proxy`: | `-listen host:port` | Accept the client on this address. The default is `127.0.0.1:0`, which picks a free port. | | `-v` | Show every message exchanged between the client and the server. | | `-json file` | Write a JSON report as well. | +| `-record file` | Write the requests the client sent to a file, as the start of a [client profile](hacking.md#adding-a-client-profile). | The exit codes are the same as for `proof run`, except that 1 means that the client failed some checks and 3 means that no client sent anything. @@ -397,7 +398,7 @@ all you need for checking output, values, errors, input and interrupts. When you stop it, `proof serve` checks the requests your client sent, just like `proof proxy` does, with the same report, exit codes and -`-listen`, `-json` and `-v` options. A test suite can run it in CI like +`-listen`, `-json`, `-record` and `-v` options. A test suite can run it in CI like this: ```shell diff --git a/internal/checks/client_profiles.go b/internal/checks/client_profiles.go index 8559051..08d1b60 100644 --- a/internal/checks/client_profiles.go +++ b/internal/checks/client_profiles.go @@ -3,6 +3,7 @@ package checks import ( "embed" "fmt" + "io/fs" "slices" "strings" "sync" @@ -41,8 +42,8 @@ type clientCheck struct { // clientStep is a request and what the client needs from its reply, // besides done. type clientStep struct { - // Send is the request. A string starting with $ stands for the - // session an earlier step got. + // Send is the request. A session starting with $ stands for the one + // an earlier step got. Send request `toml:"send"` // Snippet names a snippet of the server's profile whose code goes in // the request, and whose value the reply has to have. It stands for @@ -81,19 +82,27 @@ var clientProfiles = sync.OnceValue(func() []clientProfile { } var profiles []clientProfile for _, f := range files { - var p clientProfile - md, err := toml.DecodeFS(clientFiles, "clients/"+f.Name(), &p) + p, err := readClientProfile(clientFiles, "clients/"+f.Name()) if err != nil { - panic(fmt.Sprintf("client profile %s: %v", f.Name(), err)) - } - if undecoded := md.Undecoded(); len(undecoded) > 0 { - panic(fmt.Sprintf("client profile %s: unknown keys %v", f.Name(), undecoded)) + panic(err) } profiles = append(profiles, p) } return profiles }) +func readClientProfile(fsys fs.FS, path string) (clientProfile, error) { + var p clientProfile + md, err := toml.DecodeFS(fsys, path, &p) + if err != nil { + return p, fmt.Errorf("client profile %s: %v", path, err) + } + if undecoded := md.Undecoded(); len(undecoded) > 0 { + return p, fmt.Errorf("client profile %s: unknown keys %v", path, undecoded) + } + return p, nil +} + func clientProfileChecks() []*check.Check { var checks []*check.Check for _, p := range clientProfiles() { @@ -127,11 +136,11 @@ func (c clientCheck) replay(t *check.T) { for i, s := range c.Steps { req := nrepl.Message{} for k, v := range s.Send { - if name, ok := v.(string); ok && strings.HasPrefix(name, "$") { - v = sessions[name[1:]] - } req[k] = v } + if name, ok := req["session"].(string); ok && strings.HasPrefix(name, "$") { + req["session"] = sessions[name[1:]] + } var want string if s.Snippet != "" { sn := t.Snippet(s.Snippet) diff --git a/internal/checks/client_profiles_test.go b/internal/checks/client_profiles_test.go index 8beeb9e..2d4a560 100644 --- a/internal/checks/client_profiles_test.go +++ b/internal/checks/client_profiles_test.go @@ -41,10 +41,8 @@ func TestClientProfiles(t *testing.T) { if (s.NewSession != "" || s.Snippet != "" || len(s.Dicts) > 0) && s.Why == "" { t.Errorf("%s step %d: doesn't say what happens to the client without what it needs", c.ID, i+1) } - for _, v := range s.Send { - if name, ok := v.(string); ok && strings.HasPrefix(name, "$") && !sessions[name[1:]] { - t.Errorf("%s step %d: uses %s before a step gets it", c.ID, i+1, name) - } + if name, ok := s.Send["session"].(string); ok && strings.HasPrefix(name, "$") && !sessions[name[1:]] { + t.Errorf("%s step %d: uses %s before a step gets it", c.ID, i+1, name) } if s.NewSession != "" { sessions[s.NewSession] = true diff --git a/internal/checks/record.go b/internal/checks/record.go new file mode 100644 index 0000000..ddffb38 --- /dev/null +++ b/internal/checks/record.go @@ -0,0 +1,188 @@ +package checks + +import ( + "fmt" + "regexp" + "slices" + "strconv" + "strings" + "unicode/utf8" + + "github.com/nrepl/proof/internal/check" + "github.com/nrepl/proof/nrepl" +) + +// RecordedProfile turns the requests in traffic into a client profile, +// with a check for each connection, to be turned into a real one: it says +// what the client sent, but not yet what the client needs from the +// replies. +func RecordedProfile(traffic []check.Traffic) string { + name := "" + var checks strings.Builder + sessions := &sessionNames{byID: map[string]string{}} + for _, tr := range traffic { + steps := recordedSteps(tr.Events, sessions) + if len(steps) == 0 { + continue + } + id := strings.ReplaceAll(tr.Label, " ", "-") + fmt.Fprintf(&checks, "\n[[checks]]\nid = %s\ntitle = %s\nwhy = \"\"\n", tomlString(id), tomlString(tr.Label)) + for _, s := range steps { + if n, ok := s.Send["client-name"].(string); ok && name == "" { + name = n + } + checks.WriteString(recordedStep(s)) + } + } + return recordedHeader + "name = " + tomlString(name) + "\ncode = \"\"\n" + checks.String() +} + +// sessionNames names the sessions clients cloned, across all connections, +// as sessions outlive them. +type sessionNames struct { + byID map[string]string + n int +} + +// add names a session the server handed out. One it handed out before +// gets a new name, as it's a new session for proof. +func (s *sessionNames) add(id string) string { + s.n++ + s.byID[id] = "s" + strconv.Itoa(s.n) + return s.byID[id] +} + +// recordedSteps turns each request of a connection into a step, without +// its id, as proof gives requests ids of its own. The sessions the client +// cloned get names, so that the steps can use the sessions proof clones. +func recordedSteps(events []nrepl.Event, sessions *sessionNames) []clientStep { + var steps []clientStep + clones := map[string]int{} // the ids of clone requests, and their steps + for _, ev := range events { + m := ev.Msg + if m == nil { + continue + } + if ev.Dir == nrepl.Received { + id, _ := idKey(m) + i, ok := clones[id] + if s := m.Str("new-session"); ok && s != "" && steps[i].NewSession == "" { + steps[i].NewSession = sessions.add(s) + } + continue + } + send := request{} + for k, v := range m { + if k != "id" { + send[k] = v + } + } + if name := sessions.byID[m.Str("session")]; name != "" { + send["session"] = "$" + name + } + if m.Str("op") == "clone" { + id, _ := idKey(m) + clones[id] = len(steps) + } + steps = append(steps, clientStep{Send: send}) + } + return steps +} + +const recordedHeader = `# The requests clients sent through proof, with a check for each +# connection. To make a client profile out of it, fill in the blanks, use +# snippets for the user's code and say what the client needs from each +# reply (see "Adding a Client Profile" in doc/hacking.md). +` + +// recordedStep writes a step the way the profiles in clients/ write them. +func recordedStep(s clientStep) string { + newSession := "" + if s.NewSession != "" { + newSession = "new-session = " + tomlString(s.NewSession) + "\n" + } + // The op goes first, as that's what tells the requests apart. + keys := nrepl.Message(s.Send).Keys() + if i := slices.Index(keys, "op"); i > 0 { + keys = slices.Insert(slices.Delete(keys, i, i+1), 0, "op") + } + if send := "send = " + tomlTable(s.Send, keys); len(send) <= 100 { + return "\n[[checks.steps]]\n" + send + "\n" + newSession + } + var b strings.Builder + b.WriteString("\n[[checks.steps]]\n") + if newSession != "" { + b.WriteString(newSession + "\n") + } + b.WriteString("[checks.steps.send]\n") + for _, k := range keys { + fmt.Fprintf(&b, "%s = %s\n", tomlKey(k), tomlValue(s.Send[k])) + } + return b.String() +} + +// tomlValue writes a value decoded from bencode. +func tomlValue(v any) string { + switch v := v.(type) { + case string: + return tomlString(v) + case int64: + return strconv.FormatInt(v, 10) + case []any: + items := make([]string, len(v)) + for i, item := range v { + items[i] = tomlValue(item) + } + return "[" + strings.Join(items, ", ") + "]" + case map[string]any: + return tomlTable(v, nrepl.Message(v).Keys()) + } + panic(fmt.Sprintf("can't write %T as TOML", v)) +} + +// tomlTable writes the keys of m, in this order, as an inline table. +func tomlTable(m map[string]any, keys []string) string { + if len(keys) == 0 { + return "{}" + } + fields := make([]string, len(keys)) + for i, k := range keys { + fields[i] = tomlKey(k) + " = " + tomlValue(m[k]) + } + return "{ " + strings.Join(fields, ", ") + " }" +} + +var bareKey = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) + +func tomlKey(k string) string { + if bareKey.MatchString(k) { + return k + } + return tomlString(k) +} + +// tomlString writes s as a basic string, on one line. Bytes that aren't +// UTF-8, which TOML can't have, become U+FFFD. +func tomlString(s string) string { + var b strings.Builder + b.WriteByte('"') + for _, r := range strings.ToValidUTF8(s, string(utf8.RuneError)) { + switch { + case r == '"' || r == '\\': + b.WriteByte('\\') + b.WriteRune(r) + case r == '\n': + b.WriteString(`\n`) + case r == '\t': + b.WriteString(`\t`) + case r == '\r': + b.WriteString(`\r`) + case r < 0x20 || r == 0x7f: + fmt.Fprintf(&b, `\u%04X`, r) + default: + b.WriteRune(r) + } + } + b.WriteByte('"') + return b.String() +} diff --git a/internal/checks/record_test.go b/internal/checks/record_test.go new file mode 100644 index 0000000..43ec433 --- /dev/null +++ b/internal/checks/record_test.go @@ -0,0 +1,74 @@ +package checks + +import ( + "reflect" + "strings" + "testing" + "testing/fstest" + + "github.com/nrepl/proof/internal/check" + "github.com/nrepl/proof/nrepl" +) + +// A recording is a client profile proof can read, whose checks send the +// same requests, in sessions of their own. +func TestRecordingsAreClientProfiles(t *testing.T) { + sent := func(m nrepl.Message) nrepl.Event { return nrepl.Event{Dir: nrepl.Sent, Msg: m} } + got := func(m nrepl.Message) nrepl.Event { return nrepl.Event{Dir: nrepl.Received, Msg: m} } + done := []any{"done"} + eval := nrepl.Message{"id": "4", "op": "eval", "session": "4f1c", "code": "value", + "file": "a \"file\"\nwith\ta \x01 and \xff", "line": int64(3), "\xfe": "", "nrepl.middleware.print/stream?": []any{}, + "nrepl.middleware.print/options": map[string]any{"right-margin": int64(70), "": []any{"x", int64(1)}}} + traffic := []check.Traffic{ + {Label: "connection 1", Events: []nrepl.Event{ + sent(nrepl.Message{"id": "1", "op": "clone", "client-name": "Test"}), + got(nrepl.Message{"id": "1", "new-session": "4f1c", "status": done}), + // The same session again is a new one for proof. + sent(nrepl.Message{"id": "2", "op": "clone"}), + got(nrepl.Message{"id": "2", "new-session": "4f1c", "status": done}), + sent(nrepl.Message{"id": "3", "op": "describe", "session": "4f1c"}), + sent(eval), + got(nrepl.Message{"id": "3", "ops": map[string]any{}, "status": done}), + got(nrepl.Message{"id": "4", "value": "3", "session": "4f1c"}), + {Dir: nrepl.Sent, Closed: true}, + }}, + // Frames that aren't dicts aren't requests. + {Label: "connection 2", Events: []nrepl.Event{{Dir: nrepl.Sent, Data: []any{}}}}, + // Sessions outlive connections, and only sessions stand for them. + {Label: "connection 3", Events: []nrepl.Event{ + sent(nrepl.Message{"id": "1", "op": "eval", "session": "4f1c", "code": "$s1"}), + }}, + } + recorded := RecordedProfile(traffic) + p, err := readClientProfile(fstest.MapFS{"test.toml": {Data: []byte(recorded)}}, "test.toml") + if err != nil { + t.Fatalf("%v\n%s", err, recorded) + } + if !strings.Contains(recorded, "\nsend = { op = \"clone\", client-name = \"Test\" }\n") { + t.Errorf("the clone isn't on one line, starting with the op:\n%s", recorded) + } + if p.Name != "Test" || len(p.Checks) != 2 || p.Checks[0].ID != "connection-1" || p.Checks[1].ID != "connection-3" { + t.Fatalf("got %+v from\n%s", p, recorded) + } + wantEval := map[string]any{"op": "eval", "session": "$s2", "code": "value", "file": "a \"file\"\nwith\ta \x01 and \ufffd", + "line": int64(3), "\ufffd": "", "nrepl.middleware.print/stream?": []any{}, + "nrepl.middleware.print/options": map[string]any{"right-margin": int64(70), "": []any{"x", int64(1)}}} + want := [][]clientStep{{ + {Send: request{"op": "clone", "client-name": "Test"}, NewSession: "s1"}, + {Send: request{"op": "clone"}, NewSession: "s2"}, + {Send: request{"op": "describe", "session": "$s2"}}, + {Send: request(wantEval)}, + }, { + {Send: request{"op": "eval", "session": "$s2", "code": "$s1"}}, + }} + for i, c := range p.Checks { + if !reflect.DeepEqual(c.Steps, want[i]) { + t.Errorf("%s: got steps\n%#v\nwant\n%#v\nfrom\n%s", c.ID, c.Steps, want[i], recorded) + } + } + c := p.Checks[0] + r := runFakeChecks(t, quirks{}, []*check.Check{{ID: c.ID, Title: c.Title, Run: c.replay}})[c.ID] + if r.Verdict != check.Pass { + t.Errorf("replaying it got %s %v", r.Verdict, r.Details) + } +}