Add canonical PAPERSCOUT_<FIELD> process-env overrides for every Sett…#67
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough
ChangesCanonical PAPERSCOUT_ env var prefix support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review again |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== src/paperscout/config.py ==="
cat src/paperscout/config.py
echo ""
echo "=== tests/test_config.py ==="
cat tests/test_config.py
echo ""
echo "=== .env.example ==="
cat .env.example
echo ""
echo "=== README.md ==="
cat README.md
echo ""
echo "=== docs/onboarding.md ==="
cat docs/onboarding.md
echo ""
echo "=== deploy/SERVER_SETUP.md ==="
cat deploy/SERVER_SETUP.md
echo ""
echo "=== .github/workflows/ci.yml ==="
cat .github/workflows/ci.ymlLength of output: 72623
Review of PR
|
| Field | Would be |
|---|---|
enable_open_std |
ENABLE_OPEN_STD=false |
wg21_index_timeout_s |
WG21_INDEX_TIMEOUT_S=30 |
mq_max_retries |
MQ_MAX_RETRIES=8 |
mq_circuit_breaker_threshold |
MQ_CIRCUIT_BREAKER_THRESHOLD=5 |
mq_circuit_breaker_cooldown_seconds |
MQ_CIRCUIT_BREAKER_COOLDOWN_SECONDS=60 |
mq_max_size |
MQ_MAX_SIZE=1000 |
These may be intentionally omitted (internal / rarely tuned), but if so a brief comment in .env.example saying "message-queue internals are tunable via MQ_* vars; see config.py" would make the omission explicit.
Minor nits
config.pyLine 26:_PrefixedEnvSourceis a private class (single underscore) but is imported bytests/test_config.pyindirectly throughsettings_customise_sources. No action needed, but worth keeping the import chain in mind if the class is ever renamed..github/workflows/ci.yml: The added comment ends withetc.but only shows two examples; could say "see ENV_VAR_MAP in config.py" to be more useful to a CI reader.
Overall, the implementation is sound and the backward-compatibility design is correct. The main actionable items are the NamedTuple for ENV_VAR_MAP, the legacy_env_name alias fragility, and the missing .env-isolation regression test.
Add tests that PAPERSCOUT_* keys in .env are ignored while legacy keys still load, and clarify that all README env tables support canonical process-env overrides via the PAPERSCOUT_ prefix. Co-authored-by: Cursor <cursoragent@cursor.com>
babe7e9 to
2d926f7
Compare
There was a problem hiding this comment.
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 @.env.example:
- Line 30: The `ENABLE_OPEN_STD` variable is currently out of order and should
appear before scheduling-related variables like `POLL_INTERVAL_MINUTES`.
Reorganize the environment variables in the `.env.example` file so that all
`ENABLE_*` flags appear together as a logical group before any numeric polling
or scheduling settings. This ensures consistent alphabetical and functional
ordering throughout the configuration file.
🪄 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: 635233e8-301e-47b0-8455-7d9398de664b
📒 Files selected for processing (4)
.env.example.github/workflows/ci.ymlsrc/paperscout/config.pytests/test_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_config.py
Summary
Add canonical
PAPERSCOUT_<FIELD>process-env overrides for everySettingsfield while keeping legacy unprefixed names (DATABASE_URL,SLACK_BOT_TOKEN, etc.) fully supported in.envfiles and as fallbacks.The
Settingsmodel was already flat; this PR documents the 1:1 env mapping and wires a dual-read loader so operators can override any setting via Docker-e, shell export, or CI without editing.env.Changes
config.py:_PrefixedEnvSource+settings_customise_sources;ENV_VAR_MAPand expanded docstringtest_config.py: tests for prefixed load, legacy fallback, precedence, and field coverage.env.example, README, onboarding, SERVER_SETUP — document canonicalPAPERSCOUT_*namesBehavior
PAPERSCOUT_DATABASE_URL.env/ legacy envDATABASE_URLTest plan
uv run pytest tests/test_config.py— 7 passeduv run pytest tests/— 369 passed, 2 skippedRelated issue
Summary by CodeRabbit
New Features
PAPERSCOUT_-prefixed environment variables with higher precedence than legacy unprefixed names..env, while legacy.envbehavior remains supported.Documentation
.env.exampleto document the canonical naming scheme, override behavior, and example settings.Tests