Stop shipping the MCP service default credential; make Helm use the mcp account

[jgruberf5]

Fixes #187 — follow-up to #184 for the same class of shipped default.

The problem (worse than the issue described)

The seeded mcp service account is role=admin and shipped MCP_SERVICE_PASSWORD = "mcp-service-changeme" — a live admin credential with no rotation gate. And the Helm chart had mcpUsername: admin, so the MCP server logged in as the human admin, not the service account — it only worked because the admin default was also changeme. #184 breaks that outright (the admin password is generated now, and must-change would block it), so this PR also un-breaks MCP on Helm.

The fix

• MCP_SERVICE_PASSWORD defaults to None — no shipped value. It's a shared secret (backend seeds the account; MCP server authenticates with it), so it can't be auto-generated: validate_production fails fast in staging/prod when it's unset or the known default, mirroring JWT_SECRET_KEY/ENCRYPTION_KEY.
• Seed only when configured — startup_steps seeds the mcp account only if MCP_SERVICE_PASSWORD is set; unset → not seeded (MCP unavailable until configured), never a default.
• Helm now uses the service account, consistently — mcpUsername: admin → mcp, mcp-password generated (reused across upgrades), and _helpers.tpl injects MCP_SERVICE_USERNAME/PASSWORD into the backend from the same secret the MCP server reads as BNK_FORGE_USERNAME/PASSWORD. Both resolve to mcp / generated; the mcp account is must_change_password=False so Default admin credential (admin/changeme) is live and API-reachable on every fresh deployment #184's gate doesn't block it.
• Compose — dropped the :-mcp-service-changeme fallback; unset → empty → the MCP server fails clearly and the backend skips seeding.

Testing

Production/staging validation fails when MCP_SERVICE_PASSWORD is unset or the known default, passes when set; ensure_service_user tests already use explicit passwords. helm lint clean, ruff clean, helm template shows backend + MCP server pointing at the same secret keys.

Overlap

Touches helm/…/secrets.yaml, values.yaml, _helpers.tpl — the same generated-secret block as #180 and #184. Merge-time rebase, as already flagged on those.

Fixes #187

https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4

[mwiget]

Good PR, and the discovery buried in it is the valuable part: mcpUsername: admin meant the MCP server was logging in as the human admin all along, and only worked because both defaults happened to be changeme. That would have surfaced as "MCP broke mysteriously" the moment #186 landed. Catching it here rather than there is worth more than the credential fix itself.

Two things need attention before merge. Both are about the upgrade path -- new installs are correctly fixed.

Verified

• ensure_service_user genuinely reconciles rather than only creating: on every startup it rewrites hashed_password, and resets role, is_active and must_change_password=False. So a rotated MCP_SERVICE_PASSWORD propagates, and #186's gate can't block the service account.
• The Helm wiring is symmetric -- _helpers.tpl gives the backend MCP_SERVICE_USERNAME/PASSWORD from mcp-username/mcp-password, and mcp.yaml reads the same two keys as BNK_FORGE_USERNAME/PASSWORD. Same secret, same keys, both sides.
• Your self-review catch is a real latent bug, not a tidy-up: x-backend-env never set MCP_SERVICE_PASSWORD, so a rotated .env value reached only the MCP server. That was silently broken before this PR and would have stayed broken.

Blocking 1 -- every existing Helm install keeps changeme, permanently

$mcpPass falls back to $existing.data["mcp-password"], and on any already-deployed chart that value is changeme -- it was the published default in values.yaml. So:

• new installs: generated password. Fixed.
• existing installs: changeme survives the upgrade, silently.

And unlike the human admin in #186, nothing ever forces it to change: the mcp account is role=admin with must_change_password=False, so #186's rotation gate deliberately does not apply. The result is a permanent, ungated, publicly-documented admin credential on every deployment that already exists -- which is the exact thing #187 is about.

Preserve-on-upgrade is the right default for an operator-chosen secret. This one isn't operator-chosen: it's a generated shared secret whose only consumers are two pods that both read it from this Secret. So it can be rotated safely, and detecting the known default is cheap:

{{- if eq $mcpPass "changeme" -}}{{- $mcpPass = randAlphaNum 24 -}}{{- end -}}

One caveat that has to come with it -- see the note below on checksum/secret. If you'd rather not rotate automatically, then this needs to be loud instead: NOTES.txt and the CHANGELOG telling operators to run --set secrets.mcpPassword=... once. What it can't be is silent.

Blocking 2 -- this is a breaking change and isn't declared as one

validate_production now SystemExits in staging/prod when MCP_SERVICE_PASSWORD is unset. Any existing compose deployment running ENVIRONMENT=production that relied on the shipped default will refuse to start after upgrading. That is the correct behaviour -- failing fast beats booting with a known admin credential -- but neither commit on this branch carries a BREAKING CHANGE footer:

