feat: add retail product search skill and harden the Python recipe tooling skills - #2372
Conversation
…and supporting scripts
…ecipe-pyproject and add docstring regression tests to extract-python-environment-variables
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
ruff-formatter
[ruff-formatter] reported by reviewdog 🐶
[ruff-formatter] reported by reviewdog 🐶
[ruff-formatter] reported by reviewdog 🐶
[ruff-formatter] reported by reviewdog 🐶
[ruff-formatter] reported by reviewdog 🐶
[ruff-formatter] reported by reviewdog 🐶
[ruff-formatter] reported by reviewdog 🐶
[ruff-formatter] reported by reviewdog 🐶
[ruff-formatter] reported by reviewdog 🐶
[ruff-formatter] reported by reviewdog 🐶
|
|
||
| for field in required: | ||
| if field not in record or not record[field]: | ||
| row_errors.append(f"Row {row_num}: missing required field '{field}'") |
There was a problem hiding this comment.
Using not record[field] will falsely report a missing field if the value is a numeric 0 or 0.0 (such as a valid free price). Check instead that the value is not None or an empty string.
| """ | ||
| errors = [] | ||
| for field in REQUIRED_FIELDS: | ||
| if field not in product or not product[field]: |
There was a problem hiding this comment.
Using not product[field] will falsely report a missing required field if the value is a numeric 0 or 0.0 (such as a valid free price). Check instead that the value is not None or an empty string.
| converted = {} | ||
| converted["product_id"] = product.get("product_id", "") | ||
| converted["name"] = product.get("name", "") | ||
| converted["description"] = product.get("description", "") |
There was a problem hiding this comment.
Using and product[field] to check for numeric fields like price, rating, or stock will skip 0 or 0.0 (since they are falsy), causing them to be omitted. This will result in a BigQuery ingestion crash for the required 'price' field, and incorrect null values for rating and stock.
| # Try PATH lookup first; fall back to absolute paths for sandboxed | ||
| # shells that launch with a stripped PATH. | ||
| PYTHON_BIN="" | ||
| for py in python3.13 python3.12 python3.11 python3.10 python3; do |
There was a problem hiding this comment.
Since the recipe's Python floor is >=3.11 (enforced by the repository's alignment rules), allowing Python 3.10 in the bootstrap script will cause pip install to crash inside the created virtual environment. The bootstrap should restrict the allowed interpreters to Python >=3.11.
…eaking collection lookups and comparisons
… update workflow docs
1 participant
Summary
Two related pieces of work, developed together because the second was discovered by running the first:
skills/retail/retail-product-search, a semantic product search agent built on Vertex AI Vector Search and BigQuery..agents/skills/, found by runningprepare-python-recipeagainst that recipe. Several were actively corrupting source files.Note
AGENTS.mdasks that.agents/skills/changes not share a PR with recipe changes. They are combined here because the recipe is what surfaced the tooling bugs; happy to split if reviewers prefer.Tooling fixes
extract-python-environment-variables(2.1.0 → 2.2.0)_post_header_indexonly recognised bare"""docstringsr""",u""") gotimport osinjected above it, demoting it to a dead expression and losing__doc__. Now AST-based.os.getenv("VAR")emitted even when noload_dotenv()bootstrap could be installed.env, so the value resolved toNoneat runtime.run_step_load_dotenvnow returns whether a bootstrap exists; without one the original literal is kept as the fallback.MODEL_NAMEclaimed too eagerlyDEFAULT_EMBEDDING_MODEL→EMBEDDING_MODEL).generate-python-runnability-test(1.0.0 → 1.1.0)The generated test does
import <module>, which only resolves if the recipe root is onsys.path— previously assumed, never checked. A recipe with no[build-system]got a test that always died withModuleNotFoundErrorwhilepy_compilestill reported success.New
detect_import_support()classifies how the root is reached (installable/pythonpath-ini/existing-conftest/unresolved) and writes atests/conftest.pyshim when nothing else provides it. An existing conftest is never clobbered.align-recipe-pyproject(1.0.0 → 1.1.0)Two new report-only checks:
stale-python-version-refs— raisingrequires-pythonis not self-contained. A bootstrap script whose interpreter allowlist still accepts the old floor will build a venv the recipe then refuses to install into. The scan requires a Python-ish context, sogemini-3.5-flashis not a false positive.runnability-test-in-testpaths— atestpathsof["tests/unit", "tests/integration"]means a barepytestnever collects the required runnability test.prepare-python-recipe(1.0.0 → 1.1.0)required_dirsthat.github/policy.ymlmandates forskills/recipes (tests/unit/was missing and no phase created it).--collect-onlywas considered and rejected: the guarded test shape puts the import inside the test function, so collection proves nothing.validate manifestandvalidate structure. The pipeline previously reported a clean run on a recipe CI then rejected.Testing
521 passed(pytest .agents/skills tools/tests), up from 204 at baseline.align-recipe-pyprojectandgenerate-python-runnability-test, which had none.ruff format --checkandruff checkclean across.agents/skills.