feat(dev-portal): admin-role PATCH routing + MFA fixes + interactive --password-stdin

[matyas-jirat-keboola]

Summary

Three independent fixes against the dev-portal surface that landed in #354, all discovered while integrating ABRA Flexi (a real component registration on production apps-api):

1. Admin-role PATCH routing

complexity, categories, forwardToken, forwardTokenDetails, injectEnvironment, processTimeout, requiredMemory, features, and category are .forbidden() on the apps-api vendor schema (PATCH /vendors/{vendor}/apps/{app}). The server's error message is misleading — it says "must be one of: easy, medium, hard" because the enum-validation .error() annotation lives on the shared admin schema before clientAppSchema() overrides with .forbidden(). Source of truth: keboola/developer-portal:src/lib/validation.js → clientAppSchema().

• DeveloperPortalIdentity.role_hint becomes a real validator: only vendor (default) or admin accepted; case-folded; typos raise. The field is now load-bearing, not a free-text label.
• DeveloperPortalClient.patch_app reads self._identity.role_hint and routes admin identities to PATCH /admin/apps/{app} (permissive adminAppSchema); vendor identities stay on the vendor endpoint.
• DeveloperPortalService.prepare_patch preflights: vendor role + admin-only field ⇒ fail-fast VALIDATION_ERROR with a message that names every offending field, explains why the 422 is misleading, and tells the user the exact command to switch identity. Admin role bypasses the preflight entirely.
• Reads, create, upload-icon, deprecate keep vendor-endpoint behaviour — only PATCH has a meaningful admin variant on the server.

2. MFA login: explicit challenge + actual error surfaced

User report from a Keboola-org TOTP account:
```
MFA code: 521278
Error: Developer Portal MFA login failed (HTTP 404)
```
Root cause: the apiary spec calls challenge optional with default SOFTWARE_TOKEN_MFA, but in practice the server 404s when it's omitted. Sending it explicitly fixes it. Single attempt only — /auth/login consumes the session, so retrying with a different challenge always 404s with "Invalid code or auth state".

The error now includes the server response body (truncated to 500 chars) and a hint about TOTP code freshness, so users can tell whether the code was wrong, the session expired, or something else.

3. --password-stdin no longer hangs interactively

sys.stdin.read() waits for EOF, not Enter — users who pasted a password and pressed Enter sat there until they Ctrl-C'd out. New _read_password_stdin() helper branches on sys.stdin.isatty(): TTY uses getpass.getpass() (hidden, line-based, Enter to confirm); pipe still does read() → strip(). Both identity add --password-stdin and identity edit --password-stdin route through it.

Test plan

• make check passes (3827 passed, 8 skipped, 0 failures)
• 95 dev-portal tests pass (10 new in this PR)
• Manual: kbagent dev-portal identity add --role-hint admin ... accepts TOTP code on first attempt, no spurious 404s
• Manual: kbagent dev-portal patch --app keboola.ex-abra-flexi --data /tmp/p.json --identity admin-keboola routes to /admin/apps/keboola.ex-abra-flexi and sets complexity: easy + categories: [...] + forwardToken: true (all forbidden via the vendor path)
• Manual: same patch with vendor-keboola identity fails fast with a clear "switch to admin identity" message; no portal call is made
• Manual: kbagent dev-portal identity add --password-stdin (interactive) shows a hidden prompt, Enter completes
• Manual: `echo $PASS | kbagent dev-portal identity add --password-stdin` (piped) still works

Notes for review

• The bearer cache + stale-bearer eviction in _authed_client on main is preserved; no changes there.
• Version bumped to 0.51.1 with a changelog entry covering the three fixes.

[matyas-jirat-keboola]

Review of #366 — feat(dev-portal): admin-role PATCH routing + MFA fixes + interactive --password-stdin

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR delivers three targeted bug fixes to the dev-portal surface: admin-role-aware PATCH routing (vendor vs. admin endpoint), an MFA challenge field fix that resolves 404s on TOTP accounts, and a --password-stdin hang fix for TTY sessions. The implementation is well-scoped, all three layers are correctly touched, and the plugin synchronization surfaces are comprehensively updated. One BLOCKING backward-compatibility issue was found: the role_hint field validator added in models.py will silently break config.json loads for any user who previously stored a non-standard string (which was documented as allowed). The body-truncation magic number is a NIT. Verdict: REQUEST CHANGES.

Verdict

• Verdict: REQUEST CHANGES
• Blocking findings: 1
• Non-blocking findings: 2
• Nits: 2

Blocking findings

[B-1] src/keboola_agent_cli/models.py:147 — role_hint validator breaks config.json deserialization for existing installs with non-standard strings

The old role_hint field description on main explicitly said "Free-text label shown in dev-portal identity list (e.g. 'vendor', 'admin'). Not validated against the portal." The new @field_validator("role_hint") now raises ValueError for anything other than "vendor" or "admin". When ConfigStore deserializes config.json on startup, every DeveloperPortalIdentity is parsed through Pydantic; an existing config entry with role_hint: "keboola-admin" or any other free-text value causes an unhandled ValidationError that crashes the entire CLI with a traceback (not a KeboolaApiError with a clean exit code).

Fix: either (a) add a config-migration shim in ConfigStore.load() that normalises non-standard role_hint values to "vendor" before validation and logs a warning, or (b) use "before" validation mode (@field_validator("role_hint", mode="before")) that normalises unknown strings to "vendor" with a warning instead of raising. Option (a) is the pattern already used by this codebase for version upgrades. In either case, add a test that loads a config.json with role_hint="freetext" and verifies it degrades gracefully rather than crashing.

