review: CI Trivy timeout + 5 smoke tests (retro review of 522da27c) - #842
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds 10-minute timeouts to Trivy scan steps in CI, updates the production audit dashboard to record those CI changes and fixed items, and introduces a suite of smoke tests plus helper utilities and a pre-commit validation script to validate env, NATS, ports, and Supabase self‑hosted configuration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md (1)
761-761: Clarify "5 scan steps" across 2 workflows.The changelog states "Trivy scan timeout increased to 10m in
integrations-ghcr.yml+self-hosted-builds-hardened.yml(5 scan steps)" but earlier text (lines 50, 130) mentions just "two CI workflows." This could confuse readers about whether 5 different scans were modified or if the 2 workflows contain 5 scan steps combined.Consider clarifying this as: "...increased to 10m across 5 Trivy scan steps in 2 CI workflows (
integrations-ghcr.ymlandself-hosted-builds-hardened.yml)" to make the relationship explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md` at line 761, The changelog line is ambiguous about whether "5 scan steps" refers to scans or workflows; update the sentence in PRODUCTION_AUDIT_DASHBOARD.md to explicitly state that the Trivy timeout was increased to 10m across five Trivy scan steps distributed in two CI workflows by rephrasing to something like: "Trivy scan timeout increased to 10m across 5 Trivy scan steps in 2 CI workflows (integrations-ghcr.yml and self-hosted-builds-hardened.yml)". Reference the workflow names integrations-ghcr.yml and self-hosted-builds-hardened.yml and keep the parenthetical listing to make the relationship explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md`:
- Line 761: The tests (test_supabase_network.py, test_neo4j_dual_ports.py,
test_network_configuration.py, test_nats_authentication.py) use hardcoded
"/home/pmoves" paths which break cross-platform runs; update each test to
compute project paths via pathlib.Path.cwd() or os.getcwd() (or an env var like
PMOVES_HOME with fallback to Path.cwd()) and derive any subpaths (e.g., project
root, fixtures, config files) from that base instead of the literal string,
replacing any occurrences of "/home/pmoves/PMOVES.AI" with the computed
base.joinpath(...) so tests become platform-agnostic and work on Windows, WSL,
and CI.
In `@pmoves/tests/smoke/test_environment_consistency.py`:
- Around line 116-128: The test test_env_shared_exists currently hard-fails if
PMOVES_DIR / "env.shared" is missing; change it to fall back to PMOVES_DIR /
"env.shared.example" or skip the test when env.shared is not present. Update the
logic in test_env_shared_exists to: check for env_shared existence, and if
missing look for env_example = PMOVES_DIR / "env.shared.example" and use that
file for extract_env_vars_from_file, otherwise call pytest.skip with a clear
message that env.shared is not provided in this environment; keep the subsequent
critical variable assertions unchanged so the test remains strict when a file is
available.
- Around line 382-405: The test test_critical_services_have_required_env_vars is
checking the wrong service name: update the call to
extract_env_vars_from_compose("hirag-v2") to use the actual compose service name
"hi-rag-gateway-v2" (or a shared constant if one exists) so the test exercises
the hi-rag-gateway-v2 service and validates that QDRANT_URL and
SUPABASE_REALTIME_URL are present; keep the rest of the logic unchanged
(referencing test_critical_services_have_required_env_vars and
extract_env_vars_from_compose).
In `@pmoves/tests/smoke/test_nats_configuration.py`:
- Around line 61-78: The test test_nats_service_has_documentation_header expects
documentation strings and a NATS_CONFIGURATION.md reference near the nats
service in pmoves/docker-compose.yml but the compose file doesn't contain them;
either add the documentation comment (include "NATS Message Bus" or "NATS
provides" and a reference to "NATS_CONFIGURATION.md") above the nats: service in
pmoves/docker-compose.yml, or relax the test by removing the header/string
assertions in test_nats_service_has_documentation_header (leaving only the
assert that the nats service exists) or broaden the grep/context check so it
matches the actual location of the documentation; update the test or compose
accordingly to keep test_nats_service_has_documentation_header and
pmoves/docker-compose.yml consistent.
In `@pmoves/tests/smoke/test_port_conflicts.py`:
- Around line 26-57: The helper extract_port_mappings_from_compose is missing
port entries because port_pattern only matches quoted literal mappings like
"8010:80" and ignores variables such as "${WGER_HOST_PORT:-8000}:80"; update the
function to actually parse the compose YAML (e.g., with yaml.safe_load) and
extract services.*.ports or, if you prefer string-based fix, normalize and
expand "${VAR:-PORT}" patterns before applying regex (handle unquoted entries
and optional /protocol suffix), replacing the current port_pattern logic so that
extract_port_mappings_from_compose returns host_port, container_port, protocol
for entries like ${VAR:-8000}:80 and unquoted mappings as well.
- Around line 339-345: The bug is that the nearby "ports:" check slices
characters from content using the line number i, so the context check fails; in
the loop that enumerates content.splitlines() (for i, line in
enumerate(content.splitlines(), 1)), build a lines list (e.g., lines =
content.splitlines()), then check the slice lines[max(0, i-1-5):i-1+5] for the
presence of "ports:" instead of using content[max(0, i-5):i+5]; update the
condition that appends to lines_with_54321 to use that line-based slice so a
"54321" mapping inside a nearby ports: section is detected correctly.
- Around line 221-225: Remove the subprocess.run grep call that checks for
"ports:" (the call using subprocess.run([... "grep", "-n", 'ports:',
str(compose_file)]) ) because the test already reads the compose_file later;
replace it with a pure-Python check against the file content (e.g., read the
file into a string and use 'ports:' in content) or simply rely on the subsequent
assertion after reading the file in test_port_conflicts; update/remove
references to the subprocess call and compose_file grep to keep the test
cross-platform and eliminate the unnecessary external dependency.
In `@pmoves/tests/smoke/test_supabase_realtime_tenant.py`:
- Around line 152-156: The current naive string replacement that builds ws_url
from SUPABASE_REALTIME_URL produces an invalid URL (e.g.,
"ws://localhost:4000/localhost:4000/socket/websocket"); instead, stop doing a
blind replace and either use SUPABASE_REALTIME_URL directly or rebuild the URL
with a proper URL parser: parse SUPABASE_REALTIME_URL, replace the netloc/host
with "localhost:4000" while preserving the original scheme and path, then pass
that well-formed ws_url into websockets.connect (the variables/functions to
change are SUPABASE_REALTIME_URL, ws_url, and the call to websockets.connect).
- Around line 108-112: The test currently uses subprocess.run with ["grep",
"-A", "...", "supabase-realtime:", COMPOSE_FILE] to slice the compose file,
which is non-portable and can truncate service blocks; replace these grep-based
checks by loading the compose YAML in
pmoves/tests/smoke/test_supabase_realtime_tenant.py (use the existing
COMPOSE_FILE variable) with a YAML parser (yaml.safe_load) and then directly
inspect the services mapping for keys like "supabase-realtime" and its nested
fields (e.g., "healthcheck", "depends_on", "ports", "networks") to assert
expected values; apply the same replacement for the other grep usages referenced
(around the other blocks at the indicated locations) so all assertions read from
the parsed dict instead of grepped text.
In `@pmoves/tests/smoke/test_supabase_selfhosted.py`:
- Around line 122-126: The test currently passes a literal "docker-compose*.yml"
to subprocess.run in the subprocess.run([...]) call so the glob isn't expanded;
replace that by expanding the pattern with Python's glob (or using PMOVES_DIR as
the grep target) so grep receives actual file paths. Specifically, import and
use glob.glob on str(PMOVES_DIR / "docker-compose*.yml") and pass the expanded
list (plus "grep", "-r", "54321") to subprocess.run, or alternatively call
subprocess.run(["grep","-r","54321", str(PMOVES_DIR)]) so the search runs over
the directory; update the subprocess.run invocation that sets result
accordingly.
In `@pmoves/tests/validate-changes.sh`:
- Around line 101-105: The check_yaml_syntax function in validate-changes.sh
currently only searches "pmoves" and thus misses changed workflow files under
.github/workflows; update check_yaml_syntax to include .github (or run find from
the repo root) so the find command scans both pmoves and .github/workflows for
*.yml/*.yaml files; specifically modify the yml_files assignment in
check_yaml_syntax to include the .github directory (or use a root-level find
with -path './.github/workflows' or similar) so edited workflow files are
validated as well.
- Around line 244-250: The port extraction in validate-changes.sh (variables
compose_files and ports) uses grep -P and a PCRE that misses env-expanded
mappings like "${WGER_HOST_PORT:-8000}:80" and is not portable; replace the grep
-oP '"\K[0-9]+(?=:)' call with a POSIX-compatible sed/awk pipeline that extracts
the host side before the colon for both numeric literals and parameter
expansions (e.g., match either a plain number or the default-value part of a
${VAR:-number} pattern), and ensure the new command is used where ports is set
so it works on BSD/macOS without -P and captures values like 8000 from both
"8000:80" and "${VAR:-8000}:80".
- Around line 113-116: The fallback YAML check in validate-changes.sh uses
python3 -c "import yaml; yaml.safe_load_all(open('$file'))" which returns an
unconsumed generator so syntax errors are never raised; change the python
invocation used in the fallback to consume the generator (e.g., wrap
safe_load_all(...) with list(...)) so the YAML is actually parsed and exceptions
surface, ensuring the if ! python3 ... branch correctly fails on malformed YAML.
---
Nitpick comments:
In `@pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md`:
- Line 761: The changelog line is ambiguous about whether "5 scan steps" refers
to scans or workflows; update the sentence in PRODUCTION_AUDIT_DASHBOARD.md to
explicitly state that the Trivy timeout was increased to 10m across five Trivy
scan steps distributed in two CI workflows by rephrasing to something like:
"Trivy scan timeout increased to 10m across 5 Trivy scan steps in 2 CI workflows
(integrations-ghcr.yml and self-hosted-builds-hardened.yml)". Reference the
workflow names integrations-ghcr.yml and self-hosted-builds-hardened.yml and
keep the parenthetical listing to make the relationship explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31517444-8f8e-44cd-958c-98d2a8c692df
📒 Files selected for processing (9)
.github/workflows/integrations-ghcr.yml.github/workflows/self-hosted-builds-hardened.ymlpmoves/docs/PRODUCTION_AUDIT_DASHBOARD.mdpmoves/tests/smoke/test_environment_consistency.pypmoves/tests/smoke/test_nats_configuration.pypmoves/tests/smoke/test_port_conflicts.pypmoves/tests/smoke/test_supabase_realtime_tenant.pypmoves/tests/smoke/test_supabase_selfhosted.pypmoves/tests/validate-changes.sh
|
|
||
| | Date | Change | | ||
| |------|--------| | ||
| | 2026-03-09 | **Post-cleanup sit rep:** Trivy scan timeout increased to 10m in `integrations-ghcr.yml` + `self-hosted-builds-hardened.yml` (5 scan steps). `PMOVES.AI-Edition-Hardened-Integrations` branch synced (was 234 commits behind main). 5 new smoke tests added (env consistency, port conflicts, NATS config, Supabase realtime, Supabase selfhosted) with cross-platform path resolution. Local Hardened branch pruned. urllib3 CVE-2026-21441 already fixed in submodule. | |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify smoke tests exist with proper markers and cross-platform compatibility
echo "=== Finding smoke test files ==="
fd -t f "^test_.*\.py$" pmoves/tests/smoke/
echo -e "\n=== Verifying pytest.mark.smoke markers ==="
rg -n "@pytest\.mark\.smoke" pmoves/tests/smoke/ --type py
echo -e "\n=== Checking for hardcoded Linux paths (e.g., /home/pmoves) ==="
rg -n "/home/pmoves|/tmp/|/var/" pmoves/tests/smoke/ --type py
echo -e "\n=== Checking for subprocess grep calls (Windows compatibility) ==="
rg -n "subprocess.*grep|subprocess.*'grep'" pmoves/tests/smoke/ --type py
echo -e "\n=== Running pytest collection ==="
cd pmoves && python -m pytest --collect-only tests/smoke/test_*.py 2>&1 | head -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 15060
Smoke tests exist with pytest markers, but hardcoded /home/pmoves paths break cross-platform compatibility.
The 5 new smoke tests exist with @pytest.mark.smoke markers and are properly discoverable. However, the claimed "cross-platform path resolution" is not implemented—hardcoded /home/pmoves paths appear in test_supabase_network.py, test_neo4j_dual_ports.py, test_network_configuration.py, and test_nats_authentication.py. These paths will fail on Windows, WSL with different user locations, and any CI/CD system without the exact /home/pmoves/PMOVES.AI layout. Replace hardcoded paths with os.getcwd(), pathlib.Path.cwd(), or environment-relative resolution to achieve actual cross-platform compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/docs/PRODUCTION_AUDIT_DASHBOARD.md` at line 761, The tests
(test_supabase_network.py, test_neo4j_dual_ports.py,
test_network_configuration.py, test_nats_authentication.py) use hardcoded
"/home/pmoves" paths which break cross-platform runs; update each test to
compute project paths via pathlib.Path.cwd() or os.getcwd() (or an env var like
PMOVES_HOME with fallback to Path.cwd()) and derive any subpaths (e.g., project
root, fixtures, config files) from that base instead of the literal string,
replacing any occurrences of "/home/pmoves/PMOVES.AI" with the computed
base.joinpath(...) so tests become platform-agnostic and work on Windows, WSL,
and CI.
Comment on lines
+116
to
+128
| @pytest.mark.smoke | ||
| def test_env_shared_exists() -> None: | ||
| """Verify env.shared exists and has required variables.""" | ||
| env_shared = PMOVES_DIR / "env.shared" | ||
| assert env_shared.exists(), "env.shared should exist" | ||
|
|
||
| env_vars = extract_env_vars_from_file(env_shared) | ||
|
|
||
| # Check for critical variables | ||
| critical_vars = ["NATS_URL", "POSTGRES_USER", "POSTGRES_PASSWORD"] | ||
| for var in critical_vars: | ||
| # Variable should be defined (value may be placeholder) | ||
| assert var in env_vars, f"{var} should be defined in env.shared" |
Contributor
There was a problem hiding this comment.
This smoke test depends on an untracked env file.
In the current repo context only pmoves/env.shared.example is present. Hard-failing on pmoves/env.shared makes clean CI checkouts fail before any environment-specific file is mounted. Consider skipping or falling back to the example file here, then keep the strict presence check only in environments that intentionally provide env.shared.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/smoke/test_environment_consistency.py` around lines 116 - 128,
The test test_env_shared_exists currently hard-fails if PMOVES_DIR /
"env.shared" is missing; change it to fall back to PMOVES_DIR /
"env.shared.example" or skip the test when env.shared is not present. Update the
logic in test_env_shared_exists to: check for env_shared existence, and if
missing look for env_example = PMOVES_DIR / "env.shared.example" and use that
file for extract_env_vars_from_file, otherwise call pytest.skip with a clear
message that env.shared is not provided in this environment; keep the subsequent
critical variable assertions unchanged so the test remains strict when a file is
available.
Comment on lines
+382
to
+405
| def test_critical_services_have_required_env_vars() -> None: | ||
| """Verify critical services have their required environment variables defined.""" | ||
| # Check hirag-v2 service | ||
| hirag_vars = extract_env_vars_from_compose("hirag-v2") | ||
|
|
||
| if hirag_vars: | ||
| required_vars = [ | ||
| "QDRANT_URL", | ||
| "SUPABASE_REALTIME_URL", | ||
| ] | ||
|
|
||
| for var in required_vars: | ||
| # Variable should be defined (as literal or env reference) | ||
| found = False | ||
| for env_var in hirag_vars: | ||
| if var in env_var or var in hirag_vars: | ||
| found = True | ||
| break | ||
|
|
||
| assert found, ( | ||
| f"hirag-v2 service should have {var} defined in docker-compose.yml" | ||
| ) | ||
| else: | ||
| pytest.skip("hirag-v2 service not found in docker-compose.yml") |
Contributor
There was a problem hiding this comment.
This never validates the actual Hi-RAG v2 service.
extract_env_vars_from_compose("hirag-v2") doesn't match the current compose service name hi-rag-gateway-v2, so the test always hits the skip branch and never asserts QDRANT_URL or SUPABASE_REALTIME_URL.
🛠️ Suggested fix
- hirag_vars = extract_env_vars_from_compose("hirag-v2")
+ hirag_vars = (
+ extract_env_vars_from_compose("hi-rag-gateway-v2")
+ or extract_env_vars_from_compose("hirag-v2")
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/smoke/test_environment_consistency.py` around lines 382 - 405,
The test test_critical_services_have_required_env_vars is checking the wrong
service name: update the call to extract_env_vars_from_compose("hirag-v2") to
use the actual compose service name "hi-rag-gateway-v2" (or a shared constant if
one exists) so the test exercises the hi-rag-gateway-v2 service and validates
that QDRANT_URL and SUPABASE_REALTIME_URL are present; keep the rest of the
logic unchanged (referencing test_critical_services_have_required_env_vars and
extract_env_vars_from_compose).
Comment on lines
+152
to
+156
| ws_url = SUPABASE_REALTIME_URL.replace("ws://", "ws://localhost:4000/") | ||
|
|
||
| # Try to connect with a timeout | ||
| try: | ||
| async with websockets.connect(ws_url, close_timeout=5) as ws: |
Contributor
There was a problem hiding this comment.
This rewrites the default realtime URL into an invalid WebSocket URL.
With the default SUPABASE_REALTIME_URL, Line 152 produces ws://localhost:4000/localhost:4000/socket/websocket, so the upgrade check never targets the real endpoint. Use the configured URL directly, or rebuild it with a proper URL parser.
🛠️ Suggested fix
- ws_url = SUPABASE_REALTIME_URL.replace("ws://", "ws://localhost:4000/")
+ ws_url = SUPABASE_REALTIME_URL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/smoke/test_supabase_realtime_tenant.py` around lines 152 - 156,
The current naive string replacement that builds ws_url from
SUPABASE_REALTIME_URL produces an invalid URL (e.g.,
"ws://localhost:4000/localhost:4000/socket/websocket"); instead, stop doing a
blind replace and either use SUPABASE_REALTIME_URL directly or rebuild the URL
with a proper URL parser: parse SUPABASE_REALTIME_URL, replace the netloc/host
with "localhost:4000" while preserving the original scheme and path, then pass
that well-formed ws_url into websockets.connect (the variables/functions to
change are SUPABASE_REALTIME_URL, ws_url, and the call to websockets.connect).
Comment on lines
+101
to
+105
| check_yaml_syntax() { | ||
| log_info "Checking YAML syntax..." | ||
| local yml_files | ||
| yml_files=$(find pmoves -name "*.yml" -o -name "*.yaml" 2>/dev/null || true) | ||
|
|
Contributor
There was a problem hiding this comment.
This YAML check skips the workflow files changed in this PR.
find pmoves ... never scans .github/workflows, so validate-changes.sh can report success while either edited workflow file contains invalid YAML. If this script is meant to validate repo changes pre-commit, include .github in the search set.
🛠️ Suggested fix
- yml_files=$(find pmoves -name "*.yml" -o -name "*.yaml" 2>/dev/null || true)
+ yml_files=$(find pmoves .github \( -name "*.yml" -o -name "*.yaml" \) 2>/dev/null || true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/validate-changes.sh` around lines 101 - 105, The
check_yaml_syntax function in validate-changes.sh currently only searches
"pmoves" and thus misses changed workflow files under .github/workflows; update
check_yaml_syntax to include .github (or run find from the repo root) so the
find command scans both pmoves and .github/workflows for *.yml/*.yaml files;
specifically modify the yml_files assignment in check_yaml_syntax to include the
.github directory (or use a root-level find with -path './.github/workflows' or
similar) so edited workflow files are validated as well.
Comment on lines
+113
to
+116
| if ! yamllint "$file" -d relaxed 2>/dev/null; then | ||
| # yamllint might not be installed, try basic check | ||
| if ! python3 -c "import yaml; yaml.safe_load_all(open('$file'))" 2>/dev/null; then | ||
| log_error "YAML syntax error in: $file" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import io
import yaml
gen = yaml.safe_load_all(io.StringIO("a: [1, 2\n"))
print(type(gen).__name__)
try:
next(gen)
except yaml.YAMLError as exc:
print(type(exc).__name__)
PYRepository: POWERFULMOVES/PMOVES.AI
Length of output: 88
🏁 Script executed:
fd validate-changes.shRepository: POWERFULMOVES/PMOVES.AI
Length of output: 99
🏁 Script executed:
cat -n pmoves/tests/validate-changes.sh | sed -n '110,120p'Repository: POWERFULMOVES/PMOVES.AI
Length of output: 549
🏁 Script executed:
#!/bin/bash
# Test the current implementation (should NOT error on malformed YAML)
python3 -c "import yaml; yaml.safe_load_all(open('/tmp/broken.yaml'))" 2>/dev/null
echo "Current impl exit code: $?"
# Test the proposed implementation (should error on malformed YAML)
cat > /tmp/test_proposed.py <<'EOF'
import pathlib
import sys
import yaml
with pathlib.Path('/tmp/broken.yaml').open(encoding="utf-8") as fh:
list(yaml.safe_load_all(fh))
EOF
# Create a broken YAML file
cat > /tmp/broken.yaml <<'EOF'
a: [1, 2
EOF
# Test current
python3 -c "import yaml; yaml.safe_load_all(open('/tmp/broken.yaml'))" 2>/dev/null
echo "Current impl exit code: $?"
# Test proposed
python3 /tmp/test_proposed.py 2>/dev/null
echo "Proposed impl exit code: $?"Repository: POWERFULMOVES/PMOVES.AI
Length of output: 145
The YAML validation fallback silently approves malformed YAML files.
When yamllint is unavailable, the fallback uses yaml.safe_load_all(open('$file')) which returns a generator without consuming it. Since the generator is never iterated, no exception is raised and the command exits with code 0 even for broken YAML (e.g., unclosed brackets). This means invalid YAML files pass validation on systems without yamllint installed.
Consume the generator by wrapping it with list():
Suggested fix
- if ! python3 -c "import yaml; yaml.safe_load_all(open('$file'))" 2>/dev/null; then
+ if ! python3 - <<'PY' "$file" 2>/dev/null; then
+import pathlib
+import sys
+import yaml
+
+with pathlib.Path(sys.argv[1]).open(encoding="utf-8") as fh:
+ list(yaml.safe_load_all(fh))
+PY
log_error "YAML syntax error in: $file"
((failed++))
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/validate-changes.sh` around lines 113 - 116, The fallback YAML
check in validate-changes.sh uses python3 -c "import yaml;
yaml.safe_load_all(open('$file'))" which returns an unconsumed generator so
syntax errors are never raised; change the python invocation used in the
fallback to consume the generator (e.g., wrap safe_load_all(...) with list(...))
so the YAML is actually parsed and exceptions surface, ensuring the if ! python3
... branch correctly fails on malformed YAML.
Comment on lines
+244
to
+250
| compose_files=$(find pmoves -name "docker-compose*.yml" 2>/dev/null || true) | ||
|
|
||
| while IFS= read -r file; do | ||
| # Extract port mappings (e.g., "8000:80" -> external port 8000) | ||
| local ports | ||
| ports=$(grep -oP '"\K[0-9]+(?=:)' "$file" 2>/dev/null || true) | ||
| while IFS= read -r port; do |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the specific lines mentioned
sed -n '244,250p' pmoves/tests/validate-changes.shRepository: POWERFULMOVES/PMOVES.AI
Length of output: 379
🏁 Script executed:
# Get context around those lines
sed -n '240,260p' pmoves/tests/validate-changes.shRepository: POWERFULMOVES/PMOVES.AI
Length of output: 804
🏁 Script executed:
# Check what docker-compose files exist in the repo
find . -name "docker-compose*.yml" -type f 2>/dev/nullRepository: POWERFULMOVES/PMOVES.AI
Length of output: 2733
🏁 Script executed:
# Look for environment variable references in docker-compose files
grep -r '\${' --include="*.yml" . 2>/dev/null | grep -i portRepository: POWERFULMOVES/PMOVES.AI
Length of output: 9939
The port-conflict scan misses env-expanded mappings and isn't portable.
The grep pattern '"\K[0-9]+(?=:)' only matches literal digits immediately after an opening quote and won't extract environment variable expanded ports like "${WGER_HOST_PORT:-8000}:80" (present in pmoves/docker-compose.external.yml and used throughout the codebase). Additionally, -P isn't available in BSD/macOS grep, making this non-portable. The validator can claim "no conflicts" while silently skipping real host ports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/validate-changes.sh` around lines 244 - 250, The port extraction
in validate-changes.sh (variables compose_files and ports) uses grep -P and a
PCRE that misses env-expanded mappings like "${WGER_HOST_PORT:-8000}:80" and is
not portable; replace the grep -oP '"\K[0-9]+(?=:)' call with a POSIX-compatible
sed/awk pipeline that extracts the host side before the colon for both numeric
literals and parameter expansions (e.g., match either a plain number or the
default-value part of a ${VAR:-number} pattern), and ensure the new command is
used where ports is set so it works on BSD/macOS without -P and captures values
like 8000 from both "8000:80" and "${VAR:-8000}:80".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
pmoves/tests/smoke/test_environment_consistency.py (2)
99-109:⚠️ Potential issue | 🟡 MinorTest hard-fails on missing
env.shared.CI checkouts without
env.shared(it may be gitignored) will fail this test. Consider falling back toenv.shared.exampleor skipping.🛠️ Suggested fix
def test_env_shared_exists() -> None: """Verify env.shared exists and has required variables.""" env_shared = PMOVES_DIR / "env.shared" - assert env_shared.exists(), "env.shared should exist" + if not env_shared.exists(): + env_example = PMOVES_DIR / "env.shared.example" + if env_example.exists(): + env_shared = env_example + else: + pytest.skip("env.shared not found (may be gitignored)"),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/smoke/test_environment_consistency.py` around lines 99 - 109, The test_env_shared_exists test currently hard-fails when PMOVES_DIR / "env.shared" is missing; update test_env_shared_exists to first check for env.shared and if missing attempt to use PMOVES_DIR / "env.shared.example" (using the same extract_env_vars_from_file) and only fail if neither exists, or mark/skip the test when both are missing (pytest.skip) to avoid CI false-negatives; reference PMOVES_DIR, env_shared, env_shared_example, test_env_shared_exists and extract_env_vars_from_file when making the change.
349-349:⚠️ Potential issue | 🟡 MinorService name
hirag-v2may not match actual compose service.If the compose file uses
hi-rag-gateway-v2instead ofhirag-v2, this test will always skip. Consider checking both names.🛠️ Suggested fix
- hirag_vars = extract_env_vars_from_compose("hirag-v2") + hirag_vars = ( + extract_env_vars_from_compose("hi-rag-gateway-v2") + or extract_env_vars_from_compose("hirag-v2") + ),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/smoke/test_environment_consistency.py` at line 349, The test currently calls extract_env_vars_from_compose("hirag-v2"), which may not match the actual compose service name; update the test to try both service names (e.g., "hirag-v2" and "hi-rag-gateway-v2") and use the first non-empty result as hirag_vars (or assert with both possibilities), modifying the invocation site in test_environment_consistency.py so extract_env_vars_from_compose is called for each candidate and falls back appropriately to ensure the test doesn't always skip when the compose service is named "hi-rag-gateway-v2".pmoves/tests/validate-changes.sh (3)
104-104:⚠️ Potential issue | 🟡 MinorYAML check misses
.github/workflowsfiles.The find command only searches
pmoves, so CI workflow files edited in this PR won't be validated.🛠️ Suggested fix
- yml_files=$(find pmoves -name "*.yml" -o -name "*.yaml" 2>/dev/null || true) + yml_files=$(find pmoves .github \( -name "*.yml" -o -name "*.yaml" \) 2>/dev/null || true),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/validate-changes.sh` at line 104, The current assignment to yml_files uses find only on the pmoves directory so workflow YAML edits under .github/workflows are skipped; update the find invocation that populates yml_files (the variable defined as yml_files=$(find ...)) to also search the .github/workflows directory (e.g., include .github/workflows as an extra search path) while preserving the existing -name filters and the 2>/dev/null || true guard so missing directories don't break the script.
249-249:⚠️ Potential issue | 🟡 MinorPort extraction misses env-var expanded mappings.
The pattern
grep -oE '"[0-9]+:'only matches literal numeric ports. Mappings like"${WGER_HOST_PORT:-8000}:80"are skipped. This is an improvement over the previous-PPCRE pattern (now portable), but still incomplete.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/validate-changes.sh` at line 249, The current ports extraction (the assignment to the variable named ports) only matches literal numeric mappings and misses env-var expanded mappings like "${WGER_HOST_PORT:-8000}:80"; update the grep/regex to accept either a plain number or a ${...:-number} expression before the colon (for example allow patterns like "[0-9]+" OR "\$\{[^}]+:-[0-9]+\}") so that lines such as "\"${WGER_HOST_PORT:-8000}:80\"" are captured; modify the command that builds ports accordingly (the line with ports=$(grep -oE ...)) to use that combined pattern or a small sed/awk extraction that returns the left-hand mapping whether it is numeric or an env-expansion.
115-115:⚠️ Potential issue | 🟠 MajorYAML fallback validation doesn't actually parse the file.
yaml.safe_load_all()returns a generator. Without consuming it (e.g.,list(yaml.safe_load_all(...))), syntax errors are never raised and malformed YAML silently passes.🛠️ Suggested fix
- if ! python3 -c "import yaml; yaml.safe_load_all(open('$file'))" 2>/dev/null; then + if ! python3 -c "import yaml; list(yaml.safe_load_all(open('$file')))" 2>/dev/null; then,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/validate-changes.sh` at line 115, The Python YAML fallback in validate-changes.sh currently calls yaml.safe_load_all(open('$file')) but never consumes the generator so parsing errors are not raised; change the python3 -c invocation to consume the generator (e.g., wrap safe_load_all in list(...) or iterate over it) so syntax errors are triggered for the file variable $file and the exit status correctly reflects malformed YAML.pmoves/tests/smoke/test_supabase_realtime_tenant.py (1)
132-136:⚠️ Potential issue | 🟠 MajorThe WebSocket URL construction produces an invalid URL.
With the default
SUPABASE_REALTIME_URL = "ws://localhost:4000/socket/websocket", line 132 produces:
ws://localhost:4000/localhost:4000/socket/websocketThis is malformed. Either use
SUPABASE_REALTIME_URLdirectly or properly parse and reconstruct the URL.🛠️ Suggested fix
- ws_url = SUPABASE_REALTIME_URL.replace("ws://", "ws://localhost:4000/") + ws_url = SUPABASE_REALTIME_URL,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/smoke/test_supabase_realtime_tenant.py` around lines 132 - 136, The ws_url construction is producing a malformed URL by naively replacing "ws://" with "ws://localhost:4000/"; update the test to use SUPABASE_REALTIME_URL directly or rebuild the URL correctly (e.g., parse SUPABASE_REALTIME_URL and replace the netloc/hostname) instead of string-replacing. Locate the ws_url assignment in pmoves/tests/smoke/test_supabase_realtime_tenant.py and modify the ws_url variable so it yields a valid websocket endpoint (either use SUPABASE_REALTIME_URL as-is or use a proper URL parse/reconstruction approach).pmoves/tests/smoke/test_port_conflicts.py (1)
44-45:⚠️ Potential issue | 🟡 MinorThe port pattern still has edge cases with env-var syntax.
The regex
r'-\s*"(?:\$\{[^}]*:-)?(\d+)\}?:(\d+)(/(\w+))?"'handles${VAR:-8000}:80but misses:
- Unquoted port mappings (valid YAML):
- 8000:80- Single-quoted mappings:
- '8000:80'- Mappings without the closing
}properly handled when default is presentConsider using a YAML parser for more robust extraction, or document these limitations.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/smoke/test_port_conflicts.py` around lines 44 - 45, The current regex in port_pattern used to extract ports from service_block (port_pattern and ports) misses unquoted mappings (e.g., - 8000:80), single-quoted mappings (e.g., - '8000:80'), and cases with env-var defaults/missing braces; replace the brittle regex approach by parsing service_block as YAML (e.g., yaml.safe_load) and then iterate the parsed service definition to extract port mappings into ports, or if you must keep a regex, broaden port_pattern to accept optional surrounding quotes and optional/malformed ${...:-...} forms while validating captures for host:container pairs—update the code that fills ports from service_block accordingly.
🧹 Nitpick comments (7)
pmoves/tests/smoke/_smoke_helpers.py (2)
11-11: Unused import:Optional.
Optionalis imported but never used in the module. Remove to keep imports clean.-from typing import List, Optional +from typing import List🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/smoke/_smoke_helpers.py` at line 11, The import list includes Optional from typing but it is not used; remove Optional from the import statement (leave List) in the top-level import line to clean up unused imports in _smoke_helpers.py.
62-62: Inconsistent type hint style.Line 62 uses lowercase
list[str]while the function signatures useList[str]from typing. For consistency, consider using the same style throughout.- result_lines: list[str] = [] + result_lines: List[str] = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/smoke/_smoke_helpers.py` at line 62, The type hint for the local variable result_lines currently uses lowercase list[str], which is inconsistent with the rest of the module that uses typing.List; change the annotation to use List[str] (i.e., result_lines: List[str] = []) and ensure typing.List is imported (or already available) at the top of _smoke_helpers.py so the file uses a consistent type-hint style.pmoves/tests/smoke/test_supabase_selfhosted.py (1)
221-221: Redundant re-import ofgrep_file.
grep_fileis already imported at the top of the file (line 17). No need to re-import it as_grep.- from _smoke_helpers import grep_file as _grep required_vars = ["SUPABASE_JWT_SECRET", "SUPABASE_DB_PASSWORD", "SUPABASE_ANON_KEY"] for var in required_vars: - matches = _grep(ENV_TIER_SUPABASE, rf"^{var}=") + matches = grep_file(ENV_TIER_SUPABASE, rf"^{var}=")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/smoke/test_supabase_selfhosted.py` at line 221, Remove the redundant re-import "from _smoke_helpers import grep_file as _grep" and use the existing top-level imported function grep_file instead; delete the duplicate import and, if any tests reference _grep, replace those usages with grep_file (look for occurrences of _grep and update them) so there is a single import of grep_file.pmoves/tests/smoke/test_environment_consistency.py (1)
43-44: Catching broadExceptionmay mask unexpected errors.Consider catching more specific exceptions like
OSErrororUnicodeDecodeError.- except Exception as e: + except (OSError, UnicodeDecodeError) as e:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/smoke/test_environment_consistency.py` around lines 43 - 44, The test currently catches a broad Exception when reading a file (the except Exception as e around the read that calls pytest.fail), which can hide unexpected bugs; change that to catch more specific errors such as OSError and UnicodeDecodeError (or split into two except blocks) and call pytest.fail including the error details, and allow other unexpected exceptions to propagate (or re-raise) so failures are not masked. Ensure you update the except clause(s) referencing the same file_path and pytest.fail usage.pmoves/tests/smoke/test_port_conflicts.py (2)
92-93: Rename unused loop variables.Per static analysis,
container_portandprotocolare unpacked but unused. Use underscore prefix to indicate intentional disuse.- for host_port, container_port, protocol in ports: + for host_port, _container_port, _protocol in ports:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/smoke/test_port_conflicts.py` around lines 92 - 93, The loop unpacks container_port and protocol but never uses them; rename those variables to indicate intentional unused values (e.g., _container_port and _protocol or _ , _ ) in the for loop over ports in test_port_conflicts.py so static analysis stops flagging unused variables—update the loop header that currently reads "for host_port, container_port, protocol in ports:" to use the underscore-prefixed names while leaving host_port, host_ports, service_name, and compose_file.name unchanged.
152-153: Rename unused loop variables (second occurrence).Same issue as line 92.
- for host_port, container_port, protocol in ports: + for host_port, _container_port, _protocol in ports:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/smoke/test_port_conflicts.py` around lines 152 - 153, In the for loop iterating "for host_port, container_port, protocol in ports:" in test_port_conflicts.py the variables container_port and protocol are unused; rename them to unused placeholders (e.g., "_" or "_container_port", "_protocol") so only host_port is treated as meaningful and to match the earlier fix at line 92; keep the condition referencing host_port and PRIVILEGED_PORT_SERVICES/service_name unchanged.pmoves/tests/smoke/test_nats_configuration.py (1)
229-231: Remove extraneousfprefix from string without placeholders.The f-string on line 229 has no placeholders.
- f"Found hardcoded NATS_URL in docker-compose.yml:\n" + "Found hardcoded NATS_URL in docker-compose.yml:\n"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/tests/smoke/test_nats_configuration.py` around lines 229 - 231, Remove the unnecessary f-string prefix from the string literal that builds the failure message (the concatenation involving "Found hardcoded NATS_URL in docker-compose.yml:\n" and "\nUse ${NATS_URL:-nats://nats:pmoves@nats:4222} instead.") since there are no placeholders; update the literal to a normal string while keeping the concatenation with the hardcoded variable intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pmoves/tests/smoke/test_environment_consistency.py`:
- Around line 69-71: The current service-block parser in
extract_env_vars_from_compose exits prematurely on any colon-bearing line (e.g.,
image:, ports:) and thus misses environment entries; replace the ad-hoc
line-parsing with a YAML parse: load the compose file using yaml.safe_load,
navigate to data["services"][service_name]["environment"], then handle both list
and dict forms (for list entries split on the first "=" to get key/value, for
dict entries use keys/values directly) to build and return the env var dict.
In `@pmoves/tests/smoke/test_nats_configuration.py`:
- Line 246: The variable env_value is extracted from matches[0] in the test
(env_value = matches[0].split("=", 1)[1]) but never used; either remove the
assignment or assert it against the expected documentation value. Update the
test_nats_configuration.py test to either delete the unused env_value line or
replace it with an assertion comparing env_value to the expected string (from
docs) so the extracted value is actually validated; reference the matches list
and the env_value variable when making the change.
In `@pmoves/tests/validate-changes.sh`:
- Line 190: The current membership test using [[ " ${checked_vars[@]} " =~ "
${var_name} " ]] is fragile and can yield false matches; replace it with a
reliable check by either iterating over the checked_vars array with a simple for
loop comparing each element to var_name, or convert checked_vars into an
associative array (e.g., checked_map[var]=1) and test membership with [[ -n
"${checked_map[$var_name]}" ]] in the conditional; update the code paths that
populate and use checked_vars/checked_map (references: checked_vars, var_name)
accordingly so the membership test is exact and safe.
- Line 251: The array membership check using [[ " ${ports_used[@]} " =~ "
${port} " ]] is subject to word-splitting and ShellCheck warnings; in
validate-changes.sh replace that conditional with a safe membership test using
either a loop that iterates over ports_used and compares each element to port,
or use a case-based test against " ${ports_used[*]} " with a pattern like *"
$port "* to avoid word-splitting; update the conditional where ports_used and
port are referenced to use one of these safe patterns.
---
Duplicate comments:
In `@pmoves/tests/smoke/test_environment_consistency.py`:
- Around line 99-109: The test_env_shared_exists test currently hard-fails when
PMOVES_DIR / "env.shared" is missing; update test_env_shared_exists to first
check for env.shared and if missing attempt to use PMOVES_DIR /
"env.shared.example" (using the same extract_env_vars_from_file) and only fail
if neither exists, or mark/skip the test when both are missing (pytest.skip) to
avoid CI false-negatives; reference PMOVES_DIR, env_shared, env_shared_example,
test_env_shared_exists and extract_env_vars_from_file when making the change.
- Line 349: The test currently calls extract_env_vars_from_compose("hirag-v2"),
which may not match the actual compose service name; update the test to try both
service names (e.g., "hirag-v2" and "hi-rag-gateway-v2") and use the first
non-empty result as hirag_vars (or assert with both possibilities), modifying
the invocation site in test_environment_consistency.py so
extract_env_vars_from_compose is called for each candidate and falls back
appropriately to ensure the test doesn't always skip when the compose service is
named "hi-rag-gateway-v2".
In `@pmoves/tests/smoke/test_port_conflicts.py`:
- Around line 44-45: The current regex in port_pattern used to extract ports
from service_block (port_pattern and ports) misses unquoted mappings (e.g., -
8000:80), single-quoted mappings (e.g., - '8000:80'), and cases with env-var
defaults/missing braces; replace the brittle regex approach by parsing
service_block as YAML (e.g., yaml.safe_load) and then iterate the parsed service
definition to extract port mappings into ports, or if you must keep a regex,
broaden port_pattern to accept optional surrounding quotes and
optional/malformed ${...:-...} forms while validating captures for
host:container pairs—update the code that fills ports from service_block
accordingly.
In `@pmoves/tests/smoke/test_supabase_realtime_tenant.py`:
- Around line 132-136: The ws_url construction is producing a malformed URL by
naively replacing "ws://" with "ws://localhost:4000/"; update the test to use
SUPABASE_REALTIME_URL directly or rebuild the URL correctly (e.g., parse
SUPABASE_REALTIME_URL and replace the netloc/hostname) instead of
string-replacing. Locate the ws_url assignment in
pmoves/tests/smoke/test_supabase_realtime_tenant.py and modify the ws_url
variable so it yields a valid websocket endpoint (either use
SUPABASE_REALTIME_URL as-is or use a proper URL parse/reconstruction approach).
In `@pmoves/tests/validate-changes.sh`:
- Line 104: The current assignment to yml_files uses find only on the pmoves
directory so workflow YAML edits under .github/workflows are skipped; update the
find invocation that populates yml_files (the variable defined as
yml_files=$(find ...)) to also search the .github/workflows directory (e.g.,
include .github/workflows as an extra search path) while preserving the existing
-name filters and the 2>/dev/null || true guard so missing directories don't
break the script.
- Line 249: The current ports extraction (the assignment to the variable named
ports) only matches literal numeric mappings and misses env-var expanded
mappings like "${WGER_HOST_PORT:-8000}:80"; update the grep/regex to accept
either a plain number or a ${...:-number} expression before the colon (for
example allow patterns like "[0-9]+" OR "\$\{[^}]+:-[0-9]+\}") so that lines
such as "\"${WGER_HOST_PORT:-8000}:80\"" are captured; modify the command that
builds ports accordingly (the line with ports=$(grep -oE ...)) to use that
combined pattern or a small sed/awk extraction that returns the left-hand
mapping whether it is numeric or an env-expansion.
- Line 115: The Python YAML fallback in validate-changes.sh currently calls
yaml.safe_load_all(open('$file')) but never consumes the generator so parsing
errors are not raised; change the python3 -c invocation to consume the generator
(e.g., wrap safe_load_all in list(...) or iterate over it) so syntax errors are
triggered for the file variable $file and the exit status correctly reflects
malformed YAML.
---
Nitpick comments:
In `@pmoves/tests/smoke/_smoke_helpers.py`:
- Line 11: The import list includes Optional from typing but it is not used;
remove Optional from the import statement (leave List) in the top-level import
line to clean up unused imports in _smoke_helpers.py.
- Line 62: The type hint for the local variable result_lines currently uses
lowercase list[str], which is inconsistent with the rest of the module that uses
typing.List; change the annotation to use List[str] (i.e., result_lines:
List[str] = []) and ensure typing.List is imported (or already available) at the
top of _smoke_helpers.py so the file uses a consistent type-hint style.
In `@pmoves/tests/smoke/test_environment_consistency.py`:
- Around line 43-44: The test currently catches a broad Exception when reading a
file (the except Exception as e around the read that calls pytest.fail), which
can hide unexpected bugs; change that to catch more specific errors such as
OSError and UnicodeDecodeError (or split into two except blocks) and call
pytest.fail including the error details, and allow other unexpected exceptions
to propagate (or re-raise) so failures are not masked. Ensure you update the
except clause(s) referencing the same file_path and pytest.fail usage.
In `@pmoves/tests/smoke/test_nats_configuration.py`:
- Around line 229-231: Remove the unnecessary f-string prefix from the string
literal that builds the failure message (the concatenation involving "Found
hardcoded NATS_URL in docker-compose.yml:\n" and "\nUse
${NATS_URL:-nats://nats:pmoves@nats:4222} instead.") since there are no
placeholders; update the literal to a normal string while keeping the
concatenation with the hardcoded variable intact.
In `@pmoves/tests/smoke/test_port_conflicts.py`:
- Around line 92-93: The loop unpacks container_port and protocol but never uses
them; rename those variables to indicate intentional unused values (e.g.,
_container_port and _protocol or _ , _ ) in the for loop over ports in
test_port_conflicts.py so static analysis stops flagging unused variables—update
the loop header that currently reads "for host_port, container_port, protocol in
ports:" to use the underscore-prefixed names while leaving host_port,
host_ports, service_name, and compose_file.name unchanged.
- Around line 152-153: In the for loop iterating "for host_port, container_port,
protocol in ports:" in test_port_conflicts.py the variables container_port and
protocol are unused; rename them to unused placeholders (e.g., "_" or
"_container_port", "_protocol") so only host_port is treated as meaningful and
to match the earlier fix at line 92; keep the condition referencing host_port
and PRIVILEGED_PORT_SERVICES/service_name unchanged.
In `@pmoves/tests/smoke/test_supabase_selfhosted.py`:
- Line 221: Remove the redundant re-import "from _smoke_helpers import grep_file
as _grep" and use the existing top-level imported function grep_file instead;
delete the duplicate import and, if any tests reference _grep, replace those
usages with grep_file (look for occurrences of _grep and update them) so there
is a single import of grep_file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b9ceef5-9ea3-488a-83da-078867756050
📒 Files selected for processing (8)
pmoves/tests/smoke/_smoke_helpers.pypmoves/tests/smoke/conftest.pypmoves/tests/smoke/test_environment_consistency.pypmoves/tests/smoke/test_nats_configuration.pypmoves/tests/smoke/test_port_conflicts.pypmoves/tests/smoke/test_supabase_realtime_tenant.pypmoves/tests/smoke/test_supabase_selfhosted.pypmoves/tests/validate-changes.sh
Comment on lines
+69
to
+71
| elif in_service and stripped and not stripped.startswith("-") and ":" in stripped: | ||
| if not stripped.startswith("-"): | ||
| break |
Contributor
There was a problem hiding this comment.
Service block parser exits prematurely.
Lines 69-71 break out of the service block if a line contains : and doesn't start with -. This incorrectly exits on YAML keys like image:, ports:, networks: within the service block. The parser will likely miss most environment variables.
🛠️ Suggested fix concept
Consider tracking indentation level or using a proper YAML parser:
import yaml
def extract_env_vars_from_compose(service_name: str) -> Dict[str, str]:
compose_file = PMOVES_DIR / "docker-compose.yml"
if not compose_file.exists():
return {}
data = yaml.safe_load(compose_file.read_text())
services = data.get("services", {})
service = services.get(service_name, {})
env_list = service.get("environment", [])
env_vars = {}
for item in env_list:
if isinstance(item, str) and "=" in item:
key, value = item.split("=", 1)
env_vars[key] = value
return env_vars🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/smoke/test_environment_consistency.py` around lines 69 - 71, The
current service-block parser in extract_env_vars_from_compose exits prematurely
on any colon-bearing line (e.g., image:, ports:) and thus misses environment
entries; replace the ad-hoc line-parsing with a YAML parse: load the compose
file using yaml.safe_load, navigate to
data["services"][service_name]["environment"], then handle both list and dict
forms (for list entries split on the first "=" to get key/value, for dict
entries use keys/values directly) to build and return the env var dict.
| if not matches: | ||
| pytest.skip("NATS_URL not found in env.shared") | ||
|
|
||
| env_value = matches[0].split("=", 1)[1] |
Contributor
There was a problem hiding this comment.
Unused variable env_value.
The extracted value is never used. Either remove it or add a comparison with the documentation.
- env_value = matches[0].split("=", 1)[1]
+ # env_value could be compared with doc_match for consistency🧰 Tools
🪛 Ruff (0.15.5)
[error] 246-246: Local variable env_value is assigned to but never used
Remove assignment to unused variable env_value
(F841)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/smoke/test_nats_configuration.py` at line 246, The variable
env_value is extracted from matches[0] in the test (env_value =
matches[0].split("=", 1)[1]) but never used; either remove the assignment or
assert it against the expected documentation value. Update the
test_nats_configuration.py test to either delete the unused env_value line or
replace it with an assertion comparing env_value to the expected string (from
docs) so the extracted value is actually validated; reference the matches list
and the env_value variable when making the change.
| # Extract variable name (before =) | ||
| if [[ "$line" =~ ^([A-Z_]+)= ]]; then | ||
| local var_name="${BASH_REMATCH[1]}" | ||
| if [[ " ${checked_vars[@]} " =~ " ${var_name} " ]]; then |
Contributor
There was a problem hiding this comment.
Array membership check is fragile.
ShellCheck flags this pattern: [[ " ${checked_vars[@]} " =~ " ${var_name} " ]] implicitly concatenates the array. Use a loop or associative array for reliable membership testing.
🛠️ Suggested fix using associative array
- local checked_vars=()
+ declare -A checked_vars
...
- if [[ " ${checked_vars[@]} " =~ " ${var_name} " ]]; then
+ if [[ -n "${checked_vars[$var_name]:-}" ]]; then
log_warning "Duplicate variable $var_name in $env_file"
fi
- checked_vars+=("$var_name")
+ checked_vars[$var_name]=1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [[ " ${checked_vars[@]} " =~ " ${var_name} " ]]; then | |
| declare -A checked_vars | |
| ... | |
| if [[ -n "${checked_vars[$var_name]:-}" ]]; then | |
| log_warning "Duplicate variable $var_name in $env_file" | |
| fi | |
| checked_vars[$var_name]=1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 190-190: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 190-190: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/validate-changes.sh` at line 190, The current membership test
using [[ " ${checked_vars[@]} " =~ " ${var_name} " ]] is fragile and can yield
false matches; replace it with a reliable check by either iterating over the
checked_vars array with a simple for loop comparing each element to var_name, or
convert checked_vars into an associative array (e.g., checked_map[var]=1) and
test membership with [[ -n "${checked_map[$var_name]}" ]] in the conditional;
update the code paths that populate and use checked_vars/checked_map
(references: checked_vars, var_name) accordingly so the membership test is exact
and safe.
| local ports | ||
| ports=$(grep -oE '"[0-9]+:' "$file" 2>/dev/null | grep -oE '[0-9]+' || true) | ||
| while IFS= read -r port; do | ||
| if [[ " ${ports_used[@]} " =~ " ${port} " ]]; then |
Contributor
There was a problem hiding this comment.
Same array membership issue in port conflict check.
Same ShellCheck warning applies here.
🛠️ Suggested fix
- local ports_used=()
+ declare -A ports_used
...
- if [[ " ${ports_used[@]} " =~ " ${port} " ]]; then
+ if [[ -n "${ports_used[$port]:-}" ]]; then
log_error "Port $port used in multiple compose files"
((failed++))
else
- ports_used+=("$port")
+ ports_used[$port]=1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [[ " ${ports_used[@]} " =~ " ${port} " ]]; then | |
| declare -A ports_used | |
| ... | |
| if [[ -n "${ports_used[$port]:-}" ]]; then | |
| log_error "Port $port used in multiple compose files" | |
| ((failed++)) | |
| else | |
| ports_used[$port]=1 | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 251-251: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 251-251: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/tests/validate-changes.sh` at line 251, The array membership check
using [[ " ${ports_used[@]} " =~ " ${port} " ]] is subject to word-splitting and
ShellCheck warnings; in validate-changes.sh replace that conditional with a safe
membership test using either a loop that iterates over ports_used and compares
each element to port, or use a case-based test against " ${ports_used[*]} " with
a pattern like *" $port "* to avoid word-splitting; update the conditional where
ports_used and port are referenced to use one of these safe patterns.
- Create _smoke_helpers.py with grep_file/grep_context/grep_count/grep_numbered functions using Path.read_text() + re module (works on Windows/macOS/Linux) - Replace all ~35 subprocess.run(["grep",...]) calls across 5 smoke test files - Fix zero-assertion tests: test_services_have_consistent_nats_documentation and test_critical_services_depend_on_nats now assert instead of just printing - Fix test_nats_url_has_credentials to actually verify @ in URL - Fix test_no_cli_references_in_compose glob expansion bug (subprocess doesn't expand shell globs) - Fix test_critical_services_have_required_env_vars logic bug (var in env_var) - Fix validate-changes.sh: grep -oP (Perl regex) replaced with grep -oE, smoke test failures now tracked in FAILED_CHECKS, || true replaced with _run_check wrapper that distinguishes crashes from validation failures, check_port_conflicts now tracks failures instead of just warning - Add vacuous-pass guards (checked_count > 0) for tests iterating env files - Remove unused imports (os, time, subprocess where no longer needed) - Add conftest.py for smoke dir sys.path setup Addresses: cross-platform compatibility, silent test failures, logic bugs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… helpers Completes smoke test hardening started in 25e1aca. Replaces 17 subprocess grep/awk calls and 19 hardcoded /home/pmoves paths across 4 test files with pure-Python _smoke_helpers functions. Adds is_docker_service_running() to conftest.py and switches validate-changes.sh to mktemp. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
POWERFULMOVES
force-pushed
the
review/ci-trivy-smoke-tests
branch
from
March 9, 2026 22:38
77cd389 to
44f5275
Compare
…ixture httpx.TimeoutError doesn't exist (correct class is httpx.TimeoutException since httpx 0.20+), causing AttributeError at runtime and turning graceful pytest.skip() into hard test failures. Fixed 9 occurrences across 5 files. Also added shared http_client async fixture to conftest.py for test_network_configuration.py which referenced it without defining it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
POWERFULMOVES
pushed a commit
that referenced
this pull request
Mar 10, 2026
Captures fresh validation baseline after PR #842 (smoke test cross-platform httpx.TimeoutException fix + CI Trivy timeout increase). Results: 215 tests collected (58 pass, 123 fail, 34 skip) — all failures are pre-existing (services not running locally). Audit-layers-static: 39/40 submodules PASS. Dashboard updated to HEAD (0552699), docs-reconcile drift: 0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 tasks
5 tasks
POWERFULMOVES
pushed a commit
that referenced
this pull request
Mar 10, 2026
Root cause: self-hosted runners were installed as bare-metal services (WSL2 systemd, Windows svc.cmd) that stopped and had no auto-recovery. The local-certification phase policy was always designed for "Both runners on local Docker containers" but was never implemented. Resolution: - Started Docker-based runners via existing local_cert_runners.py (make ci-runners-local-cert-up) - Updated lane_hosts.json to reflect containerized topology - Updated runner_phase_policy.json to match actual workflow label sets (self-hosted,Linux,X64,ai-lab,gpu instead of self-hosted,ai-lab,gpu) - Dashboard updated: AB-9 RESOLVED, CI queue HEALTHY (3/4 online) Timeline of missed fixes: - PRs #832/#834/#835: Added CI throttle timeouts without addressing root - PR #842: Captured 0/4 runners, noted AB-9 REGRESSED - This fix: Discovered local_cert_runners.py already had full Docker runner management — just needed to be executed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Retrospective review of commit
522da27cpushed directly to main.Changes under review:
validate-changes.shvalidation scriptReview Focus
/home/pmoves)@pytest.mark.smoke)Test plan
pytest --collect-only pmoves/tests/smoke/test_*.pydiscovers all tests🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores
Documentation