Conversation
Replace shell=True with argument list in subprocess.run() to prevent potential command injection via database parameters. Use psql's -f flag for input and Python file handling for output redirection.
Add default value to PGVER ARG and use ${PGVER} brace syntax in FROM.
Recent GitHub Actions runner updates ship a newer BuildKit version that
performs static validation of ARG references in FROM instructions before
build arguments are applied. Without a default value, BuildKit rejects
the Dockerfile with InvalidDefaultArgInFrom even though --build-arg
PGVER is passed at build time. The default value (17) satisfies the
static check and is overridden by the matrix build-arg in CI.
PG18 Alpine image is built with --with-llvm using clang-19, but the Alpine clang package provides an unversioned clang binary. This causes PGXS to fail when trying to invoke clang-19 for bitcode generation. Since snowflake does not benefit from JIT, disable LLVM bitcode compilation with with_llvm=no and remove the now-unnecessary clang and llvm packages from the image.
Add 18config.env for the PG18 matrix entry in the CI workflow. Without this file, runner.py exits with an error when invoked with -c test/t/lib/18config.env.
Same fix as runner.py: replace shell=True with argument lists in run_cmd() and run_nc_cmd() to prevent potential command injection via the cmd parameter.
Pin all third-party actions in both workflow files to immutable commit SHAs instead of mutable tags, preventing potential supply chain attacks via tag reassignment.
Add missing query parameter to function signature and remove redundant f-string wrapper that the static analyzer flagged as potential SQL injection.
📝 WalkthroughWalkthroughThis PR introduces GitHub PR documentation standards, pins GitHub Actions to specific commit SHAs, adds PostgreSQL container defaults and simplified dependencies, updates test runners with explicit file I/O instead of shell redirection, and creates test environment configuration. Changes
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/pull_request_template.md (1)
16-16: Ask for an actual issue reference here.Line 16 is a bit misleading: a branch name does not create a durable issue link, and a plain URL does not participate in GitHub’s reference/closing flow. It would be better to ask for an explicit issue reference in the PR body, e.g.
Fixes#123or `Refs `#123.♻️ Suggested wording
-- [ ] Issue is linked (branch name or URL in PR description) +- [ ] Issue is referenced in the PR description (e.g. `Fixes `#123`` or `Refs `#123``)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/pull_request_template.md at line 16, Update the PR template checklist item that currently reads "[ ] Issue is linked (branch name or URL in PR description)" to require an explicit issue reference that participates in GitHub's reference/closing flow, e.g. change the text to something like "Issue is linked (use explicit issue reference such as 'Fixes `#123`' or 'Refs `#123`')" so contributors must include a durable issue reference in the PR body instead of just a branch name or raw URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/runner.py`:
- Around line 194-196: The exception handler can reference
result.stderr/result.returncode even if result was never assigned (errors from
build_psql_command(...) or open(actual_output_file, 'w') can occur), so
initialize result to a safe default before those calls; e.g., set result = a
placeholder object or None and update the except block to handle that
placeholder when checking result.stderr/result.returncode, ensuring references
to result in the except path cannot raise UnboundLocalError (affecting the
subprocess.run invocation and its error handling).
In `@test/t/lib/18config.env`:
- Line 27: The file test/t/lib/18config.env defines EDGE_REPUSER using backticks
which won't be evaluated by loaders; change the assignment to use
shell-expansion-friendly syntax (e.g., EDGE_REPUSER=${USER}) and remove
backticks, then ensure this file is actually loaded by the test infra (add it to
whatever config list or import in test/runner.py or load via
python-dotenv/os.path.expandvars where needed) or drop/rename it if it's
redundant; also verify whether 18config.env is intended to replace or supplement
config.env and update references accordingly so tests receive the correct
username value.
In `@test/t/util_test.py`:
- Line 88: The test uses naive whitespace splitting for command tokenization
causing quoted/escaped args to break; import shlex and change the subprocess.run
invocations that build argv from [f"{node_path}/pgedge/pgedge"] + cmd.split()
(and the similar call later) to use shlex.split(cmd) (or unpack via
*shlex.split(cmd)) so shell-style quoting is handled correctly; update the
imports to add shlex and replace cmd.split() occurrences in test/t/util_test.py
(the subprocess.run calls that reference node_path and pgedge) accordingly.
---
Nitpick comments:
In @.github/pull_request_template.md:
- Line 16: Update the PR template checklist item that currently reads "[ ] Issue
is linked (branch name or URL in PR description)" to require an explicit issue
reference that participates in GitHub's reference/closing flow, e.g. change the
text to something like "Issue is linked (use explicit issue reference such as
'Fixes `#123`' or 'Refs `#123`')" so contributors must include a durable issue
reference in the PR body instead of just a branch name or raw URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e98b9ad-3205-4d3e-8306-054125a852b6
📒 Files selected for processing (8)
.github/pull_request_template.md.github/workflows/run_runner_test_harness.yml.github/workflows/snowflake_regression_tests.ymlCLAUDE.mdDockerfiletest/runner.pytest/t/lib/18config.envtest/t/util_test.py
| # Execute the psql command, redirecting output to file via Python (not shell) | ||
| with open(actual_output_file, 'w') as outf: | ||
| result = subprocess.run(psql_command, stdout=outf, stderr=subprocess.STDOUT, text=True) |
There was a problem hiding this comment.
Initialize result before the failing calls.
Line 195 can fail before subprocess.run(...) assigns result (build_psql_command(...) and open(...) can both raise). When that happens, the except path tries to write to result.stderr / result.returncode and masks the real failure with UnboundLocalError.
🔧 Suggested fix
if extension == '.sql':
+ result = subprocess.CompletedProcess([], 1, "", "")
# Execute psql command and Compare expected/actual output files, handle any errors gracefully through try/except block
try:
# Build the psql command with appropriate switches/inputs/outputs,
psql_command, actual_output_file, expected_output_file = build_psql_command(testName)
# Execute the psql command, redirecting output to file via Python (not shell)
with open(actual_output_file, 'w') as outf:
result = subprocess.run(psql_command, stdout=outf, stderr=subprocess.STDOUT, text=True)
...
except Exception as e:
# Capture the error message
result_status = "error"
- result.stderr = f"An unexpected error occurred: {str(e)}"
- result.returncode = 1 # Set return code to 1 for error
+ result = subprocess.CompletedProcess(
+ psql_command if 'psql_command' in locals() else [testName],
+ 1,
+ "",
+ f"An unexpected error occurred: {e}",
+ )🧰 Tools
🪛 Ruff (0.15.5)
[error] 196-196: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/runner.py` around lines 194 - 196, The exception handler can reference
result.stderr/result.returncode even if result was never assigned (errors from
build_psql_command(...) or open(actual_output_file, 'w') can occur), so
initialize result to a safe default before those calls; e.g., set result = a
placeholder object or None and update the except block to handle that
placeholder when checking result.stderr/result.returncode, ensuring references
to result in the except path cannot raise UnboundLocalError (affecting the
subprocess.run invocation and its error handling).
| export EDGE_USERNAME="lcusr" | ||
| export EDGE_PASSWORD="password" | ||
| export EDGE_DB="lcdb" | ||
| export EDGE_REPUSER=`whoami` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the new variable plus the two loader implementations that read these env files.
rg -n -C2 'EDGE_REPUSER|load_dotenv|def source_config_file' test/t/lib/18config.env test/t/util_test.py test/runner.pyRepository: pgEdge/snowflake
Length of output: 1670
🏁 Script executed:
# Read the source_config_file implementation in test/runner.py (lines 424-450)
sed -n '424,450p' test/runner.pyRepository: pgEdge/snowflake
Length of output: 1654
🏁 Script executed:
# Read the source_config_file implementations in test/t/util_test.py (around lines 146-170 and 199-220)
sed -n '146,220p' test/t/util_test.pyRepository: pgEdge/snowflake
Length of output: 4262
🏁 Script executed:
# Check if 18config.env file is referenced anywhere in the codebase
rg -n '18config\.env' --type py --type shRepository: pgEdge/snowflake
Length of output: 42
🏁 Script executed:
# Search for any references to 18config or this specific env file
rg -i '18config|EDGE_REPUSER' --type py --type sh --type yaml --type toml --type jsonRepository: pgEdge/snowflake
Length of output: 42
🏁 Script executed:
# Check if 18config.env exists and is new/recent
ls -la test/t/lib/18config.env 2>/dev/null || echo "File not found"Repository: pgEdge/snowflake
Length of output: 132
🏁 Script executed:
# Search broader patterns to see if this env file is referenced in any way
rg -i 'lib.*env|config\.env' test/ --type py --type sh | head -20Repository: pgEdge/snowflake
Length of output: 257
🏁 Script executed:
# Check what's in the config.env file that IS being used
cat test/t/lib/config.env 2>/dev/null | head -30Repository: pgEdge/snowflake
Length of output: 42
🏁 Script executed:
# See if 18config.env is sourced or loaded anywhere
rg -n 'source.*config|load_dotenv' test/Repository: pgEdge/snowflake
Length of output: 1101
🏁 Script executed:
# Check what config_file is being passed to source_config_file in test/runner.py
sed -n '120,130p' test/runner.pyRepository: pgEdge/snowflake
Length of output: 478
🏁 Script executed:
# Check the context around line 125 in test/runner.py to see which file is loaded
sed -n '110,130p' test/runner.pyRepository: pgEdge/snowflake
Length of output: 1180
🏁 Script executed:
# Check if there are any test files that reference config options or 18config
find test/ -name "*.py" -o -name "*.sh" | xargs grep -l "18config\|EDGE_REPUSER" 2>/dev/null || echo "No references found"Repository: pgEdge/snowflake
Length of output: 79
🏁 Script executed:
# Check the git log or PR context to understand if this is a new file being added
git log --oneline test/t/lib/18config.env 2>/dev/null | head -5 || echo "File may be newly added"Repository: pgEdge/snowflake
Length of output: 105
This new config file is never imported or sourced anywhere in the codebase, and the backtick syntax will not be evaluated by the loaders.
The file test/t/lib/18config.env appears to be orphaned—there are no references to it in any test scripts or configuration. Additionally, if this file is ever used via the -c option in test/runner.py or loaded through the python-dotenv library, the backticks on line 27 will not be evaluated; they will become the literal string `whoami`, breaking any test expecting the current username.
The loaders (os.path.expandvars() and python-dotenv) only expand $VAR or ${VAR} syntax, not shell command substitution.
Fix: Use ${USER} instead of backticks, and verify this file is actually integrated into the test suite:
-export EDGE_REPUSER=`whoami`
+export EDGE_REPUSER="${USER}"Also confirm whether 18config.env is intended to replace config.env or serve a different purpose, since it is not currently being loaded by any test infrastructure.
📝 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.
| export EDGE_REPUSER=`whoami` | |
| export EDGE_REPUSER="${USER}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/t/lib/18config.env` at line 27, The file test/t/lib/18config.env defines
EDGE_REPUSER using backticks which won't be evaluated by loaders; change the
assignment to use shell-expansion-friendly syntax (e.g., EDGE_REPUSER=${USER})
and remove backticks, then ensure this file is actually loaded by the test infra
(add it to whatever config list or import in test/runner.py or load via
python-dotenv/os.path.expandvars where needed) or drop/rename it if it's
redundant; also verify whether 18config.env is intended to replace or supplement
config.env and update references accordingly so tests receive the correct
username value.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce .github/pull_request_template.md with Conventional Commits style, summary, checklist, and reviewer notes sections. Add CLAUDE.md to direct Claude Code to follow the template when preparing PRs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
.github/pull_request_template.mdwith Conventional Commits title guidance, summary section, checklist, and reviewer notes sectionCLAUDE.mdto instruct Claude Code to follow the PR template when preparing PR descriptionsChecklist
Notes for Reviewers
CLAUDE.mdis intentionally minimal (~5 lines) to avoid token overhead. It references the PR template by path rather than inlining its content.