feat(cli): add mem doctor and first-run guidance that names the documented deploy/compose path - #131
Closed
waterbro-8 wants to merge 1 commit into
Closed
waterbro-8 wants to merge 1 commit into
waterbro-8 wants to merge 1 commit into
Conversation
Consumed revision: R1 ## Problem A first-time user's most common failure is a missing prerequisite they cannot name. Verified at the base revision: the only signals were a per-command `not logged in` hint and raw transport errors, and the hint sent somebody with no server at all to `mem auth login` against a server that does not exist yet. ## Goal and expected behavior REQ-001: a read-only `mem doctor` reporting four checks in a fixed order -- server reachability, credential presence, the workspace the server resolved, CLI/server version skew -- each carrying its SPEC 7.1 exit code, with `--format json` per SPEC.md:579. REQ-002: every command that fails closed on a missing credential names the documented deployment path when the host has no configuration at all. REQ-003: diagnosis only -- zero writes, zero remediation, zero dependency installation, no secret value or DSN printed. Non-goals: no environment-certification matrix, no host container-runtime detection, no TTY wizard, no SPEC edit. ## Implementation - `server/cmd/mem/cmds_doctor.go`: the command, the four probes, and the shared probe-failure classification that maps `apiclient.APIError` kinds onto SPEC 7.1 codes. Reachability is probed without a credential so a bad token is not misread as an outage. A check an earlier failure made impossible is reported `skipped`, naming the blocker, instead of guessed as a pass. - `errNotLoggedIn()` replaces 23 duplicated `newCliError(3, "not logged in", ...)` constructions. It branches on `configFileExists()`, the only signal that separates "never configured" from "configured, but not logged in", because `loadConfig` deliberately succeeds without a file. Hosts that already have a configuration keep the previous hint text unchanged. - `cliVersion` becomes a variable so the skew check has something to compare. Nothing injects it today -- `release.yml` passes only `-trimpath -ldflags=-s -w` -- so the check reports "skew not computable" rather than inventing a comparison. - `redactURL` strips userinfo from any reported server URL, because a URL is a place operators put credentials. `mem version` now uses it too. - `docs/schemas/mem-doctor.v1.schema.json` pins the `mem.doctor` v1 document: closed status and name enums, fixed check order, `additionalProperties: false`. ## Acceptance-criteria mapping - AC-001: `cmds_doctor_test.go` fixtures for unreachable server, missing credential, rejected credential, unresolved workspace, quota refusal and version skew, each asserting its named check, its exit code and a hint that names the documented path, over an `httptest` stub following the `cmds_ingest_test.go` pattern. Plus real compiled-binary runs: connection-refused (exit 5), a stalled listener (exit 5, timeout hint), and a healthy stack stand-in (exit 0, empty stderr). - AC-002: the stub transport records every request and fails the test on any non-GET path, any unexpected path, or any request body; a separate assertion scans stdout and stderr for the token value. A server-side audit of the compiled-binary runs logged 7 requests, all GET, all body=0. A URL carrying credentials is reported redacted, with zero occurrences of either secret in either stream. - AC-003: `--format json` is validated against the checked-in schema by an in-test structural validator (closed enums, required keys, pinned `prefixItems` names) and against a golden file, and independently by `python3 -m jsonschema` (Draft 2020-12, 4.19.2) on the compiled binary's real stdout. ## Deliberate assertion change `TestAuthStatusWithoutTokenReturnsAuthExitCode` asserted the old hint by exact equality, which REQ-002 makes conditional on config presence. It is widened, not deleted: the exit code and the login step are still pinned, and the deployment path is now required. ## Configuration and environment - Base: `731a468` (`origin/main`) - OS: Linux 6.18.33.2 WSL2, x86_64; Go 1.25 - No new third-party dependency; `go.mod`/`go.sum` untouched Refs bytefolk#112
PeterGuy326
requested changes
Aug 31, 2026
PeterGuy326
left a comment
PeterGuy326
left a comment
Collaborator
There was a problem hiding this comment.
P1 security hold on exact head 46499b2adacfc66999cddaf7e2e9852292f12ece.
redactURL returns the original string when url.Parse fails. A malformed credential-bearing server URL can therefore reach the text or JSON doctor report unchanged; the transport-error classification path can also append an unredacted error string. The current tests cover valid URLs only, so green CI does not prove the fail-closed boundary.
Please make parse failure redact by construction (never return the raw input or raw transport error), and add deterministic text, JSON, and transport-error regressions asserting that credential material never appears. Do not put a real credential in the test fixture or review output.
This remains Draft because #112 is still needs-design and its #109 dependency has not reached a canonical ready record. Establish those records, synchronize the conflicted base, then rerun required CI on the new exact head before requesting final review.
Contributor
|
PR 整体设计很扎实，测试覆盖和文档都很充分。以下是几个具体问题： 1.
|
This was referenced Sep 2, 2026
Closed
Collaborator
Author
Fail-closed redaction patch for the P1 — offered as a comment, not pushedNot a review decision. No approve, no request-changes, no vote, no acceptance, and this does not make the PR mergeable. I did not push to The P1 hold is real and I reproduced it in the compiled binary at the exact head, then wrote a patch for it. Both halves below are executed results, not readings of the diff. 1. Reproduced before fixingBuilt
The raw output at head, text mode: Your description was accurate, and I found the boundary is exactly "the URL fails to parse": a well-formed credential URL is already safe, because There are two independent vectors, not one. Fixing This matches 2. The patch
Verified against a pristine unpack of
diff --git a/server/cmd/mem/cmds_doctor.go b/server/cmd/mem/cmds_doctor.go
index f74cb82..7e7ffe4 100644
--- a/server/cmd/mem/cmds_doctor.go
+++ b/server/cmd/mem/cmds_doctor.go
@@ -8,6 +8,7 @@ import (
"net/http"
"net/url"
"os"
+ "regexp"
"strings"
"time"
@@ -334,7 +335,7 @@ func classifyProbe(err error) (status string, code int, detail, hint string) {
if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) {
return doctorFail, exitProvider, "probe timed out", "raise --timeout, or check that the server is not behind a stalled proxy"
}
- return doctorFail, exitProvider, "cannot reach the configured server: " + err.Error(), deployPathHint()
+ return doctorFail, exitProvider, "cannot reach the configured server: " + safeTransportDetail(err), deployPathHint()
}
// deployPathHint points at the container path the docs recommend, instead of a
@@ -343,18 +344,61 @@ func deployPathHint() string {
return "start the documented container path: deploy/compose, see docs/DEPLOYMENT.md"
}
+// redactedServerURL stands in for a server URL whose credential boundary cannot
+// be established, so a report never carries the raw value.
+const redactedServerURL = "REDACTED (server URL withheld)"
+
+// serverURLUnparseable replaces the whole text of a pre-transport failure. The
+// value is not printed even partially: url.Parse rejects a space in userinfo, so
+// error text cannot be safely trimmed around a credential.
+const serverURLUnparseable = "the configured server URL could not be parsed (value withheld)"
+
+// urlTokenRe matches a URL-shaped substring embedded in another message, such as
+// the request URL inside a transport *url.Error.
+var urlTokenRe = regexp.MustCompile(`[A-Za-z][A-Za-z0-9+.\-]*://[^\s"']*`)
+
// redactURL strips userinfo so a URL that carries credentials cannot be echoed
// into a report that an operator will paste into an issue. The marker uses only
// unreserved characters, because url.User("***") would percent-encode it.
+//
+// A parse failure withholds the whole value rather than returning it: url.Parse
+// rejects malformed percent-escapes, non-numeric ports and spaces in the host,
+// all of which are reachable in a credential-bearing URL, so the credential
+// boundary is unknowable precisely when parsing fails.
func redactURL(raw string) string {
u, err := url.Parse(raw)
- if err != nil || u.User == nil {
+ if err != nil {
+ return redactedServerURL
+ }
+ if u.User == nil {
return raw
}
u.User = url.User("REDACTED")
return u.String()
}
+// safeTransportDetail keeps a transport failure readable without echoing a
+// credential-bearing URL. Go sanitises userinfo only for errors raised after
+// parsing succeeds, so the two shapes are handled apart rather than trusting
+// url.Error's own redaction to cover both.
+func safeTransportDetail(err error) string {
+ // A pre-transport parse failure carries the raw, unsanitised value in its
+ // URL field, so none of that text is reused.
+ var ue *url.Error
+ if errors.As(err, &ue) && ue.Op == "parse" {
+ return serverURLUnparseable
+ }
+ // Beyond this point the URL was parsed and re-serialised by net/http, which
+ // percent-encodes what urlTokenRe's whitespace would otherwise split on.
+ return urlTokenRe.ReplaceAllStringFunc(err.Error(), func(token string) string {
+ u, parseErr := url.Parse(token)
+ if parseErr != nil || u.User != nil {
+ return redactedServerURL
+ }
+ return token
+ })
+}
+
func printDoctorReport(cmd *cobra.Command, r doctorReport) {
out := cmd.OutOrStdout()
fmt.Fprintf(out, "mem doctor (%s v%d)\n", r.Contract, r.SchemaVersion)
diff --git a/server/cmd/mem/cmds_doctor_test.go b/server/cmd/mem/cmds_doctor_test.go
index 3c56236..06b3edb 100644
--- a/server/cmd/mem/cmds_doctor_test.go
+++ b/server/cmd/mem/cmds_doctor_test.go
@@ -2,12 +2,14 @@ package main
import (
"bytes"
+ "context"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"net/http/httptest"
+ "net/url"
"os"
"path/filepath"
"strings"
@@ -416,6 +418,157 @@ func TestRedactURLStripsUserinfo(t *testing.T) {
}
}
+// doctorSentinel is not a real credential. It only has to be distinctive enough
+// that finding it in a report is unambiguous evidence of a leak.
+const doctorSentinel = "s3ntinel-p4ssw0rd"
+
+// malformedCredentialURLs are server values that url.Parse rejects. Each carries
+// the sentinel in its userinfo, so handing back the raw input leaks it.
+var malformedCredentialURLs = []struct {
+ name string
+ url string
+}{
+ {"malformed percent-escape", "http://admin:" + doctorSentinel + "%zz@127.0.0.1:8787"},
+ {"non-numeric port", "http://admin:" + doctorSentinel + "@127.0.0.1:http"},
+ {"space in host", "http://admin:" + doctorSentinel + "@lo calhost:8787"},
+ {"space in password", "http://admin:first " + doctorSentinel + "@127.0.0.1:8787"},
+ {"malformed scheme", "%s://admin:" + doctorSentinel + "@127.0.0.1:8787"},
+}
+
+func TestRedactURLWithholdsUnparseableCredentialURL(t *testing.T) {
+ for _, tc := range malformedCredentialURLs {
+ if _, err := url.Parse(tc.url); err == nil {
+ t.Fatalf("fixture %q (%s) parses successfully; it no longer exercises the "+
+ "parse-failure path and would pass vacuously", tc.name, tc.url)
+ }
+ if got := redactURL(tc.url); got != redactedServerURL {
+ t.Errorf("redactURL(%q) [%s] = %q, want %q and no credential material",
+ tc.url, tc.name, got, redactedServerURL)
+ }
+ }
+}
+
+func TestDoctorTextReportWithholdsUnparseableCredentialURL(t *testing.T) {
+ for _, tc := range malformedCredentialURLs {
+ configureDoctor(t, "http://127.0.0.1:8787", "", false)
+ stdout, stderr, err := execDoctor(t, "--server", tc.url)
+ if err == nil {
+ t.Fatalf("doctor [%s] unexpectedly succeeded:\n%s", tc.name, stdout)
+ }
+ // Redaction must not change the documented exit code.
+ if code := cliCode(t, err); code != exitProvider {
+ t.Errorf("doctor [%s] exit code = %d, want %d", tc.name, code, exitProvider)
+ }
+ for stream, text := range map[string]string{"stdout": stdout, "stderr": stderr} {
+ if strings.Contains(text, doctorSentinel) {
+ t.Errorf("doctor [%s] leaks the credential into %s:\n%s", tc.name, stream, text)
+ }
+ }
+ if !strings.Contains(stdout, redactedServerURL) {
+ t.Errorf("doctor [%s] text report missing %q:\n%s", tc.name, redactedServerURL, stdout)
+ }
+ }
+}
+
+func TestDoctorJSONReportWithholdsUnparseableCredentialURL(t *testing.T) {
+ schema := loadDoctorSchema(t)
+ for _, tc := range malformedCredentialURLs {
+ configureDoctor(t, "http://127.0.0.1:8787", "", false)
+ stdout, stderr, err := execDoctor(t, "--format", "json", "--server", tc.url)
+ if err == nil {
+ t.Fatalf("doctor [%s] unexpectedly succeeded:\n%s", tc.name, stdout)
+ }
+ for stream, text := range map[string]string{"stdout": stdout, "stderr": stderr} {
+ if strings.Contains(text, doctorSentinel) {
+ t.Errorf("doctor [%s] leaks the credential into %s:\n%s", tc.name, stream, text)
+ }
+ }
+ rep := decodeReport(t, stdout)
+ if rep.Server != redactedServerURL {
+ t.Errorf("doctor [%s] report.server = %q, want %q", tc.name, rep.Server, redactedServerURL)
+ }
+ for _, c := range rep.Checks {
+ if strings.Contains(c.Detail, doctorSentinel) || strings.Contains(c.Hint, doctorSentinel) {
+ t.Errorf("doctor [%s] check %q carries the credential: detail=%q hint=%q",
+ tc.name, c.Name, c.Detail, c.Hint)
+ }
+ }
+ // Withholding the URL must still satisfy the versioned contract.
+ var doc json.RawMessage
+ dec := json.NewDecoder(strings.NewReader(strings.TrimSpace(stdout)))
+ if err := dec.Decode(&doc); err != nil {
+ t.Fatalf("doctor [%s] json did not decode: %v\n%s", tc.name, err, stdout)
+ }
+ validateDoctorDoc(t, schema, doc)
+ }
+}
+
+func TestClassifyProbeNeverEchoesCredentialBearingTransportError(t *testing.T) {
+ newParseFailure := func(raw string) error {
+ _, err := http.NewRequestWithContext(context.Background(), http.MethodGet, raw, nil)
+ if err == nil {
+ t.Fatalf("transport fixture %q parsed; it no longer exercises the "+
+ "pre-transport failure path", raw)
+ }
+ return err
+ }
+ cases := []struct {
+ name string
+ err error
+ // wantKept is operator-actionable text that must survive, so a green run
+ // cannot be satisfied by dropping the detail entirely.
+ wantKept string
+ }{
+ {
+ name: "request build parse failure",
+ err: newParseFailure("http://admin:" + doctorSentinel + "%zz@127.0.0.1:8787/healthz"),
+ wantKept: "could not be parsed",
+ },
+ {
+ // The sentinel sits after a space, so only withholding the value
+ // wholesale keeps the tail out of the report.
+ name: "parse failure with a space in the password",
+ err: newParseFailure("http://admin:first " + doctorSentinel + "@127.0.0.1:8787/healthz"),
+ wantKept: "could not be parsed",
+ },
+ {
+ name: "url.Error carrying userinfo",
+ err: &url.Error{
+ Op: "Get",
+ URL: "http://admin:" + doctorSentinel + "@127.0.0.1:8787/healthz",
+ Err: errors.New("dial tcp 127.0.0.1:8787: connect: connection refused"),
+ },
+ wantKept: "connection refused",
+ },
+ {
+ name: "url.Error without userinfo",
+ err: &url.Error{
+ Op: "Get",
+ URL: "http://127.0.0.1:8787/healthz",
+ Err: errors.New("dial tcp 127.0.0.1:8787: connect: connection refused"),
+ },
+ wantKept: "connection refused",
+ },
+ }
+ for _, tc := range cases {
+ status, code, detail, hint := classifyProbe(tc.err)
+ if status != doctorFail || code != exitProvider {
+ t.Errorf("classifyProbe(%s) = (%q, %d), want (%q, %d)",
+ tc.name, status, code, doctorFail, exitProvider)
+ }
+ if strings.Contains(detail, doctorSentinel) || strings.Contains(hint, doctorSentinel) {
+ t.Errorf("classifyProbe(%s) leaks the credential: detail=%q hint=%q", tc.name, detail, hint)
+ }
+ if !strings.Contains(detail, "cannot reach the configured server") {
+ t.Errorf("classifyProbe(%s) lost the finding: %q", tc.name, detail)
+ }
+ if tc.wantKept != "" && !strings.Contains(detail, tc.wantKept) {
+ t.Errorf("classifyProbe(%s) dropped %q; redaction must not erase the diagnosis: %q",
+ tc.name, tc.wantKept, detail)
+ }
+ }
+}
+
func TestDoctorTextOutputIsAFixedOrderedList(t *testing.T) {
stub := newDoctorStub()
srv := stub.server(t)3. Evidence on the patched treeAll run in the pristine+patch tree, Node-independent, Go
Mutation control. Reverting each part of the fix separately, the tests that go red are distinct per vector — so the new coverage is load-bearing and neither vector is carried by the other's test:
Two things worth flagging from A and B: no pre-existing test went red in either mutation, including Mutation C is a hole in my own first version, found before posting: a password containing a space splits the whitespace-terminated token scan and leaves the credential tail in the report. The 4. Visible output change, stated plainlyFor a well-formed credential URL the transport detail now reads No 5. What this patch does not unblockDeliberately out of scope, because they need owner decisions rather than code:
6. Two notes for whoever picks this up
|
This was referenced Sep 4, 2026
Collaborator
Author
Closing as superseded by #164 — nothing from this branch was mergedNot a review decision, not an acceptance, and not a design ruling. No commit here reached Measured from the two exact head trees (codeload archives of
Two findings raised on this thread are still open in #164Closing this PR should not read as those being resolved. Against #164's head tree:
The other two findings on this thread — the shared-context Please carry 1 and 2 onto #164. Not affected by this closure
|
Collaborator
Author
Correction to my close comment above — finding 1 is wrong, ignore itI wrote that " root.PersistentFlags().StringVar(&cliServerOverride, "server", "", "memd base URL (overrides config; e.g. http://localhost:8787)")
So finding 1 from the review above is not open in either PR — it was a bad read on my part, and it should not be transplanted anywhere. What still stands from my comment:
Correction posted because the claim is actionable as written and would otherwise cost someone a fix for a bug that does not exist. |
PeterGuy326
added a commit
that referenced
this pull request
Sep 10, 2026
…165) Refs #112 ## Summary A configured URL that carries credentials in a shape `url.Parse` does not report as **userinfo** reaches output today. This adds one shared gate that redacts such a value when it can prove it is a transport URL, and **withholds the value whole** when it cannot, and wires it into all three egresses R3 names: the API client, `memd`'s log lines, and `mem doctor`. The adjudicated direction this implements is recorded on #112 as R3 (comment 5536659086): gate on *"parses as a recognized transport scheme with userinfo redacted, otherwise withhold the whole value"*. ## Scope: all three named egresses R3 names three egresses — doctor, apiclient, `memd`'s log line. All three are covered here. | egress | on `main` at the base | covered here | | --- | --- | --- | | apiclient — request construction | leaks | gated | | apiclient — all 4 `http.Client.Do` sites | leaks, ungated entirely | gated, one test case per site | | apiclient — `newRequest` on the workspace-transfer path | leaks | gated (arrived with #164's `workspace_transfer.go`) | | `memd` startup log line | leaks (`User != nil` gate) | gated | | `memd` fatal log line | leaks via a third-party error | gated | | `mem doctor` (text + JSON) | not present | gated through the same package | ### How doctor got onto this branch `mem doctor` is not on `main`; it only existed on #164 (which sits on #131's original doctor commit). Because #131 was closed as superseded by #164, doctor had exactly one carrier, so the surviving branch had to absorb it before #164 could be closed. Rather than re-author it, this branch **merges** the work: ``` c9e0e63 docs: correct doctor's stated secret guarantee (this change) d30f1ff fix(cli): route mem doctor's URL reporting through the shared gate (this change) ad4e78f Merge origin/main d7f2dcb Merge #164 (mem doctor) into the credential-url-gate branch b229537 fix(client,memd): withhold URLs whose credentials cannot be attributed fffcd4c fix(cli): redact malformed URLs and request build errors ← #164, author PeterGuy326 46499b2 feat(cli): add mem doctor and first-run guidance ← #131, author waterbro-8 ``` Both earlier commits are **ancestors** of this head, so #164's and #131's authorship is preserved on the merge rather than claimed as mine. #164's unrelated per-probe `--timeout` fix and its `workspace_transfer.go` coverage came across with the merge and are kept. What `d30f1ff` itself does is replace doctor's two local string-level helpers with calls into `internal/redact`, so there is one policy rather than three: ```go func redactURL(raw string) string { return redact.URL(raw, redact.APIURLs) } func sanitizeProbeError(err error) string { return redact.TransportError(err, redact.APIURLs) } ``` `c9e0e63` is prose only. It corrects three statements — in `CHANGELOG.md`, `docs/DEPLOYMENT.md` and a comment in `cmds_doctor.go` — that asserted the doctor report contains **no secret value**. That was never true of the query-parameter shape described under Known gap, measured at 3 sentinel occurrences per output format on this head. The non-goal stands; the promise was the thing that was wrong, so the docs now name the residual instead of denying it. ## The shape, and why the previous gate missed it ``` url.Parse("admin:pw@host") → Scheme="admin" Opaque="pw@host" User=nil ``` A gate written as `if parsed.User == nil { return raw }` therefore echoes the credential verbatim — not as a corner case, but as the normal path for any value whose scheme happens to contain a colon. The gate also refuses to scrub error *text*, because that cannot be made tight: `url.Error` renders with `%q`, so a `"` inside a password arrives escaped and a scanner that pairs quotes mis-pairs, replaces nothing, and leaves its cursor inside the URL. Withholding is chosen over partial trimming for the same reason: a delimiter inside a credential splits the message into pieces that no longer look like a URL, and the piece without the `@` is exactly the half that leaked. `memd`'s fatal line is included because `queue.NewClient` wraps asynq's parse error, which embeds the whole DSN: ``` queue: parse redis url: asynq: could not parse redis uri: parse "redis://:PASS@ho st:6379/0": invalid character " " in host name ``` slog renders an error value as its text, so that reaches the log unmodified. ## Known gap, stated rather than smoothed over The gate proves the absence of **userinfo**, not of every credential. A secret supplied as a query parameter (`redis://host:6379/?password=x`) parses as a clean URL with `User == nil` and **is still echoed**. This is the adjudicated scope, not an oversight, and closing it is a separate decision. It is pinned by a named characterization test (`TestTextKnownGapQueryParameterCredentialsAreEchoed`) that fails if someone closes the gap without updating the expectation, so the residual cannot become folklore. Withholding also has a real diagnosability cost, accepted by design: the reason a request failed is still reported, the host is not. ``` Error: Get [withheld]: unsupported protocol scheme "admin" ``` ## Validation ledger Evidence level claimed: **E3 — independently reproduced**. Not E4: I am the author, and E4 requires a non-author to verify the acceptance criteria end to end. Environment: Linux x64, Go 1.25.0, exact tree `9fa9c14de074b4ddb5320d4f83803f1657918b34` — `HEAD^{tree}` of the current head `c9e0e63`, read back from `GET /repos/bytefolk/mem/commits/c9e0e63…` and equal to it byte-for-byte, so the tree measured here and the tree on the PR are the same object. (This head was pushed through the Git Data API because `github.com:443` was down on this box; the local commit sha differs from the remote one, the tree does not.) | Check | Result | | --- | --- | | `go build ./...` | pass | | `go vet ./...` | pass | | `gofmt -l .` | no output | | `go test -count=1 ./...` | 32 packages ok, 0 FAIL | | `cmd/mem` (includes 16 `mem doctor` tests) | 102 tests, 0 fail | | `internal/redact` | 30 tests, 0 fail | | `internal/apiclient` | 45 tests, 0 fail | | `cmd/memd` | 15 tests, 0 fail | | `git merge-base --is-ancestor origin/main HEAD` | yes (main not silently reverted) | | CI on this head | **15/15 check names `success`** on `c9e0e63` (`run_attempt` 1), including `Go` and `PostgreSQL integration`; also 15/15 on the immediately preceding `d30f1ff` | ### `mem doctor` end-to-end, measured on built binaries Seven malformed credential URL shapes, each run through `mem doctor --server <shape> --timeout 1s` in **both** `text` and `json`, counting occurrences of a sentinel password in everything the process wrote: | # | shape | #164 head `fffcd4c` | this head `c9e0e63` | | --- | --- | --- | --- | | 1 | `http://admin:PW@127.0.0.1:1` | 0 | 0 | | 2 | `http://admin:PW@127.0.0.1:%zz` | 0 | 0 | | 3 | `http://admin:PW@ho st.example.com` | 0 | 0 | | 4 | `http://admin:PW x@127.0.0.1:1` | 0 | 0 | | 5 | `http://admin:PW@%` | 0 | 0 | | 6 | `http://admin:PW@mem.internal:99999999` | 0 | 0 | | 7 | `admin:PW@mem.internal:8787` (no scheme) | **6** | **0** | Shape 7 is the `url.Parse` → `User=nil` case from the section above: #164 prints the configured URL three times (`server`, `detail`, `hint`) in each of the two formats, so 6 occurrences is one leak per field. **A zero from a binary that has no `doctor` command means nothing.** This table was first produced against a build of `b229537` and came back 0/0/0/0/0/0/0, which looked like a pass — the actual output was `Error: unknown command "doctor" for "mem"`, because that head had no doctor at all. The harness now counts `unknown command` alongside `unknown flag` and every row above was taken with both counters at 0. ### Mutation controls Run against a copy, with the mutation confirmed present in the source before the suite is trusted: 1. Reverting the gate to the pre-fix `User != nil` form turns **10 named cases red across 3 packages** (`internal/redact`, `cmd/memd`, `cmd/mem`). 2. Removing the gate at **one** apiclient `Do` site turns red **exactly that site's** test case and leaves the other three green. The four cases map one-to-one onto the four sites, so a regression at a single site cannot hide. Control 2 was found by this method — an earlier version of this port left one site raw and the table caught it. 3. Putting **#164's original `redactURL` / `sanitizeProbeError` back** into this tree — i.e. merging #164 and *not* rewiring doctor — fails **exactly one test**, `TestDoctorSchemelessServerURLDoesNotLeakCredentials`, and its built binary leaks 2 sentinel occurrences on shape 7. Before that test was added, the same mutation failed **zero** tests. So the merge on its own was unprotected, and the one new test is what closes that. ### Two pre-existing assertions were relaxed — named, because one is not mine The gate withholds a value it cannot prove is a transport URL, where #164's local helper echoed a partially cleaned one. Two assertions demanded the echo shape specifically, so they were widened to accept **redact *or* withhold**: | location | test | author of the assertion as written | | --- | --- | --- | | `cmds_doctor_test.go:418` | `TestRedactURLStripsUserinfo` | waterbro-8 (#131) | | `cmds_doctor_test.go:456` | `TestDoctorMalformedServerURLDoesNotLeakCredentials` | **PeterGuy326 (#164)** | Both keep their original first clause — the secret must not appear, and a cleaned URL must still carry `REDACTED@<host>`. Only the *form* of an acceptable answer widened. Concretely at `:418`, `strings.Contains(got, secret)` still fails the test, and the `REDACTED@mem.internal:8787` check two lines above it is untouched. **No assertion was deleted, and the count went up, not down**: #164's 15 doctor tests are all present on this head (0 dropped), plus `TestRedactURLStripsUserinfo` and the new `TestDoctorSchemelessServerURLDoesNotLeakCredentials` = **16**. The way to check this claim is mutation control 3 above: if the widening had quietly weakened coverage, restoring #164's scrubber would have gone green, and instead it fails a test. Anyone who considers the withhold form unacceptable for `:456` should say so on #164's thread rather than assume I settled it unilaterally — I changed an assertion another author wrote to pass my gate. ## What I did not verify - The leak table was rebuilt and re-run against `c9e0e63` after the docs commit; that commit is prose and comments only, and the result is unchanged (0 on all seven shapes). Earlier heads `d30f1ff` and `b229537` each had 15/15 CI at the time they were written. - **There is no prior CI baseline for the doctor half to compare against.** #164's head `fffcd4c` has 0 `check-runs` and no workflow run has ever been recorded for it, so doctor has never been green in CI anywhere — including here, where it is now inside the `Go` job for the first time. That is a gain, not a regression, but it does mean no one has seen this code pass CI before. - Nothing was exercised on Windows or macOS. All results are Linux x64. - `memd` was **not** driven end-to-end to its fatal redis path. Reaching it requires a reachable PostgreSQL (the queue client is constructed after `db Open`), and the only local server on 5432 is an unrelated instance whose credentials I do not have — my attempt failed at `db open: ... failed SASL auth` before touching the queue. That egress is therefore evidenced by executing the gate on the asynq error string **measured from the real library in this tree**, not by a live `memd` run. - No claim is made about `#112`'s exit-code contract, R2/R3 acceptance, or #131/#164 merge readiness. ## Non-goals - No change to query-string credential handling (see Known gap). - No change to `#164`'s or `#131`'s branches; nothing here is pushed to anybody else's ref. #164 is pulled *into* this branch by merge, which is why #164 can be closed without losing its work. - No re-adjudication of R3. Doctor's `--timeout` behaviour, its check set and its JSON schema are #164's, unchanged except where they called the old local helpers. This PR stays `Refs` on #112 rather than `Closes`, because merging it would not by itself settle #112's acceptance — that needs a reviewer's decision, not just a diff landing. **Review strength of this PR, stated plainly:** 15/15 checks pass on `c9e0e63`, and I am the author of `b229537`, `d30f1ff` and `c9e0e63` — automated checks and my own sign-off are not a review. What this PR is missing is exactly one independent human review; nothing else blocks it (`mergeable=true`, no conflict with `main`, which is an ancestor of this head). It needs one human approval, and I am not that reviewer — on the doctor half especially, since I edited an assertion PeterGuy326 wrote. --------- Co-authored-by: waterbro-8 <waterbro-8@users.noreply.github.com> Co-authored-by: PeterGuy326 <47820304+PeterGuy326@users.noreply.github.com> Co-authored-by: waterbro-8 <318569545+waterbro-8@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the CLI half of "#109: a first-time user is told which prerequisite they are missing" (issue #112, R1):
mem doctor— a strictly read-only diagnosis of why the CLI cannot talk to a working server, with a versioned machine-readable output contract.mem auth loginagainst a server that does not exist yet.server_reachabilityGET /healthz, sent without a credential5provider/timeout,2if the URL answers but is not memcredential3authworkspaceGET /v1/capabilities2if the server resolved no workspace,3/4/5by error kindversion_skewGET /v1/versionwarncontributes0The process exits with the first failing check's SPEC §7.1 code. A check that an earlier failure made impossible is reported as
skipped, naming the blocker — it is never inferred as a pass.docs/schemas/mem-doctor.v1.schema.jsonpins themem.doctorv1 document (closed status enum, pinned check names and order,additionalProperties: false).REQ-003 is held literally: the command issues only
GETrequests, writes no file, creates no token, starts no container, installs no dependency, and prints no secret value or DSN. A server URL that carries credentials is reported with userinfo removed.Why this is a draft
#112isstatus: needs-designat R1, andAGENTS.mdstep 2 wantsstatus:readyplus acceptance criteria before a material change lands. It was driven forward on the technical owner's request so the shape of the surface — and the four rulings below — can be reviewed as code rather than prose. Not merge-eligible until the issue is promoted and its open decisions are ratified.Refs #112rather thanCloses: the issue's own evidence plan (E3/E4) asks for one run against a healthy deployed stack, which this environment cannot produce (see the ledger), and the open decisions could change the surface.Dependency check: #109 at R2
The issue requires
#109 R2to land first "so this issue names a path that exists in the docs rather than inventing one". The substance is already true at this base, and the new text was written to say nothing that is false before #109 merges:deploy/compose/exists at731a468(compose.yaml,generate-env.sh,.env.example, backup/restore scripts).docs/DEPLOYMENT.mdat731a468already documentscd deploy/composein four places, andREADME.md:63already listsdeploy/compose/as the production single-node path.So the hint points at a real, documented path today. What #109 R2 changes is how prominently the README leads with it, not whether the path exists. If the owner would rather this wait, the branch is trivially rebase-able onto #126 with one string to revisit.
Acceptance-criteria mapping
server/cmd/mem/cmds_doctor_test.go(14 tests) follows thecmds_ingest_test.gohttpteststub pattern and pins, for each of unreachable server, missing credential (first-run and configured host), rejected credential, workspace unresolved, quota refusal, and version skew: the named check, itsstatus, its exit code, the process exit code, and a hint naming the documented path. Skipped-check behavior and text-mode ordering are asserted separately.Beyond the fixtures, the compiled binary was run over real sockets:
--server http://127.0.0.1:1(nothing listening)server_reachability fail … connection refused,credential fail,workspace/version_skew skipped, exit5--server http://127.0.0.1:8787(accepts TCP, never answers HTTP)probe timed out+ "raise--timeout, or check that the server is not behind a stalled proxy", exit5-X main.cliVersion=0.9.9, server0.9.9)okchecks, exit0, stderr 0 bytes0.8.0version_skew warn — CLI reports 0.9.9, server reports 0.8.0, exit0t.Errorfon any non-GETmethod, any path outside the pinned three (GET /healthz,GET /v1/capabilities,GET /v1/version— asserted as an exact ordered list), or any non-empty request body.TestDoctorNeverPrintsSecretValuesscans both streams for the token value.0non-GET,0with a body;/healthzand/v1/versionshowauth=no(the unauthenticated probe working as designed),/v1/capabilitiesshowsauth=yes. A token set via$MEM_TOKENappears0times in stdout and stderr. A URL of the formhttp://<secret>:<secret>@hostis reported ashttp://REDACTED@host, with0occurrences of either secret.--format jsonvalidates against a checked-in artifact. Both kinds the issue allows, and from two independent engines:docs/schemas/mem-doctor.v1.schema.json, draft 2020-12) validated in-test by a small structural validator (no new dependency), andserver/cmd/mem/testdata/doctor_healthy.golden.json) compared after normalizing the ephemeral stub port.python3+jsonschema4.19.2,Draft202012Validator.check_schematheniter_errors, against the compiled binary's real stdout →schema_valid: True.Deliberate assertion change (disclosed)
TestAuthStatusWithoutTokenReturnsAuthExitCodeasserted the old hint by exact equality, which REQ-002 makes conditional on whether a config file exists. It is widened, not deleted — exit code3and the login step are still pinned, and the deployment path is now required to appear. This is the only pre-existing test touched;git diffon that file shows no other change.Honest limits in this design
version_skewcannot be computed in the shipped binaries today. Nothing injects a CLI version:.github/workflows/release.ymlbuilds withGOFLAGS: "-trimpath -ldflags=-s -w"only. The check therefore reportsskew not computable …as awarnrather than claiming agreement. This PR adds the seam (var cliVersion = devCLIVersion, overridable with-X main.cliVersion=…, demonstrated above) but deliberately does not change release builds — that is a release-side decision./healthzwould otherwise read as "the server is down" to a user whose only problem is a bad token; the credential check owns that finding.mem lsand a few other surfaces have no credential gate at all, so they cannot show first-run guidance. REQ-002 scopes to "every command that currently returnsnewCliError(3, "not logged in", …)" — all 23 such sites now route througherrNotLoggedIn()(the only remaining literals are inside that helper). Extending the gate to commands that today dial the network raw is a separate decision, not taken here.configFileExists()is the first-run signal, becauseloadConfigdeliberately succeeds with no file. A corrupt-but-present config reads as "configured", which is the correct distinction for this hint.Open decisions needing a ruling
mem doctor. The alternative named in feat(cli): add mem doctor and first-run guidance that names the documented deploy/compose path #112 wasmem auth status --doctor;auth statusstill does its own narrower job and is unchanged apart from the hint. Which one is the contract?memdlog an equivalent readiness line at startup (issue decision 2)? Not attempted here — this PR is CLI-only.mem-mcpand the web setup screen (issue decision 3)? The four probes are currently plain functions inpackage main; extracting them is easy now and harder after a second consumer appears.mem doctoris not added to the Phase-1 command list. Precedent:mem ingest(shipped in feat(ingest): add mem ingest qoder for AI-agent conversation transcripts (#103) #108) is also absent —grep -c ingest SPEC.mdis0— so §7.2 does not appear to be maintained as a live inventory. If the owner wants it listed, say so and I will add the line.0(skew warn,skippedafter a fail never happens without a real failure upstream). Confirm that advisory-only means "script-safe".Validation ledger
Base
731a468(origin/main, includes #128). Head46499b2, single commit.gofmt -l .go vet ./...go build ./...go test ./... -count=1ok, 11 with no test files, 0FAILgo test ./cmd/mem/ -count=1 -v--- PASS, 0 fail, 0 skipgo test ./cmd/mem/ -count=1 -run 'Doctor|NotLoggedIn|RedactURL' -vgo test -race ./cmd/mem/ -count=1git diff --check HEAD~1 HEADpython3/jsonschemaon real--format jsonstdoutschema_valid: TrueGET, 0 bodiesNot run, and not claimed: a run against a real deployed stack (Docker daemon unavailable in this environment —
docker infocannot connect; the healthy leg used a local stand-in serving memd's documented shapes over a real socket, which is not the same as E4 and is why this stays a draft); hosted CI on this head; the PostgreSQL/MinIO integration jobs; any web or MCP surface; coverage measurement. No test result above is an expectation — each is an observed output.Compatibility, operations, and rollback
not logged infailure on a host with no config file gains a second clause. Hosts that have a configuration see the previous string verbatim.mem versionnow prints the injected version instead of a hard-codeddev, and redacts userinfo from the server URL it echoes. Both are output-only.go.mod/go.sumuntouched (no new third-party dependency).contract,schema_version: 1) so ops tooling can pin it.Automated assistance
Implementation and validation were performed with automated assistance by the submitting account, and every result above is an actual measured output rather than an expectation. The human submitter remains the author and accountable reviewer. Independent
CODEOWNERSapproval is still required (AGENTS.mdstep 6), and no merge, label change, or branch-protection bypass was performed.Refs #112
Refs #109