Skip to content

feat(config): PINKY_DEPLOYMENT_MODE env + cached app.state.deployment_mode (#434) - #472

Open
olegbrok wants to merge 1 commit into
mainfrom
feat/deployment-mode-config
Open

feat(config): PINKY_DEPLOYMENT_MODE env + cached app.state.deployment_mode (#434)#472
olegbrok wants to merge 1 commit into
mainfrom
feat/deployment-mode-config

Conversation

@olegbrok

Copy link
Copy Markdown
Collaborator

Summary

PR-1 of the #433 web-exposed hardening track. Establishes the single env-var switch that downstream middleware (#436 LAN filter, #437 headers, #439 admin elevation, #441 boot guards, etc.) will branch on, instead of each feature carrying its own knob.

Closes part of #433. Refs #434.

What

  • pinky_daemon/config.py (new):

    • DeploymentMode enum: trusted (default, current Mac Mini), lan, web
    • resolve_deployment_mode() — reads PINKY_DEPLOYMENT_MODE, normalises case + whitespace, fails closed on invalid values (InvalidDeploymentModeError). A typo like prod aborts startup, not silently fall back to trusted.
    • default_bind_host(mode)0.0.0.0 for trusted/lan, 127.0.0.1 for web (assumes reverse proxy in front).
    • warn_if_insecure_exposure(mode, host) — loud stderr warning when trusted + 0.0.0.0, so accidental VPS exposure shows up in logs while back-compat is preserved.
  • pinky_daemon/__main__.py:

  • pinky_daemon/api.py create_api:

    • New deployment_mode kwarg; if None, resolved from env so embedded callers and tests don't have to wire it through.
    • Cached on app.state.deployment_mode — single source of truth for the rest of Hardening: PinkyBot for web-exposed deployments (tracking) #433.
    • /api payload exposes deployment_mode so VPS operators can confirm posture without scraping logs.

Murzik review followups (from #434) — addressed

  • Fail closed on invalid modeInvalidDeploymentModeError, daemon exits 2
  • --host defaults to None internally — argparse default is None; mode-appropriate default applied after resolution
  • Expose effective mode in /api — added deployment_mode field
  • Loud warning when trusted + 0.0.0.0warn_if_insecure_exposure() printed to stderr at startup
  • Pure resolver, no aliases — only canonical values; tests assert production/dev/local/staging all reject
  • Other middleware reads from app.state.deployment_mode — established as the single cache point

Tests

26 new tests in tests/test_deployment_mode.py covering the resolver, the bind-host helper, the insecure-exposure warning, and the create_api wiring (app.state + /api exposure).

Existing test_api.py: 286 tests still green (verified locally).

Test plan

  • pytest tests/test_deployment_mode.py -v (26 passing locally)
  • pytest tests/test_api.py -v (286 passing locally; no regression)
  • ruff check src/pinky_daemon/config.py src/pinky_daemon/__main__.py src/pinky_daemon/api.py tests/test_deployment_mode.py (clean)
  • Manual: launch with PINKY_DEPLOYMENT_MODE=prod → confirm exit 2 + clear error
  • Manual: launch unset → confirm "Mode: trusted" in logs + insecure-exposure warning
  • Manual: launch PINKY_DEPLOYMENT_MODE=web without --host → confirm bind to 127.0.0.1
  • curl /api confirms deployment_mode field

Out of scope (other PRs in the track)

The middleware that acts on deployment_mode lives in subsequent PRs: #442 client-IP resolver, #436 LAN filter + bind enforcement, #437 headers, #435 bearer auth, #443 CSRF, #440 audit, #438 rate limits, #439 elevated admin, #441 refuse-to-boot.

🤖 Opened by Barsik

…_mode (#434)

PR-1 of the #433 web-exposed hardening track. Establishes the single env-var
switch downstream middleware will branch on (#436 LAN filter, #437 headers,
#439 admin elevation, #441 boot guards, etc.) instead of each feature carrying
its own knob.

What
----
* New `pinky_daemon.config` module:
  - `DeploymentMode` enum: `trusted` (default, current Mac Mini), `lan`,
    `web`.
  - `resolve_deployment_mode()` reads `PINKY_DEPLOYMENT_MODE`, normalises
    case + whitespace, **fails closed** on invalid values
    (`InvalidDeploymentModeError`) — a typo like `prod` aborts startup
    rather than silently falling back to `trusted`.
  - `default_bind_host(mode)`: `0.0.0.0` for trusted/lan, `127.0.0.1` for
    web (assumes a reverse proxy in front).
  - `warn_if_insecure_exposure(mode, host)`: loud stderr warning when
    `trusted` + `0.0.0.0`, so accidental VPS exposure is obvious in logs
    while back-compat is preserved.

* `pinky_daemon.__main__`:
  - `--host` default flipped to `None` so we can tell "operator picked
    0.0.0.0" from "old hardcoded default" — needed by #436/#441.
  - On startup: resolve mode → resolve host → warn → log mode + host.
  - `InvalidDeploymentModeError` → `sys.exit(2)` with a useful message.

* `pinky_daemon.api.create_api`:
  - New `deployment_mode` kwarg; if `None`, resolved from env so embedded
    callers and tests don't have to wire it through.
  - Cached on `app.state.deployment_mode` — single source of truth for
    the rest of #433.
  - `/api` payload exposes `deployment_mode` so VPS operators can confirm
    posture without scraping logs.

Tests (26 new)
--------------
* Resolver: empty/unset → trusted; each canonical value; case-insensitive;
  whitespace stripped; invalid value raises with a useful message; aliases
  like "production" / "dev" don't silently pass.
* `default_bind_host`: trusted/lan → 0.0.0.0, web → 127.0.0.1.
* `warn_if_insecure_exposure`: warns only on trusted + 0.0.0.0; quiet on
  lan/web and on specific bind addresses.
* `create_api`: explicit mode cached on `app.state`; defaults resolve from
  env; `/api` exposes the mode for each posture.

Existing test_api.py: 286 tests still green.

Closes part of #433.
Refs #434.

🤖 Authored by Barsik
def test_api_endpoint_exposes_mode_for_each_value(self, tmp_db_path):
from pinky_daemon.api import create_api

for mode in DeploymentMode:
Comment thread src/pinky_daemon/api.py
from typing import TYPE_CHECKING, Literal

if TYPE_CHECKING:
from pinky_daemon.config import DeploymentMode
@olegbrok

Copy link
Copy Markdown
Collaborator Author

CodeQL findings (overnight triage)

CodeQL surfaced 1 error + 1 note on this PR. Both reviewed:

1. ❗ Real bug: tests/test_deployment_mode.py:210-214

for mode in DeploymentMode:
    app = create_api(db_path=tmp_db_path, deployment_mode=mode)
    client = TestClient(app)
    resp = client.get("/api")
assert resp.json()["deployment_mode"] == mode.value  # ← outside loop

The assert is at the same indent as for, not inside it. The test only verifies the last enum value (passes accidentally because mode/resp leak out of the loop). Fix: indent the assert one level deeper.

CodeQL's py/non-iterable-in-for-loop message itself is misleading (enum classes are iterable), but it flagged the right block.

2. ⚠️ False positive: src/pinky_daemon/api.py:30 unused-import

if TYPE_CHECKING:
    from pinky_daemon.config import DeploymentMode

Used at line 628 as a stringified annotation: deployment_mode: "DeploymentMode | None" = None. CodeQL's py/unused-import doesn't see through string-quoted forward refs under TYPE_CHECKING. Severity is "note", non-blocking. Can suppress with # noqa: F401 if we want a clean run, or leave it.


Holding off on a fixup commit until you greenlight — pushing to feat/deployment-mode-config cascades through all 7 stacked PRs above this one.

— Barsik 🤖

@olegbrok

Copy link
Copy Markdown
Collaborator Author

Update: full-stack CI passes clean

Opened #480 as a CI-validation draft on the cumulative diff of all 8 stack PRs targeting `main`. Result: all 11 checks green, including CodeQL.

So the CodeQL findings I flagged earlier dissolve:

  • `py/unused-import` on `DeploymentMode` — false positive that resolves on the full stack (later PRs use it more visibly). Not a real issue.
  • `py/non-iterable-in-for-loop` on the test — also a false positive (enum classes are iterable). CodeQL's not great at recognizing enum iteration.

The latent test bug I called out still stands though: `tests/test_deployment_mode.py:210-214` has the `assert` outside the for-loop, so the test only verifies the last enum value. Not a CI failure — the assertion passes on the last iteration — but worth fixing for correctness. Easy nit, doesn't need to block merge.

Net: this stack should be safe to merge as-is once Murzik signs off.

— Barsik 🤖

@olegbrok

Copy link
Copy Markdown
Collaborator Author

Correction on the latent test bug

Looked closer: the assert-outside-loop bug is not in #472. On this branch, `tests/test_deployment_mode.py:205-209` has the assert correctly inside the for-loop. The bug got introduced in #474 (`feat/lan-filter-stacked`) — when that PR wrapped the test in `patch.dict()` to bypass the new LAN filter, the rewrap moved the assert one indent level out of the loop.

Filing the correction comment on #474. This PR (#472) is clean on its own; the CodeQL fail it surfaced was both false positives.

— Barsik 🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants