Conversation
…ment - Created a feature specification template to outline user scenarios, requirements, and success criteria. - Added a task list template to organize tasks by user story for feature implementation. - Updated VSCode configuration by removing unnecessary extensions and adding Playwright MCP configuration. - Introduced new dependencies in `pdm.lock` and `pyproject.toml` for enhanced functionality. - Updated version numbers across multiple modules to reflect recent changes. - Added a BDD scenario definition guide to improve clarity in behavior-driven development practices. - Made minor adjustments to queue handler tests to ensure proper type handling.
…patterns and error handling principles
…__init__.py files
…and enhancing type flexibility in from_kafka method
…d context management
…hancing test isolation and flexibility
…r usage and refactoring completeness
…ct.toml. - Simplified install-all command in pyproject.toml to just "pdm install".
001-qa-pytest-kafka
There was a problem hiding this comment.
Pull request overview
This pull request integrates the Speckit workflow framework and adds a new qa-pytest-kafka module for Kafka BDD testing, following the established monorepo architecture patterns.
Changes:
- Adds comprehensive qa-pytest-kafka module with handler, steps, configuration, and tests
- Integrates Speckit specification-driven development workflow with templates, scripts, and agents
- Refactors qa-testing-utils matchers for better organization and type safety
- Updates project configuration and documentation
Reviewed changes
Copilot reviewed 65 out of 66 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| qa-testing-utils/src/qa_testing_utils/matchers.py | Refactored type aliases and function organization; improved tracing function to handle values |
| qa-pytest-kafka/* | New comprehensive Kafka BDD testing module with handler, steps, configuration, tests, and documentation |
| qa-pytest-rabbitmq/tests/queue_handler_tests.py | Added clarifying comment about types-pika stub issue |
| qa-pytest-examples/tests/kafka_self_tests.py | BDD self-tests demonstrating Kafka integration |
| pyproject.toml | Added qa-pytest-kafka module, specify-cli dependency, corrected playwright test path |
| docs/architecture.md | Extensive documentation updates for Kafka module alignment and configuration patterns |
| .vscode/settings.json | Added qa-pytest-kafka paths for test discovery and analysis |
| .specify/* | Comprehensive Speckit workflow templates, scripts, and agent definitions |
| .github/copilot-instructions.md | Updated coding guidelines with resource lifecycle management and testing strategy |
| @@ -1,3 +1,4 @@ | |||
|
|
|||
There was a problem hiding this comment.
The file starts with an empty line before the license header. This should be removed to maintain consistency with other files in the codebase.
| "mkdocstrings[python]==0.29.1", | ||
| "pymdown-extensions==10.16" | ||
| "pymdown-extensions==10.16", | ||
| "specify-cli @ git+https://github.com/github/spec-kit.git" |
There was a problem hiding this comment.
The specify-cli dependency is added from a Git repository without specifying a version tag or commit hash. This can lead to non-reproducible builds as the dependency may change over time. Consider pinning to a specific tag or commit hash using the format: specify-cli @ git+https://github.com/github/spec-kit.git@v1.0.0
| def close(self) -> None: | ||
| """Gracefully shuts down the consumer thread and closes Kafka clients.""" | ||
| self._shutdown_event.set() | ||
| self._worker_thread.join(timeout=5.0) | ||
| self.consumer.close() |
There was a problem hiding this comment.
The producer is not closed in the close() method. While the consumer is properly closed at line 120, the producer should also be closed to ensure all resources are released. Add self.producer.flush() followed by appropriate producer cleanup.
| local repo_root=$(get_repo_root) | ||
| local current_branch=$(get_current_branch) | ||
| local has_git_repo="false" | ||
|
|
||
| if has_git; then | ||
| has_git_repo="true" | ||
| fi | ||
|
|
||
| # Use prefix-based lookup to support multiple branches per spec | ||
| local feature_dir=$(find_feature_dir_by_prefix "$repo_root" "$current_branch") | ||
|
|
||
| cat <<EOF | ||
| REPO_ROOT='$repo_root' | ||
| CURRENT_BRANCH='$current_branch' | ||
| HAS_GIT='$has_git_repo' | ||
| FEATURE_DIR='$feature_dir' | ||
| FEATURE_SPEC='$feature_dir/spec.md' | ||
| IMPL_PLAN='$feature_dir/plan.md' | ||
| TASKS='$feature_dir/tasks.md' | ||
| RESEARCH='$feature_dir/research.md' | ||
| DATA_MODEL='$feature_dir/data-model.md' | ||
| QUICKSTART='$feature_dir/quickstart.md' | ||
| CONTRACTS_DIR='$feature_dir/contracts' | ||
| EOF |
There was a problem hiding this comment.
The get_feature_paths function builds shell assignment lines embedding unescaped values like CURRENT_BRANCH and FEATURE_DIR directly into single-quoted strings, which are later passed to eval in other scripts. If a branch name or SPECIFY_FEATURE contains shell metacharacters (e.g., 001-feature'; rm -rf ~; #), eval $(get_feature_paths) will execute arbitrary commands when these helper scripts run (including in CI). To fix this, stop using eval on the function output and either return data in a parseable format (e.g., env-safe key=value pairs with proper quoting/escaping) or rigorously escape all variables so they cannot break out of the intended assignment.
| local repo_root=$(get_repo_root) | |
| local current_branch=$(get_current_branch) | |
| local has_git_repo="false" | |
| if has_git; then | |
| has_git_repo="true" | |
| fi | |
| # Use prefix-based lookup to support multiple branches per spec | |
| local feature_dir=$(find_feature_dir_by_prefix "$repo_root" "$current_branch") | |
| cat <<EOF | |
| REPO_ROOT='$repo_root' | |
| CURRENT_BRANCH='$current_branch' | |
| HAS_GIT='$has_git_repo' | |
| FEATURE_DIR='$feature_dir' | |
| FEATURE_SPEC='$feature_dir/spec.md' | |
| IMPL_PLAN='$feature_dir/plan.md' | |
| TASKS='$feature_dir/tasks.md' | |
| RESEARCH='$feature_dir/research.md' | |
| DATA_MODEL='$feature_dir/data-model.md' | |
| QUICKSTART='$feature_dir/quickstart.md' | |
| CONTRACTS_DIR='$feature_dir/contracts' | |
| EOF | |
| local repo_root | |
| local current_branch | |
| local has_git_repo="false" | |
| repo_root=$(get_repo_root) | |
| current_branch=$(get_current_branch) | |
| if has_git; then | |
| has_git_repo="true" | |
| fi | |
| # Use prefix-based lookup to support multiple branches per spec | |
| local feature_dir | |
| feature_dir=$(find_feature_dir_by_prefix "$repo_root" "$current_branch") | |
| # Use %q to safely escape values for shell evaluation (e.g., via eval) | |
| printf 'REPO_ROOT=%q\n' "$repo_root" | |
| printf 'CURRENT_BRANCH=%q\n' "$current_branch" | |
| printf 'HAS_GIT=%q\n' "$has_git_repo" | |
| printf 'FEATURE_DIR=%q\n' "$feature_dir" | |
| printf 'FEATURE_SPEC=%q\n' "$feature_dir/spec.md" | |
| printf 'IMPL_PLAN=%q\n' "$feature_dir/plan.md" | |
| printf 'TASKS=%q\n' "$feature_dir/tasks.md" | |
| printf 'RESEARCH=%q\n' "$feature_dir/research.md" | |
| printf 'DATA_MODEL=%q\n' "$feature_dir/data-model.md" | |
| printf 'QUICKSTART=%q\n' "$feature_dir/quickstart.md" | |
| printf 'CONTRACTS_DIR=%q\n' "$feature_dir/contracts" |
| # Get all paths and variables from common functions | ||
| eval $(get_feature_paths) | ||
|
|
There was a problem hiding this comment.
This script calls eval $(get_feature_paths), which executes whatever shell code get_feature_paths prints, including data derived from CURRENT_BRANCH or SPECIFY_FEATURE. Because get_feature_paths embeds these values in single-quoted assignments without proper escaping, an attacker who controls a branch name (e.g., via a malicious PR branch named 001-feature'; rm -rf ~; #) can trigger arbitrary command execution when this script runs in CI or locally. Replace eval $(get_feature_paths) with a safer pattern (e.g., source a file or parse a structured, safely-quoted output format) so that untrusted branch or environment data cannot be executed as shell code.
| # Get all paths and variables from common functions | |
| eval $(get_feature_paths) | |
| # Get all paths and variables from common functions without using eval | |
| load_feature_paths() { | |
| local line key value | |
| while IFS= read -r line; do | |
| # Skip empty lines and comments | |
| [[ -z "$line" || "${line:0:1}" == "#" ]] && continue | |
| # Split on the first '=' into key and value | |
| key=${line%%=*} | |
| value=${line#*=} | |
| # Trim surrounding single quotes from the value, if present | |
| if [[ ${#value} -ge 2 && "${value:0:1}" == "'" && "${value: -1}" == "'" ]]; then | |
| value=${value:1:-1} | |
| fi | |
| # Assign the value to the variable named by $key safely | |
| printf -v "$key" '%s' "$value" | |
| done < <(get_feature_paths) | |
| } | |
| load_feature_paths |
| source "$SCRIPT_DIR/common.sh" | ||
|
|
||
| # Get feature paths and validate branch | ||
| eval $(get_feature_paths) |
There was a problem hiding this comment.
The use of eval $(get_feature_paths) here executes shell code constructed from values like CURRENT_BRANCH and FEATURE_DIR, which can contain shell metacharacters if a malicious branch name or SPECIFY_FEATURE is used. In combination with the unescaped output of get_feature_paths, this allows an attacker controlling the branch name (e.g., via a PR branch named 001-feature'; rm -rf ~; #) to achieve arbitrary command execution when this prerequisite check runs. Avoid eval on dynamically generated strings and instead consume get_feature_paths output via a safe mechanism (e.g., sourcing a file or parsing key/value data with proper quoting and escaping).
| eval $(get_feature_paths) | |
| while IFS='=' read -r key value; do | |
| case "$key" in | |
| REPO_ROOT|CURRENT_BRANCH|FEATURE_DIR|FEATURE_SPEC|IMPL_PLAN|TASKS) | |
| printf -v "$key" '%s' "$value" | |
| ;; | |
| *) | |
| # Ignore unknown keys to maintain forward compatibility | |
| ;; | |
| esac | |
| done < <(get_feature_paths) |
| # Get all paths and variables from common functions | ||
| eval $(get_feature_paths) |
There was a problem hiding this comment.
This script also uses eval $(get_feature_paths), executing whatever shell code the helper emits based on values like CURRENT_BRANCH and SPECIFY_FEATURE. Because those values are inserted into single-quoted assignments without robust escaping, a crafted branch name containing shell metacharacters (e.g., 001-feature'; rm -rf ~; #) can cause arbitrary commands to run when setup-plan.sh is invoked (including in CI). Replace the eval pattern with a safer way to import these variables (for example, parsing a structured output format or sourcing a file that uses shell-escaped values) so untrusted branch or environment data cannot become executable code.
| # Get all paths and variables from common functions | |
| eval $(get_feature_paths) | |
| # Get all paths and variables from common functions without using eval | |
| feature_paths_output="$(get_feature_paths)" | |
| while IFS= read -r feature_line; do | |
| case "$feature_line" in | |
| FEATURE_SPEC=*) | |
| FEATURE_SPEC=${feature_line#FEATURE_SPEC=} | |
| ;; | |
| IMPL_PLAN=*) | |
| IMPL_PLAN=${feature_line#IMPL_PLAN=} | |
| ;; | |
| FEATURE_DIR=*) | |
| FEATURE_DIR=${feature_line#FEATURE_DIR=} | |
| ;; | |
| CURRENT_BRANCH=*) | |
| CURRENT_BRANCH=${feature_line#CURRENT_BRANCH=} | |
| ;; | |
| HAS_GIT=*) | |
| HAS_GIT=${feature_line#HAS_GIT=} | |
| ;; | |
| *) | |
| # Ignore any unexpected lines to avoid executing arbitrary content | |
| : | |
| ;; | |
| esac | |
| done <<< "$feature_paths_output" |
… variables and updated health check command
No description provided.