Non-blocking findings

[NB-1] src/keboola_agent_cli/dev_portal_client.py:170 — except Exception: is overly broad and silences unexpected errors

At line 170, except Exception: catches the result of resp.text[:500]. In practice resp.text on an httpx.Response does not raise — it returns a str or falls back to bytes decoding. Catching Exception here instead of (UnicodeDecodeError, AttributeError) would silently hide a logic error (e.g., resp being None due to a future refactor) and replace it with the misleading <unreadable> string in the error message. Per CONTRIBUTING.md "Specific exception handling -- never bare except:".

Fix: replace with except (UnicodeDecodeError, AttributeError): which covers the realistic decode-failure scenarios without catching everything.

[NB-2] tests/test_dev_portal_cli.py — no CliRunner-level test that exercises identity add --password-stdin through the full command stack

The TestReadPasswordStdin class tests the _read_password_stdin() helper function directly. That is good isolation coverage. However, no CliRunner test invokes identity add (or identity edit) with --password-stdin and an input= payload to verify that the full Typer-dispatch-to-helper path works end-to-end. This matters because identity_add reads password_stdin as a Typer bool flag and then calls _read_password_stdin(), but the wiring between the flag and the helper is untested at the CLI layer. Per CONTRIBUTING.md "CLI-layer tests -- use CliRunner, test JSON output, error exit codes": CLI-layer tests are mandatory for new command behavior.

