fix(mt#4910): Replace six hand-rolled connection-string redactions with maskConnectionString

[minsky-ai]

Summary

Six sites hand-rolled a connection-string userinfo redaction instead of calling
maskConnectionString, in three different spellings. Every one of them required a non-empty
username AND a non-empty password, so a connection string with either half empty matched nothing
— and a regex that matches nothing returns its input unchanged, printing the credential.

The empty-username form is the severe one: postgresql://:realpassword@host/db goes through
verbatim with a live password. The empty-password form (postgresql://user:@host/db, which
Postgres accepts) exposes a username and host. All six were also non-global, so a second connection
string embedded in the same text survived regardless of which halves were populated.

This is the failure mode terminal-command-best-practices.mdc §Secret handling names outright — "a
pattern matching nothing emits its input UNCHANGED, indistinguishable from a redaction that fired" —
and the repo has already leaked a production password exactly this way (mem#808).

Key changes

Six sites converted to maskConnectionString:

Site	Leaked on
packages/domain/src/persistence/validation-operations.ts:103	empty user, empty pass, 2nd occurrence
packages/domain/src/persistence/postgres-migration-operations.ts:613	same
scripts/verify-persistence-self-heal.ts:111	2nd occurrence only
scripts/smoke-task-id-reuse.ts:73	empty pass
scripts/smoke-task-kinds.ts:66	empty pass
scripts/smoke-memory-domain-routing.ts:51	empty pass

validation-operations.ts already imported maskConnectionString and called it 55 lines above the
weak copy — that site was a one-token fix.

Mask vs. host, decided once for all six: maskConnectionString, not connectionTargetHost. The
latter returns new URL(cs).host — no database name — and every one of these lines exists to answer
"which database?" (three say Database: outright). Masking keeps host/port/db while replacing both
userinfo halves globally, so it reaches the same safety without dropping the detail that makes the
line worth printing.

verify-persistence-self-heal.ts gains a second benefit: its old rendering was ://***@, and
credential-shape-check.ts recognizes exactly one masked form — maskConnectionString's own output.
://***@ was not a form the checker could confirm safe; ://***:***@ is.

Scope found during planning

The task spec named three sites. Its verification grep matched only the :\/\/-prefixed
spelling, so it was blind to the other two spellings — four of the six sites. The widened grep found
them. Full inventory and the corrected grep are in
mt#4910's ## Planning Audit and
## Implementation sections.

Out of scope — filed as mt#4963

The sanctioned detector shares this exact blind spot. CREDENTIAL_SHAPES's
postgres-url-credentials regex (credential-scrubber.ts:151) also requires non-empty halves, and
it feeds three consumers: the transcript-ingest scrubber, minsky security check-credentials, and
(by the same shape) .gitleaks.toml. Measured by running them:

scrubText("postgresql://:hunter2@db.example.com:5432/minsky")        -> passed through verbatim
checkForUnmaskedCredentials("postgresql://:hunter2@host:5432/db")    -> no hit
checkForUnmaskedCredentials("postgresql://user:@host:5432/db")       -> no hit

That is why AT3 below asserts by reading the line rather than with check-credentials: on this
defect class the checker cannot discriminate a fixed tree from a broken one. Filed as mt#4963 rather
than absorbed — different subsystem, different files.

Testing

The task's ## Acceptance Tests numbering is used verbatim below.

Execution evidence:

AT1 — the divergence, reproduced (discharged during planning; both functions run, not read):

--- EMPTY password: postgresql://user:@host:5432/db
  helper : postgresql://***:***@host:5432/db
  inline : postgresql://user:@host:5432/db      <-- UNCHANGED
--- EMPTY username: postgresql://:hunter2@host:5432/db
  helper : postgresql://***:***@host:5432/db
  inline : postgresql://:hunter2@host:5432/db   <-- UNCHANGED, live password exposed

AT2 / SC3 — the widened grep returns zero leaking sites. The form the spec originally specified
([^)]*) cannot span a ), so it could not see the sanctioned helper's own regex — a probe that
could not fully fail, which is this task's own defect class. Corrected to [^,]*:

$ grep -rnE '\.replace\(/[^,]*@' --include='*.ts' packages src scripts services
packages/domain/src/persistence/providers/postgres-provider.ts:551:    return connectionString.replace(/(@[^/?]*):6543(?=\/|$|\?)/, "$1:5432");
packages/domain/src/persistence/providers/postgres-provider.ts:1022:    const displayString = connectionString.replace(/\/\/[^@]+@/, "//***@");
packages/domain/src/persistence/connection-string.ts:23:  return input.replace(/(:\/\/)[^:/@]*:[^@]*@/g, "$1***:***@");
packages/domain/src/rules/rule-mention-parser.ts:55:    .replace(/(?:^|[\s])@[a-zA-Z0-9_-]+(?=[\s]|$)/g, " ")
src/hooks/deploy-domain-detector.ts:161:  h = h.replace(/^[^@/]*@/, ""); // strip user:pass@
scripts/smoke-setup-db.ts:63:    const fragment = pgUrl.replace(/^postgres(ql)?:\/\/[^@]*@/, "");

Six hits, none leaking: the sanctioned helper (line 23 — expected, and what makes this grep able to
fail); the :6543→:5432 port swap; getConnectionInfo, which masks the empty-half cases correctly
and is parked on mt#4963 behind PR #3412 and mt#3497; an @mention stripper in rule prose; and two
sites that STRIP userinfo to build a host or substring needle rather than to print a haystack.

AT3 — persistence check against the live database, asserting the mask fired rather than
printing the raw line:

$ bun src/cli.ts persistence check
Testing connection to: postgresql://***:***@aws-0-us-west-2.pooler.supabase.com:6543/postgres

$ <output> | bun src/cli.ts security check-credentials --quiet   # exit 0 (clean)
$ <output> | grep -cE 'postgres(ql)?://[^:/@*]+:[^@*]+@'          # 0 unmasked shapes

AT5 / related tests:

$ bun scripts/run-related-tests.ts <the 7 changed files>
382 pass / 0 fail / 976 expect() calls across 16 files [15.73s]
  incl. postgres-migration-operations.test.ts, validation-operations.test.ts, setup-db.test.ts

Typecheck 0 errors across 8 projects (session workspace); lint 0 errors / 0 warnings across 4376
files.

Negative control — AT4, per site, against the un-fixed patterns:

Each pre-fix pattern was run verbatim against the failing inputs alongside the new helper. Sites
1–2 leave BOTH empty-half forms unchanged; sites 4–6 leave the empty-password form unchanged; all
three spellings drop the second occurrence.

### validation-operations.ts:103 + postgres-migration-operations.ts:613
  empty username (LIVE password)
    PRE-FIX  : postgresql://:hunter2@db.example.com:5432/minsky   <-- UNCHANGED, LEAKS
    POST-FIX : postgresql://***:***@db.example.com:5432/minsky
  empty password
    PRE-FIX  : postgresql://user:@db.example.com:5432/minsky   <-- UNCHANGED, LEAKS
    POST-FIX : postgresql://***:***@db.example.com:5432/minsky
  two in one string
    PRE-FIX  : failed "postgresql://***:***@h1/d1"; retry "postgresql://b:s2@h2/d2"
    POST-FIX : failed "postgresql://***:***@h1/d1"; retry "postgresql://***:***@h2/d2"

### smoke-task-id-reuse.ts:73 + smoke-task-kinds.ts:66 + smoke-memory-domain-routing.ts:51
  empty password
    PRE-FIX  : postgresql://user:@db.example.com:5432/minsky   <-- UNCHANGED, LEAKS
    POST-FIX : postgresql://***:***@db.example.com:5432/minsky
  two in one string
    PRE-FIX  : failed "postgresql://a:<REDACTED>@h1/d1"; retry "postgresql://b:s2@h2/d2"
    POST-FIX : failed "postgresql://***:***@h1/d1"; retry "postgresql://***:***@h2/d2"

On the added test, stated plainly: the multi-occurrence test in setup-db.test.ts (SC4) covers
the g flag, the one case the existing five-case suite did not. It exercises the HELPER, which this
PR does not change, so it passes on main too and has no negative control of its own — the control
above is at the six call sites, which is where the behaviour changed.

SC5 — no per-site behavioural test was added, and the reason is the code shape. Each site is
template(mask(connectionString)): a log or format line whose only behaviour IS which function it
names. The value is already tested at the helper across all six edge cases, so a per-site test could
only assert which function is called — and the honest instrument for that is AT2's grep. Reaching
the two domain lines behaviourally would need a live Postgres (getPostgresMigrationsStatus opens a
connection before computing maskedConn) or a spyOn(log, "cli"), which /implement-task §6 names
as design feedback rather than a test to write, and which TEST_LOGGER_SILENCED_FLAG would silence
anyway.

Runtime import checked, not just typechecked. The @minsky/domain/persistence/connection-string
subpath was confirmed to resolve at runtime via a bun -e import returning the working function —
the mem#577 / mt#2760 hazard where a subpath passes tsconfig paths and fails bun's package
exports.

Deploy verification: three of the seven changed files are deploy surface per
isDeploySurfaceFile (packages/domain/src/persistence/{postgres-migration-operations,validation-operations}.ts
and packages/domain/src/setup-db.test.ts; the four scripts/ files are not). The post-merge
deploy will be verified per /implement-task §10 with notBefore set to the merge timestamp and
expectCommitSha set to the merge commit.

[minsky-reviewer]

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 598K prompt, 3K completion | Duration: 92s
Mode: normal

Commands

• /review — request a fresh review

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Focused PR replacing six ad-hoc connection-string redactions with the vetted maskConnectionString, plus a missing multi-occurrence test. The changes reduce leakage risk on empty-username/password and ensure global masking. I noted one minor inconsistency: dry-run now prints the helper’s masked form while execute-mode still shows host+pathname; consider standardizing or documenting that divergence. Otherwise, scope matches the spec, tests and typecheck remain green, and no docs updates are needed. Approving.

