feat(config): share validated env parsing for tunable concurrency constants - #1220
Conversation
…stants
`TAG_WRITE_CONCURRENCY` in intelligent_cache.py was a hardcoded literal, so
tuning Redis tag-write fan-out for a given deployment required an application
release. Its sibling in firestore_state.py was already env-parsed, but the
parser was private to that module -- and had already been copy-pasted once
into cloud_ai/providers/aws_rekognition.py.
Extract the two parsers verbatim into youtube_extension/core/env_config.py and
have both call sites import them, then wire TAG_WRITE_CONCURRENCY through
positive_int_env().
Semantics are preserved exactly, including the deliberate split that the
merged firestore implementation settled on:
- absent or blank falls back to the shipped default, because Compose and
Helm routinely render an empty string for an unconfigured value; and
- malformed or out-of-range fails fast at import rather than being clamped,
so an operator typo surfaces at startup instead of silently running the
process with a concurrency limit or deadline nobody chose.
The only behavioural change is the error text, which now names the offending
variable and echoes the input instead of surfacing int()'s built-in message.
core/ is chosen over core/config/ and utils/ because its __init__.py is empty:
importing the helper pulls in no logging or proxy stack, which matters for a
module read at import time. Imports are relative so the helper resolves under
either package root in use in this repo (youtube_extension.* and
src.youtube_extension.*) rather than loading a second copy of the package.
Verification:
- tests/unit/test_env_config.py (new, 46 tests) covers unset, blank,
whitespace, valid, zero, negative, non-numeric, inf and nan for both
parsers, and asserts the messages are diagnosable.
- Import-time wiring is proven in a subprocess rather than with
importlib.reload, which would rebind module classes and leave the rest of
the session holding stale references. With the env unset the constants
resolve to exactly the shipped 8 / 16 / 30.0; with an override set they
take the override; with an invalid value the import exits non-zero.
- The 9 pre-existing parser tests in test_firestore_state.py were repointed
at the re-exported names and still pass unchanged, which is what
demonstrates the extraction is behaviour-preserving.
- 247 passed across test_firestore_state.py and test_intelligent_cache.py;
ruff clean; mypy --strict clean on the new module.
Deliberately out of scope: the duplicate parser in aws_rekognition.py, which
is already modified by open PR #1216 and would conflict; and
TAG_WRITE_POOL_RESERVE, which is a headroom allowance rather than a
concurrency limit.
Closes #1180
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds shared parsers for positive integer and positive finite float environment variables. Intelligent cache tag writes and Firestore cleanup settings now use validated, environment-configurable values with existing defaults. ChangesEnvironment configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Sentry Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
There was a problem hiding this comment.
🟡 Not ready to approve
Invalid values do not safely fall back, and cleanup concurrency remains unbounded despite #1180’s requirements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Centralizes validated environment parsing and makes cache/Firestore concurrency tunable.
Changes:
- Adds shared positive-number environment parsers.
- Applies parsing to cache and Firestore constants.
- Adds parser and import-time wiring tests.
File summaries
| File | Description |
|---|---|
src/youtube_extension/core/env_config.py |
Adds shared environment parsers. |
src/youtube_extension/services/cloud/firestore_state.py |
Uses shared parsing for cleanup settings. |
src/youtube_extension/backend/services/intelligent_cache.py |
Makes tag-write concurrency configurable. |
tests/unit/test_env_config.py |
Tests parsing and constant wiring. |
tests/unit/test_firestore_state.py |
Updates parser references. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Independent red-team review — cleanReviewed the full diff at head No actionable findings. The one change that could have failed silently at runtime — removing Also verified:
CI: green across Terminal state: Generated by Claude Code |
|
@coderabbitai full review Auto-review was skipped at PR creation (no qualifying label present yet); the Generated by Claude Code |
|
✅ Action performedFull review finished. |
groupthinking
left a comment
groupthinking
left a comment
There was a problem hiding this comment.
Review — not ready to merge: one clear fix + one decision needed
I read the full diff and cross-checked both of @copilot-pull-request-reviewer's threads against the acceptance criteria in #1180. Both threads are correct and grounded in the issue owner's own triage note (#1180 comment) — they are not dismissible.
Also verified clean, for the record: removing import math from firestore_state.py is safe (no other use in the file), and no code imports the deleted _positive_int_env / _positive_finite_float_env names — aws_rekognition.py keeps its own independent copies (correctly deferred here vs. #1216).
1. CLEANUP_DELETE_CONCURRENCY needs a ceiling, not just a floor — clear fix
The owner's triage says verbatim: "CLEANUP_DELETE_CONCURRENCY … is used to size the semaphore directly. So it needs the parse and a floor/ceiling", suggesting _env_int(name, default, minimum=1, maximum=…). positive_int_env enforces only >= 1, so a positive-but-absurd value (e.g. 160000) flows straight into min(CLEANUP_DELETE_CONCURRENCY, len(docs)) and can allocate one task per expired document — reintroducing the unbounded fan-out the worker pool exists to prevent. This is uncontested; recommend adding an optional maximum to positive_int_env and applying a documented cap at the CLEANUP_DELETE_CONCURRENCY call site. TAG_WRITE_CONCURRENCY needs only the floor (it is already clamped downstream by _resolve_tag_write_limit()), consistent with the owner's note.
2. Malformed value → fail-fast (this PR) vs log-and-fall-back (owner's stated AC) — needs a decision
The owner's triage says a malformed value ("abc", "", "-1", "0") should "log and fall back rather than raising at import — that last case is the actual regression this issue exists to prevent." This PR deliberately does the opposite (raise at import), and the PR description argues fail-fast is the safer reading and preserves the semantics already merged for the Firestore constants. Both positions are defensible and they conflict, so I'm not inverting ~20 tests on my own judgment.
One wrinkle worth naming: the owner's note was written against #1170's bare int(os.getenv(...)); a validating fail-fast parser has since merged for CLEANUP_DELETE_*, so "log and fall back" may be partly overtaken by events.
@groupthinking — please rule on #2: keep fail-fast (I'll update #1180's AC-1/AC-3 wording to match), or switch to log-and-fall-back (the new tests and the two already-merged constants change with it). I'll then implement your choice plus the #1 ceiling in one pass.
I did not approve or merge — base main is protected and the PR isn't ready regardless.
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/youtube_extension/backend/services/intelligent_cache.py`:
- Line 34: Replace the relative env_config import in
src/youtube_extension/backend/services/intelligent_cache.py:34-34 with an
absolute youtube_extension.core.env_config import for positive_int_env, and
update src/youtube_extension/services/cloud/firestore_state.py:17-17 to import
both helpers from the same absolute module.
In `@src/youtube_extension/core/env_config.py`:
- Around line 64-72: The integer and float environment helpers in
`src/youtube_extension/core/env_config.py` at lines 64-72 and 95-107 should use
a shared Pydantic validator instead of manual `int`/`float` conversion and
bounds checks; preserve returning the default for blank or missing overrides and
retain the existing invalid-value errors and minimum-value constraints at both
sites.
In `@src/youtube_extension/services/cloud/firestore_state.py`:
- Around line 39-40: Bound CLEANUP_DELETE_CONCURRENCY to a safe maximum instead
of relying only on positive_int_env(). Define or reuse an upper-limit constant
and validate the configured value before the cleanup worker scheduling logic
around the cleanup method at line 370, rejecting values above the limit while
preserving the existing bounded-task behavior.
🪄 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: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 23dfec09-5a39-4200-9d80-154558376620
⛔ Files ignored due to path filters (2)
tests/unit/test_env_config.pyis excluded by!tests/**tests/unit/test_firestore_state.pyis excluded by!tests/**
📒 Files selected for processing (3)
src/youtube_extension/backend/services/intelligent_cache.pysrc/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.py
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Agent completion enforcement / 0_Agent completion enforcement.txt: feat(config): share validated env parsing for tunable concurrency constants
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const fs = require('fs');
const pull = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: Number(process.env.PR)
});
let verdict = {
conclusion: 'failure',
reason: 'verifier_did_not_publish',
details: {}
};
try {
verdict = JSON.parse(fs.readFileSync(
'enforcement-verdict.json', 'utf8'
));
} catch (error) {
core.warning(error.message);
}
const conclusion = verdict.conclusion === 'success'
? 'success'
: 'failure';
const summary = JSON.stringify(verdict);
await github.rest.checks.create({
owner: context.repo.owner,
repo: context.repo.repo,
name: 'Agent completion enforcement',
head_sha: pull.data.head.sha,
status: 'completed',
conclusion,
output: {
title: conclusion === 'success'
? 'Trusted evidence verified'
: 'Trusted evidence blocked',
summary: summary.slice(0, 60000)
}
});
if (conclusion !== 'success') {
core.setFailed(verdict.reason || 'trusted evidence blocked');
}
github-***REDACTED_SECRET_ASSIGNMENT***
debug: false
user-agent: actions/github-script
result-encoding: json
retries: 0
retry-exempt-status-codes: 400,401,403,404,422
env:
PR: 1220
##[endgroup]
##[error]missing_trusted_publication
GitHub Actions: Agent completion enforcement / Agent completion enforcement: feat(config): share validated env parsing for tunable concurrency constants
Conclusion: failure
##[group]Run actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3
with:
script: const fs = require('fs');
const pull = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: Number(process.env.PR)
});
let verdict = {
conclusion: 'failure',
reason: 'verifier_did_not_publish',
details: {}
};
try {
verdict = JSON.parse(fs.readFileSync(
'enforcement-verdict.json', 'utf8'
));
} catch (error) {
core.warning(error.message);
}
const conclusion = verdict.conclusion === 'success'
? 'success'
: 'failure';
const summary = JSON.stringify(verdict);
await github.rest.checks.create({
owner: context.repo.owner,
repo: context.repo.repo,
name: 'Agent completion enforcement',
head_sha: pull.data.head.sha,
status: 'completed',
conclusion,
output: {
title: conclusion === 'success'
? 'Trusted evidence verified'
: 'Trusted evidence blocked',
summary: summary.slice(0, 60000)
}
});
if (conclusion !== 'success') {
core.setFailed(verdict.reason || 'trusted evidence blocked');
}
github-***REDACTED_SECRET_ASSIGNMENT***
debug: false
user-agent: actions/github-script
result-encoding: json
retries: 0
retry-exempt-status-codes: 400,401,403,404,422
env:
PR: 1220
##[endgroup]
##[error]missing_trusted_publication
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{py,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,js,jsx,ts,tsx}: Use Python 3.9+ and Node 18+ for development
Never hardcode API keys, database URLs, or secrets in code
Make minimal, surgical changes and avoid deleting working code unless fixing security issues
Files:
src/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.pysrc/youtube_extension/backend/services/intelligent_cache.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always use type hints for Python functions
Use Black formatter with 88 character line length for Python code
Follow PEP 8 conventions for Python code
Use descriptive variable names and add docstrings to all public functions in Python
Group imports in Python: standard library, third-party, local
Use SQLAlchemy ORM and never write raw SQL queries
Use environment variables via os.getenv() or pydantic-settings to access configuration
Wrap database operations in try-except blocks and use context managers or FastAPI dependencies for connection cleanup
Implement comprehensive error handling with proper logging in all functions
Version APIs using /api/v1/ prefix for stability
Use JSON-RPC 2.0 protocol for all MCP communication
Follow the single-flow workflow: YouTube link → context extraction → agent dispatch → outputs
Use context managers for resource management in Python code
Implement comprehensive input validation and sanitize outputs for security
Use parameterized queries and SQLAlchemy ORM to prevent SQL injection
Define API request/response models using Pydantic for FastAPI endpoints
Store the single unified workflow as the only workflow; never introduce alternate flows or manual triggers
Use SQLAlchemy with connection pooling for database connections and manage sessions with context managers
Provide sensible defaults for non-sensitive configuration in Python settings
Maintain backward compatibility and do not break existing API endpoints
Include comprehensive logging for debugging in MCP implementations
Files:
src/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.pysrc/youtube_extension/backend/services/intelligent_cache.py
⚙️ CodeRabbit configuration file
Python backend code. Check for type hints, proper exception handling, async context manager usage, and potential blocking calls in async functions. Flag any bare except clauses or missing timeout parameters on network calls. CRITICAL: Flag any file that contains placeholder/stub implementations — especially in unified_ai_sdk. Any class or function that says "TODO: Replace with production implementation" or returns mock/fake data must be flagged as a blocking issue. Flag any code generation output that reaches users without AST validation or syntax checking.
Files:
src/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.pysrc/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain >80% code coverage for new features
Files:
src/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.pysrc/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,ts,tsx}: Keep frontend and backend data models synchronized using matching Pydantic (backend) and TypeScript (frontend) interfaces
Use type-safe interfaces for backend-frontend data exchange
**/*.{py,ts,tsx}: Production code must use real behavior only: no mock delays, fake data, or simulated responses.
Maintain strict type safety: mypy strict mode for Python and TypeScript strict mode for the frontend.
Name events using the<domain>.<entity>.<action>format.
Files:
src/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.pysrc/youtube_extension/backend/services/intelligent_cache.py
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Strictly verify that GitHub Copilot has explicitly reviewed and approved the pull request; human approvals alone must not satisfy this check.
Before allowing a merge, require thecopilot-rabbitlabel and AI-generated unit tests committed alongside the code changes; fail the check if either is missing.For Vercel-specific work, include
https://vercel.com/docs/llms-full.txtin the AI assistant context set.
Files:
src/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.pysrc/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,pyw}
📄 CodeRabbit inference engine (AGENTS.md)
Write Python code to remain compatible with Linux and Windows where possible, including correct handling of
asyncioevent loops.
Files:
src/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.pysrc/youtube_extension/backend/services/intelligent_cache.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Format Python code with Black using an 88-character line length.
Sort Python imports with isort using the Black profile.
Run Ruff with E, W, F, I, B, C4, and UP rules; E501 is ignored.
Use mypy strict mode; untyped function definitions are disallowed.
Target Python 3.9 or newer.
Validate backend inputs with Pydantic and sanitize subprocess arguments.
Use Anthropic SDK featuresthinking={"type": "adaptive"}andoutput_config={"effort": "..."}withanthropic>=0.105.0; do not addTypeErrorfallbacks for these parameters.Use the service container dependency-injection pattern in
backend/containers/.
Files:
src/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.pysrc/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not commit secrets; store keys and credentials in gitignored
.envfiles.
Files:
src/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.pysrc/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (GEMINI.md)
**/*.{py,pyi}: Use Black formatting with an 88-character line length for Python code.
Use Ruff with rules E, W, F, I, B, C4, and UP for Python linting.
Use strict mypy type checking; do not define untyped functions.
Target Python 3.9 or newer.
Validate Python inputs with Pydantic.
Sanitize subprocess arguments before execution.
Do not add mock delays or fake data to production Python code; production runs in REAL_MODE_ONLY.
Keep secrets out of Python source code; load keys and credentials from environment variables instead.
Use absolute imports that resolve withPYTHONPATH=srcin the Python backend.
Files:
src/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.pysrc/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,pyi,ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
**/*.{py,pyi,ts,tsx}: Preserve the single workflow: YouTube link → transcript → events → agents → outputs; do not introduce alternative flows or manual triggers that bypass it.
Use event names following<domain>.<entity>.<action>, such asyoutube.video.captured.
Make surgical, precise changes and do not delete working code without justification.
Files:
src/youtube_extension/core/env_config.pysrc/youtube_extension/services/cloud/firestore_state.pysrc/youtube_extension/backend/services/intelligent_cache.py
🔍 Remote MCP GitHub Copilot, Linear
Relevant review context
- PR
#1220is open againstmainand changes 5 files. Its current implementation validates only a minimum of 1; Firestore createsmin(CLEANUP_DELETE_CONCURRENCY, len(docs))workers, so an extremely large override can defeat the intended task-memory bound. This is an unresolved review thread. - There is a requirements mismatch: the Linear triage note explicitly expects malformed values such as
abc,-1, and0to log and fall back, while this PR intentionally raisesValueErrorduring import. The new tests assert the fail-fast behavior rather than fallback. - The Redis path clamps its configured value to the connection-pool budget via
_resolve_tag_write_limit(), but no equivalent use-site cap exists for Firestore cleanup. - The Firestore timeout behavior is pre-existing from merged PR
#1170; this PR only moves its parser into the shared module. - At the current head, tests, coverage, lint, CodeQL, security scans, and build checks are successful.
Agent completion enforcementreports failure, and the top-level Trivy check is neutral.
🔇 Additional comments (1)
src/youtube_extension/core/env_config.py (1)
67-72: 🎯 Functional CorrectnessResolve the invalid-override contract before merge.
The Linear triage requirement says malformed values must log and fall back. These branches instead terminate module import. The PR objective specifies fail-fast behavior. Confirm the approved contract, then update both parsers and their tests to match it.
src/youtube_extension/core/env_config.py#L67-L72: apply the approved invalid-integer behavior.src/youtube_extension/core/env_config.py#L98-L107: apply the same approved invalid-float behavior.Source: MCP tools
| raw = _raw_override(name) | ||
| if raw is None: | ||
| return default | ||
| try: | ||
| value = int(raw) | ||
| except ValueError: | ||
| raise ValueError(f"{name} must be an integer >= 1, got {raw!r}") from None | ||
| if value < 1: | ||
| raise ValueError(f"{name} must be >= 1, got {raw!r}") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the declared Pydantic version and existing validation conventions.
fd -a -t f '^(pyproject\.toml|requirements.*\.txt|poetry\.lock|uv\.lock)$' . \
-x sh -c 'echo "--- $1"; rg -n "pydantic" "$1" || true' sh {}
rg -n -C 2 'TypeAdapter|BaseModel|BaseSettings|field_validator|validator' \
-g '*.py' .Repository: groupthinking/EventRelay
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the target env_config.py file and exact validation helpers.
printf '--- env_config.py outline ---\n'
ast-grep outline src/youtube_extension/core/env_config.py --view compact || true
printf '\n--- env_config.py relevant sections ---\n'
sed -n '1,130p' src/youtube_extension/core/env_config.py | cat -n
printf '\n--- Pydantic validation occurrences in src --max-count 50 ---\n'
rg -n -C 2 'TypeAdapter|BaseModel|BaseSettings|field_validator|validator|field_validation' -g '*.py' src/ --max-count 50 || trueRepository: groupthinking/EventRelay
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the target env_config.py file and exact validation helpers.
printf '%s\n' '--- env_config.py relevant sections ---'
sed -n '1,130p' src/youtube_extension/core/env_config.py | cat -n
printf '%s\n' ''
printf '%s\n' '--- Pydantic validation occurrences in src --max-count 50 ---'
rg -n -C 2 'TypeAdapter|BaseModel|BaseSettings|field_validator|validator|field_validation' -g '*.py' src/ --max-count 50 || trueRepository: groupthinking/EventRelay
Length of output: 50380
Use a reusable Pydantic validator for environment values.
These helpers run manual int/float parsing plus bounds checks. Use Pydantic for these overrides and preserve the current blank fallback and invalid-value failure behavior.
📍 Affects 1 file
src/youtube_extension/core/env_config.py#L64-L72(this comment)src/youtube_extension/core/env_config.py#L95-L107
🤖 Prompt for 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.
In `@src/youtube_extension/core/env_config.py` around lines 64 - 72, The integer
and float environment helpers in `src/youtube_extension/core/env_config.py` at
lines 64-72 and 95-107 should use a shared Pydantic validator instead of manual
`int`/`float` conversion and bounds checks; preserve returning the default for
blank or missing overrides and retain the existing invalid-value errors and
minimum-value constraints at both sites.
Source: Coding guidelines
Update after CodeRabbit's full review (
|
|
Returned to draft during delivery control. The implementation is linked to #1180, but the current active-agent receipt and independent exact-head review/deployment evidence must be reconciled before state advancement. |
|
@coderabbitai review Please re-review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='96c53e2d0adce2fb68730937719958bf6d2c30a2'
actual="$(git rev-parse HEAD)"
printf 'HEAD=%s\nEXPECTED=%s\n' "$actual" "$expected"
echo '--- changed files at requested commit ---'
git diff-tree --no-commit-id --name-status -r "$expected"
echo '--- diff against first parent ---'
git diff --find-renames "${expected}^" "$expected" -- \
src/youtube_extension/core/env_config.py \
src/youtube_extension/services/cloud/firestore_state.py \
src/youtube_extension/backend/services/intelligent_cache.py \
tests
echo '--- source file outlines ---'
for f in \
src/youtube_extension/core/env_config.py \
src/youtube_extension/services/cloud/firestore_state.py \
src/youtube_extension/backend/services/intelligent_cache.py
do
if [ -f "$f" ]; then
printf '\n### %s\n' "$f"
ast-grep outline "$f" --items all || true
fi
done
echo '--- parser, cap, and fallback references with context ---'
rg -n -C 5 \
'positive_int_env|positive_finite_float_env|CLEANUP_DELETE_CONCURRENCY|CLEANUP_DELETE_TIMEOUT_SECONDS|TAG_WRITE_CONCURRENCY|_resolve_tag_write_limit|logging|logger\.' \
src/youtube_extension tests 2>/dev/null || true
echo '--- relevant test definitions ---'
rg -n -C 3 \
'positive_int_env|positive_finite_float_env|CLEANUP_DELETE_CONCURRENCY|TAG_WRITE_CONCURRENCY|fallback|ceiling|cap' \
tests 2>/dev/null || true
echo '--- PR check state ---'
gh pr checks 1220 --repo groupthinking/EventRelay || trueLength of output: 201854 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='96c53e2d0adce2fb68730937719958bf6d2c30a2'
echo '--- commit verification ---'
git rev-parse HEAD
git show -s --format='commit=%H%nparents=%P%nsubject=%s' "$commit"
echo '--- files changed by this commit ---'
git diff-tree --no-commit-id --name-status -r "$commit"
echo '--- focused patch ---'
git diff --find-renames --unified=80 "${commit}^" "$commit" -- \
src/youtube_extension/core/env_config.py \
src/youtube_extension/services/cloud/firestore_state.py \
src/youtube_extension/backend/services/intelligent_cache.py \
tests \
test
echo '--- source at requested commit: env_config.py ---'
git show "${commit}:src/youtube_extension/core/env_config.py" | nl -ba | sed -n '1,260p'
echo '--- source at requested commit: firestore_state.py relevant references ---'
git show "${commit}:src/youtube_extension/services/cloud/firestore_state.py" \
| nl -ba \
| grep -E -C 12 'CLEANUP_DELETE|positive_|create_task|gather|Semaphore|delete' \
|| true
echo '--- source at requested commit: intelligent_cache.py relevant references ---'
git show "${commit}:src/youtube_extension/backend/services/intelligent_cache.py" \
| nl -ba \
| grep -E -C 12 'TAG_WRITE|positive_|_resolve_tag_write_limit|create_task|gather|Semaphore' \
|| true
echo '--- changed test files and their exact content ---'
git diff-tree --no-commit-id --name-only -r "$commit" \
| grep -E '(^|/)(test|tests)[^/]*.*\.py$' \
| while IFS= read -r file; do
echo "### $file"
git show "${commit}:$file" | nl -ba | sed -n '1,360p'
done
echo '--- annotations for failed/pending checks ---'
for run in 30750022538 30750022540 30750022556 30750023490 30750023518; do
echo "### run $run"
gh api "repos/groupthinking/EventRelay/actions/runs/$run" \
--jq '{name,status,conclusion,event,head_sha,html_url}' || true
gh api "repos/groupthinking/EventRelay/actions/runs/$run/jobs?filter=latest" \
--jq '.jobs[] | select(.conclusion == "failure" or .status != "completed") | {name,status,conclusion,html_url,steps:[.steps[]? | select(.conclusion == "failure") | {name,number,conclusion}]}' \
|| true
doneLength of output: 40057 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='96c53e2d0adce2fb68730937719958bf6d2c30a2'
show_range() {
local path="$1"
local start="$2"
local end="$3"
echo "### ${path}:${start}-${end}"
git show "${commit}:${path}" \
| sed -n "${start},${end}p" \
| awk -v base="$start" '{printf "%6d %s\n", NR + base - 1, $0}'
}
show_range src/youtube_extension/services/cloud/firestore_state.py 1 65
show_range src/youtube_extension/services/cloud/firestore_state.py 260 430
show_range src/youtube_extension/backend/services/intelligent_cache.py 20 75
show_range src/youtube_extension/backend/services/intelligent_cache.py 520 690
show_range tests/unit/test_env_config.py 1 260
show_range tests/unit/test_firestore_state.py 780 1040
echo '--- package-root and import use sites ---'
git grep -nE '^from (src\.)?youtube_extension\.core\.env_config import|^from \.{2,}core\.env_config import' "$commit" -- \
'src/**/*.py' || true
echo '--- current workflow failures ---'
for run in 30750022538 30750022540 30750022556; do
echo "### failed log for workflow run $run"
gh run view "$run" --repo groupthinking/EventRelay --log-failed 2>&1 | tail -n 180 || true
done
echo '--- latest check state ---'
gh pr checks 1220 --repo groupthinking/EventRelay || trueLength of output: 108811
Verified:
CI status:
The remaining failures are workflow infrastructure failures. They are not caused by this code change. ✅ Action performedReview finished.
|
|
Reviewed the new head
On the absolute imports: withdrawing my earlier objection. The concern (a second No code issues from me on Generated by Claude Code |
The fail-safe rework left three lint regressions relative to the branch point, and dropped six malformed-input cases from the parametrize lists. - ruff I001: the absolute import in firestore_state.py is 89 chars, one over the limit, so the import block needed rewrapping. - black: _fallback()'s signature and the expected-value ternary in test_env_config.py both exceeded 88 chars. - Re-add the invalid inputs dropped in the rewrite: -42, 8x, 0x10 for int; NaN, 0.0, 12s for float. Each exercises a distinct rejection path (parse error, range check, finiteness check). Pre-existing debt left untouched: firestore_state.py is already black-dirty on origin/main, and the unused 'result' at test_firestore_state.py:530 predates this branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai review Please review exact head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="groupthinking/EventRelay"
pr="1220"
commit="1fbfe0aacc1064a13d99b37223d9f82565a42209"
echo '--- exact-head verification ---'
printf 'checked_out=%s\n' "$(git rev-parse HEAD)"
git show -s --format='commit=%H%nparents=%P%nsubject=%s%ncommitted=%cI' "$commit"
echo '--- commit file list ---'
git diff-tree --no-commit-id --name-status -r "$commit"
echo '--- commit patch ---'
git diff --find-renames --unified=100 "${commit}^" "$commit" -- \
src/youtube_extension/core/env_config.py \
src/youtube_extension/services/cloud/firestore_state.py \
src/youtube_extension/backend/services/intelligent_cache.py \
tests test
echo '--- affected-file outlines ---'
git diff-tree --no-commit-id --name-only -r "$commit" |
while IFS= read -r file; do
case "$file" in
*.py)
echo "### $file"
ast-grep outline "$file" --items all || true
;;
esac
done
echo '--- environment configuration and concurrency references at exact head ---'
git grep -n -E \
'positive_int_env|positive_finite_float_env|CLEANUP_DELETE_CONCURRENCY|CLEANUP_DELETE_CONCURRENCY_MAX|TAG_WRITE_CONCURRENCY|_resolve_tag_write_limit|create_task|Semaphore|gather' \
"$commit" -- src/youtube_extension tests test || true
echo '--- exact-head pull request checks ---'
gh pr checks "$pr" --repo "$repo" || true
echo '--- non-successful exact-head workflow runs ---'
gh api "repos/${repo}/actions/runs?head_sha=${commit}&per_page=100" \
--jq '.workflow_runs[] | select(.status != "completed" or (.conclusion != "success" and .conclusion != "skipped" and .conclusion != "neutral")) | {id,name,status,conclusion,html_url}' \
|| trueLength of output: 59024 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="groupthinking/EventRelay"
pr="1220"
commit="1fbfe0aacc1064a13d99b37223d9f82565a42209"
run="30750464152"
echo '--- pull request head at completion ---'
gh pr view "$pr" --repo "$repo" --json headRefOid,isDraft,state \
--jq '{headRefOid,isDraft,state}'
echo '--- branch-cleanup workflow metadata ---'
gh api "repos/${repo}/actions/runs/${run}" \
--jq '{id,name,event,status,conclusion,head_sha,head_branch,workflow_id,created_at,updated_at,html_url}'
echo '--- failed jobs and failed steps ---'
gh api "repos/${repo}/actions/runs/${run}/jobs?filter=latest" \
--jq '.jobs[] | {name,status,conclusion,started_at,completed_at,html_url,steps:[.steps[]? | select(.conclusion == "failure") | {number,name,status,conclusion}]}'
echo '--- failed workflow log tail ---'
gh run view "$run" --repo "$repo" --log-failed 2>&1 | tail -n 200 || true
echo '--- check-run association for requested commit ---'
gh api "repos/${repo}/commits/${commit}/check-runs?per_page=100" \
--jq '.check_runs[] | select(.conclusion == "failure" or .status != "completed") | {name,status,conclusion,details_url,app:.app.slug}' || trueLength of output: 2493
Verified:
Exact-head checks pass for test, coverage, build, Python lint, CodeQL, security scans, dependency review, and The separate You are interacting with an AI system. |
Share validated env parsing for tunable concurrency constants
Canonical issue
Closes #1180
The issue asked for two things, and a check on a third. Reading the current code first showed only one of them was actually outstanding:
mainbefore this PRfirestore_state.pyalready had it;intelligent_cache.pyhad a bareTAG_WRITE_CONCURRENCY = 8.CLEANUP_DELETE_TIMEOUT_SECONDSexists, is documented in-module, and the non-finite/non-positive rejection path is covered intest_firestore_state.py. Left alone.The issue also said, of the two constants: "If this is done, do both constants together behind one shared helper." That is what this PR does.
Outcome
TAG_WRITE_CONCURRENCYwas a hardcoded literal, so tuning Redis tag-write fan-out for a particular deployment required an application release. Its sibling infirestore_state.pywas already env-parsed — but the parser was private to that module, and had already been copy-pasted once intocloud_ai/providers/aws_rekognition.py. A third call site was going to copy it again.src/youtube_extension/core/env_config.pyholdspositive_int_envandpositive_finite_float_env.firestore_state.pyimports them instead of defining them.intelligent_cache.pywiresTAG_WRITE_CONCURRENCYthroughpositive_int_env("TAG_WRITE_CONCURRENCY", 8).Semantics: fail-safe, bounded
The original version of this PR made malformed input fail fast — raise at import. Review reversed that, and the reversal is right. The rule now is:
Ignoring invalid TAG_WRITE_CONCURRENCY='abc'; expected an integer >= 1. Using default 8.The governing constraint, in the reviewer's words, is that runtime tuning must not make a service unimportable. A typo'd concurrency override is a bad reason for a pod to CrashLoopBackOff. The diagnosability that fail-fast bought is preserved — the variable name and the bad value still reach the operator — it just arrives as a log line rather than a stack trace.
This also settles a genuine ambiguity in the issue text. The issue asked for "a validating parse with a fallback", which reads as fall back on malformed input too. The already-merged Firestore implementation had resolved it the other way. The current behaviour matches the issue's original wording.
Bounded overrides
positive_int_envnow takes an optionalmaximum. One call site uses it:Without a ceiling,
CLEANUP_DELETE_CONCURRENCY=100000would allocate roughly one task per queued document. An upper bound turns that from an outage into a log line.Why
core/core/env_config.pyrather thancore/config/orutils/: those two packages have eager__init__.pyimports (a logging stack and a proxy/video-utils stack respectively).core/__init__.pyis empty, so importing this helper pulls in nothing — which matters for a module read at import time by other modules.The original version of this PR used relative imports and argued that an absolute import would load a second copy of the package, because this repo contains both
youtube_extension...andsrc.youtube_extension...import roots. Review switched them to absolute, so I tested the claim instead of restating it. Both roots are real, and with both onsys.paththe two spellings do produce two distinct module objects. Butenv_configis a stateless pure function overos.environ— two copies compute identical values. The original argument was overstated, and the absolute import is safe.Risk
firestore_state.py. That is de-risked by the pre-existing tests for those parsers, which were repointed at the re-exported names and still pass unchanged.Verification
tests/unit/test_env_config.py— 45 tests, all passing. Covers unset, empty, whitespace-only, valid, surrounding-whitespace, and the full invalid matrix across both parsers, plus themaximumboundary (64 accepted, 65 rejected) and assertions that the warning names the variable and echoes the raw value.-42,8x,0x10for the int parser;NaN,0.0,12sfor the float parser. Each exercises a distinct rejection path — parse error, range check, finiteness check — rather than duplicating an existing case.importlib.reload. Reload would rebind the module's classes and leave the rest of the session holding stale references; a clean interpreter tests the real thing. Redis is not installed in CI, so the child process installs the samesys.modulesstubtest_intelligent_cache.pyuses.test_tag_write_limit_scales_down_for_small_pools, which assertsdefault._tag_write_limit == TAG_WRITE_CONCURRENCYand would catch a wiring mistake.ruffandblackrestored to the branch-point baseline. The rework had left three regressions: anI001(the absolute import infirestore_state.pyis 89 chars, one over the limit) and twoblackviolations (_fallback()'s signature and a ternary in the tests). All fixed.mypy --strictclean on the new module.Commands run:
Two pre-existing lint findings were deliberately not fixed, to keep the diff scoped:
firestore_state.pyis alreadyblack-dirty onorigin/main, and the unusedresultattest_firestore_state.py:530predates this branch. Both were confirmed againstorigin/mainrather than assumed.Production evidence
The acceptance criterion that actually protects production is #3 — no behaviour change when the env vars are unset. That is not asserted here, it is measured. Each constant is imported in a clean interpreter with the variable removed from the environment, and the resolved value is compared against the literal that shipped:
0)TAG_WRITE_CONCURRENCY8(was8)3→38, name + value in logCLEANUP_DELETE_CONCURRENCY16(was16)4→416, name + value in logCLEANUP_DELETE_TIMEOUT_SECONDS30.0(was30.0)2.5→2.530.0, name + value in logAll nine cells are parametrised tests in
TestTunableConstantWiring, so the "unset == shipped default" guarantee is enforced on every future run, not just observed once here.The third column is the operator-facing half, and it is the column the review changed. A bad override no longer stops the process — the service comes up on its documented default and says so in the log. The failure mode that was being guarded against, a service silently running on a limit nobody chose, is still covered, because the warning names both the variable and the value that was rejected.
Agent handoff
Two things were deliberately left out of this diff, both of which are follow-ups rather than omissions:
cloud_ai/providers/aws_rekognition.py:66still holds a duplicate_positive_int_env. It is outside this issue's stated scope, and that file is already modified by open PR fix(security): sandbox local media paths in cloud AI providers (#1209) #1216 — editing it from amain-based branch would create a self-inflicted merge conflict. It should be collapsed onto the shared helper once fix(security): sandbox local media paths in cloud AI providers (#1209) #1216 lands.TAG_WRITE_POOL_RESERVE = 4is left hardcoded. The issue asked for concurrency constants; a reserve is a headroom allowance for non-tag traffic, not a concurrency limit, and making it tunable would let an operator configure a pool with no headroom at all.Neither constant is documented in
.env.example, matching the existing precedent —CLEANUP_DELETE_CONCURRENCYis not documented there either. Both are explained where they are defined.