feat(cli): add mem doctor and first-run guidance that names the documented deploy/compose path #112

Activity

1. PeterGuy326 commented on Sep 1, 2026

   

   PeterGuy326

   on Sep 1, 2026

   Collaborator

   More actions

   schemaVersion: requirement-decision.v1
   canonicalIssue: "https://github.com/fullstack-ai-infra/mem/issues/112"
   previousRevision: R1
   resultingRevision: R2
   resultingStatus: ready
   decisionType: scope-and-security-contract
   approver: "@PeterGuy326"
   decidedAt: "2026-09-01T02:54:08Z"
   dependency: "https://github.com/fullstack-ai-infra/mem/issues/109 R2 accepted with verification evidence"

   R2 owner decision — secure, read-only mem doctor

   This is the effective product-owner decision for R2. The contract is ready, but implementation is prohibited until #109 R2 documentation is accepted with its verification evidence. It is not a PR approval or merge authorization.

   Approved scope

   • The v1 command is exactly top-level mem doctor.
   • It performs only these fixed checks: server reachability, credential presence, selected workspace, and CLI/server version skew.
   • It emits fixed ordered text and a versioned JSON v1 representation. A release build injects the CLI version used for skew evaluation.
   • It is diagnostic and advisory only: requests are read-only GET operations, no writes or remediation occur, and v1 returns exit code 0 while reporting findings.

   Security acceptance criteria

   Tests must cover valid and malformed credential URLs plus text, JSON, and transport-error paths. Every such path must fail closed without printing raw URLs, raw error values, token or credential values, DSNs, or equivalent secret-bearing data. A transport stub must prove that any non-read request fails the test.

   Non-goals

   No dependency installation, environment certification, Compose or Docker-host detection, container start/stop, TTY wizard, memd readiness log, shared check registry, automatic remediation, or credential mutation is authorized.

   After #109 R2 is accepted, implementation may be planned against R2 with normal independent review, required CI, and merge evidence still required.
2. sun-970

   added a commit that references this issue on Sep 1, 2026

   feat(cli): address all bytefolk#112 acceptance criteria

   ...

   35a25d2