Fix: add a CliRunner test that invokes dev-portal identity add --alias x --username u --password-stdin with input="mypassword\n" (simulating pipe mode via CliRunner's non-TTY stdin) and asserts exit code 0 and that the stored identity has a non-empty password.

Nits

• [NIT-1] src/keboola_agent_cli/dev_portal_client.py:169 — magic number 500 should be MAX_API_ERROR_LENGTH from constants.py. The constant already exists (MAX_API_ERROR_LENGTH: int = 500, used by http_base.py:282) and expresses exactly the same intent: cap error body previews at 500 characters. Using the literal 500 here instead of the constant means the two places can diverge silently if the cap is ever changed.

• [NIT-2] src/keboola_agent_cli/commands/dev_portal.py:67-68 — import getpass as _getpass and import sys as _sys are deferred inside _read_password_stdin(). Both are standard-library modules with negligible import cost and no circularity risk. Promoting them to top-level imports (where sys is already used elsewhere via import sys as _sys at line 274) makes the dependency explicit and avoids the pattern of hiding imports inside functions. The underscore alias at module level is already established for sys at line 274.

Verification log

• gh auth status → authenticated as matyas-jirat-keboola ✓
• git rev-parse --abbrev-ref HEAD → feat/dev-portal-admin-mfa (matches <branch>) ✓
• gh pr view 366 --json state → OPEN ✓
• gh pr view 366 --json files → 20 files, +487/-38, conventional feat(dev-portal): prefix ✓
• wc -l /tmp/kbagent-pr-366.diff → 846 lines ✓
• grep typer/click src/services/ → empty ✓ (no layer violation)
• grep httpx src/commands/ → empty ✓ (no layer violation)
• grep formatter/typer src/dev_portal_client.py → empty ✓ (no layer violation)
• grep -E 'error_code\s*=\s*"[A-Z_]+"' → empty ✓ (no raw error-code strings)
• grep -E '^\+\s*except\s*:' → empty ✓ (no bare except:)
• grep -E '^\+\s*print\(' src/ → empty ✓ (no raw print())
• grep 'dev.portal' src/keboola_agent_cli/permissions.py → all dev-portal commands registered in OPERATION_REGISTRY ✓ (no new commands added in this PR beyond pre-existing registration)
• CONTRIBUTING.md Plugin synchronization map — all "NO" rows checked:
  • AGENT_CONTEXT (commands/context.py) → updated with role_hint and password-stdin details ✓
  • CLAUDE.md ## All CLI Commands → updated with behavioral note ✓
  • plugins/kbagent/agents/keboola-expert.md → Rule 6 VERSION GATE updated (dev-portal = 0.49.0+ (admin-role PATCH = 0.51.1+)) ✓
  • plugins/kbagent/skills/kbagent/references/commands-reference.md → updated with role_hint routing and new gotchas ✓
  • plugins/kbagent/skills/kbagent/references/gotchas.md → 3 new entries, all tagged (since v0.51.1) ✓
  • plugins/kbagent/skills/kbagent/references/dev-portal-workflow.md → updated with role_hint table and password-stdin docs ✓
  • src/keboola_agent_cli/changelog.py → "0.51.1" key with 3 entries ✓
  • pyproject.toml version → bumped to 0.51.1 ✓
  • plugin.json and marketplace.json → bumped to 0.51.1 ✓
• File-size budgets: commands/dev_portal.py = 570 LOC (soft ceiling 800), services/dev_portal_service.py = 345 LOC, dev_portal_client.py = 322 LOC ✓
• make check → 3829 passed, 8 skipped, 0 failures (exit 0) ✓
• Backward-compat: git show origin/main:src/keboola_agent_cli/models.py | grep -A 6 role_hint → confirmed old description was "Free-text label … Not validated against the portal." The new field_validator is a breaking change for existing configs with non-standard strings → B-1 flagged
• resp.text[:500] at line 169: MAX_API_ERROR_LENGTH = 500 exists in constants.py and is used by http_base.py; literal 500 is a duplicate → NIT-1
• except Exception: at line 170: covers realistic decode failure but is broader than needed → NB-1
• Behavior reproduction: could not run against a live Developer Portal (no portal credentials in this environment). The PR description's manual test plan is detailed and convincing; E2E tests cover the offline-verifiable paths (role-hint validator, vendor-preflight fail-fast). Marking online behavior paths (admin PATCH to /admin/apps/, MFA challenge field) as author-confirmed ✓ (server-side tests in test_dev_portal_client.py use pytest-httpx mock with exact URL matching — test_patch_app_admin_role_hits_admin_endpoint will fail if the client routes to the vendor URL, which is a strong offline signal).

Open questions for the author

(none)

[matyas-jirat-keboola]

Review of #366 — feat(dev-portal): admin-role PATCH routing + MFA fixes + interactive --password-stdin

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR delivers three independent bug fixes to the dev-portal surface introduced in #354: (1) admin-role PATCH routing so that admin-only fields like complexity, categories, and forwardToken can be set via PATCH /admin/apps/{app} rather than the vendor endpoint that silently rejects them with a misleading 422; (2) explicit challenge: SOFTWARE_TOKEN_MFA in the MFA login step to fix a server-side 404 when the field is omitted; and (3) a _read_password_stdin() helper that branches on sys.stdin.isatty() so --password-stdin no longer hangs waiting for EOF on interactive TTYs. The plugin documentation (AGENT_CONTEXT, gotchas.md, commands-reference.md, dev-portal-workflow.md, keboola-expert.md) is fully updated. The implementation is correct. One type error introduced by the new test suite fails make typecheck (which CONTRIBUTING.md lists as a mandatory BLOCKING gate). Additionally, the E2E test test_role_hint_validator_rejects_typo has a factually incorrect docstring describing a behavior that the code deliberately does not implement (silent downgrade, not a raise).

Verdict: REQUEST CHANGES — one make typecheck regression introduced by this PR.

Verdict

• Verdict: REQUEST CHANGES
• Blocking findings: 1
• Non-blocking findings: 2
• Nits: 1

Blocking findings

[B-1] tests/test_dev_portal_cli.py:35 — make typecheck regression: invalid-assignment on isatty lambda

The new TestReadPasswordStdin.test_pipe_reads_until_eof test assigns a lambda: False to fake_stdin.isatty, which ty flags as error[invalid-assignment]: Object of type () -> Literal[False] is not assignable to attribute isatty of type def isatty(self) -> bool. The comment # type: ignore[method-assign] is present but ty does not suppress this diagnostic (it uses its own suppression surface, not mypy's). CONTRIBUTING.md §"Type checking -- ty is mandatory and BLOCKING" states: "make typecheck must stay clean (0 diagnostics); the backlog was cleared in 0.45.0, so the gate is blocking, not warning-only." Running make typecheck on the current branch head returns 4 diagnostics; this specific error is the one introduced by this PR (the other three are pre-existing from earlier commits on this branch and are not your responsibility to fix here).

Fix: replace the lambda assignment with monkeypatch.setattr of the full isatty method using a compatible signature, or add the correct # ty: ignore[invalid-assignment] suppression (with the required one-line explanation comment per CONTRIBUTING.md convention — reserve ty: ignore for genuinely dynamic surfaces).

# Option A — keep monkeypatch consistent:
fake_stdin = io.StringIO("pw-piped\n")
monkeypatch.setattr(fake_stdin, "isatty", lambda: False)
monkeypatch.setattr(_sys, "stdin", fake_stdin)

# Option B — ty suppress (add explanation comment):
fake_stdin.isatty = lambda: False  # ty: ignore[invalid-assignment] -- StringIO.isatty is a bound method; lambda is a compatible duck-type override for test isolation

Non-blocking findings

[NB-1] tests/test_e2e.py:10699 — test name and docstring describe "rejection" but the code silently downgrades

test_role_hint_validator_rejects_typo has a docstring that says "The pydantic ValidationError raised on model construction surfaces as a non-zero exit" and "validator fires before any portal call". Neither claim is accurate: DeveloperPortalIdentity.validate_role_hint deliberately does not raise on unknown values — it silently downgrades to "vendor" and emits a stderr warning. The test passes because add_identity subsequently calls _ensure_authenticated() which triggers a login probe that fails (HTTP 403 on the real portal) and exits 1. The test is therefore correct in its assertion (exit_code != 0) but correct for the wrong reason. If someone runs this test against real portal credentials that work for the vendor role, the identity will be silently accepted with the downgraded role — and the test would pass too, but the assertion exit_code != 0 would fail and break CI for a valid login. The test name "rejects_typo" is also misleading: the design decision is "downgrade + warn", which is documented in the unit tests (test_role_hint_typo_downgrades_to_vendor_with_warning) but contradicted by the E2E test's name and description.

Fix: rename to test_role_hint_typo_silently_downgrades_to_vendor and rewrite the docstring to match the actual behavior. Alternatively, if the intent is to test that a typo never silently creates a working admin-role identity, add an assertion on result.output containing the warning text or mock add_identity to verify it receives role_hint="vendor".

[NB-2] src/keboola_agent_cli/models.py:165 — deferred import sys as _sys inside a Pydantic field_validator

The validate_role_hint validator uses import sys as _sys inline (deferred import inside the method body) to avoid adding sys to models.py's top-level imports. However, sys is part of the Python standard library and has effectively zero import overhead; the deferred pattern is typically reserved for heavy optional dependencies (e.g. import numpy). Mixing top-level imports (from urllib.parse import urlparse) with inline deferred standard-library imports (import sys as _sys) inside a validator is inconsistent with the rest of models.py and with the project style. This is the same pattern the PR removes from commands/dev_portal.py (the import sys as _sys inside write commands), making the residual inline import in models.py a stylistic inconsistency within this PR's own diff.

Fix: add import sys to the top-level imports block in models.py and use sys.stderr.write(...) in the validator body.

Nits

• [NIT-1] src/keboola_agent_cli/dev_portal_client.py:149 — the string literal "SOFTWARE_TOKEN_MFA" is used in a single location and is unlikely to need reuse, but if the apps-api ever adds EMAIL_OTP or another challenge type, the value would need to be tracked. Extracting it to constants.py as DP_MFA_CHALLENGE_TYPE = "SOFTWARE_TOKEN_MFA" would make any future extension point obvious. This is a low-signal nit given it is a true constant with no variation path in scope.

Verification log

• gh pr view 366 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state → 20 files, +568/-44, state=OPEN, base=main, head=feat/dev-portal-admin-mfa, conventional feat(dev-portal): prefix ✓
• git rev-parse --abbrev-ref HEAD → feat/dev-portal — working tree is on the PR's branch per the parent agent setup; the diff was fetched via gh pr diff 366 against the current HEAD ✓
• grep -E 'typer|click|formatter\.|console\.print' diff | grep services/ → empty ✓ (no layer violations in services)
• grep -E 'httpx|requests\.' diff | grep commands/ → empty ✓ (no HTTP calls in commands)
• grep -n "dev.portal\|dev_portal" src/keboola_agent_cli/permissions.py → all 10 dev-portal commands (identity.add/list/remove/edit/use/current/verify + list/get/create/patch/upload-icon/publish/deprecate) registered ✓
• Plugin synchronization map scan: AGENT_CONTEXT updated with admin-role routing doc (context.py:1129) ✓; CLAUDE.md All CLI Commands updated with v0.51.1 annotation ✓; keboola-expert.md §1 Rule 6 VERSION GATE updated (dev-portal = 0.49.0+ (admin-role PATCH = 0.51.1+)) ✓; keboola-expert.md §2 Tool Selection Matrix already has dev-portal row from #354 (no new group added) ✓; commands-reference.md updated ✓; gotchas.md three new entries all tagged (since v0.51.1) ✓; dev-portal-workflow.md updated ✓; no new workflow file needed (existing surface extended) ✓
• make check → 3831 passed, 8 skipped, 0 failures ✓
• make typecheck → Found 4 diagnostics, exit 1 — tests/test_dev_portal_cli.py:35 (invalid-assignment for isatty lambda with ineffective # type: ignore[method-assign]) is introduced by this PR; the other three are pre-existing on the feature branch from earlier commits ✗ [B-1]
• make skill-check → SKILL.md is up-to-date ✓
• Behavior reproduction: DeveloperPortalIdentity(username="u", password="p", role_hint="vendr") → role_hint="vendor" + stderr warning; no exception raised — confirmed downgrade-not-reject behavior. test_role_hint_validator_rejects_typo E2E test passes (exit 1) but due to login probe failure, not validation rejection [NB-1]
• patch_app admin routing: if self._identity.role_hint == "admin": path = f"/admin/apps/{app_id}" — correct; vendor arg still threaded for error reporting only ✓
• prepare_patch preflight: _ADMIN_ONLY_PATCH_FIELDS frozenset has 9 fields matching apps-api clientAppSchema() source; check fires only for role_hint != "admin" ✓
• _read_password_stdin() branches on sys.stdin.isatty() ✓; _assert_tty() uses top-level sys (now cleaned up from _sys alias) ✓
• Server router server/routers/dev_portal.py — write commands pre-existing documented as CLI-only (TTY confirmation required, no HTTP bypass); no new routes needed ✓
• File-size budgets: commands/dev_portal.py 566 LOC (soft 800 ✓), dev_portal_client.py 323 LOC (soft 1500 ✓), services/dev_portal_service.py 345 LOC (soft 1000 ✓)
• Token discipline: no tokens in logged output; MAX_API_ERROR_LENGTH constant used for body truncation (not a magic number) ✓
• E2E offline tests test_role_hint_validator_rejects_typo and test_vendor_role_admin_only_field_fails_fast both pass ✓

Open questions for the author

• The validate_role_hint validator intentionally issues a one-shot stderr warning rather than raising, to preserve backward compat with pre-0.51.1 configs that may carry free-text values. This is well-reasoned and documented. The question is whether identity edit --role-hint vendr (interactive, not ConfigStore.load()) should have the same silent-downgrade behavior or should fail fast. Currently both paths go through the same Pydantic validator, so both silently downgrade. If the intent for interactive edit is "fail fast" (the user typed a typo right now, not in an old config file), a separate CLI-layer validation before constructing the model would make that possible without changing the backward-compat logic. This is not a blocking concern but may be worth a follow-up issue.

[matyas-jirat-keboola]

Review of #366 — feat(dev-portal): admin-role PATCH routing + MFA fixes + interactive --password-stdin

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR delivers three independent bug fixes to the dev-portal surface introduced in #354: (1) admin-role PATCH routing so that admin-only fields like complexity, categories, and forwardToken can be set via PATCH /admin/apps/{app} rather than the vendor endpoint that silently rejects them with a misleading 422; (2) explicit challenge: SOFTWARE_TOKEN_MFA in the MFA login step to fix a server-side 404 when the field is omitted; and (3) a _read_password_stdin() helper that branches on sys.stdin.isatty() so --password-stdin no longer hangs waiting for EOF on interactive TTYs. The plugin documentation (AGENT_CONTEXT, gotchas.md, commands-reference.md, dev-portal-workflow.md, keboola-expert.md) is fully updated. The implementation is correct. One type error introduced by the new test suite fails make typecheck (which CONTRIBUTING.md lists as a mandatory BLOCKING gate). Additionally, the E2E test test_role_hint_validator_rejects_typo has a factually incorrect docstring describing a behavior that the code deliberately does not implement (silent downgrade, not a raise).

Verdict: REQUEST CHANGES — one make typecheck regression introduced by this PR.

Verdict

• Verdict: REQUEST CHANGES
• Blocking findings: 1
• Non-blocking findings: 2
• Nits: 1

Blocking findings

[B-1] tests/test_dev_portal_cli.py:35 — make typecheck regression: invalid-assignment on isatty lambda

The new TestReadPasswordStdin.test_pipe_reads_until_eof test assigns a lambda: False to fake_stdin.isatty, which ty flags as error[invalid-assignment]: Object of type () -> Literal[False] is not assignable to attribute isatty of type def isatty(self) -> bool. The comment # type: ignore[method-assign] is present but ty does not suppress this diagnostic (it uses its own suppression surface, not mypy's). CONTRIBUTING.md §"Type checking -- ty is mandatory and BLOCKING" states: "make typecheck must stay clean (0 diagnostics); the backlog was cleared in 0.45.0, so the gate is blocking, not warning-only." Running make typecheck on the current branch head returns 4 diagnostics; this specific error is the one introduced by this PR (the other three are pre-existing from earlier commits on this branch and are not your responsibility to fix here).

Fix: replace the lambda assignment with monkeypatch.setattr of the full isatty method using a compatible signature, or add the correct # ty: ignore[invalid-assignment] suppression (with the required one-line explanation comment per CONTRIBUTING.md convention — reserve ty: ignore for genuinely dynamic surfaces).

# Option A — keep monkeypatch consistent:
fake_stdin = io.StringIO("pw-piped\n")
monkeypatch.setattr(fake_stdin, "isatty", lambda: False)
monkeypatch.setattr(_sys, "stdin", fake_stdin)

# Option B — ty suppress (add explanation comment):
fake_stdin.isatty = lambda: False  # ty: ignore[invalid-assignment] -- StringIO.isatty is a bound method; lambda is a compatible duck-type override for test isolation

Non-blocking findings

[NB-1] tests/test_e2e.py:10699 — test name and docstring describe "rejection" but the code silently downgrades

test_role_hint_validator_rejects_typo has a docstring that says "The pydantic ValidationError raised on model construction surfaces as a non-zero exit" and "validator fires before any portal call". Neither claim is accurate: DeveloperPortalIdentity.validate_role_hint deliberately does not raise on unknown values — it silently downgrades to "vendor" and emits a stderr warning. The test passes because add_identity subsequently calls _ensure_authenticated() which triggers a login probe that fails (HTTP 403 on the real portal) and exits 1. The test is therefore correct in its assertion (exit_code != 0) but correct for the wrong reason. If someone runs this test against real portal credentials that work for the vendor role, the identity will be silently accepted with the downgraded role — and the test would pass too, but the assertion exit_code != 0 would fail and break CI for a valid login. The test name "rejects_typo" is also misleading: the design decision is "downgrade + warn", which is documented in the unit tests (test_role_hint_typo_downgrades_to_vendor_with_warning) but contradicted by the E2E test's name and description.

Fix: rename to test_role_hint_typo_silently_downgrades_to_vendor and rewrite the docstring to match the actual behavior. Alternatively, if the intent is to test that a typo never silently creates a working admin-role identity, add an assertion on result.output containing the warning text or mock add_identity to verify it receives role_hint="vendor".

[NB-2] src/keboola_agent_cli/models.py:165 — deferred import sys as _sys inside a Pydantic field_validator

The validate_role_hint validator uses import sys as _sys inline (deferred import inside the method body) to avoid adding sys to models.py's top-level imports. However, sys is part of the Python standard library and has effectively zero import overhead; the deferred pattern is typically reserved for heavy optional dependencies (e.g. import numpy). Mixing top-level imports (from urllib.parse import urlparse) with inline deferred standard-library imports (import sys as _sys) inside a validator is inconsistent with the rest of models.py and with the project style. This is the same pattern the PR removes from commands/dev_portal.py (the import sys as _sys inside write commands), making the residual inline import in models.py a stylistic inconsistency within this PR's own diff.

Fix: add import sys to the top-level imports block in models.py and use sys.stderr.write(...) in the validator body.

Nits

• [NIT-1] src/keboola_agent_cli/dev_portal_client.py:149 — the string literal "SOFTWARE_TOKEN_MFA" is used in a single location and is unlikely to need reuse, but if the apps-api ever adds EMAIL_OTP or another challenge type, the value would need to be tracked. Extracting it to constants.py as DP_MFA_CHALLENGE_TYPE = "SOFTWARE_TOKEN_MFA" would make any future extension point obvious. This is a low-signal nit given it is a true constant with no variation path in scope.

Verification log

• gh pr view 366 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state → 20 files, +568/-44, state=OPEN, base=main, head=feat/dev-portal-admin-mfa, conventional feat(dev-portal): prefix ✓
• git rev-parse --abbrev-ref HEAD → feat/dev-portal — working tree is on the PR's branch per the parent agent setup; the diff was fetched via gh pr diff 366 against the current HEAD ✓
• grep -E 'typer|click|formatter\.|console\.print' diff | grep services/ → empty ✓ (no layer violations in services)
• grep -E 'httpx|requests\.' diff | grep commands/ → empty ✓ (no HTTP calls in commands)
• grep -n "dev.portal\|dev_portal" src/keboola_agent_cli/permissions.py → all 10 dev-portal commands (identity.add/list/remove/edit/use/current/verify + list/get/create/patch/upload-icon/publish/deprecate) registered ✓
• Plugin synchronization map scan: AGENT_CONTEXT updated with admin-role routing doc (context.py:1129) ✓; CLAUDE.md All CLI Commands updated with v0.51.1 annotation ✓; keboola-expert.md §1 Rule 6 VERSION GATE updated (dev-portal = 0.49.0+ (admin-role PATCH = 0.51.1+)) ✓; keboola-expert.md §2 Tool Selection Matrix already has dev-portal row from #354 (no new group added) ✓; commands-reference.md updated ✓; gotchas.md three new entries all tagged (since v0.51.1) ✓; dev-portal-workflow.md updated ✓; no new workflow file needed (existing surface extended) ✓
• make check → 3831 passed, 8 skipped, 0 failures ✓
• make typecheck → Found 4 diagnostics, exit 1 — tests/test_dev_portal_cli.py:35 (invalid-assignment for isatty lambda with ineffective # type: ignore[method-assign]) is introduced by this PR; the other three are pre-existing on the feature branch from earlier commits ✗ [B-1]
• make skill-check → SKILL.md is up-to-date ✓
• Behavior reproduction: DeveloperPortalIdentity(username="u", password="p", role_hint="vendr") → role_hint="vendor" + stderr warning; no exception raised — confirmed downgrade-not-reject behavior. test_role_hint_validator_rejects_typo E2E test passes (exit 1) but due to login probe failure, not validation rejection [NB-1]
• patch_app admin routing: if self._identity.role_hint == "admin": path = f"/admin/apps/{app_id}" — correct; vendor arg still threaded for error reporting only ✓
• prepare_patch preflight: _ADMIN_ONLY_PATCH_FIELDS frozenset has 9 fields matching apps-api clientAppSchema() source; check fires only for role_hint != "admin" ✓
• _read_password_stdin() branches on sys.stdin.isatty() ✓; _assert_tty() uses top-level sys (now cleaned up from _sys alias) ✓
• Server router server/routers/dev_portal.py — write commands pre-existing documented as CLI-only (TTY confirmation required, no HTTP bypass); no new routes needed ✓
• File-size budgets: commands/dev_portal.py 566 LOC (soft 800 ✓), dev_portal_client.py 323 LOC (soft 1500 ✓), services/dev_portal_service.py 345 LOC (soft 1000 ✓)
• Token discipline: no tokens in logged output; MAX_API_ERROR_LENGTH constant used for body truncation (not a magic number) ✓
• E2E offline tests test_role_hint_validator_rejects_typo and test_vendor_role_admin_only_field_fails_fast both pass ✓

Open questions for the author

• The validate_role_hint validator intentionally issues a one-shot stderr warning rather than raising, to preserve backward compat with pre-0.51.1 configs that may carry free-text values. This is well-reasoned and documented. The question is whether identity edit --role-hint vendr (interactive, not ConfigStore.load()) should have the same silent-downgrade behavior or should fail fast. Currently both paths go through the same Pydantic validator, so both silently downgrade. If the intent for interactive edit is "fail fast" (the user typed a typo right now, not in an old config file), a separate CLI-layer validation before constructing the model would make that possible without changing the backward-compat logic. This is not a blocking concern but may be worth a follow-up issue.

[matyas-jirat-keboola]

Review of #366 — feat(dev-portal): admin-role PATCH routing + MFA fixes + interactive --password-stdin

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR delivers three targeted bug fixes to the dev-portal surface: admin-role-aware PATCH routing (vendor vs. admin endpoint), an MFA challenge field fix that resolves 404s on TOTP accounts, and a --password-stdin hang fix that branches on sys.stdin.isatty(). This is a re-review after the prior round requested changes. All four prior findings (B-1 ty invalid-assignment lambda, NB-1 misleading test name/docstring, NB-2 deferred inline import sys, NIT-1 SOFTWARE_TOKEN_MFA magic string) have been genuinely resolved. The ty gate has zero PR-introduced regressions: the two unresolved-attribute errors visible on the branch (test_config_store.py:965, test_services.py:70) are pre-existing on origin/main (introduced by the headless __env__ commits already merged) and are not attributable to this PR. make check passes cleanly (3831 passed, 8 skipped). One new NIT was found. Verdict: APPROVE.

Verdict

• Verdict: APPROVE
• Blocking findings: 0
• Non-blocking findings: 0
• Nits: 1

Blocking findings

(none)

Non-blocking findings

(none)

Nits

• [NIT-1] tests/test_dev_portal_client.py:69,101 — match_json assertions hardcode the string "SOFTWARE_TOKEN_MFA" rather than importing DP_MFA_CHALLENGE_TYPE from constants. If the constant is ever renamed or a second challenge type is added, the test will silently continue passing while the production code diverges. The fix is a one-liner: from keboola_agent_cli.constants import DP_MFA_CHALLENGE_TYPE at the top of the test file and substituting the constant in both match_json dicts.

Verification log

• gh auth status → authenticated as matyas-jirat-keboola ✓
• git rev-parse --abbrev-ref HEAD → feat/dev-portal-admin-mfa ✓ (matches <branch>)
• gh pr view 366 --json state → "OPEN" ✓; 21 files, +603/-45, conventional feat(dev-portal): ✓
• B-1 prior finding (lambda isatty ty regression) resolved: tests/test_dev_portal_cli.py:39 now uses monkeypatch.setattr(fake_stdin, "isatty", lambda: False) instead of a direct attribute assignment. make typecheck emits zero invalid-assignment diagnostics for this file. ✓
• ty footprint baseline: make typecheck on origin/main HEAD → 3 diagnostics (1 warning[unresolved-import] + 2 error[unresolved-attribute] in test_config_store.py:965 and test_services.py:70). Same count on the PR branch head. The 2 errors were introduced by cd3b19f (headless __env__, already merged to main) and are not in any file this PR touches. PR-introduced ty delta: 0 new diagnostics. ✓
• NB-1 prior finding (misleading test name/docstring) resolved: tests/test_e2e.py:10699 is now test_role_hint_typo_rejected_at_cli_layer with a docstring that accurately describes the two-layer design (CLI raises via click.Choice; model downgrades silently for backward compat). tests/test_models.py:598 is test_role_hint_typo_downgrades_to_vendor_with_warning with a matching docstring. ✓
• NB-2 prior finding (deferred import sys in models.py) resolved: src/keboola_agent_cli/models.py:3 now has import sys at module top-level. Same for src/keboola_agent_cli/commands/dev_portal.py:11. All three inline import sys as _sys blocks in dev_portal.py are removed and replaced with the module-level import + the _read_password_stdin() helper. ✓
• NIT-1 prior finding (SOFTWARE_TOKEN_MFA magic literal) resolved: src/keboola_agent_cli/constants.py:37 defines DP_MFA_CHALLENGE_TYPE: str = "SOFTWARE_TOKEN_MFA" with a descriptive comment. dev_portal_client.py:24 imports it; dev_portal_client.py:149 uses it as "challenge": DP_MFA_CHALLENGE_TYPE. ✓ (Test files still hardcode the string — see NIT-1 above.)
• 3-layer compliance: grep -E '(from typer|import typer|formatter\.|console\.print)' services/dev_portal_service.py → no hits in new lines. click import added only in commands/dev_portal.py (correct layer for click.Choice). ✓
• OPERATION_REGISTRY: src/keboola_agent_cli/permissions.py:140-176 has pre-existing entries for all dev-portal.* commands. No new command surface added by this PR (only behavior change to existing patch). ✓
• Plugin sync map: keboola-expert.md §1 Rule 6 updated (dev-portal = 0.49.0+ (admin-role PATCH = 0.51.1+)). CLAUDE.md ## All CLI Commands updated with v0.51.1 behavior note. AGENT_CONTEXT (context.py) updated with role_hint and --password-stdin documentation. commands-reference.md updated. gotchas.md updated with 3 new entries, all tagged (since v0.51.1). dev-portal-workflow.md updated. ✓
• make check → 3831 passed, 8 skipped, 114 deselected, 12 warnings (exit 0) ✓
• Behavior reproduction: The --password-stdin fix and MFA fix require a real portal TTY / TOTP device to exercise end-to-end; could not reproduce at runtime due to lack of portal credentials. The offline E2E tests (test_role_hint_typo_rejected_at_cli_layer, test_vendor_role_admin_only_field_fails_fast) were confirmed to pass within the make check run. ✓

Open questions for the author

(none)

[matyas-jirat-keboola]

@padak couple of fixes

[padak]

Review of #366 — feat(dev-portal): admin-role PATCH routing + MFA fixes + interactive --password-stdin

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR delivers three independent bug fixes to the dev-portal command group: (1) admin-role routing so that the 9 vendor-endpoint-forbidden fields can be set via PATCH /admin/apps/{app}, (2) explicit challenge field in the MFA second-step call to fix a silent 404 on TOTP accounts, and (3) a _read_password_stdin() helper that branches on TTY vs. pipe so --password-stdin no longer hangs interactively. The implementation is clean, the layering is respected, all three fixes have comprehensive test coverage at service/client/CLI/E2E layers, and all required plugin synchronization surfaces are updated with proper (since v0.51.1) version tags. No new commands are added; no permission registry entries are needed. make check passes clean (3831 passed, 8 skipped). Verdict: APPROVE.

Verdict

• Verdict: APPROVE
• Blocking findings: 0
• Non-blocking findings: 2
• Nits: 2

Blocking findings

(none)

Non-blocking findings

[NB-1] src/keboola_agent_cli/models.py:166 — sys.stderr.write() in a Pydantic field_validator instead of a logger

The validate_role_hint validator writes the downgrade warning directly to sys.stderr rather than using the logging module. models.py has no logger set up (unlike services/, which uses _log = logging.getLogger(__name__)), and the pattern is consistent with auto_update.py's deliberate "no OutputFormatter yet" rationale. However, in models.py this fires during ConfigStore.load() which is called at CLI startup — early enough that the logging subsystem is fully initialized. Using warnings.warn() (propagates to the logging system; suppressible with -W ignore) or a module-level logging.getLogger(__name__).warning(...) would be more idiomatic and allow the warning to be silenced in test suites without capsys. The existing tests use capsys.readouterr().err which works but is more fragile than pytest.warns. This is a NIT-level concern in most codebases, but since the CONTRIBUTING.md section "Python conventions" explicitly says "use logging module for production logging, not print()", the same principle extends to sys.stderr.write in a validator.

Fix: add import logging; _log = logging.getLogger(__name__) and replace sys.stderr.write(f"Warning: ...") with _log.warning("role_hint %r is not 'vendor' or 'admin' — downgrading to 'vendor'. ..."). Remove the import sys from models.py if it becomes unused.

[NB-2] plugins/kbagent/agents/keboola-expert.md §2 Tool Selection Matrix — dev-portal row does not mention admin routing as a distinct use case

The existing matrix row reads: "reads kbagent dev-portal list|get (agent-safe); writes create|patch|upload-icon|publish|deprecate (0.49.0+) need a human to type a random code on a real TTY; --dry-run is the agent-safe preview". After this PR, an AI agent running in a TTY-enabled context that needs to set admin-only fields (complexity, categories, etc.) needs to know that it must use --role-hint admin on the identity — otherwise it will hit the fail-fast preflight error with no contextual guidance from the matrix. The gotchas.md entry (added correctly with (since v0.51.1)) covers the topic in depth, but the matrix is the static prompt that fires before the agent even consults gotchas.md. A one-line addition to the "First choice" cell is sufficient: append "admin-only fields (complexity/categories/forwardToken/…) need --role-hint admin on the identity (0.51.1+)".

Nits

• [NIT-1] src/keboola_agent_cli/services/dev_portal_service.py:249 — error message string contains a literal {app} placeholder (not an f-string interpolation) in the phrase "/admin/apps/{app} instead". The error string is a multi-line implicit concatenation and the lines at 249-251 do not carry an f prefix, so {app} prints literally rather than expanding to the actual app_id. This is intentional (it reads as a command template in the guidance text) and unambiguous in context, but the adjacent f-string lines (f"on PATCH /vendors/{vendor}/apps/{app_id}") make the inconsistency visually jarring. Consider quoting it as code: \/admin/apps/<APP_ID>`` so the intent is unambiguous.

• [NIT-2] tests/test_e2e.py:10699 — test_role_hint_typo_rejected_at_cli_layer and test_vendor_role_admin_only_field_fails_fast are placed inside TestDevPortalE2E (an "E2E" class) but they are actually fully offline unit-style tests that make no network calls and require no credentials. They run in the standard make test suite because TestDevPortalE2E is not decorated with @skip_without_credentials (it was intentionally left ungated for its identity-list smoke test). This is fine operationally, but placing offline tests in a class named E2E risks confusing future contributors about what external setup is required. Moving them to tests/test_dev_portal_cli.py or tests/test_dev_portal_service.py alongside the other unit tests would be cleaner.

Verification log

• gh pr view 366 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state → state=OPEN, 21 files, +604/-45, conventional feat(dev-portal): ✓
• git rev-parse --abbrev-ref HEAD → feat/dev-portal-admin-mfa after gh pr checkout 366 ✓
• Layer violation checks (typer/click imports in services, httpx in commands, formatter in clients) → empty ✓
• Convention checks (magic numbers, raw error_code strings, bare except, print() in production, token in logs) → all clean ✓
• New commands scan (@*_app.command() additions) → no new commands; no OPERATION_REGISTRY entry needed ✓
• Plugin synchronization map: commands/context.py AGENT_CONTEXT updated ✓, CLAUDE.md ## All CLI Commands comment added ✓, plugins/kbagent/agents/keboola-expert.md VERSION GATE updated to dev-portal = 0.49.0+ (admin-role PATCH = 0.51.1+) ✓, plugins/kbagent/skills/kbagent/references/commands-reference.md role_hint and version noted ✓, plugins/kbagent/skills/kbagent/references/gotchas.md three new entries with (since v0.51.1) tags ✓, plugins/kbagent/skills/kbagent/references/dev-portal-workflow.md role_hint table and password-stdin section added ✓
• Admin-only field set (_ADMIN_ONLY_PATCH_FIELDS) matches PR description: 9 fields, set equality ✓
• prepare_patch() order: admin-only preflight check fires BEFORE _authed_client context manager (no portal call on vendor+admin-only-field error) ✓
• _assert_tty() guard: skipped when dry_run=True; prepare_patch() preflight fires before portal fetch in --dry-run path ✓ (offline test test_vendor_role_admin_only_field_fails_fast confirms this)
• typecheck (make typecheck) → 3 pre-existing diagnostics in tests/test_services.py, tests/test_config_store.py, scripts/hatch_build.py (none in files touched by this PR; all three present on main before this branch)
• make check (after uv sync --extra server) → 3831 passed, 8 skipped, 0 failures ✓ (initial run failed with ModuleNotFoundError: No module named 'fastapi' from test_serve_ui.py — pre-existing worktree setup gap, not introduced by this PR; fixed by uv sync --extra server)
• New tests: 14 functions across test_dev_portal_cli.py (3), test_dev_portal_client.py (3), test_dev_portal_service.py (2), test_e2e.py (2), test_models.py (4) ✓
• Backward compat: DeveloperPortalIdentity.validate_role_hint downgrades unknown pre-0.51.1 free-text values to "vendor" with a stderr warning rather than raising (preserves ConfigStore.load() on upgrade) ✓; CLI layer wires click.Choice(["vendor", "admin"]) so fresh user input fails loudly — intentional two-layer design ✓
• Behavior: cannot reproduce manually (no Developer Portal credentials in this environment); the author's manual test plan covers TTY/pipe/MFA/admin-routing/vendor-preflight scenarios. Marked as author-confirmed.

Open questions for the author

(none)