Skip to content

Defer Settings credential validation to daemon startup#90

Merged
wpak-ai merged 3 commits into
developfrom
feat/defer-credential-validation
Jul 7, 2026
Merged

Defer Settings credential validation to daemon startup#90
wpak-ai merged 3 commits into
developfrom
feat/defer-credential-validation

Conversation

@henry0816191

@henry0816191 henry0816191 commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Remove the import-time @model_validator that required _PAPERSCOUT_TESTING=1 whenever Slack credentials were blank, so from paperscout.config import settings always succeeds with safe defaults.
  • Add Settings.validate_for_run() to raise ConfigurationError when slack_bot_token, slack_signing_secret, or database_url are missing, and call it from __main__.py before init_pool() (replacing the previous database_url-only guard).
  • Drop _PAPERSCOUT_TESTING from tests/conftest.py, benchmarks/conftest.py, CI/CD workflows, .env.example, and CONTRIBUTING.md.
  • Update tests/test_config.py to assert validation via validate_for_run() instead of construction-time failures.

Addresses week 2 plan item T3 (Test 9 — Settings defaults requiring a hidden testing flag).

Test plan

  • uv run pytest -q (414 passed)
  • uv run mypy src/paperscout/config.py src/paperscout/__main__.py
  • uv run ruff check src tests
  • python -c "from paperscout.config import settings" succeeds with no env vars
  • python -m paperscout exits with a clear error when Slack/DB credentials are missing
  • ./run check (or make check)
  • pre-commit run --all-files

Related issues

Summary by CodeRabbit

  • Bug Fixes
    • Configuration is now validated during startup, so missing Slack credentials or a database setting fail fast with clear error messages.
    • Removed the legacy testing-only bypass so runtime behavior matches production expectations more closely.
  • Documentation
    • Updated environment examples and contributing guidance to align with the new validation flow.
  • CI/CD
    • Removed the old testing environment flag from CI/CD workflows and test container invocations.
  • Tests
    • Refreshed configuration tests to validate via runtime checks directly, and removed automatic test env flag setup.

@henry0816191 henry0816191 self-assigned this Jul 6, 2026
@henry0816191 henry0816191 requested a review from wpak-ai as a code owner July 6, 2026 17:24
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d5eef932-4f02-49a7-a4bf-34f5b341b890

📥 Commits

Reviewing files that changed from the base of the PR and between 1549b1d and 075355b.

📒 Files selected for processing (1)
  • tests/test_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/test_config.py

📝 Walkthrough

Walkthrough

Moves credential validation out of import time into Settings.validate_for_run(), calls it during daemon startup, and removes _PAPERSCOUT_TESTING setup or references from tests, CI/CD, fixtures, and docs.

Changes

Runtime validation refactor

Layer / File(s) Summary
Settings.validate_for_run and ENV_VAR_MAP fix
src/paperscout/config.py
Replaces the model-validator-based Slack enforcement with a public validate_for_run() method that raises ConfigurationError for missing Slack credentials or database_url, and updates ENV_VAR_MAP and override_settings to iterate Settings.model_fields.keys().
Daemon startup calls validate_for_run
src/paperscout/__main__.py
Imports ConfigurationError and wraps settings.validate_for_run() in startup error handling that logs the message and exits with status code 1.
Tests for validate_for_run behavior
tests/test_config.py
Calls validate_for_run() directly for blank Slack, valid Slack, blank database_url, and empty-default construction cases.
Remove _PAPERSCOUT_TESTING from fixtures, CI/CD, and docs
tests/conftest.py, benchmarks/conftest.py, .github/workflows/ci.yml, .github/workflows/cd.yml, CONTRIBUTING.md, .env.example
Removes _PAPERSCOUT_TESTING setup or references and updates comments and examples to reflect runtime validation.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant __main__.py
  participant Settings
  participant Logger

  __main__.py->>Settings: validate_for_run()
  alt invalid configuration
    Settings-->>__main__.py: raise ConfigurationError
    __main__.py->>Logger: log error message
    __main__.py->>__main__.py: exit(1)
  else valid configuration
    Settings-->>__main__.py: return
  end
Loading

Possibly related PRs

  • cppalliance/paperscout#42: Both PRs adjust Slack credential validation behavior and _PAPERSCOUT_TESTING gating around configuration loading.

Suggested reviewers: wpak-ai

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately describes the main change: moving Settings validation to daemon startup.
Description check ✅ Passed The PR description follows the required Summary, Test plan, and Related issues sections and includes concrete details.
Linked Issues check ✅ Passed The changes implement the issue's goals: import-time validation is removed, runtime validation is added, and tests and fixtures are updated.
Out of Scope Changes check ✅ Passed The modified files all support the validation shift and removal of _PAPERSCOUT_TESTING, with no clear unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/defer-credential-validation

Comment @coderabbitai help to get the list of available commands.

@henry0816191

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_config.py`:
- Around line 25-42: Suppress the Ruff S105/S106 false positives in the test
fixture values used by Settings tests, since the literals in
test_settings_accepts_slack_when_not_testing and
test_settings_rejects_blank_database_url are dummy data. Either add the
appropriate per-line noqa on the hardcoded token/secret assertions in
tests/test_config.py, or configure a tests/** per-file ignore for S105/S106 in
the project lint settings so these test-only constants don’t fail CI.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b0f5a4c1-41f0-45f8-83d5-6292b1559f49

📥 Commits

Reviewing files that changed from the base of the PR and between dfce167 and 1549b1d.

📒 Files selected for processing (9)
  • .env.example
  • .github/workflows/cd.yml
  • .github/workflows/ci.yml
  • CONTRIBUTING.md
  • benchmarks/conftest.py
  • src/paperscout/__main__.py
  • src/paperscout/config.py
  • tests/conftest.py
  • tests/test_config.py
💤 Files with no reviewable changes (3)
  • CONTRIBUTING.md
  • .github/workflows/cd.yml
  • .github/workflows/ci.yml

Comment thread tests/test_config.py

@wpak-ai wpak-ai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should fix

  1. tests/test_config.py:56,63,71,79,97,107 — remove the six dead _PAPERSCOUT_TESTING monkeypatches.
  2. tests/test_config.py:45 (test_settings_constructs_with_empty_defaults) — not .env-hermetic; delenv only clears process env while config reads .env, so a dev whose .env has SLACK_* (i.e. anyone who copied .env.example) gets false failures. Pass _env_file=<empty tmp> like the sibling dotenv tests.

- Remove 6 dead _PAPERSCOUT_TESTING monkeypatches
- test_settings_constructs_with_empty_defaults hermeticity
@henry0816191 henry0816191 requested a review from wpak-ai July 7, 2026 19:16
@wpak-ai wpak-ai merged commit 0d02186 into develop Jul 7, 2026
10 checks passed
@wpak-ai wpak-ai deleted the feat/defer-credential-validation branch July 7, 2026 19:18
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.

Make Settings defaults usable without _PAPERSCOUT_TESTING; move credential validation out of import time

2 participants