3. PeterGuy326 commented on Sep 4, 2026

   

   PeterGuy326

   on Sep 4, 2026

   Collaborator

   More actions

   schemaVersion: requirement-decision.v1
   canonicalIssue: "https://github.com/bytefolk/mem/issues/112"
   previousRevision: R2
   resultingRevision: R3
   resultingStatus: ready
   decisionType: contract-adjudication
   approver: "@PeterGuy326"
   decidedAt: "2026-09-04T06:30:41Z"

   R3 owner decision — doctor exit codes follow SPEC.md §7.1

   This adjudicates the single contradiction recorded against R2: its approved scope said "v1 returns exit code 0 while reporting findings", while REQ-001 and SPEC.md §7.1 (line 582) map exit codes 0 ok · 2 not_found · 3 auth · 4 plan/quota · 5 provider/timeout, and the implementation (#131/#164) plus its tests already conform to the SPEC mapping and pin it (cmds_doctor_test.go asserts a non-zero/5 exit for an unreachable server).

   Decision. The SPEC.md §7.1 mapping is the doctor's contract. R2's "returns exit code 0 while reporting findings" sentence is replaced by: v1 reports findings with exit codes 0 ok · 2 not_found · 3 auth · 4 plan/quota · 5 provider/timeout, per SPEC.md §7.1. Everything else in R2 stands unchanged: the fixed four checks, fixed ordered text and versioned JSON v1, read-only GET probes only, no writes or remediation, and the fail-closed secret-handling acceptance criteria.

   Why this direction. SPEC.md §7.1 is the repository-wide CLI contract, so doctor deviating from it would be the exception that needs justifying, not the rule. The conforming implementation and tests already exist and are green; enforcing R2's sentence would have required reworking code and tests that pin the opposite behaviour, while still leaving doctor inconsistent with every other command's exit semantics. Diagnostic-and-advisory is preserved in what matters: probes are still read-only and the command never remediates; the exit code is a machine-readable classification of the finding, not an error in the CLI itself.

   Scope of this adjudication. It resolves the exit-code contradiction only. It is not an approval or merge authorization for #131 or #164. Their remaining blockers stand: the no-scheme/Opaque credential shape still leaks (fix direction: gate on "parses as a recognized transport scheme with userinfo redacted, otherwise withhold the whole value", one shared gate at each of the three egresses — doctor, apiclient, memd log line), #164's CHANGELOG conflict, the absence of any CI run on its head, and the independent acceptance signature #112 R2 requires from a reviewer who is neither the technical owner nor the PR author.
4. waterbro-8

   added a commit that references this issue on Sep 6, 2026

   feat(cli): add mem doctor and first-run guidance

   ...

   46499b2
5. sun-970 commented on Sep 8, 2026

   

   sun-970

   on Sep 8, 2026

   Contributor

   More actions

   Questions and proposed solutions

   1. Credential leak on no-scheme / Opaque URLs

   R3 decision mentions the no-scheme/Opaque credential shape still leaks across three egresses (doctor, apiclient, memd log line). The suggested fix direction is "parses as a recognized transport scheme with userinfo redacted, otherwise withhold the whole value."

   Question: Should the redaction gate live in a single shared helper (e.g. internal/credutil.Redact) that all three call sites import, or should each site implement its own guard? A shared helper is easier to audit and keeps the fail-closed invariant in one place.

   Proposed solution: Introduce one redactCredential(raw string) (string, error) in an internal package. It returns the redacted string for recognized schemes and a sentinel error (not the raw value) for anything unparseable. All three egresses call this single function. One test table covers valid URL, no-scheme, Opaque, empty, and malformed inputs — and asserts the raw value never appears in any output path.

   ---

   2. Command name is still an open decision

   The issue lists three candidates: mem doctor, mem doctor env, mem auth status --doctor. R2 already approved top-level mem doctor, but it was listed again as an open decision.

   Question: Is the command name truly still open, or was it settled by the R2 decision? Re-opening it would delay implementation without adding user value.

   Proposed solution: Treat the R2 decision as final on naming — mem doctor it is. If there's new evidence that another shape fits better, raise it before implementation starts, not during review.

   ---

   3. memd startup readiness line

   Open decision (2) asks whether memd should log an equivalent readiness line at startup so compose operators see the same facts without exec-ing into the container.

   Question: Is this in scope for v1, or deferred? If deferred, operators lose the main benefit of the doctor (visibility without SSH/exec).

   Proposed solution: Defer to v2. v1 should ship the CLI command first, get operator feedback on the check set and output format, then mirror it in memd once the contract is stable. Adding it now doubles the review surface and risks the two outputs drifting.

   ---

   4. Shared check registry for mem-mcp and web setup

   Open decision (3) asks whether the check set should be a registry reusable by mem-mcp and the web setup screen.

   Question: Is there a concrete consumer today, or is this speculative? Building a registry for one caller is premature abstraction (YAGNI).

   Proposed solution: Keep checks as plain functions in the doctor package for v1. If mem-mcp or web setup actually needs the same checks, extract a registry at that point with real call-site requirements. One caller now, refactor later when the second caller arrives.

   ---

   5. Path to unblocking #131 / #164

   R3 lists four blockers: credential leak fix, CHANGELOG conflict in #164, no CI run on its head, and missing independent acceptance signature.

   Proposed solution:

   1. Land the redactCredential helper first (smallest, highest-risk fix).
   2. Rebase fix(cli): redact malformed URLs and request build errors #164 onto the new helper and resolve the CHANGELOG conflict.
   3. Push and let CI run green.
   4. Request review from a CODEOWNERS member who is neither @waterbro-8 nor the PR author, per the R2 requirement.

   These are sequential — each depends on the previous — so a single stacked-PR approach or one PR with ordered commits would keep the review tractable.

   ---

   Happy to discuss any of these further. The credential leak fix feels like the most urgent item — everything else can follow once that's closed.
6. PeterGuy326

   added a commit that references this issue on Sep 10, 2026

   fix(cli-memd): withhold URLs whose credentials cannot be attributed (#…

   ...

   d9d0969
7. PeterGuy326

   added

   type:featureA new user-facing capabilityA new user-facing capability

   area:cliCommand-line interfaceCommand-line interface

   priority:p2Useful backlog work outside the immediate critical pathUseful backlog work outside the immediate critical path

   evidence:e2-sourceSource or log evidence identifies the likely causeSource or log evidence identifies the likely cause

   on Sep 10, 2026
8. PeterGuy326 commented on Sep 10, 2026

   

   PeterGuy326

   on Sep 10, 2026

   Collaborator

   More actions

   Closing as completed. The deliverable is on main, and I re-verified each acceptance criterion against main @ 55d09b203ba9dc011f140d13d6bbb32f186fc16c today rather than relying on the delivery comments.

   Carrier: merge commit d9d0969deae252f3619429e8eb07aa2533e26e0c (#165), whose message carries Refs #112. #165 deliberately used Refs and not Closes, stating that "merging it would not by itself settle #112's acceptance — that needs a reviewer's decision, not just a diff landing." This comment is that decision.

   Acceptance criteria

   AC	Requirement	Evidence on main	Verdict
   AC-001	Fixture per finding class — unreachable server, no credential, no workspace selected, version skew — each producing its named check, its code, and a hint naming the documented path, asserted at the HTTP-contract level	server/cmd/mem/cmds_doctor_test.go (748 lines): TestDoctorUnreachableServer :180, TestDoctorMissingCredential :213, TestDoctorMissingCredentialOnConfiguredHost :244, TestDoctorNoWorkspaceSelected :263, TestDoctorVersionSkew :328; exit-code classes TestDoctorRejectedCredential :285 (3), TestDoctorQuotaIsItsOwnCode :309 (4); healthy path TestDoctorHealthyReportsAllChecksAndExitsZero :143	met
   AC-002	No write request of any kind — a stub transport fails on any non-read call — and never prints a secret, token value, or DSN	Stub fails on any request body (cmds_doctor_test.go:47-50) and on any unexpected method/path (:63); exact request set asserted as GET /healthz, GET /v1/capabilities, GET /v1/version (:172, :237). Secret handling: TestDoctorNeverPrintsSecretValues :371, TestRedactURLStripsUserinfo :405, TestDoctorMalformedServerURLDoesNotLeakCredentials :428, TestDoctorSchemelessServerURLDoesNotLeakCredentials :468	met
   AC-003	--format json validates against a checked-in schema or golden file	Both exist and are both consumed: docs/schemas/mem-doctor.v1.schema.json (3024 B, blob 3f8dd30a) and server/cmd/mem/testdata/doctor_healthy.golden.json (842 B, blob 7fcef975). TestDoctorJSONMatchesCheckedInSchema :580 reads the schema from disk (:562), validates the document against it (:592-593), diffs the golden (:609-614), and enforces additionalProperties: false (:631)	met

   Requirements

   • REQ-001 — four checks and the SPEC §7.1 exit mapping. server/cmd/mem/cmds_doctor.go (377 lines) documents the check set at :25-28 (server_reachability, credential, workspace, version_skew) and implements them at :207, :231, :257, :296. Exit constants exitOK=0, exitNotFound=2, exitAuth=3, exitPlanQuota=4, exitProvider=5 at :56-60, with the §7.1 citation at :53 — i.e. the R3 adjudication, not R2's superseded exit-0 sentence.
   • REQ-002 — first-run guidance naming the documented deployment path. deployPathHint() at cmds_doctor.go:349 returns start the documented container path: deploy/compose, see docs/DEPLOYMENT.md. Both targets exist on main today: deploy/compose/ (compose.yaml, .env.example, generate-env.sh, backup.sh, restore.sh, fixtures/compose.env) and docs/DEPLOYMENT.md. Guidance path covered by TestNotLoggedInGuidance :705.
   • REQ-003 — diagnosis only, zero writes. Enforced mechanically by AC-002's stub, and stated at cmds_doctor.go:31 ("no dependency is installed, no Docker or compose command is issued").

   On the stated dependency

   #112 recorded "Dependency: #109 at R2 must land first, so this issue names a path that exists in the docs rather than inventing one." #109 is still open, so I want to be explicit about why that does not block this close: the dependency's purpose was that the hint name a real path. deploy/compose and docs/DEPLOYMENT.md both exist on main independently of #109's README/onboarding repositioning, so the hint is not inventing anything. #109 remains open on its own docs merits and is unaffected by this close.

   Evidence level, stated honestly

   I am not raising this above what the record supports. #165's own ledger claims E3 (independently reproduced) and explicitly declines E4 because its author wrote the change. I am a co-author of #165 and the author of #164, whose fffcd4c doctor commit is an ancestor of the delivered head — so my verification here is not a non-author E4 either. What I can attest to is source- and test-level: the files, line numbers and assertions above are read off main at 55d09b20 today, and AC-002/AC-003 are enforced by checked-in tests that fail on regression rather than by prose. The doctor half also has 15/15 CI checks green on the delivered head. If anyone wants E4, the gap is a non-author running the seven malformed-credential shapes end to end — #165's table is the procedure.

   Two residual facts worth keeping visible rather than burying in a closed issue:

   1. The query-parameter credential gap is real and by design (redis://host:6379/?password=x still echoes), pinned by the named characterization test TestTextKnownGapQueryParameterCredentialsAreEchoed. Closing feat(cli): add mem doctor and first-run guidance that names the documented deploy/compose path #112 does not close that gap; it was adjudicated out of R3's scope.
   2. fix(cli-memd): withhold URLs whose credentials cannot be attributed #165 widened an assertion PeterGuy326 wrote at cmds_doctor_test.go:456 to accept redact-or-withhold. Its author asked that anyone who considers the withhold form unacceptable say so on fix(cli): redact malformed URLs and request build errors #164's thread. Nobody has, and the assertion count went up (15 → 16 doctor tests, none dropped), so I am treating that as settled.
9. PeterGuy326

   closed this as completedon Sep 10, 2026