fa5a174 review fix: compose backend must actually receive MCP_SERVICE_PASSWORD   (no BREAKING marker)
c0e4a6e fix: stop shipping the MCP service default; make Helm use the mcp account (#187)   (no BREAKING marker)

scripts/extract-breaking-changes.sh reads exactly that footer, so as it stands the release notes will not mention that a previously-optional variable is now mandatory. Operators find out when the backend won't boot. Given how #177 went, this is worth getting right at the commit rather than patching the notes afterwards:

BREAKING CHANGE: MCP_SERVICE_PASSWORD is now required in staging/production.
Deployments that relied on the shipped default ("mcp-service-changeme") will
exit at startup until it is set to the same value the MCP server receives as
BNK_FORGE_PASSWORD.

Non-blocking

• No checksum/secret annotation anywhere in the chart, so changing a generated secret doesn't roll the pods that consume it. Pre-existing, but it matters more now that mcp-password is generated and shared between two deployments: if a helm upgrade rolls the backend and not the MCP server, the backend re-seeds the account with the new value while MCP still holds the old one, and MCP auth breaks until its pod restarts. This is also the caveat on the auto-rotate suggestion above -- rotating without an annotation opens exactly that window.
• docker compose up no longer yields a working MCP without setting the variable first. Deliberate and documented in .env.example, but worth a line wherever the compose quickstart lives, since "MCP tools return auth errors" is a confusing first-run experience.
• Overlaps #180 (approved), #184/#186 and this PR all in secrets.yaml/values.yaml/_helpers.tpl. You've flagged it; just noting the rebase order will matter, since #180 and #188 make different edits to the same mcp-password block.

[mwiget]

On an existing install this resolves to changeme -- that was the published default in values.yaml, so it is what is sitting in every already-deployed Secret. The upgrade preserves it.

That leaves a role=admin account with a documented password and, unlike the human admin in #186, no gate that will ever force a change: ensure_service_user sets must_change_password=False on every startup, deliberately, so the service account isn't blocked. New installs get a generated value; every existing one silently keeps the credential #187 was filed about.

Preserve-on-upgrade is right for a secret an operator chose. This one is generated, and its only two consumers both read it from this Secret, so it can be rotated without asking anyone:

{{- $mcpPass := .Values.secrets.mcpPassword -}}
{{- if and (not $mcpPass) $existing -}}
{{- $mcpPass = (index $existing.data "mcp-password" | b64dec) -}}
{{- end -}}
{{- /* #187: the published default must not survive an upgrade. */ -}}
{{- if eq $mcpPass "changeme" -}}{{- $mcpPass = randAlphaNum 24 -}}{{- end -}}
{{- if not $mcpPass -}}{{- $mcpPass = randAlphaNum 24 -}}{{- end -}}

With one caveat: no template in this chart carries a checksum/secret annotation, so a Secret change alone doesn't roll the pods. If an upgrade rolls the backend but not the MCP server, the backend re-seeds the account with the rotated value while MCP still holds the old one. Worth adding

annotations:
  checksum/secret: {{ include (print $.Template.BasePath "/secrets.yaml") . | sha256sum }}

to both pod templates alongside the rotation, so the two always move together.

If you'd rather not rotate automatically, the alternative is fine too -- but then it has to be loud in NOTES.txt and the CHANGELOG, because the failure mode of staying quiet is that nobody ever changes it.

[mwiget]

This is the right call -- an unset shared secret should stop the boot rather than fall back to something guessable -- but it changes the contract for deployments that already exist. Anyone running ENVIRONMENT=production on compose today relies on the shipped default and has never set this; after upgrading, the backend exits at startup.

Neither commit on this branch declares it:

fa5a174 review fix: compose backend must actually receive MCP_SERVICE_PASSWORD   (no BREAKING marker)
c0e4a6e fix: stop shipping the MCP service default; make Helm use the mcp account (#187)   (no BREAKING marker)

scripts/extract-breaking-changes.sh matches BREAKING CHANGE in the commit body, so without the footer this won't reach the release notes at all and the version derivation won't see it either. A footer on the squash-merge commit:

BREAKING CHANGE: MCP_SERVICE_PASSWORD is now required in staging/production.
Deployments that relied on the shipped default ("mcp-service-changeme") will
exit at startup until it is set to the same value the MCP server receives as
BNK_FORGE_PASSWORD.

Worth also confirming the check can't fire in development -- validate_production is only called for staging/prod as far as I can see, which is what makes the compose-dev path (unset -> warn, don't seed) work rather than crash.

[jgruberf5]

Both blockers fixed (e67dd31).

Blocking 1 — existing installs kept changeme forever. Added the known-default detection you suggested to secrets.yaml: after $mcpPass resolves from values/existing, {{- if eq $mcpPass "changeme" -}}{{- $mcpPass = randAlphaNum 24 -}}{{- end -}}. New installs generate as before; existing installs on the published default now rotate on the next upgrade. Rationale is in a comment there — generated shared secret, two pod consumers, not operator-chosen, so safe to rotate.

I took the caveat with it rather than leaving it: added a checksum/secret annotation to the api and mcp pod templates, so the backend (which re-seeds the account) and the MCP server (which authenticates with it) roll together when the Secret changes — closing exactly the drift window you described. On a stable upgrade every value is loaded from the existing Secret via lookup, so the checksum is stable and the pods don't roll needlessly.

Blocking 2 — undeclared breaking change. e67dd31 now carries the BREAKING CHANGE: footer, verbatim to your wording (MCP_SERVICE_PASSWORD now required in staging/prod; deployments on the shipped default exit at startup until set). Confirmed scripts/extract-breaking-changes.sh picks it up from this branch, so the release notes will carry it.

Non-blocking noted: the compose-quickstart line and the #180 rebase order on the shared mcp-password block.

[mwiget]

Approving at e67dd317. Both blockers from my last pass are fixed, all 25 checks are green (17 pass, 8 skipped by path filters, 0 pending, 0 failed), and I re-verified the rotation end to end rather than reading it.

Blocker 1 — existing installs keep changeme: fixed. Rendered the chart at this head three ways:

secrets.mcpPassword	rendered mcp-password
unset (default "")	FkRMpTL6nP37tCJuAv0rM3do (generated)
changeme	drC78mncTooJPFjeh14NrTIs (rotated — the known default never survives)
myrealpass	myrealpass (operator value honored)

So the new {{- if eq $mcpPass "changeme" -}} line catches exactly the case it needs to — the value that arrives from lookup on every already-installed chart — and leaves a real operator-chosen password alone.

Blocker 2 — the two pods drifting apart: fixed, and the rotation actually lands. I traced the whole chain rather than trusting the annotation alone:

1. Helm rotates the Secret;
2. checksum/secret on both api.yaml and mcp.yaml changes → both roll together;
3. seed_auth_step() calls ensure_service_user(...) on every backend start, and its else-branch reconciles hashed_password to the current env var unconditionally — so the DB hash follows the rotated Secret without any manual step;
4. the MCP server reads BNK_FORGE_PASSWORD from that same Secret, so both sides land on the same value.

Point 3 is the part that makes this safe: without an unconditional reconcile, a rotated Secret would lock the MCP account out permanently. It's there, it's idempotent, and its docstring says why.

Also confirmed the MCP_SERVICE_PASSWORD unset path degrades honestly — a warning plus an unseeded account (MCP unavailable), not a shipped default — and that the BREAKING CHANGE: footer is in the commit body, so extract-breaking-changes.sh will pick it up and the release will be a major.

Two non-blocking notes, neither worth another round:

• Transient rollout window. During helm upgrade the new MCP pod can come up with the rotated password while the old api pod is still serving the old hash, so MCP auth fails until the new backend finishes seed_auth_step(). Self-healing on reconnect, and the alternative (ordering hooks) costs more than it buys — just be aware the MCP server may log auth failures for a few seconds mid-upgrade.
• secrets.mcpPassword: changeme set deliberately (a lab reproducing the old default) is silently replaced rather than rejected. Consistent, since both consumers read the same Secret — but it is the one input the chart ignores.

[mwiget]

Re-approving at ba780490 — 25/25 green. This covers 4de8884 and ba780490, which landed after my approval of e67dd317.

Extending the scheme to dist/ was the right call — the customer package was the one surface still shipping admin / changeme as a live MCP credential, which is the actual exposure #187 is about. All three compose files are wired consistently (MCP_SERVICE_USERNAME/MCP_SERVICE_PASSWORD on the backend, BNK_FORGE_USERNAME/BNK_FORGE_PASSWORD on the MCP server, same variable behind both), and folding the ibm_cloud_bnk_forge.sh admin-login line into this PR rather than #186 is the right instinct — that file has an MCP edit here too, so splitting it would have conflicted across branches.

I checked the thing that would have made this a blocker: whether an empty MCP_PASSWORD now bricks a customer install. It doesn't. validate_production refuses an unset MCP_SERVICE_PASSWORD, but it returns early unless ENVIRONMENT is staging/production (config.py:193), and dist/docker-compose.yml sets no ENVIRONMENT — so the default development applies. A customer who skips the variable gets a booting stack, the seed_auth_step warning, and an MCP server that can't authenticate until they set it. Visible in the logs, recoverable by editing .env and restarting, and no silent live credential. That's the right failure shape for a customer package.

Two non-blocking notes:

1. The same secret now has two names across the two packages. Root .env.example documents MCP_SERVICE_USERNAME / MCP_SERVICE_PASSWORD (lines 87-88) and root compose reads exactly those; dist/.env.example uses MCP_USERNAME / MCP_PASSWORD and dist compose reads those. Each package is internally consistent, so nothing is broken — but an operator who follows the repo docs while editing a dist/.env will set MCP_SERVICE_PASSWORD, dist compose will ignore it, and MCP stays unseeded with only a log line to explain why. If the divergence is deliberate (not breaking existing customer .env files), a one-line pointer in dist/.env.example — "called MCP_SERVICE_PASSWORD in the source repo" — would cost nothing.

2. #183's install guide is now slightly stale on this variable. It says MCP_PASSWORD — "Change from the default." After this PR there is no default to change; it must be set, and leaving it empty disables MCP rather than leaving a weak credential — a different instruction to the reader. The same callout also says these values "cannot be changed easily after the stack first starts", which is now untrue for this one: ensure_service_user reconciles the hash on every boot, so changing MCP_PASSWORD and restarting just works. #183 is already approved and green, so this is a small follow-up on whichever branch is convenient.

[jgruberf5]

Both of your non-blocking notes addressed:

1. Var-name divergence — added a pointer in dist/.env.example noting these are MCP_SERVICE_USERNAME / MCP_SERVICE_PASSWORD in the source repo (kept the short dist names to avoid breaking existing customer .env files).
2. Fix registry/cosign/upgrade metadata + USER-gate authoring guide — PR #177 Majors #183 install guide — updated on that branch: the MCP_PASSWORD row now says "set a value, no default ships, empty disables MCP", and the "cannot be changed easily" callout is split so it applies only to postgres/redis, with MCP called out as set-required-but-rotatable (reconciled each boot).

Also added a Quick-Start note in README.md that MCP needs MCP_SERVICE_PASSWORD set (the "MCP tools return auth errors" first-run you flagged), placed away from #186's Login-table edit so the two merge cleanly.

[mwiget]

Re-approving at 770c4c48 — 25/25 green. Both of my non-blocking notes are closed.

795c651 adds the var-name mapping to dist/.env.example, which is the case I was worried about: an operator reading the source repo's docs sets MCP_SERVICE_PASSWORD in a dist/.env, the dist compose reads MCP_PASSWORD, and MCP stays unseeded with only a log line to explain it. The note says both names and why they differ (existing customer .env files keep working), so the divergence is now documented intent rather than a trap.

770c4c48 goes past what I raised. The README's quick-start had no mention of the MCP credential at all, so a developer following it got a stack where MCP tools return auth errors with nothing pointing at the cause. Stating that the stack still comes up and only MCP is affected is the useful half — that's the difference between "did my install fail?" and "I skipped one variable."

Everything from the earlier passes stands: the changeme rotation verified across three renders, the ensure_service_user reconcile chain that makes rotation land on both sides, the checksum/secret annotations rolling the two pods together, and dist/ deployments defaulting to ENVIRONMENT=development so a missing MCP_PASSWORD degrades rather than refusing to boot.

Worth noting for sequencing: #183's install guide now documents this PR's scheme (and #186's), so #183 wants both merged ahead of it. All three are approved and green.

[bonnyr-f5]

Review: BLOCK

Reviewed under the review-discipline pipeline at 770c4c48 (base 4a52ed45): invariant sweep, a context-isolated cold audit with pytest probes against the real db fixture, plus a cross-PR sweep over all seven open PRs in this series. The upgrade question you'd expect to be the crux was chased to a definite answer, and there is a second, worse one next to it.

Verified correct — the shared-secret consistency

This is the part the PR gets right, and it's the part that's easy to get wrong: all four compose renders agree (MCP_SERVICE_* on backend/worker/beat, BNK_FORGE_* on mcp, same source var), and Helm is genuinely consistent — api/worker/beat and mcp all resolve to rel-bnk-forge-secrets:mcp-username/mcp-password. The MCP server hardcodes nothing (mcp-server config defaults to mcp). mcpUsername: admin → mcp is the right call.

Blocker — on two shipped install paths, the reconcile is pointed at the human admin row

ensure_service_user reconciles by username, and the username var isn't namespaced:

scripts/ibm_cloud_bnk_forge.sh:177   MCP_USERNAME=admin          (untouched by this PR)
scripts/ibm_cloud_bnk_forge.sh:385   MCP_SERVICE_USERNAME: ${MCP_USERNAME:-mcp}   → "admin"

docker compose config on the script's own compose heredoc plus its own .env yields backend {'MCP_SERVICE_USERNAME': 'admin', 'MCP_SERVICE_PASSWORD': '<openssl rand -hex 24>'}. auth_service.py:157-186 then rewrites that row. pytest probe against the real db fixture:

must_change_password: False (was True)
hash changed: True
old DEFAULT_ADMIN_PASSWORD still works: False
MCP secret logs in as admin: True

So on a fresh IBM Cloud install the admin password silently becomes the MCP secret — and the banner this PR edits at :705 tells the operator to log in with DEFAULT_ADMIN_PASSWORD / /app/keys/initial_admin_password. Neither works.

The dist/ path is worse, because the description's own compatibility argument is what triggers it. Existing customer .env files came from the previous dist/.env.example, which shipped MCP_USERNAME=admin / MCP_PASSWORD=changeme. Proven:

docker compose --env-file <old .env> -f dist/docker-compose.yml config
→ backend {'MCP_SERVICE_USERNAME': 'admin', 'MCP_SERVICE_PASSWORD': 'changeme'}

That re-creates the publicly-known default admin credential #186 exists to remove, with the forced-change gate disabled — because ensure_service_user sets must_change_password=False. Same hole on Helm if values pin the chart's former mcpUsername: admin (reproduced via helm template --set secrets.mcpUsername=admin).

Class fix: ensure_service_user must refuse a username it did not create as a service account (or the service username must not be derivable from a var whose shipped value is admin). Renaming the var alone isn't enough — already-distributed .env files are the population at risk.

Blocker — the upgrade path: the account is left pre-existing and the old default still authenticates

startup_steps.py:232-243 wraps ensure_service_user in if settings.MCP_SERVICE_PASSWORD:. Unset on upgrade → the branch is skipped → the pre-existing row is never rotated, disabled or deleted. No migration does it either (grep -rln mcp backend/alembic/versions/ → nothing).

Reached independently two ways. By inspection: staging's config.py:103 ships MCP_SERVICE_PASSWORD = "mcp-service-changeme" and seed_auth_step reconciles unconditionally, so every existing deployment holds mcp with that hash. And by pytest probe (pre-seeded mcp row + MCP_SERVICE_PASSWORD="" → seed_auth_step()):

is_active: True   role: admin   must_change_password: False
SHIPPED DEFAULT STILL VALID: True
authenticate_user() succeeds: True

So "unset → not seeded, never a default" is true for fresh installs only — and upgrades are exactly the population that has the default. secrets.yaml:36 already does the analogous rotation for the chart secret; the DB side needs the equivalent. The fix belongs on the unset branch (deactivate or randomize), not the set one.

Blocker — .env.example:88 ships a new known credential

# MCP_SERVICE_PASSWORD=   # shared secret, no shipped default (#187); same value on backend + MCP server

Compose does not strip that inline comment. I reproduced it directly:

MCP_SERVICE_PASSWORD: '# shared secret, no shipped default (#187); same value on backend + MCP server'

Uncommented verbatim — the obvious thing to do with a commented example — the password becomes the literal comment text on both sides, so MCP works, nothing looks wrong, and validate_production accepts it (non-empty, and not equal to the one literal it checks). Move the prose to its own line.

Major

• The fail-fast fires in zero shipped deployments. grep -rn ENVIRONMENT dist/ docker-compose*.yml scripts/ibm_cloud_bnk_forge.sh → none, so every compose path is development and validate_production returns at config.py:193. The only path setting ENVIRONMENT=production is Helm (values.yaml:94), where the chart always generates a non-empty mcp-password — so unreachable there too. Ten-permutation run confirms.
• The default check is one literal. Only mcp-service-changeme. The actually shipped default was changeme (dist/.env.example, helm values.yaml, ${MCP_PASSWORD:-changeme} in the ibm script, docs/E2E-CRITICAL-004:91) — and it passes: ENVIRONMENT=production, MCP=changeme → BOOTS, rc=0.
• The healthcheck reports HEALTHY on the new default. has_credentials is False with an empty password, so probe() returns 0. A default make deploy yields a green bnk-forge-mcp container where every tool call 401s — falsifying "the MCP server fails clearly".
• role=admin blast radius, unchanged: mint further admins, the whole /api/system router (POST /upgrade, /restore, /containers/restart), snapshot restore, PUT /api/settings, release sources.
• CI cannot catch any of this. mcp-tests is the only job absent from ci-gate.needs, and its path filter (mcp / ci) matches nothing this PR touches — so the PR that changes how MCP authenticates gets no MCP coverage and couldn't be blocked by it anyway. No CI job runs helm lint or helm template, and there is no helm/bnk-forge/tests/: 7 of 17 changed files have no gate at all.

Minor

checksum/secret isn't a checksum of the applied Secret — api and mcp get different values from the same render (randAlphaNum re-runs), stable only when nothing is generated. Plus a stale-doc set now that "no auth drift possible" is false: docs/DEPLOYMENT.md:250,255,343, docs/E2E-CRITICAL-004:90, mcp-server/README.md:61,98,106, Makefile:527,551, scripts/mcp_live_smoke.py:232, user-pack/install-guide.html:217-226, .env.example:73,77-79.

Not covered

seed_auth_step has no test at all — neither new branch. ensure_service_user is only ever called with username="mcp", so the admin-collision branch above has no fixture. No fixture for the upgrade state, for MCP_SERVICE_PASSWORD="" (what compose actually emits), or for "changeme".

Merge order — please rebase onto #186 rather than merging independently

The pair conflicts in five files: .env.example, helm/bnk-forge/templates/_helpers.tpl, helm/bnk-forge/templates/secrets.yaml, helm/bnk-forge/values.yaml, scripts/ibm_cloud_bnk_forge.sh. config.py, startup_steps.py, api/mcp.yaml and the tests auto-merge cleanly and correctly (merged-tree pytest: 58 passed).

Two of the resolutions are booby-trapped, which is why this shouldn't be resolved at the merge button:

• --theirs on values.yaml deletes secrets.adminPassword while the auto-merged _helpers.tpl still references it. helm lint says "0 chart(s) failed" and helm template exits 0 — yet api/beat/worker all carry DEFAULT_ADMIN_PASSWORD → admin-password, a key absent from the rendered Secret → CreateContainerConfigError, release dead.
• --ours instead renders mcp-username: "admin" / mcp-password: "changeme".
• A union of .env.example re-emits both removed strings.

A hand-written union of the variable blocks is correct and was verified (dangling secret refs: NONE). After resolving, please prove it with helm template on the merged tree: api and mcp must receive the same MCP key, and a distinct admin one.

[jgruberf5]

@bonnyr-f5 — reproduced every finding; all real, all fixed in b4eac59e. The two takeover paths were the important ones and you're right on both.

Blockers — fixed

• ensure_service_user reconciled the human admin row. Confirmed: MCP_USERNAME=admin (old .env default, and ibm_cloud_bnk_forge.sh:177) → MCP_SERVICE_USERNAME=admin → the reconcile rewrote admin's hash and cleared must_change_password, re-creating the publicly-known admin with Stop shipping a default admin credential; generate it and enforce rotation #186's gate off. Class fix: ensure_service_user now refuses a reserved human username (raises; startup logs it loud, non-fatal), and I fixed ibm_cloud MCP_USERNAME=admin → mcp. Test: reconciling admin raises and leaves the hash + gate intact.
• Upgrade with MCP_SERVICE_PASSWORD unset left the old mcp/mcp-service-changeme account authenticating (reconcile only ran on the set branch). Added disable_stale_service_user on the unset branch — deactivates the stale account until a real password re-seeds it. Never touches a reserved name. Tested.
• .env.example shipped a live credential — the inline comment on MCP_SERVICE_PASSWORD= becomes the password when uncommented, and validate_production accepts it. Moved the prose to its own line. Nasty one; good catch.

Major — fixed

• The default check was one literal. You're right that the actually-shipped default was changeme (dist/helm/scripts), not just mcp-service-changeme. validate_production now rejects both.

Acknowledged / cross-PR

• fail-fast fires in zero shipped deployments (ENVIRONMENT never production on compose; Helm always generates) and the healthcheck reports HEALTHY on an empty password — both valid. The role=admin blast-radius is contained by Stop shipping a default admin credential; generate it and enforce rotation #186's middleware gate + admin rotation (fixed there). I'll file the compose ENVIRONMENT/healthcheck items as a follow-up rather than expand this PR further.
• checksum/secret non-determinism and the stale-doc set (DEPLOYMENT.md, mcp-server/README, Makefile, etc.) — valid; tracking as a docs-sweep follow-up.
• Merge order — agreed: rebase Stop shipping the MCP service default credential; make Helm use the mcp account #188 onto Stop shipping a default admin credential; generate it and enforce rotation #186; the five-file overlap needs the hand-written union you describe, not a merge-button --ours/--theirs. I'll do that at merge time and prove it with helm template (api+mcp same MCP key, distinct admin key).

Nothing here was wrong — thank you for the depth.

[bonnyr-f5]

Review: BLOCK

Round 2, cold re-audit of b4eac59 against origin/staging 4a52ed4.

The takeover I flagged in round 1 is closed at the value level and I verified it directly:
scripts/ibm_cloud_bnk_forge.sh:177 is now MCP_USERNAME=mcp, dist/.env.example ships
MCP_USERNAME=mcp with an empty password, and ensure_service_user now refuses a reserved name.
Defence in depth — the default moved and the code refuses the dangerous value. Also verified good:
no secret logging, no literal default, bcrypt compare, the heredoc correctly single-quoted, and
INV-13 upheld (mcp-password has been in the chart since its first version).

But the refusal is a denylist of one name, and the mutation it guards is wider than the hash.

BLOCKER 1 — INV-18: a one-name denylist, guarding a mutation that escalates privilege

backend/services/auth_service.py:162:

_RESERVED_HUMAN_USERNAMES = frozenset({"admin"})

and the reconcile branch at :196-199:

user.hashed_password = hash_password(password)
user.must_change_password = False
user.role = role          # default "admin"
user.is_active = True

Point MCP_SERVICE_USERNAME at any human account that isn't literally named admin — operator, or
any named user — and on the next boot that account's password is overwritten with the MCP shared
secret, it is promoted to role=admin, its must-change gate is cleared, and it is re-activated
even if someone deliberately disabled it. Round 1's finding was about admin because admin was
the value in the shipped .env; the class is "any existing human row".

disable_stale_service_user:215 has the mirror-image bug: it deactivates any existing account whose
name matches and isn't admin — an operator lockout triggered by a config typo.

Class fix: stop identifying service accounts by name. Gate on a property of the row — a
service-account flag, or provenance recorded at creation — and refuse any pre-existing row the seeder
did not create. Separately, narrow the mutation to what the stated purpose needs: keeping the hash in
sync does not require writing role or is_active, and a reconcile should never widen privilege.

BLOCKER 2 — INV-11: the rotation branch never runs for the shipped dist/IBM population

backend/startup_steps.py:233. The disable_stale_service_user path is gated on
MCP_SERVICE_PASSWORD being falsy. But dist/docker-compose.yml:38 maps:

MCP_SERVICE_PASSWORD: ${MCP_PASSWORD:-}

and every existing dist/IBM install has MCP_PASSWORD=changeme on disk from staging's
dist/.env.example:30. That's truthy, so the if branch runs instead: ensure_service_user
reconciles the pre-existing mcp / role=admin row to the hash of changeme — a known, published
default — and it keeps authenticating after the upgrade. The stale-account rotation this PR adds is
unreachable for exactly the population that needs it.

Class fix: treat a known-default value as equivalent to unset on the rotation path, not just in
validate_production. The set of known defaults is already enumerated at config.py:213 — share it.

BLOCKER 3 — the fail-closed boot change has no upgrade note

backend/core/config.py:213-218 appends an issue when MCP_SERVICE_PASSWORD is unset or in
("mcp-service-changeme", "changeme"), and validate_production ends in raise SystemExit(1)
(:243). Trigger is ENVIRONMENT in ("staging","production"), which is what docs/DEPLOYMENT.md:336
instructs.

Failing closed on a shipped credential is the right call — the defect is that nothing tells the
operator. Every existing staging/production install has one of those exact values, so they all stop
booting on upgrade, and no PR in this series (#183 owns CHANGELOG.md) documents it. Note this also
contradicts README.md:91-94, added by this same PR.

Compounding, same site: the FATAL block at :238-241 prints copy-paste remediation for
JWT_SECRET_KEY and ENCRYPTION_KEY only — the one variable the operator actually tripped on is
missing from the block that claims to tell them how to fix it.

Major — the IBM banner cites a file that doesn't exist in this tree

scripts/ibm_cloud_bnk_forge.sh:708 now points at /app/keys/initial_admin_password. That path
exists only in #186 (git grep -l initial_admin_password origin/staging → nothing). Until #186 lands,
the real credential is admin/changeme on a public-IP VSI, and this PR deleted the warning that
disclosed it. Either merge after #186 or keep the accurate warning until then.

Minors

• INV-17 — docs/DEPLOYMENT.md:250-256,343, Makefile:527,551, mcp-server/README.md:61,98,106, scripts/mcp_live_smoke.py:147,231, docs/E2E-CRITICAL-004_MCP_SANITY.md:90, user-pack/install-guide.html:217 still instruct MCP_USERNAME=admin / MCP_PASSWORD=<admin password> — the exact input that triggers BLOCKER 1 — and those names are read by nothing in either root compose file.
• helm/bnk-forge/templates/api.yaml:32, mcp.yaml:33 — checksum/secret re-renders a template containing five randAlphaNum calls, so it's non-deterministic (two different hashes in one render) rather than a checksum of the applied Secret. worker.yaml/beat.yaml consume the same Secret and got no annotation at all.
• mcp-server/src/bnk_forge_mcp/healthcheck.py:36-40 — with the new empty-password default has_credentials is False, the probe exits 0, and the container reports HEALTHY while every tool call 401s. Contradicts docker-compose.yml:478-481.

Tests

All five new tests fail on revert, which is the right property. But none of them reach
startup_steps.py:230-252, none cover the role / is_active mutation in the reconcile branch, and
the production-default test omits "changeme" — the value the dist population actually carries, and
the one in BLOCKER 2.

Integration

git merge-tree pr-186-r2 pr-188-r2 reports content conflicts in 9 files (both Helm templates,
three compose files, .env.example, ibm_cloud_bnk_forge.sh, and the shared
test_auth_service.py). Both PRs' suites pass on their own trees; no tree containing both has ever
been built or tested. Please resolve once on an integration branch and run the auth suites +
helm template + docker compose config against that tree.

Review Assessment

• Verdict: BLOCK
• Audit SHA: b4eac59
• Cold Audit Performed: Yes
• Invariants Verified: INV-4 (violated), INV-9 (upheld), INV-11 (violated for dist/IBM), INV-12 (violated), INV-13 (upheld), INV-16 (violated), INV-17 (code upheld, docs violated), INV-18 (violated), INV-23 (violated)
• Git & Harness Cleanliness: Clean

Findings & Action Items

• Major (Blockers):
  • backend/services/auth_service.py:162,196-199: identify service accounts by a row property, not a name; stop writing role/is_active on reconcile
  • backend/services/auth_service.py:215: same class in disable_stale_service_user (operator lockout)
  • backend/startup_steps.py:233: treat known defaults as unset on the rotation path — dist/IBM carry a truthy changeme
  • backend/core/config.py:213-218: add the upgrade note (and add the var to the remediation block at :238-241)
  • scripts/ibm_cloud_bnk_forge.sh:708: cites a Stop shipping a default admin credential; generate it and enforce rotation #186-only path; keep the accurate warning until Stop shipping a default admin credential; generate it and enforce rotation #186 lands
  • Integrate with Stop shipping a default admin credential; generate it and enforce rotation #186 on one branch — 9-file conflict on the credential surface
• Minor (Non-blocking):
  • INV-17 doc sweep: 6 files still instruct MCP_USERNAME=admin
  • helm/.../api.yaml:32, mcp.yaml:33: non-deterministic checksum/secret; worker/beat unannotated
  • mcp-server/.../healthcheck.py:36-40: HEALTHY while every call 401s
• Nits:
  • none beyond the above

[jgruberf5]

@bonnyr-f5 round 2 — the one-name denylist was exactly as shallow as you said. Fixed on the class in 1e1118f4.

• Property-based provenance: added users.is_service_account (model + migration v2_154). ensure_service_user records it at creation and refuses to reconcile any pre-existing row it didn't create — name-independent. Test: pointing it at a real operator now raises and leaves its role + password intact.
• Narrowed the mutation: reconcile writes only the hash + must_change, never role/is_active — a reconcile can't widen privilege or re-activate a disabled account.
• Known-default rotation: changeme is truthy, so the old code re-seeded it every dist/IBM upgrade. A known default is now treated as unset on the rotation path too (shared MCP_KNOWN_DEFAULT_PASSWORDS).
• FATAL block now prints the MCP_SERVICE_PASSWORD remediation; healthcheck fails (not HEALTHY) with no credentials. The upgrade note landed on Fix registry/cosign/upgrade metadata + USER-gate authoring guide — PR #177 Majors #183's CHANGELOG.

[jgruberf5]

@bonnyr-f5 Round-3 BLOCK addressed. You were right on all four blockers — one root cause (v2_154 never backfilled the provenance column), plus the disable/reconcile contradiction. Here's the fix, the reproduction, and the backfill-scope reasoning.

Root cause & fix

v2_154 added users.is_service_account with server_default false and zero backfill, so every pre-existing row — including the mcp row every deployed install carries — was classified False, and both consumers gate on it. upgrade() now backfills it.

Backfill scope (deliberately narrow — the justification)

UPDATE users SET is_service_account = true
WHERE username = 'mcp' AND email = 'mcp@bnk-forge.local'

• username = 'mcp' — the only value the legacy service account was ever created under. MCP_SERVICE_USERNAME defaults to 'mcp', and a migration can't know an operator's override at apply time. Critically, backfilling the configured name would be dangerous: some legacy .env files point that var at admin (your INV-12), so backfilling the configured username would reclassify the human admin as a service account — the exact takeover Stop shipping the MCP service default credential; make Helm use the mcp account #188 exists to prevent. So we backfill the known legacy default only.
• email = 'mcp@bnk-forge.local' — the second signal you asked me to consider. create_user synthesised the service email as f"{username}@bnk-forge.local", so the legacy mcp row provably carries this address. Requiring it means a real human who merely happens to be named mcp (with any real email) is left untouched, at zero cost to genuine installs.

The pair (username + synthesised email) is the creation fingerprint and can't collide with a human provisioned through normal signup. Documented edge: an operator who set a custom MCP_SERVICE_USERNAME before upgrading isn't backfilled — reclassifying an operator-named row we can't prove we created is strictly worse than requiring a one-line rename. downgrade() drops the column, reversing the backfill with it.

Reproduction (isolated py3.11 venv, alembic Operations against SQLite)

OLD migration — the stale row survives (your BLOCKER 1/2):

=== after v2_154 upgrade (no backfill) ===
   {'username': 'admin', 'email': 'admin@corp.com',       'is_service_account': 0}
   {'username': 'alice', 'email': 'alice@corp.com',       'is_service_account': 0}
   {'username': 'mcp',   'email': 'mcp@bnk-forge.local',  'is_service_account': 0}   <-- stale mcp NOT classified
   {'username': 'mcp2',  'email': 'mcp@real-human.com',   'is_service_account': 0}

FIXED migration — only the provable service row flips:

   {'username': 'admin', 'email': 'admin@corp.com',       'is_service_account': 0}   human admin untouched
   {'username': 'alice', 'email': 'alice@corp.com',       'is_service_account': 0}
   {'username': 'mcp',   'email': 'mcp@bnk-forge.local',  'is_service_account': 1}   <-- backfilled
   {'username': 'mcp2',  'email': 'mcp@real-human.com',   'is_service_account': 0}   human named 'mcp' untouched
=== after downgrade, is_service_account present? === False

Full trap table — resolved end-to-end (real auth_service + backfill)

--- upgraded, is_service_account NOT yet backfilled (False) ---
[old default login]               : SUCCESS (role=admin, active=True)     <-- BLOCKER 1 reproduced
[disable_stale ran] mcp.is_active = True                                  <-- stale row NOT disabled
[old default login after disable] : SUCCESS (role=admin, active=True)

--- APPLY BACKFILL ---  mcp.is_service_account = True

--- TRAP ROW 1: MCP_SERVICE_PASSWORD unset / known default ---
after disable_stale: mcp.is_active = False
[old default login]               : DENIED (Account is disabled)          <-- #187 now closed

--- TRAP ROW 2: operator sets a REAL MCP_SERVICE_PASSWORD ---
after ensure_service_user: is_active = True  is_service_account = True     <-- reconciled AND re-activated
[real secret login]               : SUCCESS (role=admin, active=True)
[old default login]               : DENIED (Invalid username or password)

MCP_SERVICE_PASSWORD on upgrade	before	after
unset / known default	stale row authenticates as admin	disabled — old default rejected
set to a real secret	reconcile refuses forever, MCP dead	reconciled + re-activated, MCP works

BLOCKER 3 (docstring vs code)

ensure_service_user's reconcile branch now sets user.is_active = True. The provenance guard just above already proved the row is our service account, so re-activation is safe and it's exactly what disable_stale_service_user's docstring promised ("re-seeds and re-activates it"). Without it a stale→reconciled row stayed is_active=False and hit Account is disabled despite a correct password. Role is still left untouched — no privilege widening.

Other items in this PR's scope

• Healthcheck inverted row: probe() now returns healthy (and never touches the backend) whenever a bearer token is set, even alongside a stale password — a token is a self-sufficient auth path. New test covers token+password.
• worker.yaml / beat.yaml: added the checksum/secret annotation so they roll on the changeme→random rotation like api/mcp (verified via helm template).
• Model drift nit: User.is_service_account now carries server_default=false() to match the migration.

Verification

• alembic heads → single head v2_154; v2_154 upgrade→downgrade→upgrade round-trips cleanly (SQLite, Operations).
• 84 passed — test_auth_service, test_migrations, test_startup_steps, test_auth_service_pure (incl. 3 new: backfill scope+roundtrip, reconcile-reactivates, and the full trap-table assertions).
• 12 passed — mcp-server test_healthcheck (incl. token+stale-password).
• ruff clean on all changed files.

Cross-PR notes (not fixed here — for the integration branch)

These are the shared credential surface with #186 / cross-PR ordering; flagging, not touching, per the integration plan:

• INV-12 dist/ legacy .env → MCP as admin/changeme and INV-23 ibm_cloud_bnk_forge.sh:708 initial_admin_password both live in the #186 credential surface (the 9-file git merge-tree conflict set). They need resolving once on the merge-integration branch, not divergently here.
• Healthcheck wired into only the dev compose (dist/, ibm_cloud_bnk_forge.sh, Helm tcpSocket) — those files are in the same conflict set; wiring belongs on the integrated tree.
• docs/DEPLOYMENT.md boot-halt note lives in Fix registry/cosign/upgrade metadata + USER-gate authoring guide — PR #177 Majors #183, which merges last — move that CHANGELOG hunk forward or merge Fix registry/cosign/upgrade metadata + USER-gate authoring guide — PR #177 Majors #183 together, as you noted.

[bonnyr-f5]

Review: BLOCK

Round 4, cold re-audit of cd521ca against origin/staging 4a52ed4 — whole diff (26 files,
+577/-41, 11 commits), fresh context, re-audited as new code. All execution ran in git archive
sandboxes; the shared checkout was untouched.

BLOCKER 1 — the stale-credential disable is a no-op on the shipped dist upgrade path (INV-11)

backend/startup_steps.py:252 calls disable_stale_service_user(db, settings.MCP_SERVICE_USERNAME),
and auth_service.py returns immediately when that username is in _RESERVED_HUMAN_USERNAMES. On an
install upgrading from the base .env, dist/docker-compose.yml:37 resolves
MCP_SERVICE_USERNAME: ${MCP_USERNAME:-mcp} to admin (the base dist/.env.example shipped
MCP_USERNAME=admin), so disable_stale_service_user(db, "admin") early-returns and the pre-existing
mcp/mcp-service-changeme service row (role=admin, active, must_change=False) is never touched.
Execution-proven against the real seed_auth_step(): mcp/mcp-service-changeme STILL AUTHENTICATES: role=admin active=True, both with MCP_SERVICE_PASSWORD unset and set. This is 100%
of the upgrading dist/IBM population. The remediation must resolve the target from the legacy row's
identity, not from a settings value that upgrades to a reserved name.

BLOCKER 2 — migration backfill appended after the revision id already shipped (INV-7)

v2_154_user_is_service_account.py: the revision id was introduced in commit 1e1118f, and the
op.execute backfill (:52-59) was appended later in cd521ca. Any environment that ran the
branch at the earlier commit already has alembic_version = v2_154 and will never get the
backfill — an applied revision is immutable. Proven via git history + a probe: a pre-existing
service row under a custom username is skipped by the is_service_account IS TRUE filter and then
permanently refused by ensure_service_user (ValueError forever). Bounded: v2_154 is absent from
staging/main/all 15 tags, so the blast radius is dev/QA today — but the fix is to cut a new
revision that backfills, not to edit v2_154.

Major findings (all execution-proven unless noted)

• auth_service.py:237 / routes/auth.py:358 — disable flips is_active only; the hash stays
  bcrypt("mcp-service-changeme"), and PUT /api/auth/users/{id} has no service-account exclusion,
  so re-enabling restores a login with the published default (probe proven).
• healthcheck.py:47-53 (INV-10) — the new "no creds → unhealthy" signal has exactly one
  consumer (docker-compose.yml:482); zero in docker-compose.local.yml, both dist/ compose
  files, and the IBM compose; Helm uses tcpSocket. dist/.env.example:36 is where the empty
  default actually ships, and dist/README.md never mentions MCP_PASSWORD.
• helm/.../secrets.yaml:36 — eq $mcpPass "changeme" fires on the values-supplied value, so an
  operator whose values file carries the chart's own old default gets a new random password on
  every render: two helm template runs gave different passwords and 4 mutually-different
  checksum/secret values → perpetual GitOps drift + api/worker/beat/mcp roll on every sync.
• docs/DEPLOYMENT.md:255-256,343 (UNPROVEN — doc read) — the file config.py:245 points
  operators to still says MCP_USERNAME=admin/MCP_PASSWORD=<current-admin-password>, the exact
  input that triggers BLOCKER 1 and now hard-refuses; MCP_SERVICE_PASSWORD appears nowhere in it.

Minors (selected)

• git merge-tree <#186> cd521ca → rc=1, 8 content conflicts incl. helm/.../secrets.yaml and
  values.yaml; a resolution taking Stop shipping a default admin credential; generate it and enforce rotation #186's trailing block silently deletes Stop shipping the MCP service default credential; make Helm use the mcp account #188's rotation (lines
  28-37) with no test failing.
• docs/E2E-CRITICAL-004_MCP_SANITY.md:90 — the diff fixed line 91 and left MCP_USERNAME | admin
  on line 90; same half-update in mcp-server/README.md and Makefile:527/551.
• secrets.yaml:29-31 — unguarded index $existing.data "mcp-password" (key predates
  MIN_UPGRADE_FROM so INV-13 holds for supported upgrades; Stop shipping a default admin credential; generate it and enforce rotation #186 adds a hasKey guard 20 lines up).

Genuinely fixed, and verified

INV-16: all four new test groups map to real CI jobs and pass (26 + 32 + mcp). INV-9: make shellcheck identical at both refs, no new findings. The has_token short-circuit is genuine
(config.py:32-34), so the MagicMock tests are not vacuous.

Cross-PR (BLOCK-class)

The 9-file credential conflict with #186 (CROSS-1) is the series' central integration wall — see the
#186 comment. Neither #186 nor #188 alone closes issue #187: #186 flags the mcp default as out of
scope, #188's remediation misses it on the dist path (BLOCKER 1), and #183's CHANGELOG advertises a
boot-refusal that only #188 implements. Land #186 + #188 on one integration branch, resolve the
conflicts once, fix the MCP_SERVICE_USERNAME=${MCP_USERNAME}=admin collision, then run the auth
suites + helm template + docker compose config over all five compose files against the merged tree.

Review Assessment

• Verdict: BLOCK
• Audit SHA: cd521ca
• Cold Audit Performed: Yes
• Invariants Verified: INV-7, INV-10, INV-11, INV-12, INV-23, INV-25, INV-26 VIOLATED; INV-4 (merge) VIOLATED; INV-9, INV-13 (caveat), INV-16, INV-20 UPHELD
• Git & Harness Cleanliness: Clean

Findings & Action Items

• Major (Blockers):
  • startup_steps.py:252: disable keys on MCP_SERVICE_USERNAME(=admin on dist upgrade) → no-op; legacy default row keeps authenticating (INV-11)
  • v2_154…py:52-59: backfill appended after the revision id shipped → immutable-migration hole (INV-7); cut a new revision
• Minor (Non-blocking): re-enable restores default hash; healthcheck signal has one consumer; secrets.yaml:36 non-idempotent render; DEPLOYMENT.md triggers BLOCKER 1; Stop shipping a default admin credential; generate it and enforce rotation #186↔Stop shipping the MCP service default credential; make Helm use the mcp account #188 8-file conflict; half-updated docs
• Nits: validate_production MCP check can't fire on any shipped surface; MCP service account still role=admin

[jgruberf5]

@bonnyr-f5 — round-4 BLOCKERS addressed. Both reproduced first, fixed, then verified with a py3.11 venv (uv, CPython 3.11.15) running alembic + pytest. Head is now e86b8757.

BLOCKER 1 (INV-11) — stale-credential disable was a no-op on the dist upgrade path

Reproduced against the real disable_stale_service_user. On the dist upgrade path MCP_SERVICE_USERNAME resolves from the pre-existing .env (MCP_USERNAME=admin) to admin, so disable_stale_service_user(db, "admin") hit the reserved-username early-return and the legacy mcp/mcp-service-changeme row survived:

mcp.is_active     = True   (BUG -> default keeps authenticating)
admin.is_active   = True
AUTH mcp/mcp-service-changeme -> role=admin active=True  <-- shipped default STILL AUTHENTICATES
RESULT: BUG REPRODUCED - stale mcp row NOT disabled

Fix (auth_service.py / startup_steps.py): key the disable on provenance, not the configured username. disable_stale_service_user(db) now deactivates every active row where is_service_account IS TRUE, independent of MCP_SERVICE_USERNAME. The provenance flag is never set on a human row, so the reserved-name guard is not just unnecessary — it was exactly what made this a no-op — and is removed here. Startup calls it with no username at all.

Verified (same probe, after fix):

Disabled stale MCP service account 'mcp' — no usable MCP_SERVICE_PASSWORD is set ...
mcp.is_active     = False
admin.is_active   = True
AUTH mcp/mcp-service-changeme -> refused (UnauthorizedError)
RESULT: OK - mcp disabled

BLOCKER 2 (INV-7) — backfill appended after the revision id already shipped

Fix: v2_154 is restored to its shipped add-column-only form; the backfill moves into a NEW revision v2_155_backfill_is_service_account that chains from v2_154, keeping the same conservative dual-signal scope (username='mcp' AND email='mcp@bnk-forge.local').

Single head:

$ alembic heads
v2_155 (head)
$ alembic history | head
v2_154 -> v2_155 (head), Backfill users.is_service_account for the legacy mcp service account.
v2_153 -> v2_154, Add users.is_service_account for service-account provenance.

Verified via real alembic commands — an install already stamped at v2_154 (the exact immutable-migration hole) now gets the backfill on the next upgrade, and up/down round-trips cleanly:

stamped at v2_154, mcp.is_service_account BEFORE upgrade = 0  (the bug: mis-classified human)
now at v2_155, mcp.is_service_account AFTER upgrade  = 1
RESULT: FIX VERIFIED - v2_155 backfilled the v2_154 install
round-trip: after downgrade to v2_154 flag=0, after re-upgrade flag=1  -> OK

Guards mutation-tested (broke what they protect, confirmed red)

• Re-introducing name-keying into the disable → test_disable_stale_is_keyed_on_provenance_not_configured_username + test_disable_stale_service_user_deactivates_mcp FAIL (2 failed).
• Neutering v2_155.upgrade() → test_v2_155_backfills_only_the_legacy_mcp_service_row FAIL (1 failed).

Both files restored after each mutation.

Test output (py3.11)

• tests/component/test_auth_service.py + tests/test_migrations.py → 50 passed
• tests/test_routes_auth.py, tests/integration/test_routes_auth.py, tests/contract/test_auth_contracts.py, tests/component/test_startup_steps.py, tests/unit/test_core_config.py → 59 passed

Also fixed (directly triggers BLOCKER 1)

docs/DEPLOYMENT.md no longer instructs operators to set MCP_USERNAME=admin / MCP_PASSWORD=<current-admin-password> — the exact input that now hits the reserved-username refusal. It documents the dedicated mcp service account + MCP_SERVICE_PASSWORD instead.

Not addressed here (your non-blocking Minors, called out for the record)

Left for a follow-up / the #186 integration branch, as they are non-blocking and/or cross-PR: the PUT /api/auth/users/{id} re-enable path still leaves the disabled row's default hash intact; the healthcheck "no creds → unhealthy" signal still has a single compose consumer; helm/.../secrets.yaml:36 non-idempotent render; and the half-updated MCP_USERNAME | admin doc/Makefile lines.

Cross-PR (#186): this change touches only auth_service.py, startup_steps.py, the two migrations, tests, and DEPLOYMENT.md — none of the helm credential files (secrets.yaml, values.yaml) that the 8-file conflict with #186 centers on, so it does not widen that conflict surface. The MCP_SERVICE_USERNAME=${MCP_USERNAME}→admin collision you flagged for the merged tree is now defused on the backend side (reserved-username refusal + provenance-keyed disable), independent of how the compose var resolves.

[jgruberf5]

@bonnyr-f5 — round-4 Major findings addressed. The earlier pass wrongly deferred these as Minors; you classify them as Major and a PASS needs zero Majors, so they're fixed here. Head 1e152fc7 (on e86b8757). Every item reproduced first, then mutation-tested. Built an isolated py3.11 venv (system Python is 3.14, no wheels).

Major 1 — auth_service.py / routes/auth.py: re-enable resurrected the default hash

Reproduced (route level): seed mcp with mcp-service-changeme → disable_stale_service_user (is_active=False) → PUT /api/auth/users/{id} {"is_active": true} → 200, and authenticate_user("mcp","mcp-service-changeme") returned role=admin active=True. The published default was back.

Fix: new holds_known_default_password(user) (verifies the stored hash against MCP_KNOWN_DEFAULT_PASSWORDS). update_user now refuses the re-enable transition (inactive→active) on a service-account row that still holds a known default — the operator must rotate first (set MCP_SERVICE_PASSWORD, which re-seeds + re-activates with a real hash). Scoped to is_service_account so human rows and legit rotated service accounts are untouched.

Mutation test (TestServiceAccountReEnableGuard, 2 tests): after the fix the PUT returns 400, is_active stays False, and mcp-service-changeme no longer authenticates; a service account carrying a real rotated password still re-enables and authenticates, and the old default is gone.

Major 2 — healthcheck.py (INV-10): signal had one consumer

The no-creds → exit 1 auth-probe was wired only into the dev docker-compose.yml. Fix: wired the same probe (python -m bnk_forge_mcp.healthcheck) into dist/docker-compose.yml, replacing the bare urllib liveness ping that can't see a 401. The dist local overlay (dist/docker-compose.local.yml) and dev overlay inherit it — verified via docker compose config (rendered test: [CMD, python, -m, bnk_forge_mcp.healthcheck] on both base and overlay; config exit 0). Documented MCP_PASSWORD (ships empty → MCP disabled until set, never admin) in dist/README.md — you noted it was absent there. Probe behavior itself is unit-covered (test_probe_returns_1_when_no_auth_configured); 17 mcp-server healthcheck/config tests pass.

Major 3 — helm/.../secrets.yaml:36: non-idempotent render

Reproduced: helm template --set secrets.mcpPassword=changeme twice → two different mcp-password values (ObU1… vs RwIw…) — randAlphaNum fired on the values-supplied value every render → GitOps drift.

Fix: scope the shipped-changeme rotation to the persisted Secret ($existing.data) only; a values-supplied value is preserved verbatim. helm lint clean; helm template now:

• values changeme → identical "changeme" across two renders (no drift);
• values my-real-secret-123 → preserved verbatim;
• unset → generated.

The upgrade path (persisted changeme → rotate once, then converge on the reused value) can't be exercised by helm template (no cluster lookup) but is the only branch that now rotates.

Major 4 — docs/DEPLOYMENT.md:255-256,343 (you marked UNPROVEN)

Refuted on this head. Your read was against cd521ca, where it said MCP_USERNAME=admin / MCP_PASSWORD=<current-admin-password>. The BLOCKER-fix commit (e86b8757) already rewrote it: MCP_USERNAME=mcp, an explicit "do not point at admin" warning, and MCP_SERVICE_PASSWORD cited at lines 252 and 348. git diff cd521ca..HEAD -- docs/DEPLOYMENT.md shows the replacement. No further change needed.

Minors picked up (quick + clearly correct)

• Half-updated docs: fixed the MCP_USERNAME | admin leftover in docs/E2E-CRITICAL-004_MCP_SANITY.md:90 (→ mcp), and the stale "rotate the admin password → update MCP creds" framing in mcp-server/README.md and Makefile:527 — all now point at the dedicated mcp service account / MCP_SERVICE_PASSWORD, decoupled from the human admin login.

Minors deferred (hard evidence they're #186's surface)

• git merge-tree <#186> 8-file conflict (CROSS-1) and secrets.yaml:29-31 unguarded index / hasKey guard — both land on Stop shipping a default admin credential; generate it and enforce rotation #186's credential surface (Stop shipping a default admin credential; generate it and enforce rotation #186 adds the hasKey guard 20 lines up), and you note INV-13 holds for supported upgrades (MIN_UPGRADE_FROM). With my change, an absent mcp-password key decodes to empty and falls through to random generation (same unguarded pattern the pg/redis/jwt/enc blocks already use), so there's no new hole to close here — the guard belongs to the Stop shipping a default admin credential; generate it and enforce rotation #186 integration merge.

Evidence summary

Suites (py3.11 venv): test_auth_service 33, test_routes_auth integration 19 (incl. 2 new guard tests), test_routes_auth 3, test_startup_steps 9, test_auth_contracts 5, test_migrations 17, mcp-server healthcheck/config 17 — all pass. helm lint clean; docker compose config exit 0. Not merging.

[bonnyr-f5]

Review: BLOCK

Round 5, cold re-audit of 1e152fc against origin/staging 4a52ed4. Auditor had no prior-round
context. The blocker is re-verified by me below.

Merges standalone: mechanically yes (fast-forward, no conflict). Semantically no — one line
documents a #186-only mechanism (Major-4). For sequencing: #188 and #186 overlap on 20 files and
conflict on 11, including helm/.../secrets.yaml, both root composes, both dist composes,
docs/DEPLOYMENT.md and scripts/ibm_cloud_bnk_forge.sh. (My own cross-PR sweep counts 16
conflicting files across the pair under both merge strategies.)

Round-4 blockers: confirmed closed

Both are genuinely fixed, and the migration hygiene is now clean:

• The re-enable hole is closed — holds_known_default_password plus the inactive→active refusal on
  service-account rows, scoped to is_service_account so human rows are untouched.
• INV-7 is closed structurally: the in-branch v2_154 mutation was correctly extracted into a
  new v2_155. Verified independently — --diff-filter=M over alembic/versions is empty for all
  seven PRs, alembic reports a single head v2_155, the chain is v2_153 → v2_154 → v2_155, no
  revision-id collision across any open PR, the backfill is idempotent with working downgrades, and
  migrations run before the seeder.
• The Helm randAlphaNum-on-every-render drift is fixed by scoping rotation to the persisted Secret.

BLOCKER-1 · the #187 remediation never fires on the path a diligent operator takes

backend/startup_steps.py:237-259. disable_stale_service_user is reachable only from the else
branch — i.e. only when no usable MCP_SERVICE_PASSWORD is set. Verified: it has exactly one call
site in the whole tree, at :256, inside that else. Meanwhile ensure_service_user reconciles
one row, matched by name.

So whenever a real password is configured under any username other than mcp, the legacy row is
neither reconciled nor disabled. Proven by driving the real seed_auth_step against a legacy-install
fixture:

[PROBE] MCP_SERVICE_USERNAME='admin'      -> legacy mcp row is_active=True
[PROBE] login mcp/mcp-service-changeme    -> OK  role=admin must_change_password=False
[PROBE] MCP_SERVICE_USERNAME='mcp-svc'    -> legacy mcp row is_active=True   (same)
[PROBE] MCP_SERVICE_USERNAME='forge-mcp'  -> legacy mcp row is_active=True   (same)
[PROBE] control (username='mcp')          -> default rejected; rotation works

The real-world instance is the topology this PR's own docstrings name: a dist customer's existing
.env carries MCP_USERNAME=admin; they follow the new dist/README.md and set a strong
MCP_PASSWORD; nothing tells them to also fix MCP_USERNAME (dist/.env.example is a new-install
template). End-to-end: docker compose --env-file legacy.env config → backend gets
{MCP_SERVICE_USERNAME: 'admin', MCP_SERVICE_PASSWORD: 'strong-new-secret-xyz'} → reserved-name
ValueError → logged and swallowed → cleanup skipped.

Every existing Helm release is in the same population, since pre-#188 Helm never delivered these
vars, so every install carries the mcp row.

Note the shape: your round-4 fix made the disable provenance-keyed precisely so a name-keyed
disable couldn't no-op — and the comment at :252-255 says so. But it left that disable reachable
only when no password is set, so the operator who does the right thing is the one left exposed. The
fix is correct and unreachable on the path that matters.

Useful side result from the same investigation: the legacy row is universally named mcp
(MIN_UPGRADE_FROM=v3.1.6; MCP_SERVICE_USERNAME was undeliverable pre-PR — no env_file, absent
from every x-backend-env). Good news for v2_155 — its "documented edge" is unreachable, so the
backfill's scope is complete. Bad news for the fix, since a name-keyed reconcile can never cover the
class.

Major-2 · the chart declines to rotate a value the same PR makes fatal

helm/.../secrets.yaml:37-46 deliberately preserves an operator-supplied mcpPassword: changeme
(the old chart's own default), while config.py:219 in this same PR makes that value fatal at import
under Helm's ENVIRONMENT: production → api/worker/beat crashloop. Executed matrix confirms
changeme + correct ALLOWED_ORIGINS -> SystemExit(1). Honest scope: Helm's default
ALLOWED_ORIGINS: https://localhost already fails that validator on staging, so that part is
pre-existing.

Major-3 · the documented fail-fast is unreachable on the entire dist population

dist/.env.example:29 promises "the backend refuses to boot in production without it".
grep -rn ENVIRONMENT over every compose file, Dockerfile, entrypoint and Makefile returns
nothing — ENVIRONMENT is never set, so production is never reached on dist/IBM/dev. The new
unit tests construct Settings(ENVIRONMENT="production") directly, so they pass while proving
nothing about reachability. INV-17 + INV-16.

Major-4 · INV-23 recurrence

scripts/ibm_cloud_bnk_forge.sh:708 documents /app/keys/initial_admin_password, which exists only
in refs/maf/pr-186-r5, and deletes the accurate admin / changeme line. INV-23's Origin already
names "#188 round 2" for this exact string — third round it has appeared.

Minors

• Auth probe applied to 2 of 4 MCP surfaces (Helm keeps tcpSocket, the ibm embedded compose has no
  probe, the image HEALTHCHECK is still the ping).
• checksum/secret added to the Secret's clients but not to postgres/redis, over a Secret proven
  non-deterministic across three helm template renders.
• seed_auth_step has zero test coverage — which is exactly where BLOCKER-1 lives.
  is_service_account isn't exposed, so the UI toggle 400s blind.

Verified clean by execution

Sweep for hardcoded MCP passwords and admin username defaults: clean everywhere. Compose plumbing
complete across all 4 permutations and 5 services in both composes; both ibm-script compose paths
covered; Helm env complete for api/worker/beat/mcp. The disable genuinely revokes (both auth
resolvers check is_active; only one route writes it, and it's guarded). 106 tests pass at the ref;
ruff clean; mypy identical to baseline; make shellcheck equally red on base and ref (pre-existing).

BLOCKER-1 is the one to fix, and the fix is small: make the stale-row cleanup unconditional rather
than the else of the password check.

[bonnyr-f5]

Cross-PR merge-order constraints for this series are now tracked in #192.

Relevant here: this PR and #186 conflict on 16 files under both strategies, across the whole credential surface and both auth test modules. The coupling runs the other way too: this PR holds the _RESERVED_HUMAN_USERNAMES refusal that #186 needs in order to close its wiring gap safely. Land these two together, or this one first with #186 rebased onto it. Migration heads are clean — v2_154 → v2_155 off v2_153, no collisions, no in-place edits anywhere in the series.

[jgruberf5]

@/tmp/claude-1000/-mnt-d-project-bnk-forge/01a289e5-e845-497f-99d9-622be0bf9a98/scratchpad/comment.md

[jgruberf5]

Superseded by the consolidated integration PR #193 (branch pr177-integration), which merges this and the other six #177 follow-up PRs in the dependency order from #192 and passes full CI (CI Gate green). Per #192, these seven share the credential and release/CI surfaces and could not merge in arbitrary order, so they land together via #193.

Closing unmerged, not abandoning: the branch is retained and all review history stays on this page for reference. See #193 for the integrated, validated result and #192 for the cross-PR conflict analysis.