Migrate AI TAP group to Unified CI Infrastructure pattern#5461
Migrate AI TAP group to Unified CI Infrastructure pattern#5461renecannao wants to merge 3 commits intov3.0from
Conversation
This commit migrates the ai TAP group from its legacy hook-based approach to the standard test/infra/ Unified CI Infrastructure pattern. Changes: - Added generic hook system to infrastructure scripts: * ensure-infras.bash: Executes group-specific setup-infras.bash hook * run-tests-isolated.bash: Executes group-specific pre-cleanup.bash hook - Migrated ai group infrastructure: * Created infras.lst with infra-mysql84 and docker-pgsql16-single * Renamed seed files: mysql-seed.sql -> seed-mysql.sql, pgsql-seed.sql -> seed-pgsql.sql * Deleted legacy docker-compose.yml, docker-compose-init.bash, docker-compose-destroy.bash * Deleted legacy hooks: pre-proxysql.bash, post-proxysql.bash - Added group-specific MCP configuration: * setup-infras.bash: Configures MCP targets and seeds test data * pre-cleanup.bash: Removes MCP configuration after tests * mcp-config.sql: SQL template for MCP setup * cleanup.sql: SQL template for MCP cleanup * seed-mysql.sql & seed-pgsql.sql: Test data for AI tests - Updated documentation in README.md with usage examples Architecture: - ai is the supergroup (defines infrastructure and shared config) - ai-g1, ai-g2, etc. are subgroups (define test sets in groups.json) - Group-specific hooks live in test/tap/groups/ai/ directory - Infrastructure scripts remain generic and delegate to group hooks
📝 WalkthroughWalkthroughRefactors AI TAP group infra to use centralized Unified CI infrastructure: adds infra selector exports, subgroup→supergroup hook invocation, MCP SQL config/cleanup, DB seeding hooks, and replaces local Docker Compose artifacts with group-scoped setup and pre-cleanup scripts. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the ai test group's continuous integration setup by migrating it from a legacy, custom Docker Compose-based approach to the standardized Unified CI Infrastructure pattern. This change centralizes infrastructure management, introduces a flexible hook system for group-specific operations, and enhances the maintainability and scalability of the AI/MCP test suite. The migration ensures that the ai group benefits from consistent environment provisioning and teardown, aligning with broader CI practices. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Pull request overview
Migrates the ai TAP supergroup (and ai-g1 subgroup) from a legacy, group-local docker-compose lifecycle to the repository’s Unified CI Infrastructure (test/infra/control) pattern by introducing standardized group hook points and moving MCP + seeding logic into those hooks.
Changes:
- Added generic TAP-group hook execution to the unified infra control scripts (
setup-infras.bashafter backends start;pre-cleanup.bashbefore runner cleanup). - Reworked AI group infrastructure to use
infras.lst+ hook scripts, and added MCP setup/cleanup SQL templates plus new seed files. - Removed the legacy AI group docker-compose stack and pre/post proxysql hook scripts; updated AI group documentation.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/infra/control/ensure-infras.bash | Exports default backend infra names and runs group setup-infras.bash hook (with base-group fallback). |
| test/infra/control/run-tests-isolated.bash | Runs group pre-cleanup.bash hook before removing the test runner container. |
| test/tap/groups/ai/setup-infras.bash | New AI post-infra hook: waits for ProxySQL admin, configures MCP, seeds MySQL/PostgreSQL. |
| test/tap/groups/ai/pre-cleanup.bash | New AI pre-cleanup hook: removes MCP config from ProxySQL via templated SQL. |
| test/tap/groups/ai/mcp-config.sql | SQL template to configure MCP variables, servers, auth profiles, and targets. |
| test/tap/groups/ai/cleanup.sql | SQL template to remove MCP profiles/targets and related hostgroup entries. |
| test/tap/groups/ai/seed-mysql.sql | Renamed/updated MySQL seed script for AI/MCP tests (DB name changed to test). |
| test/tap/groups/ai/seed-pgsql.sql | Renamed/updated PostgreSQL seed script for AI/MCP tests. |
| test/tap/groups/ai/infras.lst | Declares required backends (infra-mysql84, docker-pgsql16-single). |
| test/tap/groups/ai/env.sh | AI group environment defaults (infra names + MCP identifiers + DB names). |
| test/tap/groups/ai/README.md | Updated documentation for the Unified CI Infrastructure workflow and hooks. |
| test/tap/groups/ai-g1/env.sh | New subgroup env shim that sources the parent ai env. |
| test/tap/groups/ai/pre-proxysql.bash | Removed legacy hook-based local infra startup + MCP configuration. |
| test/tap/groups/ai/post-proxysql.bash | Removed legacy hook-based MCP cleanup + local infra teardown. |
| test/tap/groups/ai/docker-compose.yml | Removed legacy AI group-local MySQL/Postgres compose stack. |
| test/tap/groups/ai/docker-compose-init.bash | Removed legacy compose init helper. |
| test/tap/groups/ai/docker-compose-destroy.bash | Removed legacy compose destroy helper. |
Comments suppressed due to low confidence (2)
test/tap/groups/ai/seed-pgsql.sql:4
- The header comment says this file is executed by
docker-pgsql-post.bashwhenTAP_GROUPstarts with 'ai', but in this PR the seeding is performed by the newsetup-infras.bashgroup hook instead. Please update the comment to reflect the new execution path to avoid confusion when debugging CI runs.
test/tap/groups/ai/seed-mysql.sql:4 - The header comment says this seed runs via
docker-mysql-post.bashwhenTAP_GROUPstarts with 'ai', but in this PR seeding is performed by the newsetup-infras.bashgroup hook. Update the comment so it matches the new Unified CI Infrastructure flow.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Execute group-specific pre-cleanup hook if it exists | ||
| # This runs before the test runner container is removed, allowing cleanup | ||
| # of ProxySQL-specific configuration while admin is still accessible | ||
| if [ -n "${TAP_GROUP}" ]; then | ||
| BASE_GROUP="${TAP_GROUP%%-g[0-9]*}" | ||
| PRE_CLEANUP_HOOK="${WORKSPACE}/test/tap/groups/${TAP_GROUP}/pre-cleanup.bash" | ||
| if [ ! -f "${PRE_CLEANUP_HOOK}" ]; then | ||
| PRE_CLEANUP_HOOK="${WORKSPACE}/test/tap/groups/${BASE_GROUP}/pre-cleanup.bash" | ||
| fi | ||
|
|
||
| if [ -f "${PRE_CLEANUP_HOOK}" ]; then | ||
| echo ">>> Executing group pre-cleanup hook: ${PRE_CLEANUP_HOOK}" | ||
| "${PRE_CLEANUP_HOOK}" || true # Allow cleanup to fail | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Because this script runs with set -e, a non-zero exit from the docker run test runner will terminate the script before this pre-cleanup hook executes. That means MCP/group cleanup won’t happen on test failures (the main time you want cleanup), and the runner container cleanup below also won’t run. Consider capturing the docker run exit code (temporarily disabling -e), running the hook + container cleanup unconditionally, then exiting with the original test exit code.
| # Fallback: use bash variable expansion | ||
| bash -c "cat <<EOF | ||
| $(cat "${MCP_CONFIG_SQL}") | ||
| EOF" | docker exec -i "${PROXY_CONTAINER}" mysql -uradmin -pradmin -h127.0.0.1 -P6032 |
There was a problem hiding this comment.
The fallback path for variable substitution uses bash -c with an unquoted here-doc body built from $(cat ...). If the SQL template ever contains command substitutions (e.g. $(...) or backticks) or other shell metacharacters, they will be executed on the host during substitution. Safer options are to require envsubst, or implement a substitution routine that doesn’t evaluate the template as shell code (e.g., python -c template expansion, or envsubst executed inside a container image that always has it).
| # Fallback: use bash variable expansion | |
| bash -c "cat <<EOF | |
| $(cat "${MCP_CONFIG_SQL}") | |
| EOF" | docker exec -i "${PROXY_CONTAINER}" mysql -uradmin -pradmin -h127.0.0.1 -P6032 | |
| echo "ERROR: envsubst is required to apply MCP configuration but was not found in PATH." | |
| echo "Please install envsubst (e.g., from gettext) or run this script in an environment where it is available." | |
| exit 1 |
| # Fallback: use bash variable expansion | ||
| bash -c "cat <<EOF | ||
| $(cat "${CLEANUP_SQL}") | ||
| EOF" | docker exec -i "${PROXY_CONTAINER}" mysql -uradmin -pradmin -h127.0.0.1 -P6032 2>/dev/null || true |
There was a problem hiding this comment.
Same as setup hook: the bash -c here-doc fallback will evaluate the SQL template as shell code, so any $(...)/backticks/etc. in cleanup.sql would be executed on the host. Prefer requiring envsubst or using a substitution approach that does not execute the template via the shell.
| # Fallback: use bash variable expansion | |
| bash -c "cat <<EOF | |
| $(cat "${CLEANUP_SQL}") | |
| EOF" | docker exec -i "${PROXY_CONTAINER}" mysql -uradmin -pradmin -h127.0.0.1 -P6032 2>/dev/null || true | |
| echo ">>> WARNING: envsubst not found; skipping MCP cleanup because safe template substitution is unavailable" | |
| exit 0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
test/tap/groups/ai/seed-mysql.sql (1)
1-6: Minor documentation inconsistency.Line 3 states this is executed by
docker-mysql-post.bash, but context snippet 2 shows this file is executed bysetup-infras.bash(lines 99-109). Consider updating the comment to reflect the actual execution path.📝 Suggested fix
-- AI Group MySQL Test Data Seeding -- Creates tables needed by AI/MCP tests --- This is executed by docker-mysql-post.bash when TAP_GROUP starts with 'ai' +-- This is executed by setup-infras.bash as part of AI group infrastructure setup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/ai/seed-mysql.sql` around lines 1 - 6, Update the top comment in seed-mysql.sql to accurately reflect the actual execution path: replace or clarify the mention of docker-mysql-post.bash with setup-infras.bash (or note both if both run it) so the header correctly states that setup-infras.bash executes this SQL when TAP_GROUP starts with 'ai'; reference the file seed-mysql.sql and the script name setup-infras.bash (and docker-mysql-post.bash if you choose to mention it) to locate and edit the comment.test/infra/control/run-tests-isolated.bash (1)
141-155: RedundantBASE_GROUPassignment and inconsistent derivation across scripts.
BASE_GROUPis already defined at line 15, making the assignment at line 145 redundant.The
BASE_GROUPderivation method differs fromensure-infras.bash:
- Here:
${TAP_GROUP%%-g[0-9]*}(only handles-gprefix)ensure-infras.bash:sed -E "s/[-_]g[0-9]+.*//"(handles both-gand_g)For a group like
postgres15_g1, these produce different results (postgres15_g1vspostgres15), potentially causing hook resolution inconsistencies.♻️ Suggested fix
# Execute group-specific pre-cleanup hook if it exists # This runs before the test runner container is removed, allowing cleanup # of ProxySQL-specific configuration while admin is still accessible if [ -n "${TAP_GROUP}" ]; then - BASE_GROUP="${TAP_GROUP%%-g[0-9]*}" PRE_CLEANUP_HOOK="${WORKSPACE}/test/tap/groups/${TAP_GROUP}/pre-cleanup.bash" if [ ! -f "${PRE_CLEANUP_HOOK}" ]; then PRE_CLEANUP_HOOK="${WORKSPACE}/test/tap/groups/${BASE_GROUP}/pre-cleanup.bash" fiConsider also standardizing the
BASE_GROUPderivation method across all infrastructure scripts for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/control/run-tests-isolated.bash` around lines 141 - 155, Remove the redundant BASE_GROUP reassignment and derive BASE_GROUP using the same logic as ensure-infras.bash to handle both "-g" and "_g" variants: when TAP_GROUP is set, compute BASE_GROUP with the sed-style transformation (s/[-_]g[0-9]+.*//) instead of using the current parameter expansion, then use that BASE_GROUP to locate PRE_CLEANUP_HOOK and fall back to the base group; refer to TAP_GROUP, BASE_GROUP and PRE_CLEANUP_HOOK in your changes so the hook resolution is consistent across scripts.test/tap/groups/ai-g1/env.sh (1)
1-5: Add a shell directive for consistency.Similar to the parent
ai/env.sh, add a shell directive to satisfy shellcheck and clarify intent.📝 Suggested fix
+# shellcheck shell=bash # AI-g1 Subgroup Environment Configuration # Sources the parent ai group configurationThe inheritance pattern using
BASH_SOURCE[0]to resolve the script directory and source the parent configuration is clean and appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/ai-g1/env.sh` around lines 1 - 5, Add the shellcheck directive at the top of the file to match the parent and satisfy shellcheck: insert a line like "# shellcheck shell=bash" (or the exact directive used in ai/env.sh) above the SCRIPT_DIR assignment so the script that uses BASH_SOURCE and sets SCRIPT_DIR is explicitly declared as bash-compatible.test/tap/groups/ai/env.sh (1)
1-2: Add a shell directive to clarify the intended shell.Since this file is sourced by other scripts (e.g.,
ai-g1/env.sh), adding a shell directive helps tooling and clarifies the expected interpreter.📝 Suggested fix
+# shellcheck shell=bash # AI TAP Group Environment Configuration # Defines the primary targets for AI/MCP tests using standard test/infra/ pattern🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/ai/env.sh` around lines 1 - 2, Add an explicit shell directive to env.sh to clarify the intended interpreter: insert a shebang as the first line (e.g., #!/usr/bin/env bash or #!/bin/sh depending on POSIX vs bash intent) and also add a ShellCheck directive comment (e.g., # shellcheck shell=bash) immediately after if this file is primarily sourced by other scripts like ai-g1/env.sh so tooling and linters recognize the expected shell.test/tap/groups/ai/setup-infras.bash (1)
37-37: Same SC2155 pattern here (export VAR=$(...)).Line 37 has the same status-masking issue as in pre-cleanup.
Proposed fix
-export ROOT_PASSWORD=$(echo -n "${INFRA_ID}" | sha256sum | head -c 10) +ROOT_PASSWORD="$(echo -n "${INFRA_ID}" | sha256sum | head -c 10)" +export ROOT_PASSWORD🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/ai/setup-infras.bash` at line 37, The export uses command substitution inline (export ROOT_PASSWORD=$(...)) which triggers SC2155; replace with a two-step assignment so the command substitution is evaluated first into a local variable (e.g., ROOT_PASSWORD_TMP or ROOT_PASSWORD) and then export that variable; update the occurrence of ROOT_PASSWORD in the script (the export line and any subsequent references) to use the assigned variable name to avoid inline export with $(...) and satisfy the linter.test/tap/groups/ai/pre-cleanup.bash (1)
22-22: Avoidexport VAR=$(...)masking command status (SC2155).Line 22 can hide a failed substitution because
exportreturns success even if the command substitution fails.Proposed fix
-export ROOT_PASSWORD=$(echo -n "${INFRA_ID}" | sha256sum | head -c 10) +ROOT_PASSWORD="$(echo -n "${INFRA_ID}" | sha256sum | head -c 10)" +export ROOT_PASSWORD🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/groups/ai/pre-cleanup.bash` at line 22, The export using a command substitution for ROOT_PASSWORD masks failures (SC2155); instead run the substitution first, verify it succeeded, assign the result to ROOT_PASSWORD, then export it. Locate the line that sets ROOT_PASSWORD (export ROOT_PASSWORD=$(echo -n "${INFRA_ID}" | sha256sum | head -c 10)) and change it so you perform the pipeline to compute the hash into a variable, check the command exit status or non-empty result, assign to ROOT_PASSWORD, and then export ROOT_PASSWORD to ensure failures are not hidden.
🤖 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/tap/groups/ai/mcp-config.sql`:
- Around line 29-33: The current SQL uses INSERT OR IGNORE into the mysql_users
table which leaves existing rows (and old passwords) unchanged; change the logic
to either use INSERT OR REPLACE INTO mysql_users for the two username rows or
perform a deterministic delete+insert (DELETE FROM mysql_users WHERE username =
'...'; then INSERT) so credentials are updated on reruns; update the two
statements that insert '${TAP_MYSQLUSERNAME}' and 'testuser' and keep the final
LOAD MYSQL USERS TO RUNTIME; callout: target the mysql_users insert statements
shown in the diff.
In `@test/tap/groups/ai/pre-cleanup.bash`:
- Around line 44-50: The cleanup commands currently hide failures by redirecting
stderr and forcing success; update both branches (the envsubst path and the bash
fallback that cat's "${CLEANUP_SQL}") to stop using `2>/dev/null || true` —
instead capture the exit status of the docker exec mysql command run against
"${PROXY_CONTAINER}", and if it fails print a clear diagnostic to stderr
(include the CLEANUP_SQL contents or a short preview and the docker logs for
"${PROXY_CONTAINER}") and exit non‑zero so CI/consumers notice the failure.
In `@test/tap/groups/ai/README.md`:
- Around line 93-127: Add the missing WORKSPACE export to each runnable example:
insert export WORKSPACE=$(pwd) before the line that sources
test/infra/common/env.sh in the "MCP static harvest"
(TEST_PY_TAP_INCL=test_mcp_static_harvest-t), "MCP Discovery Phase B"
(TEST_PY_TAP_INCL=test_mcp_llm_discovery_phaseb-t), and "Claude Headless Flow"
(TEST_PY_TAP_INCL=test_mcp_claude_headless_flow-t) blocks so the AI hooks that
rely on WORKSPACE do not exit with an error.
In `@test/tap/groups/ai/setup-infras.bash`:
- Around line 23-35: After deriving DEFAULT_MYSQL_INFRA and DEFAULT_PGSQL_INFRA
from INFRAS_LST, add explicit validation (considering set -u) to check that
DEFAULT_MYSQL_INFRA and DEFAULT_PGSQL_INFRA are non-empty before they are used;
if either is empty or unset, emit a clear error message referencing INFRAS_LST
and SCRIPT_DIR and exit with a non-zero status so the script fails fast. Perform
the same validation for the other derivation block (the code referenced at lines
74-79) to ensure both DEFAULT_MYSQL_INFRA and DEFAULT_PGSQL_INFRA are validated
immediately after their respective derivation loops.
---
Nitpick comments:
In `@test/infra/control/run-tests-isolated.bash`:
- Around line 141-155: Remove the redundant BASE_GROUP reassignment and derive
BASE_GROUP using the same logic as ensure-infras.bash to handle both "-g" and
"_g" variants: when TAP_GROUP is set, compute BASE_GROUP with the sed-style
transformation (s/[-_]g[0-9]+.*//) instead of using the current parameter
expansion, then use that BASE_GROUP to locate PRE_CLEANUP_HOOK and fall back to
the base group; refer to TAP_GROUP, BASE_GROUP and PRE_CLEANUP_HOOK in your
changes so the hook resolution is consistent across scripts.
In `@test/tap/groups/ai-g1/env.sh`:
- Around line 1-5: Add the shellcheck directive at the top of the file to match
the parent and satisfy shellcheck: insert a line like "# shellcheck shell=bash"
(or the exact directive used in ai/env.sh) above the SCRIPT_DIR assignment so
the script that uses BASH_SOURCE and sets SCRIPT_DIR is explicitly declared as
bash-compatible.
In `@test/tap/groups/ai/env.sh`:
- Around line 1-2: Add an explicit shell directive to env.sh to clarify the
intended interpreter: insert a shebang as the first line (e.g., #!/usr/bin/env
bash or #!/bin/sh depending on POSIX vs bash intent) and also add a ShellCheck
directive comment (e.g., # shellcheck shell=bash) immediately after if this file
is primarily sourced by other scripts like ai-g1/env.sh so tooling and linters
recognize the expected shell.
In `@test/tap/groups/ai/pre-cleanup.bash`:
- Line 22: The export using a command substitution for ROOT_PASSWORD masks
failures (SC2155); instead run the substitution first, verify it succeeded,
assign the result to ROOT_PASSWORD, then export it. Locate the line that sets
ROOT_PASSWORD (export ROOT_PASSWORD=$(echo -n "${INFRA_ID}" | sha256sum | head
-c 10)) and change it so you perform the pipeline to compute the hash into a
variable, check the command exit status or non-empty result, assign to
ROOT_PASSWORD, and then export ROOT_PASSWORD to ensure failures are not hidden.
In `@test/tap/groups/ai/seed-mysql.sql`:
- Around line 1-6: Update the top comment in seed-mysql.sql to accurately
reflect the actual execution path: replace or clarify the mention of
docker-mysql-post.bash with setup-infras.bash (or note both if both run it) so
the header correctly states that setup-infras.bash executes this SQL when
TAP_GROUP starts with 'ai'; reference the file seed-mysql.sql and the script
name setup-infras.bash (and docker-mysql-post.bash if you choose to mention it)
to locate and edit the comment.
In `@test/tap/groups/ai/setup-infras.bash`:
- Line 37: The export uses command substitution inline (export
ROOT_PASSWORD=$(...)) which triggers SC2155; replace with a two-step assignment
so the command substitution is evaluated first into a local variable (e.g.,
ROOT_PASSWORD_TMP or ROOT_PASSWORD) and then export that variable; update the
occurrence of ROOT_PASSWORD in the script (the export line and any subsequent
references) to use the assigned variable name to avoid inline export with $(...)
and satisfy the linter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25446e38-2ffd-495a-8ad2-4229c64a6b39
📒 Files selected for processing (17)
test/infra/control/ensure-infras.bashtest/infra/control/run-tests-isolated.bashtest/tap/groups/ai-g1/env.shtest/tap/groups/ai/README.mdtest/tap/groups/ai/cleanup.sqltest/tap/groups/ai/docker-compose-destroy.bashtest/tap/groups/ai/docker-compose-init.bashtest/tap/groups/ai/docker-compose.ymltest/tap/groups/ai/env.shtest/tap/groups/ai/infras.lsttest/tap/groups/ai/mcp-config.sqltest/tap/groups/ai/post-proxysql.bashtest/tap/groups/ai/pre-cleanup.bashtest/tap/groups/ai/pre-proxysql.bashtest/tap/groups/ai/seed-mysql.sqltest/tap/groups/ai/seed-pgsql.sqltest/tap/groups/ai/setup-infras.bash
💤 Files with no reviewable changes (5)
- test/tap/groups/ai/docker-compose-destroy.bash
- test/tap/groups/ai/docker-compose.yml
- test/tap/groups/ai/docker-compose-init.bash
- test/tap/groups/ai/post-proxysql.bash
- test/tap/groups/ai/pre-proxysql.bash
| envsubst < "${CLEANUP_SQL}" | docker exec -i "${PROXY_CONTAINER}" mysql -uradmin -pradmin -h127.0.0.1 -P6032 2>/dev/null || true | ||
| else | ||
| # Fallback: use bash variable expansion | ||
| bash -c "cat <<EOF | ||
| $(cat "${CLEANUP_SQL}") | ||
| EOF" | docker exec -i "${PROXY_CONTAINER}" mysql -uradmin -pradmin -h127.0.0.1 -P6032 2>/dev/null || true | ||
| fi |
There was a problem hiding this comment.
Don’t silently swallow cleanup failures.
Both execution paths suppress stderr and force success (2>/dev/null || true), so MCP cleanup can fail without any signal. This makes cross-run contamination and CI triage harder.
Proposed fix
- envsubst < "${CLEANUP_SQL}" | docker exec -i "${PROXY_CONTAINER}" mysql -uradmin -pradmin -h127.0.0.1 -P6032 2>/dev/null || true
+ if ! envsubst < "${CLEANUP_SQL}" | docker exec -i "${PROXY_CONTAINER}" mysql -uradmin -pradmin -h127.0.0.1 -P6032; then
+ echo ">>> WARNING: MCP cleanup failed"
+ fi
@@
- bash -c "cat <<EOF
+ if ! bash -c "cat <<EOF
$(cat "${CLEANUP_SQL}")
-EOF" | docker exec -i "${PROXY_CONTAINER}" mysql -uradmin -pradmin -h127.0.0.1 -P6032 2>/dev/null || true
+EOF" | docker exec -i "${PROXY_CONTAINER}" mysql -uradmin -pradmin -h127.0.0.1 -P6032; then
+ echo ">>> WARNING: MCP cleanup failed"
+ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/groups/ai/pre-cleanup.bash` around lines 44 - 50, The cleanup
commands currently hide failures by redirecting stderr and forcing success;
update both branches (the envsubst path and the bash fallback that cat's
"${CLEANUP_SQL}") to stop using `2>/dev/null || true` — instead capture the exit
status of the docker exec mysql command run against "${PROXY_CONTAINER}", and if
it fails print a clear diagnostic to stderr (include the CLEANUP_SQL contents or
a short preview and the docker logs for "${PROXY_CONTAINER}") and exit non‑zero
so CI/consumers notice the failure.
| if [ -z "${DEFAULT_MYSQL_INFRA:-}" ] || [ -z "${DEFAULT_PGSQL_INFRA:-}" ]; then | ||
| INFRAS_LST="${SCRIPT_DIR}/infras.lst" | ||
| if [ -f "${INFRAS_LST}" ]; then | ||
| for INFRA_NAME in $(cat "${INFRAS_LST}"); do | ||
| if [[ "${INFRA_NAME}" == *mysql* ]] || [[ "${INFRA_NAME}" == *mariadb* ]]; then | ||
| DEFAULT_MYSQL_INFRA="${DEFAULT_MYSQL_INFRA:-${INFRA_NAME}}" | ||
| fi | ||
| if [[ "${INFRA_NAME}" == *pgsql* ]] || [[ "${INFRA_NAME}" == *pgdb* ]]; then | ||
| DEFAULT_PGSQL_INFRA="${DEFAULT_PGSQL_INFRA:-${INFRA_NAME}}" | ||
| fi | ||
| done | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Validate derived infra names before using them.
With set -u, missing detection from infras.lst will crash later on variable expansion with a non-actionable unbound-variable error. Fail fast with explicit checks right after derivation.
Proposed fix
if [ -z "${DEFAULT_MYSQL_INFRA:-}" ] || [ -z "${DEFAULT_PGSQL_INFRA:-}" ]; then
@@
fi
+
+if [ -z "${DEFAULT_MYSQL_INFRA:-}" ]; then
+ echo "ERROR: Could not resolve DEFAULT_MYSQL_INFRA (check ${SCRIPT_DIR}/infras.lst)"
+ exit 1
+fi
+
+if [ -z "${DEFAULT_PGSQL_INFRA:-}" ]; then
+ echo "ERROR: Could not resolve DEFAULT_PGSQL_INFRA (check ${SCRIPT_DIR}/infras.lst)"
+ exit 1
+fiAlso applies to: 74-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/groups/ai/setup-infras.bash` around lines 23 - 35, After deriving
DEFAULT_MYSQL_INFRA and DEFAULT_PGSQL_INFRA from INFRAS_LST, add explicit
validation (considering set -u) to check that DEFAULT_MYSQL_INFRA and
DEFAULT_PGSQL_INFRA are non-empty before they are used; if either is empty or
unset, emit a clear error message referencing INFRAS_LST and SCRIPT_DIR and exit
with a non-zero status so the script fails fast. Perform the same validation for
the other derivation block (the code referenced at lines 74-79) to ensure both
DEFAULT_MYSQL_INFRA and DEFAULT_PGSQL_INFRA are validated immediately after
their respective derivation loops.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the AI TAP group to the Unified CI Infrastructure pattern, which is a great improvement. The changes are well-structured, replacing the legacy hook-based approach with a more standardized one. The documentation in README.md is also significantly improved and very clear.
I have a few suggestions to improve the robustness and maintainability of the new scripts:
- One important point is to ensure
BASE_GROUPis derived withinensure-infras.bashto make it self-contained. - I've also suggested making the
BASE_GROUPderivation logic consistent across all scripts. - Finally, I've pointed out some code duplication that could be refactored to improve maintainability.
Overall, this is a solid contribution that modernizes the testing infrastructure for the AI group.
|
|
||
| # 6. Execute group-specific setup hook if it exists | ||
| # This allows TAP groups to perform additional setup after all backends are running | ||
| SETUP_HOOK="${WORKSPACE}/test/tap/groups/${TAP_GROUP}/setup-infras.bash" |
There was a problem hiding this comment.
The script uses the BASE_GROUP variable (on line 98) to find a fallback setup hook, but it doesn't define this variable within the script. This relies on BASE_GROUP being present in the execution environment, which is risky and inconsistent with other scripts like run-tests-isolated.bash and destroy-infras.bash that derive it internally. To make the script more robust and self-contained, you should derive BASE_GROUP from TAP_GROUP before it's used. For consistency, it's best to use the same sed command used in destroy-infras.bash.
| SETUP_HOOK="${WORKSPACE}/test/tap/groups/${TAP_GROUP}/setup-infras.bash" | |
| BASE_GROUP=$(echo "${TAP_GROUP}" | sed -E "s/[-_]g[0-9]+.*//") | |
| SETUP_HOOK="${WORKSPACE}/test/tap/groups/${TAP_GROUP}/setup-infras.bash" |
| # This runs before the test runner container is removed, allowing cleanup | ||
| # of ProxySQL-specific configuration while admin is still accessible | ||
| if [ -n "${TAP_GROUP}" ]; then | ||
| BASE_GROUP="${TAP_GROUP%%-g[0-9]*}" |
There was a problem hiding this comment.
For consistency and robustness, consider using the same method to derive BASE_GROUP as in destroy-infras.bash. The sed command handles both - and _ as separators for the group suffix, which is more resilient to future naming convention changes.
| BASE_GROUP="${TAP_GROUP%%-g[0-9]*}" | |
| BASE_GROUP=$(echo "${TAP_GROUP}" | sed -E "s/[-_]g[0-9]+.*//") |
| # Derive DEFAULT_MYSQL_INFRA and DEFAULT_PGSQL_INFRA from infras.lst if not set | ||
| if [ -z "${DEFAULT_MYSQL_INFRA:-}" ] || [ -z "${DEFAULT_PGSQL_INFRA:-}" ]; then | ||
| INFRAS_LST="${SCRIPT_DIR}/infras.lst" | ||
| if [ -f "${INFRAS_LST}" ]; then | ||
| for INFRA_NAME in $(cat "${INFRAS_LST}"); do | ||
| if [[ "${INFRA_NAME}" == *mysql* ]] || [[ "${INFRA_NAME}" == *mariadb* ]]; then | ||
| DEFAULT_MYSQL_INFRA="${DEFAULT_MYSQL_INFRA:-${INFRA_NAME}}" | ||
| fi | ||
| if [[ "${INFRA_NAME}" == *pgsql* ]] || [[ "${INFRA_NAME}" == *pgdb* ]]; then | ||
| DEFAULT_PGSQL_INFRA="${DEFAULT_PGSQL_INFRA:-${INFRA_NAME}}" | ||
| fi | ||
| done | ||
| fi | ||
| fi |
There was a problem hiding this comment.
This block of code to derive DEFAULT_MYSQL_INFRA and DEFAULT_PGSQL_INFRA is duplicated from test/infra/control/ensure-infras.bash. While this makes the script runnable standalone, it introduces a maintainability issue. If the logic for deriving default infrastructures changes, it will need to be updated in multiple places.
Consider extracting this shared logic into a common helper script that can be sourced by both ensure-infras.bash and this hook script. This would improve code reuse and maintainability.
Fixes from PR #5461 review: 1. Fix BASE_GROUP derivation inconsistency in run-tests-isolated.bash - Changed from bash pattern matching `${TAP_GROUP%%-g[0-9]*}` to sed - Now uses same pattern as ensure-infras.bash: `sed -E "s/[-_]g[0-9]+.*//"` - Removed redundant BASE_GROUP reassignment 2. Update outdated comments in seed files - seed-mysql.sql and seed-pgsql.sql now correctly reference setup-infras.bash 3. Change INSERT OR IGNORE to INSERT OR REPLACE in mcp-config.sql - Ensures credentials are updated on reruns for deterministic state 4. Add shellcheck directives to env.sh files - Added `# shellcheck shell=bash` to ai/env.sh and ai-g1/env.sh 5. Add explicit validation for derived infrastructure names - setup-infras.bash now validates DEFAULT_MYSQL_INFRA and DEFAULT_PGSQL_INFRA - Provides clear error messages if infras.lst is misconfigured 6. Add WORKSPACE export to README examples - All runnable examples now include `export WORKSPACE=$(pwd)` Test verification: - ai-g1 group infrastructure setup works correctly - legacy-g2 group tests still pass (backward compatibility verified)
The Python tester sources env files using /bin/sh, but the previous implementation used bash-specific features (BASH_SOURCE[0] and source). Changed to inline the values from ai/env.sh for POSIX compatibility.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/infra/control/run-tests-isolated.bash (1)
141-154: Pre-cleanup hook logic looks good.The fallback from subgroup to supergroup hook correctly mirrors the pattern used in
ensure-infras.bashfor setup hooks. Using|| trueis appropriate here to ensure cleanup failures don't break the overall test run.One minor consideration: if the hook file exists but isn't executable, the script will fail silently (due to
|| true). You might want to add an explicit check or a warning message.💡 Optional: Add executable check with warning
if [ -f "${PRE_CLEANUP_HOOK}" ]; then + if [ ! -x "${PRE_CLEANUP_HOOK}" ]; then + echo "WARNING: Pre-cleanup hook exists but is not executable: ${PRE_CLEANUP_HOOK}" + fi echo ">>> Executing group pre-cleanup hook: ${PRE_CLEANUP_HOOK}" "${PRE_CLEANUP_HOOK}" || true # Allow cleanup to fail fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/infra/control/run-tests-isolated.bash` around lines 141 - 154, The pre-cleanup hook block in run-tests-isolated.bash currently tests for file existence using PRE_CLEANUP_HOOK but may silently fail if the file exists and is not executable; update the logic around PRE_CLEANUP_HOOK so before attempting to run it you check executable permission (test -x) and if the file exists but is not executable emit a warning via echo or logger referencing PRE_CLEANUP_HOOK, then skip execution (still honoring the || true behavior); keep the existing fallback to BASE_GROUP and the call-site that runs "${PRE_CLEANUP_HOOK}" || true, but gate that call with an executable check to avoid silent no-op failures.
🤖 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/tap/groups/ai/setup-infras.bash`:
- Around line 116-127: The seed step currently swallows all failures by
appending "|| echo ..." to the docker exec lines for MySQL and PostgreSQL;
update the MySQL seed invocation using MYSQL1_CONTAINER/MYSQL_SEED_SQL and the
PostgreSQL seed invocation using PGSQL_CONTAINER/PGSQL_SEED_SQL so that any
non-zero exit from docker exec is treated as a hard failure: capture and log the
command stderr/stdout (with a descriptive message including the container and
seed file variables) and then exit 1 (do not silently convert failures to
warnings). Ensure you replace the trailing "|| echo ..." with explicit error
handling that logs details and aborts the script on failure.
- Line 48: The assignment using export with command substitution for
ROOT_PASSWORD can mask failures; change to two steps so failures surface: first
compute and assign the hashed value to the variable (e.g., ROOT_PASSWORD="$(echo
-n "${INFRA_ID}" | sha256sum | head -c 10")"), check the command exit status or
ensure the variable is non-empty, then export ROOT_PASSWORD; update occurrences
of ROOT_PASSWORD in the script to use the newly exported value and ensure error
handling when the hash pipeline fails.
---
Nitpick comments:
In `@test/infra/control/run-tests-isolated.bash`:
- Around line 141-154: The pre-cleanup hook block in run-tests-isolated.bash
currently tests for file existence using PRE_CLEANUP_HOOK but may silently fail
if the file exists and is not executable; update the logic around
PRE_CLEANUP_HOOK so before attempting to run it you check executable permission
(test -x) and if the file exists but is not executable emit a warning via echo
or logger referencing PRE_CLEANUP_HOOK, then skip execution (still honoring the
|| true behavior); keep the existing fallback to BASE_GROUP and the call-site
that runs "${PRE_CLEANUP_HOOK}" || true, but gate that call with an executable
check to avoid silent no-op failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 448b2fc9-a643-4966-b953-f6a280339b8b
📒 Files selected for processing (8)
test/infra/control/run-tests-isolated.bashtest/tap/groups/ai-g1/env.shtest/tap/groups/ai/README.mdtest/tap/groups/ai/env.shtest/tap/groups/ai/mcp-config.sqltest/tap/groups/ai/seed-mysql.sqltest/tap/groups/ai/seed-pgsql.sqltest/tap/groups/ai/setup-infras.bash
✅ Files skipped from review due to trivial changes (3)
- test/tap/groups/ai-g1/env.sh
- test/tap/groups/ai/seed-pgsql.sql
- test/tap/groups/ai/mcp-config.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- test/tap/groups/ai/seed-mysql.sql
- test/tap/groups/ai/README.md
- test/tap/groups/ai/env.sh
| exit 1 | ||
| fi | ||
|
|
||
| export ROOT_PASSWORD=$(echo -n "${INFRA_ID}" | sha256sum | head -c 10) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
# Verify whether `export VAR=$(...)` masks substitution failures in this shell.
set -u
bash -c 'set -euo pipefail; export X=$(false); echo "masked: continued after export"; echo "status=$?"'Repository: sysown/proxysql
Length of output: 98
Avoid export ROOT_PASSWORD=$(...); this can mask hashing failures.
On Line 48, export with command substitution may return success even if the substitution fails. Split assignment and export so failures are not hidden.
Proposed fix
-export ROOT_PASSWORD=$(echo -n "${INFRA_ID}" | sha256sum | head -c 10)
+ROOT_PASSWORD="$(printf '%s' "${INFRA_ID}" | sha256sum | cut -c1-10)"
+export ROOT_PASSWORD📝 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 ROOT_PASSWORD=$(echo -n "${INFRA_ID}" | sha256sum | head -c 10) | |
| ROOT_PASSWORD="$(printf '%s' "${INFRA_ID}" | sha256sum | cut -c1-10)" | |
| export ROOT_PASSWORD |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 48-48: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/groups/ai/setup-infras.bash` at line 48, The assignment using export
with command substitution for ROOT_PASSWORD can mask failures; change to two
steps so failures surface: first compute and assign the hashed value to the
variable (e.g., ROOT_PASSWORD="$(echo -n "${INFRA_ID}" | sha256sum | head -c
10")"), check the command exit status or ensure the variable is non-empty, then
export ROOT_PASSWORD; update occurrences of ROOT_PASSWORD in the script to use
the newly exported value and ensure error handling when the hash pipeline fails.
| docker exec -i "${MYSQL1_CONTAINER}" mysql -h127.0.0.1 -uroot -p"${ROOT_PASSWORD}" < "${MYSQL_SEED_SQL}" || echo ">>> WARNING: MySQL seed may have partially failed (data may already exist)" | ||
| echo ">>> MySQL test data seeding completed" | ||
| else | ||
| echo ">>> WARNING: MySQL seed SQL not found at ${MYSQL_SEED_SQL}" | ||
| fi | ||
|
|
||
| # Seed test data on PostgreSQL | ||
| echo ">>> AI post-infras hook: Seeding PostgreSQL test data..." | ||
| PGSQL_SEED_SQL="${SCRIPT_DIR}/seed-pgsql.sql" | ||
| if [ -f "${PGSQL_SEED_SQL}" ]; then | ||
| PGSQL_CONTAINER="${DEFAULT_PGSQL_INFRA}-${INFRA_ID}-pgdb1-1" | ||
| docker exec -i "${PGSQL_CONTAINER}" psql -X -Upostgres < "${PGSQL_SEED_SQL}" || echo ">>> WARNING: PostgreSQL seed may have partially failed (data may already exist)" |
There was a problem hiding this comment.
Don’t swallow all seed failures; this can hide real setup regressions.
Lines 116 and 127 convert any seed failure into a warning, including auth/connectivity/SQL syntax errors. That can make CI pass setup while tests fail later with harder-to-debug symptoms.
Proposed fix
- docker exec -i "${MYSQL1_CONTAINER}" mysql -h127.0.0.1 -uroot -p"${ROOT_PASSWORD}" < "${MYSQL_SEED_SQL}" || echo ">>> WARNING: MySQL seed may have partially failed (data may already exist)"
+ if ! docker exec -i "${MYSQL1_CONTAINER}" mysql -h127.0.0.1 -uroot -p"${ROOT_PASSWORD}" < "${MYSQL_SEED_SQL}"; then
+ echo "ERROR: MySQL seed failed (${MYSQL_SEED_SQL})"
+ exit 1
+ fi
@@
- docker exec -i "${PGSQL_CONTAINER}" psql -X -Upostgres < "${PGSQL_SEED_SQL}" || echo ">>> WARNING: PostgreSQL seed may have partially failed (data may already exist)"
+ if ! docker exec -i "${PGSQL_CONTAINER}" psql -X -Upostgres < "${PGSQL_SEED_SQL}"; then
+ echo "ERROR: PostgreSQL seed failed (${PGSQL_SEED_SQL})"
+ exit 1
+ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/groups/ai/setup-infras.bash` around lines 116 - 127, The seed step
currently swallows all failures by appending "|| echo ..." to the docker exec
lines for MySQL and PostgreSQL; update the MySQL seed invocation using
MYSQL1_CONTAINER/MYSQL_SEED_SQL and the PostgreSQL seed invocation using
PGSQL_CONTAINER/PGSQL_SEED_SQL so that any non-zero exit from docker exec is
treated as a hard failure: capture and log the command stderr/stdout (with a
descriptive message including the container and seed file variables) and then
exit 1 (do not silently convert failures to warnings). Ensure you replace the
trailing "|| echo ..." with explicit error handling that logs details and aborts
the script on failure.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tap/groups/ai-g1/env.sh`:
- Around line 1-2: Add a ShellCheck dialect declaration to the script header to
silence SC2148: insert the comment "# shellcheck shell=sh" as the first line of
the file (before the existing descriptive comments) so the linter knows this is
POSIX sh; this change targets the environment script's header used by the test
harness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b47243b-5551-44c9-b3c5-d0f001938447
📒 Files selected for processing (1)
test/tap/groups/ai-g1/env.sh
| # AI-g1 Subgroup Environment Configuration | ||
| # Inherits from parent ai group - duplicated here for POSIX sh compatibility |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify shell declaration presence in this file.
sed -n '1,3p' test/tap/groups/ai-g1/env.shRepository: sysown/proxysql
Length of output: 179
🏁 Script executed:
# Find other env.sh files in the project and check how they handle shell declarations
fd 'env\.sh' test/tap -type f | head -10 | xargs -I {} sh -c 'echo "=== {} ===" && head -3 {}'Repository: sysown/proxysql
Length of output: 229
🏁 Script executed:
# Find other env.sh files in the test/tap directory and check their shell declarations
fd 'env\.sh' test/tap --type f | head -15 | while read file; do echo "=== $file ===" && head -3 "$file"; doneRepository: sysown/proxysql
Length of output: 2011
🏁 Script executed:
# Search for how these env.sh files are sourced in the codebase
rg -n 'test/tap/groups.*env\.sh|source.*env\.sh|\. .*env\.sh' test/ --type sh --type bash -A 2 | head -50Repository: sysown/proxysql
Length of output: 87
🏁 Script executed:
# Search for how env.sh files are sourced
rg -n 'test/tap/groups.*env\.sh|source.*env\.sh|\. .*env\.sh' test/ -A 2 | head -60Repository: sysown/proxysql
Length of output: 2642
🏁 Script executed:
# Check how proxysql-tester.py sources the env files
sed -n '655,680p' test/scripts/bin/proxysql-tester.pyRepository: sysown/proxysql
Length of output: 1788
Declare the shell dialect for lint stability.
Line 1 triggers ShellCheck SC2148 because the script type is unspecified. Since this file is sourced in /bin/sh context (via the test harness), add # shellcheck shell=sh at the top to match the file's stated POSIX sh compatibility.
Proposed patch
+# shellcheck shell=sh
# AI-g1 Subgroup Environment Configuration
# Inherits from parent ai group - duplicated here for POSIX sh compatibility📝 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.
| # AI-g1 Subgroup Environment Configuration | |
| # Inherits from parent ai group - duplicated here for POSIX sh compatibility | |
| # shellcheck shell=sh | |
| # AI-g1 Subgroup Environment Configuration | |
| # Inherits from parent ai group - duplicated here for POSIX sh compatibility |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/groups/ai-g1/env.sh` around lines 1 - 2, Add a ShellCheck dialect
declaration to the script header to silence SC2148: insert the comment "#
shellcheck shell=sh" as the first line of the file (before the existing
descriptive comments) so the linter knows this is POSIX sh; this change targets
the environment script's header used by the test harness.
|
This commit migrates the ai TAP group from its legacy hook-based approach to the standard test/infra/ Unified CI Infrastructure pattern.
Changes:
Added generic hook system to infrastructure scripts:
Migrated ai group infrastructure:
Added group-specific MCP configuration:
Updated documentation in README.md with usage examples
Architecture:
Summary by CodeRabbit
Refactor
New Features
Documentation
Chores