Findings

• [NON-BLOCKING] packages/domain/src/persistence/postgres-migration-operations.ts:236 — Inconsistent masking form between dry-run and execute paths
  getPostgresMigrationsStatus now uses maskConnectionString(connectionString) and surfaces Database: ${maskedConn} in dry-run output, preserving host/port/db with ***:*** userinfo. In the execute path, the masked value is still built via new URL(connectionString) and renders only host + pathname (no scheme / no explicit userinfo mask). See packages/domain/src/persistence/postgres-migration-operations.ts, the const masked = (() => { ... })(); block used by the execute-mode logs. This leaves two different display formats for the same "Database:" line depending on --dry-run vs --execute. If this divergence is intentional, consider documenting it inline; otherwise, consider standardizing on maskConnectionString for execute-mode too for consistency and easier greppability by tooling that recognizes the masked form.

Documentation impact

• no-update-needed — I reviewed the diff: it replaces several inline regex redactions with the existing helper maskConnectionString and adds a unit test covering multi-occurrence masking. No new user-visible commands, flags, or configuration are introduced, and existing CLI lines continue to display a masked connection string. I did not read specific docs files; this verdict rests on the diff's scope and observed behavior not changing semantics beyond masking implementation. If a doc explicitly described the prior inline forms (unlikely), it would not be invalidated by this internal refactor.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2

---

Verification pass: the fix commit replaces all six identified hand-rolled redaction sites with maskConnectionString, adds the missing multi-occurrence test, and wires imports correctly. I verified each changed site now calls the shared helper and that the domain-layer relative import resolves. No regressions or new defects surfaced in the touched files; behavior changes are limited to safer masking. The prior round’s non-blocking note about dry-run vs execute display format remains intentionally unchanged and tracked separately. Repo-wide grep verification and planning-run reproductions are out-of-band and thus marked Unverifiable in spec checks, but the localized code changes meet the task’s normative criteria. Approving.

Spec verification

Criterion	Status	Evidence
The divergence is reproduced and recorded (discharged during planning, 2026-09-04 — see ## Planning Audit for the run). It reproduces AND the spec understated it: the inline pattern also leaves an empty USERNAME string unmasked, which exposes a real password, where the empty-password case exposes only a username and host.	Unverifiable	This criterion depends on live-run planning evidence outside the diff. I cannot verify execution logs from planning within this review. No repo artifact in the diff captures the runtime reproduction.
All six leaking sites (enumerated in ## Scope) call maskConnectionString. Decided against connectionTargetHost at every site — it drops the database name, which is the detail all six lines exist to convey.	Met	All six sites now call maskConnectionString:

• packages/domain/src/persistence/validation-operations.ts: log line uses maskConnectionString(connectionString) (validation-operations.ts:66–68)
• packages/domain/src/persistence/postgres-migration-operations.ts: maskedConn uses maskConnectionString(connectionString) (postgres-migration-operations.ts:614–616)
• scripts/verify-persistence-self-heal.ts: SKIP line uses maskConnectionString(LIVE) (verify-persistence-self-heal.ts:114–116)
• scripts/smoke-task-id-reuse.ts: prints Database: via maskConnectionString(connectionString) (smoke-task-id-reuse.ts:64–73)
• scripts/smoke-task-kinds.ts: same (smoke-task-kinds.ts:60–68)
• scripts/smoke-memory-domain-routing.ts: same (smoke-memory-domain-routing.ts:39–52). |
  | A repo-wide grep for any inline redaction of a connection-string userinfo returns no leaking site: grep -rnE '.replace(/[^,]@' --include='.ts' packages src scripts services | Unverifiable | This requires executing a repo-wide grep. I cannot run shell commands from the review. The changed files show conversions away from inline regexes, but repo-wide confirmation is not verifiable here. |
  | maskConnectionString gains a test for the multi-occurrence case (two connection strings in one string; the g flag is what the inline copies drop). Added to packages/domain/src/setup-db.test.ts. | Met | packages/domain/src/setup-db.test.ts: adds test("masks EVERY occurrence, not just the first", ...) (setup-db.test.ts:102–119) exercising multiple occurrences. |
  | Each converted site has a test, or a recorded reason it has none. One test added; five recorded as grep-verified — each site is template(mask(connectionString)). | Met | A new helper-level test covering the missing multi-occurrence case was added (setup-db.test.ts:102–119). The remaining five sites are simple log-template changes that delegate masking to the shared helper (already comprehensively tested in this file). Given their shape (no additional logic beyond calling the helper), per the spec’s rationale, per-site behavioral tests are not required. |

Documentation impact

• no-update-needed — The PR replaces ad-hoc connection-string redactions with the existing helper maskConnectionString and adds a unit test for multi-occurrence masking. No commands, flags, or user-facing APIs were added or changed; the displayed "Database:" lines remain masked, only via a safer shared helper. I checked the changed files and found no behavior that would require documentation updates.