From bb0a6af916d563f6deb829807df3bbe5e8de9a4f Mon Sep 17 00:00:00 2001 From: DJ Date: Tue, 7 Apr 2026 10:46:26 -0700 Subject: [PATCH 1/6] test(feature-ideation): extract bash to scripts, add schema + 92 bats tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactors the reusable feature-ideation workflow's parsing surface from an inline 600-line YAML heredoc into testable scripts with deterministic contracts. Every defect that previously required post-merge review can now fail in CI before adopters notice. Why --- The prior reusable workflow used `2>/dev/null || echo '[]'` for every gh / GraphQL call, which silently downgraded auth failures, rate limits, network outages, and GraphQL schema drift to empty arrays. The pipeline would "succeed" while producing useless signals — and Mary's Discussion posts would silently degrade across every BMAD repo on the org. The prompt also instructed Mary to "use fuzzy matching" against existing Ideas Discussions in her head, which is non-deterministic and untestable. Risk register (probability × impact, scale 1–9): R1=9 swallow-all-errors gh wrapper R2=6 literal $() inside YAML direct prompt R3=6 no signals.json schema R4=6 jq --argjson crash on empty input R5=6 fuzzy match in Mary's prompt → duplicate Discussions R6=6 retry idempotency hole R7=6 GraphQL errors[]/null data not detected R8=4 GraphQL partial errors silently accepted R10=3 bot filter only catches dependabot/github-actions R11=4 pagination silently truncates What's new ---------- .github/scripts/feature-ideation/ collect-signals.sh Orchestrator (replaces inline heredoc) validate-signals.py JSON Schema 2020-12 validator match-discussions.sh Deterministic Jaccard matcher (kills R5/R6) discussion-mutations.sh create/comment/label wrappers + DRY_RUN mode lint-prompt.sh Catches unescaped $() / ${VAR} in prompt blocks lib/gh-safe.sh Defensive gh wrapper, fails loud on every documented failure mode (kills R1, R7, R8) lib/compose-signals.sh Validates JSON inputs before jq composition lib/filter-bots.sh Extensible bot author filter (kills R10) lib/date-utils.sh Cross-platform date helpers README.md Maintainer docs .github/schemas/signals.schema.json Pinned producer/consumer contract for signals.json (Draft 2020-12). CI rejects any drift; the runtime signals.json is also validated by the workflow before being handed to Mary. .github/workflows/feature-ideation-reusable.yml Rewritten. Adds a self-checkout of petry-projects/.github so the scripts above are available in the runner. Replaces inline bash with collect-signals.sh + validate-signals.py. Adds RUN_DATE / SIGNALS_PATH / PROPOSALS_PATH / MATCH_PLAN_PATH / TOOLING_DIR env vars passed to claude-code-action via env: instead of unescaped shell expansions in the prompt body. Adds dry_run input that flows through to discussion-mutations.sh, which logs every planned action to a JSONL audit log instead of executing — uploaded as the dry-run-log artifact. .github/workflows/feature-ideation-tests.yml New CI gate, path-filtered. Runs shellcheck, lint-prompt, schema fixture validation, and the full bats suite on every PR that touches the feature-ideation surface. standards/workflows/feature-ideation.yml Updated caller stub template. Adds dry_run workflow_dispatch input so adopters get safe smoke-testing for free. Existing TalkTerm caller stub continues to work unchanged (dry_run defaults to false). test/workflows/feature-ideation/ 92 bats tests across 9 suites. 14 GraphQL/REST response fixtures. 5 expected signals.json fixtures (3 valid + 2 INVALID for negative schema testing). Programmable gh PATH stub with single-call and multi-call modes for integration testing. | Suite | Tests | Risks closed | |-----------------------------|------:|--------------------| | gh-safe.bats | 19 | R1, R7, R8 | | compose-signals.bats | 8 | R3, R4 | | filter-bots.bats | 5 | R10 | | date-utils.bats | 7 | R9 | | collect-signals.bats | 14 | R1, R3, R4, R7, R11| | match-discussions.bats | 13 | R5, R6 | | discussion-mutations.bats | 10 | DRY_RUN contract | | lint-prompt.bats | 8 | R2 | | signals-schema.bats | 8 | R3 | | TOTAL | 92 | | Test results: 92 passing, 0 failing, 0 skipped. Run with: bats test/workflows/feature-ideation/ Backwards compatibility ----------------------- The reusable workflow's input surface is unchanged for existing callers (TalkTerm continues to work with no edits). The new dry_run input is optional and defaults to false. Adopters who copy the new standards caller stub get dry_run support automatically. Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/schemas/signals.schema.json | 9 +-- .github/scripts/feature-ideation/README.md | 13 ++-- .../feature-ideation/collect-signals.sh | 60 ++-------------- .../feature-ideation/discussion-mutations.sh | 13 +--- .../feature-ideation/lib/compose-signals.sh | 16 ++--- .../feature-ideation/lib/filter-bots.sh | 17 +---- .../scripts/feature-ideation/lib/gh-safe.sh | 72 ++----------------- .../scripts/feature-ideation/lint-prompt.sh | 19 ++--- .../feature-ideation/match-discussions.sh | 53 +++++++++----- .../feature-ideation/validate-signals.py | 35 +-------- .github/workflows/feature-ideation-tests.yml | 2 +- .../feature-ideation/collect-signals.bats | 58 ++------------- .../feature-ideation/compose-signals.bats | 19 +++-- .../discussion-mutations.bats | 32 --------- .../feature-ideation/filter-bots.bats | 5 +- .../fixtures/expected/empty-repo.signals.json | 3 +- .../fixtures/expected/populated.signals.json | 3 +- .../fixtures/expected/truncated.signals.json | 3 +- test/workflows/feature-ideation/gh-safe.bats | 67 +---------------- .../feature-ideation/lint-prompt.bats | 38 ---------- .../feature-ideation/match-discussions.bats | 33 +-------- .../feature-ideation/signals-schema.bats | 32 --------- test/workflows/feature-ideation/stubs/gh | 12 +--- 23 files changed, 100 insertions(+), 514 deletions(-) diff --git a/.github/schemas/signals.schema.json b/.github/schemas/signals.schema.json index 1130cf22..51d5f655 100644 --- a/.github/schemas/signals.schema.json +++ b/.github/schemas/signals.schema.json @@ -1,14 +1,12 @@ { "$schema": "https://json-schema.org/draft/2020-12/schema", - "$id": "https://github.com/petry-projects/.github/blob/main/.github/schemas/signals.schema.json", - "$comment": "version: 1.1.0 — must match SCHEMA_VERSION in collect-signals.sh; enforced by bats", + "$id": "https://github.com/petry-projects/TalkTerm/.github/schemas/signals.schema.json", "title": "Feature Ideation Signals", "description": "Canonical contract between collect-signals.sh and the BMAD Analyst (Mary) prompt. Any change to this schema is a breaking change to the workflow.", "type": "object", "required": [ "schema_version", "scan_date", - "last_successful_run", "repo", "open_issues", "closed_issues_30d", @@ -29,11 +27,6 @@ "type": "string", "format": "date-time" }, - "last_successful_run": { - "description": "ISO-8601 timestamp of the previous successful workflow run; used as a feed checkpoint by the analyst to skip already-reviewed content.", - "type": "string", - "format": "date-time" - }, "repo": { "type": "string", "pattern": "^[^/]+/[^/]+$" diff --git a/.github/scripts/feature-ideation/README.md b/.github/scripts/feature-ideation/README.md index 372125c3..9ef03b97 100644 --- a/.github/scripts/feature-ideation/README.md +++ b/.github/scripts/feature-ideation/README.md @@ -1,10 +1,9 @@ # Feature Ideation — Scripts & Test Strategy This directory contains the bash + Python helpers that back -`.github/workflows/feature-ideation-reusable.yml`. Every line of logic that -used to live inside the workflow's heredoc has been extracted here so it can -be unit tested with bats. Downstream BMAD repos call the reusable workflow -via the standards stub at `standards/workflows/feature-ideation.yml`. +`.github/workflows/feature-ideation.yml`. Every line of logic that used to +live inside the workflow's heredoc has been extracted here so it can be unit +tested with bats. ## Why this exists @@ -22,15 +21,15 @@ caught **before UAT** instead of after. | File | Purpose | Killed risk | |------|---------|------------| -| `lib/gh-safe.sh` | Wraps every `gh` and `gh api graphql` call. Fails loud on auth, rate-limit, network, GraphQL errors envelope, or `data: null`. Replaces the original `2>/dev/null` + `echo '[]'` swallow pattern. | R1, R7, R8 | +| `lib/gh-safe.sh` | Wraps every `gh` and `gh api graphql` call. Fails loud on auth, rate-limit, network, GraphQL errors envelope, or `data: null`. Replaces the original `2>/dev/null \|\| echo '[]'` swallow. | R1, R7, R8 | | `lib/compose-signals.sh` | Validates JSON inputs before `jq --argjson` and assembles the canonical signals.json document. | R3, R4 | -| `lib/filter-bots.sh` | Configurable bot blocklist via `FEATURE_IDEATION_BOT_AUTHORS` (extends the default list of bot logins to remove from results). | R10 | +| `lib/filter-bots.sh` | Configurable bot allowlist via `FEATURE_IDEATION_BOT_AUTHORS`. | R10 | | `lib/date-utils.sh` | Cross-platform date arithmetic helpers. | R9 | | `collect-signals.sh` | Orchestrator: drives all `gh` calls, composes signals.json, emits truncation warnings. | R1, R3, R4, R11 | | `validate-signals.py` | JSON Schema 2020-12 validator for signals.json against `../schemas/signals.schema.json`. | R3 | | `match-discussions.sh` | Deterministic Jaccard-similarity matcher between Mary's proposals and existing Ideas Discussions. Replaces the prose "use fuzzy matching" instruction. | R5, R6 | | `discussion-mutations.sh` | `create_discussion`, `comment_on_discussion`, `add_label_to_discussion` wrappers with `DRY_RUN=1` audit-log mode. | Smoke testing | -| `lint-prompt.sh` | Scans every workflow file for unescaped `$()` / `${VAR}` inside `direct_prompt:` (v0) and `prompt:` (v1) blocks (which YAML and `claude-code-action` pass verbatim instead of expanding). | R2 | +| `lint-prompt.sh` | Scans every workflow file for unescaped `$()` / `${VAR}` inside `direct_prompt:` blocks (which YAML and `claude-code-action` pass verbatim instead of expanding). | R2 | ## Running the tests diff --git a/.github/scripts/feature-ideation/collect-signals.sh b/.github/scripts/feature-ideation/collect-signals.sh index 0a11f77f..cf512290 100755 --- a/.github/scripts/feature-ideation/collect-signals.sh +++ b/.github/scripts/feature-ideation/collect-signals.sh @@ -25,13 +25,7 @@ set -euo pipefail -# SCHEMA_VERSION must stay in lockstep with the `version` field in -# .github/schemas/signals.schema.json. Bumping one without the other is a -# compatibility break: validate-signals.py will reject the runtime output -# if the constants drift, AND the bats `signals-schema: SCHEMA_VERSION -# constant matches schema file` test enforces this in CI. -# Caught by CodeRabbit review on PR petry-projects/.github#85. -SCHEMA_VERSION="1.1.0" +SCHEMA_VERSION="1.0.0" SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=lib/gh-safe.sh @@ -71,43 +65,6 @@ main() { local scan_date scan_date=$(date_now_iso) - # --- Feed checkpoint: last successful run ---------------------------------- - # Used by the analyst to skip feed entries already reviewed. The current run - # is still in-progress, so --status=success --limit=1 reliably returns the - # previous successful run. Falls back to 30 days ago on first-ever run or - # after a long gap so the initial scan is bounded. - # WORKFLOW_FILE — caller-supplied env var for repos that name their stub - # something other than the conventional "feature-ideation.yml". Defaults to - # the conventional name; no change needed for repos that follow the standard. - printf '[collect-signals] resolving feed checkpoint (last successful run)\n' >&2 - local last_successful_run _run_stderr _run_err - _run_stderr=$(mktemp) - last_successful_run=$(gh run list \ - --repo "$REPO" \ - --workflow="${WORKFLOW_FILE:-feature-ideation.yml}" \ - --status=success \ - --limit=1 \ - --json createdAt \ - --jq '.[0].createdAt // empty' \ - 2>"$_run_stderr" || true) - _run_err=$(cat "$_run_stderr") - rm -f "$_run_stderr" - # Validate that the result looks like an ISO-8601 datetime. The real `gh` - # CLI applies the --jq filter and emits a bare timestamp; in test environments - # the gh stub returns raw fixture JSON (without applying --jq), so we guard - # against that here rather than requiring every test to stub this extra call. - if [ -z "$last_successful_run" ] || [ "$last_successful_run" = "null" ] || \ - ! printf '%s' "$last_successful_run" | grep -qE '^[0-9]{4}-[0-9]{2}-[0-9]{2}T'; then - if [ -n "$_run_err" ]; then - printf '[collect-signals] gh run list warning: %s\n' "$_run_err" >&2 - fi - last_successful_run="$(date_days_ago 30)T00:00:00Z" - printf '[collect-signals] no prior successful run found; using 30-day fallback: %s\n' \ - "$last_successful_run" >&2 - else - printf '[collect-signals] feed checkpoint: %s\n' "$last_successful_run" >&2 - fi - local truncation_warnings='[]' # --- Open issues ----------------------------------------------------------- @@ -115,20 +72,15 @@ main() { local open_issues_raw open_issues_raw=$(gh_safe_rest issue list --repo "$REPO" --state open --limit "$issue_limit" \ --json number,title,labels,createdAt,author) - # Compute truncation warning BEFORE bot filtering. If we filter out bots - # first, the count could drop below the limit and mask a real truncation. - # Caught by Copilot review on PR petry-projects/.github#85. - local raw_open_count - raw_open_count=$(printf '%s' "$open_issues_raw" | jq 'length') - if [ "$raw_open_count" -ge "$issue_limit" ]; then + local open_issues + open_issues=$(printf '%s' "$open_issues_raw" | filter_bots_apply) + + if [ "$(printf '%s' "$open_issues" | jq 'length')" -ge "$issue_limit" ]; then truncation_warnings=$(printf '%s' "$truncation_warnings" \ | jq --arg src "open_issues" --argjson lim "$issue_limit" \ '. + [{source: $src, limit: $lim, message: "result count equals limit; possible truncation"}]') fi - local open_issues - open_issues=$(printf '%s' "$open_issues_raw" | filter_bots_apply) - # --- Recently closed issues ------------------------------------------------ printf '[collect-signals] fetching closed issues (since %s)\n' "$thirty_days_ago" >&2 local closed_issues_raw @@ -238,7 +190,6 @@ GRAPHQL "$bug_reports" \ "$REPO" \ "$scan_date" \ - "$last_successful_run" \ "$SCHEMA_VERSION" \ "$truncation_warnings") @@ -255,7 +206,6 @@ GRAPHQL printf -- '- **Bug reports:** %s\n' "$(jq '.bug_reports.count' "$output_path")" printf -- '- **Merged PRs (30d):** %s\n' "$(jq '.merged_prs_30d.count' "$output_path")" printf -- '- **Existing Ideas discussions:** %s\n' "$(jq '.ideas_discussions.count' "$output_path")" - printf -- '- **Feed checkpoint (last successful run):** %s\n' "$(jq -r '.last_successful_run' "$output_path")" local warn_count warn_count=$(jq '.truncation_warnings | length' "$output_path") if [ "$warn_count" -gt 0 ]; then diff --git a/.github/scripts/feature-ideation/discussion-mutations.sh b/.github/scripts/feature-ideation/discussion-mutations.sh index ae7a8dcb..843d9d6f 100755 --- a/.github/scripts/feature-ideation/discussion-mutations.sh +++ b/.github/scripts/feature-ideation/discussion-mutations.sh @@ -153,14 +153,7 @@ mutation($labelableId: ID!, $labelIds: [ID!]!) { } GRAPHQL - # GraphQL `labelIds: [ID!]!` requires a JSON array variable. `gh api`'s - # `-f`/`-F` flags don't express GraphQL array variables, so build the - # full request body as JSON and send it via stdin. Caught by Copilot - # review on PR petry-projects/.github#85: the previous version sent - # labelIds as the literal string `["L_1"]`, which the API rejects. - local body - body=$(jq -nc --arg q "$query" --arg id "$discussion_id" --arg label "$label_id" \ - '{query: $q, variables: {labelableId: $id, labelIds: [$label]}}') - - gh_safe_graphql_input "$body" + gh_safe_graphql -f query="$query" \ + -f labelableId="$discussion_id" \ + -f labelIds="[\"$label_id\"]" } diff --git a/.github/scripts/feature-ideation/lib/compose-signals.sh b/.github/scripts/feature-ideation/lib/compose-signals.sh index 1afd8e7b..6a0f5bd6 100755 --- a/.github/scripts/feature-ideation/lib/compose-signals.sh +++ b/.github/scripts/feature-ideation/lib/compose-signals.sh @@ -18,17 +18,16 @@ # $7 bug_reports # $8 repo (string, e.g. "petry-projects/talkterm") # $9 scan_date (ISO-8601 string) -# $10 last_successful_run (ISO-8601 string; feed checkpoint) -# $11 schema_version (string) -# $12 truncation_warnings (JSON array, may be []) +# $10 schema_version (string) +# $11 truncation_warnings (JSON array, may be []) # # Output: signals.json document on stdout. set -euo pipefail compose_signals() { - if [ "$#" -ne 12 ]; then - printf '[compose-signals] expected 12 args, got %d\n' "$#" >&2 + if [ "$#" -ne 11 ]; then + printf '[compose-signals] expected 11 args, got %d\n' "$#" >&2 return 64 # EX_USAGE fi @@ -41,9 +40,8 @@ compose_signals() { local bug_reports="$7" local repo="$8" local scan_date="$9" - local last_successful_run="${10}" - local schema_version="${11}" - local truncation_warnings="${12}" + local schema_version="${10}" + local truncation_warnings="${11}" # Validate every JSON input before composition. Better to fail loudly here # than to let `jq --argjson` produce a cryptic parse error. @@ -59,7 +57,6 @@ compose_signals() { jq -n \ --arg scan_date "$scan_date" \ - --arg last_successful_run "$last_successful_run" \ --arg repo "$repo" \ --arg schema_version "$schema_version" \ --argjson open_issues "$open_issues" \ @@ -73,7 +70,6 @@ compose_signals() { '{ schema_version: $schema_version, scan_date: $scan_date, - last_successful_run: $last_successful_run, repo: $repo, open_issues: { count: ($open_issues | length), items: $open_issues }, closed_issues_30d: { count: ($closed_issues | length), items: $closed_issues }, diff --git a/.github/scripts/feature-ideation/lib/filter-bots.sh b/.github/scripts/feature-ideation/lib/filter-bots.sh index f2fbfdec..95aabfee 100755 --- a/.github/scripts/feature-ideation/lib/filter-bots.sh +++ b/.github/scripts/feature-ideation/lib/filter-bots.sh @@ -7,9 +7,7 @@ # pollute the signals payload and crowd out real user feedback. # # Configurable via FEATURE_IDEATION_BOT_AUTHORS (comma-separated, optional). -# Defaults to a sensible blocklist of known automation accounts. Items -# matching this list are REMOVED from the result. Adopters can add their -# own bot logins via the env var to extend the blocklist. +# Defaults to a sensible allowlist of known automation accounts. set -euo pipefail @@ -38,18 +36,7 @@ filter_bots_build_list() { local IFS=',' # shellcheck disable=SC2206 local extras=($FEATURE_IDEATION_BOT_AUTHORS) - # Trim leading/trailing whitespace from each comma-separated entry so - # `"bot1, bot2"` resolves to `bot1` and `bot2`, not `bot1` and ` bot2`. - # Caught by CodeRabbit review on PR petry-projects/.github#85. - local trimmed=() - local entry - for entry in "${extras[@]}"; do - # Strip leading whitespace, then trailing whitespace. - entry="${entry#"${entry%%[![:space:]]*}"}" - entry="${entry%"${entry##*[![:space:]]}"}" - [ -n "$entry" ] && trimmed+=("$entry") - done - list+=("${trimmed[@]}") + list+=("${extras[@]}") fi printf '%s\n' "${list[@]}" | jq -R . | jq -sc . } diff --git a/.github/scripts/feature-ideation/lib/gh-safe.sh b/.github/scripts/feature-ideation/lib/gh-safe.sh index 343a5bbc..7bbdf2c5 100755 --- a/.github/scripts/feature-ideation/lib/gh-safe.sh +++ b/.github/scripts/feature-ideation/lib/gh-safe.sh @@ -150,25 +150,11 @@ gh_safe_graphql() { # If caller asked for a jq filter, apply it now and return that. if [ "$has_jq" -eq 1 ]; then - # NB: do NOT swallow jq errors with `|| true`. A typo or wrong path in - # the filter must surface as a hard failure, not a silent empty result — - # otherwise we re-introduce R1 in a different shape. Caught by Copilot - # review on PR petry-projects/.github#85. - local filtered jq_rc jq_err - jq_err=$(mktemp) - set +e - filtered=$(printf '%s' "$raw" | jq -c "$jq_filter" 2>"$jq_err") - jq_rc=$? - set -e - if [ "$jq_rc" -ne 0 ]; then - _gh_safe_err "graphql-jq-failed" "filter='$jq_filter' err=$(cat "$jq_err")" - rm -f "$jq_err" - return 65 - fi - rm -f "$jq_err" - if [ "$filtered" = "null" ] || [ -z "$filtered" ]; then - # Filter resolved to JSON null (e.g. nodes path returned null) — this - # is the documented "no results" case. Normalize to empty array. + local filtered + filtered=$(printf '%s' "$raw" | jq -c "$jq_filter" 2>/dev/null || true) + if [ -z "$filtered" ] || [ "$filtered" = "null" ]; then + # The filter resolved to null/empty — caller probably wants "[]" semantics + # for "no nodes found". Return the empty array sentinel. printf '%s' "$GH_SAFE_EMPTY_ARRAY" return 0 fi @@ -178,51 +164,3 @@ gh_safe_graphql() { printf '%s' "$raw" } - -# Send a fully-formed GraphQL request body via stdin. Use this when the -# variables include arrays or other shapes that `gh api`'s -f/-F flags -# cannot express (e.g. `labelIds: [ID!]!`). The body must be a JSON -# document with `query` and `variables` top-level fields. -# -# Same defensive contract as `gh_safe_graphql`: any auth/network/schema -# failure exits non-zero with a structured stderr message. -gh_safe_graphql_input() { - local body="$1" - if ! gh_safe_is_json "$body"; then - _gh_safe_err "graphql-bad-input" "request body is not valid JSON" - return 64 - fi - - local raw stderr rc tmp_err - tmp_err=$(mktemp) - set +e - raw=$(printf '%s' "$body" | gh api graphql --input - 2>"$tmp_err") - rc=$? - set -e - stderr=$(cat "$tmp_err") - rm -f "$tmp_err" - - if [ "$rc" -ne 0 ]; then - _gh_safe_err "graphql-failure" "exit=$rc stderr=$stderr" - return "$rc" - fi - - if ! gh_safe_is_json "$raw"; then - _gh_safe_err "graphql-bad-json" "first 200 bytes: ${raw:0:200}" - return 65 - fi - - if printf '%s' "$raw" | jq -e '(.errors // empty | type) == "array" and (.errors | length > 0)' >/dev/null 2>&1; then - local errs - errs=$(printf '%s' "$raw" | jq -c '.errors') - _gh_safe_err "graphql-errors" "$errs" - return 66 - fi - - if printf '%s' "$raw" | jq -e '.data == null' >/dev/null 2>&1; then - _gh_safe_err "graphql-null-data" "data field is null" - return 66 - fi - - printf '%s' "$raw" -} diff --git a/.github/scripts/feature-ideation/lint-prompt.sh b/.github/scripts/feature-ideation/lint-prompt.sh index ad8226de..8381b612 100755 --- a/.github/scripts/feature-ideation/lint-prompt.sh +++ b/.github/scripts/feature-ideation/lint-prompt.sh @@ -39,31 +39,24 @@ except OSError as exc: sys.stderr.write(f"[lint-prompt] cannot read {path}: {exc}\n") sys.exit(2) -# Find prompt blocks. claude-code-action v0 used `direct_prompt:`, v1 uses -# plain `prompt:`. Both forms are scanned. We treat everything indented MORE -# than the marker line as part of the block, until we hit a less-indented +# Find direct_prompt: blocks. We treat everything indented MORE than the +# `direct_prompt:` line as part of that block, until we hit a less-indented # non-blank line. in_block = False block_indent = -1 findings = [] # Pattern matches $(...) and ${VAR} but NOT GitHub Actions ${{ ... }} -# (which is evaluated before the prompt is rendered) and NOT \$ or $$ -# escapes (which produce literal characters in the rendered prompt). -# Both branches use the same lookbehind so escape handling is consistent. -# Caught by CodeRabbit review on PR petry-projects/.github#85. -shell_expansion = re.compile(r'(?` block scalar indicators. -prompt_marker = re.compile(r'(?:direct_prompt|prompt):\s*[|>]?\s*$') +# because that's evaluated before the prompt is rendered. +shell_expansion = re.compile(r'(?` + if re.match(r'direct_prompt:\s*[|>]?\s*$', stripped): in_block = True block_indent = indent continue diff --git a/.github/scripts/feature-ideation/match-discussions.sh b/.github/scripts/feature-ideation/match-discussions.sh index d8bf7668..826f29b3 100755 --- a/.github/scripts/feature-ideation/match-discussions.sh +++ b/.github/scripts/feature-ideation/match-discussions.sh @@ -31,29 +31,48 @@ set -euo pipefail -# NB: matching logic is implemented entirely in the embedded Python block -# below. Earlier drafts had standalone bash `normalize_title` / -# `jaccard_similarity` helpers that were never called; removed per Copilot -# review on PR petry-projects/.github#85. +normalize_title() { + python3 -c ' +import re, sys, unicodedata +raw = sys.argv[1] +# Strip emoji and other symbol/other categories. +no_emoji = "".join(ch for ch in raw if unicodedata.category(ch)[0] not in ("S",)) +# Lowercase + ascii fold. +ascii_text = unicodedata.normalize("NFKD", no_emoji).encode("ascii", "ignore").decode() +ascii_text = ascii_text.lower() +# Replace non-alphanumeric with space. +collapsed = re.sub(r"[^a-z0-9]+", " ", ascii_text).strip() +# Drop stopwords. +stopwords = {"a","an","the","of","for","to","and","or","with","in","on","by","via","as","is","are","be","support","add","new","feature","idea"} +tokens = [t for t in collapsed.split() if t and t not in stopwords] +print(" ".join(tokens)) +' "$1" +} +export -f normalize_title + +# Compute Jaccard similarity between two normalized token strings. +jaccard_similarity() { + python3 -c ' +import sys +a = set(sys.argv[1].split()) +b = set(sys.argv[2].split()) +if not a and not b: + print("1.0") +elif not a or not b: + print("0.0") +else: + inter = len(a & b) + union = len(a | b) + print(f"{inter / union:.4f}") +' "$1" "$2" +} +export -f jaccard_similarity match_discussions_main() { local signals_path="$1" local proposals_path="$2" local threshold="${MATCH_THRESHOLD:-0.6}" - # Validate threshold is a parseable float in [0, 1] BEFORE handing it to - # Python, so a typo doesn't surface as an opaque traceback. Caught by - # Copilot review on PR petry-projects/.github#85. - if ! printf '%s' "$threshold" | grep -Eq '^[0-9]+(\.[0-9]+)?$'; then - printf '[match-discussions] MATCH_THRESHOLD must be a non-negative number, got: %s\n' "$threshold" >&2 - return 64 - fi - # Validate range using awk (portable, no bash float math). - if ! awk -v t="$threshold" 'BEGIN { exit !(t >= 0 && t <= 1) }'; then - printf '[match-discussions] MATCH_THRESHOLD must be in [0, 1], got: %s\n' "$threshold" >&2 - return 64 - fi - if [ ! -f "$signals_path" ]; then printf '[match-discussions] signals not found: %s\n' "$signals_path" >&2 return 64 diff --git a/.github/scripts/feature-ideation/validate-signals.py b/.github/scripts/feature-ideation/validate-signals.py index eb82ff7c..b71a1bd6 100755 --- a/.github/scripts/feature-ideation/validate-signals.py +++ b/.github/scripts/feature-ideation/validate-signals.py @@ -13,13 +13,11 @@ from __future__ import annotations import json -import re import sys -from datetime import datetime from pathlib import Path try: - from jsonschema import Draft202012Validator, FormatChecker + from jsonschema import Draft202012Validator except ImportError: sys.stderr.write( "[validate-signals] python jsonschema not installed. " @@ -55,12 +53,8 @@ def main(argv: list[str]) -> int: try: signals = json.loads(signals_path.read_text()) except json.JSONDecodeError as exc: - # Per the docstring contract, exit 2 means usage / file error and - # exit 1 means schema validation error. A malformed signals file - # is a file/data error, not a schema violation. Caught by - # CodeRabbit review on PR petry-projects/.github#85. sys.stderr.write(f"[validate-signals] invalid JSON in {signals_path}: {exc}\n") - return 2 + return 1 try: schema = json.loads(schema_path.read_text()) @@ -68,30 +62,7 @@ def main(argv: list[str]) -> int: sys.stderr.write(f"[validate-signals] invalid schema JSON: {exc}\n") return 2 - # `format` keywords (e.g. "format": "date-time") are not enforced by - # Draft202012Validator unless an explicit FormatChecker is supplied. - # The default FormatChecker also does NOT include `date-time` — that - # requires the optional `rfc3339-validator` dependency. Avoid pulling - # in extra deps by registering a small inline validator that accepts - # ISO-8601 / RFC-3339 strings via datetime.fromisoformat (Python 3.11+ - # accepts the trailing `Z`; for older interpreters we substitute it). - # Caught by Copilot review on PR petry-projects/.github#85. - format_checker = FormatChecker() - - @format_checker.checks("date-time", raises=(ValueError, TypeError)) - def _check_date_time(instance): # noqa: ANN001 — jsonschema callback signature - if not isinstance(instance, str): - return True # non-strings handled by `type` keyword, not format - # Must look like a date-time, not just any string. Require at least - # YYYY-MM-DDTHH:MM[:SS] - if not re.match(r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}", instance): - raise ValueError(f"not an ISO-8601 date-time: {instance!r}") - # datetime.fromisoformat in Python <3.11 doesn't accept "Z"; swap it. - candidate = instance.replace("Z", "+00:00") if instance.endswith("Z") else instance - datetime.fromisoformat(candidate) - return True - - validator = Draft202012Validator(schema, format_checker=format_checker) + validator = Draft202012Validator(schema) errors = sorted(validator.iter_errors(signals), key=lambda e: list(e.absolute_path)) if not errors: print(f"[validate-signals] OK: {signals_path}") diff --git a/.github/workflows/feature-ideation-tests.yml b/.github/workflows/feature-ideation-tests.yml index 589c9349..91c1a628 100644 --- a/.github/workflows/feature-ideation-tests.yml +++ b/.github/workflows/feature-ideation-tests.yml @@ -103,7 +103,7 @@ jobs: - name: Upload bats output on failure if: failure() - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: name: bats-output path: | diff --git a/test/workflows/feature-ideation/collect-signals.bats b/test/workflows/feature-ideation/collect-signals.bats index cf306ff2..9543b97b 100644 --- a/test/workflows/feature-ideation/collect-signals.bats +++ b/test/workflows/feature-ideation/collect-signals.bats @@ -21,17 +21,15 @@ teardown() { # Build a multi-call gh script for the standard happy path. # Order MUST match collect-signals.sh: -# 1. gh run list (feed checkpoint — last successful run) -# 2. gh issue list --state open -# 3. gh issue list --state closed -# 4. gh api graphql (categories) -# 5. gh api graphql (discussions) -# 6. gh release list -# 7. gh pr list --state merged +# 1. gh issue list --state open +# 2. gh issue list --state closed +# 3. gh api graphql (categories) +# 4. gh api graphql (discussions) +# 5. gh release list +# 6. gh pr list --state merged build_happy_script() { local script="${TT_TMP}/gh-script.tsv" : >"$script" - printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/run-list-last-success.txt" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/issue-list-open.json" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/issue-list-closed.json" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/graphql-categories.json" >>"$script" @@ -117,9 +115,7 @@ build_happy_script() { script="${TT_TMP}/gh-script.tsv" err_file="${TT_TMP}/auth-err.txt" printf 'HTTP 401: Bad credentials\n' >"$err_file" - # run list (feed checkpoint — silenced with || true, so failure falls back) - printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/run-list-last-success.txt" >"$script" - printf '4\t-\t%s\n' "$err_file" >>"$script" + printf '4\t-\t%s\n' "$err_file" >"$script" export GH_STUB_SCRIPT="$script" rm -f "${TT_TMP}/.gh-stub-counter" @@ -131,7 +127,6 @@ build_happy_script() { @test "collect-signals: FAILS LOUD on GraphQL errors envelope (categories)" { script="${TT_TMP}/gh-script.tsv" : >"$script" - printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/run-list-last-success.txt" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/issue-list-open.json" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/issue-list-closed.json" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/graphql-errors-envelope.json" >>"$script" @@ -168,47 +163,9 @@ build_happy_script() { # Truncation warnings # --------------------------------------------------------------------------- -@test "collect-signals: open-issue truncation is detected BEFORE bot filtering" { - # Caught by Copilot review on PR petry-projects/.github#85: previously the - # truncation check ran AFTER filter_bots_apply, so a result set composed - # entirely of bots could drop below ISSUE_LIMIT and mask real truncation. - # Build a fixture with ISSUE_LIMIT=3 raw items, all bot-authored. Bot - # filter strips them to 0, but the truncation warning must still fire - # because the raw count hit the limit. - bot_file="${TT_TMP}/bot-only.json" - cat >"$bot_file" <<'JSON' -[ - {"number":1,"title":"a","labels":[],"createdAt":"2026-03-01T00:00:00Z","author":{"login":"dependabot[bot]"}}, - {"number":2,"title":"b","labels":[],"createdAt":"2026-03-02T00:00:00Z","author":{"login":"renovate[bot]"}}, - {"number":3,"title":"c","labels":[],"createdAt":"2026-03-03T00:00:00Z","author":{"login":"copilot[bot]"}} -] -JSON - empty_file="${TT_TMP}/empty.json" - echo '[]' >"$empty_file" - - script="${TT_TMP}/gh-script.tsv" - : >"$script" - printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/run-list-last-success.txt" >>"$script" # feed checkpoint - printf '0\t%s\t-\n' "$bot_file" >>"$script" # open issues — all bots - printf '0\t%s\t-\n' "$empty_file" >>"$script" # closed issues - printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/graphql-no-ideas-category.json" >>"$script" - printf '0\t%s\t-\n' "$empty_file" >>"$script" # releases - printf '0\t%s\t-\n' "$empty_file" >>"$script" # merged PRs - export GH_STUB_SCRIPT="$script" - export ISSUE_LIMIT=3 - rm -f "${TT_TMP}/.gh-stub-counter" - - run bash "${TT_SCRIPTS_DIR}/collect-signals.sh" - [ "$status" -eq 0 ] - open_count=$(jq '.open_issues.count' "$SIGNALS_OUTPUT") - [ "$open_count" = "0" ] - jq -e '.truncation_warnings[] | select(.source == "open_issues")' "$SIGNALS_OUTPUT" >/dev/null -} - @test "collect-signals: emits truncation warning when discussions hasNextPage=true" { script="${TT_TMP}/gh-script.tsv" : >"$script" - printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/run-list-last-success.txt" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/issue-list-open.json" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/issue-list-closed.json" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/graphql-categories.json" >>"$script" @@ -232,7 +189,6 @@ JSON @test "collect-signals: skips discussions when Ideas category absent" { script="${TT_TMP}/gh-script.tsv" : >"$script" - printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/run-list-last-success.txt" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/issue-list-open.json" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/issue-list-closed.json" >>"$script" printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/graphql-no-ideas-category.json" >>"$script" diff --git a/test/workflows/feature-ideation/compose-signals.bats b/test/workflows/feature-ideation/compose-signals.bats index 35359e90..4dced2c8 100644 --- a/test/workflows/feature-ideation/compose-signals.bats +++ b/test/workflows/feature-ideation/compose-signals.bats @@ -18,7 +18,6 @@ compose_empty() { '[]' '[]' '[]' '[]' '[]' '[]' '[]' \ 'foo/bar' \ '2026-04-07T00:00:00Z' \ - '2026-03-07T00:00:00Z' \ '1.0.0' \ '[]' } @@ -35,14 +34,14 @@ compose_empty() { @test "compose: rejects empty string for any JSON arg" { run compose_signals \ '' '[]' '[]' '[]' '[]' '[]' '[]' \ - 'foo/bar' '2026-04-07T00:00:00Z' '2026-03-07T00:00:00Z' '1.0.0' '[]' + 'foo/bar' '2026-04-07T00:00:00Z' '1.0.0' '[]' [ "$status" -ne 0 ] } @test "compose: rejects non-JSON for any JSON arg" { run compose_signals \ 'not json' '[]' '[]' '[]' '[]' '[]' '[]' \ - 'foo/bar' '2026-04-07T00:00:00Z' '2026-03-07T00:00:00Z' '1.0.0' '[]' + 'foo/bar' '2026-04-07T00:00:00Z' '1.0.0' '[]' [ "$status" -ne 0 ] } @@ -53,9 +52,9 @@ compose_empty() { @test "compose: produces all required top-level fields with empty inputs" { run compose_empty [ "$status" -eq 0 ] - for field in schema_version scan_date last_successful_run repo open_issues \ - closed_issues_30d ideas_discussions releases merged_prs_30d \ - feature_requests bug_reports truncation_warnings; do + for field in schema_version scan_date repo open_issues closed_issues_30d \ + ideas_discussions releases merged_prs_30d feature_requests \ + bug_reports truncation_warnings; do printf '%s' "$output" | jq -e "has(\"$field\")" >/dev/null done } @@ -64,7 +63,7 @@ compose_empty() { open='[{"number":1,"title":"a","labels":[]},{"number":2,"title":"b","labels":[]}]' run compose_signals \ "$open" '[]' '[]' '[]' '[]' '[]' '[]' \ - 'foo/bar' '2026-04-07T00:00:00Z' '2026-03-07T00:00:00Z' '1.0.0' '[]' + 'foo/bar' '2026-04-07T00:00:00Z' '1.0.0' '[]' [ "$status" -eq 0 ] count=$(printf '%s' "$output" | jq '.open_issues.count') items_len=$(printf '%s' "$output" | jq '.open_issues.items | length') @@ -75,7 +74,7 @@ compose_empty() { @test "compose: schema_version is preserved verbatim" { run compose_signals \ '[]' '[]' '[]' '[]' '[]' '[]' '[]' \ - 'foo/bar' '2026-04-07T00:00:00Z' '2026-03-07T00:00:00Z' '2.5.1' '[]' + 'foo/bar' '2026-04-07T00:00:00Z' '2.5.1' '[]' [ "$status" -eq 0 ] v=$(printf '%s' "$output" | jq -r '.schema_version') [ "$v" = "2.5.1" ] @@ -85,7 +84,7 @@ compose_empty() { warnings='[{"source":"open_issues","limit":50,"message":"truncated"}]' run compose_signals \ '[]' '[]' '[]' '[]' '[]' '[]' '[]' \ - 'foo/bar' '2026-04-07T00:00:00Z' '2026-03-07T00:00:00Z' '1.0.0' "$warnings" + 'foo/bar' '2026-04-07T00:00:00Z' '1.0.0' "$warnings" [ "$status" -eq 0 ] src=$(printf '%s' "$output" | jq -r '.truncation_warnings[0].source') [ "$src" = "open_issues" ] @@ -94,7 +93,7 @@ compose_empty() { @test "compose: scan_date and repo round-trip exactly" { run compose_signals \ '[]' '[]' '[]' '[]' '[]' '[]' '[]' \ - 'octocat/hello-world' '2030-01-15T12:34:56Z' '2029-12-15T12:34:56Z' '1.0.0' '[]' + 'octocat/hello-world' '2030-01-15T12:34:56Z' '1.0.0' '[]' [ "$status" -eq 0 ] d=$(printf '%s' "$output" | jq -r '.scan_date') r=$(printf '%s' "$output" | jq -r '.repo') diff --git a/test/workflows/feature-ideation/discussion-mutations.bats b/test/workflows/feature-ideation/discussion-mutations.bats index 671e9b3a..b42dabe6 100644 --- a/test/workflows/feature-ideation/discussion-mutations.bats +++ b/test/workflows/feature-ideation/discussion-mutations.bats @@ -100,35 +100,3 @@ teardown() { run comment_on_discussion "D_1" "body" [ "$status" -ne 0 ] } - -# --------------------------------------------------------------------------- -# Regression test for Copilot review on PR petry-projects/.github#85: -# add_label_to_discussion previously sent labelIds via -f as the literal -# string `["L_1"]` which the GraphQL API rejects (labelIds is [ID!]!). -# The fix routes through gh_safe_graphql_input with a properly built JSON -# variables block. -# --------------------------------------------------------------------------- - -@test "live: add_label_to_discussion variables block has labelIds as JSON array" { - # Replace the gh stub with one that captures stdin to a file so we can - # introspect the actual request body that gh would have received. - body_file="${TT_TMP}/captured-body.json" - stub_dir="${TT_TMP}/bin" - mkdir -p "$stub_dir" - cat >"$stub_dir/gh" <"$body_file" -echo '{"data":{"addLabelsToLabelable":{"clientMutationId":null}}}' -EOF - chmod +x "$stub_dir/gh" - PATH="${stub_dir}:${PATH}" add_label_to_discussion "D_1" "L_enhancement" - - [ -f "$body_file" ] - # Body must parse as JSON and have variables.labelIds as a length-1 array - # whose sole element is the literal label id. - jq -e '.variables.labelIds | type == "array"' "$body_file" >/dev/null - jq -e '.variables.labelIds | length == 1' "$body_file" >/dev/null - jq -e '.variables.labelIds[0] == "L_enhancement"' "$body_file" >/dev/null - jq -e '.variables.labelableId == "D_1"' "$body_file" >/dev/null - jq -e '.query | contains("addLabelsToLabelable")' "$body_file" >/dev/null -} diff --git a/test/workflows/feature-ideation/filter-bots.bats b/test/workflows/feature-ideation/filter-bots.bats index 78376b72..6f4e3ff3 100644 --- a/test/workflows/feature-ideation/filter-bots.bats +++ b/test/workflows/feature-ideation/filter-bots.bats @@ -45,11 +45,8 @@ JSON } @test "filter-bots: env extension adds custom bot logins" { - # NOTE: must use `bash -c`, not `sh -c`. On Ubuntu, /bin/sh is dash, which - # doesn't support `set -o pipefail` and would choke when sourcing a script - # that uses bash-isms. macOS /bin/sh is bash so it works there but CI fails. input='[{"number":1,"title":"x","author":{"login":"my-custom-bot"}},{"number":2,"title":"y","author":{"login":"alice"}}]' - result=$(FEATURE_IDEATION_BOT_AUTHORS="my-custom-bot" bash -c " + result=$(FEATURE_IDEATION_BOT_AUTHORS="my-custom-bot" sh -c " . '${TT_SCRIPTS_DIR}/lib/filter-bots.sh' printf '%s' '$input' | filter_bots_apply ") diff --git a/test/workflows/feature-ideation/fixtures/expected/empty-repo.signals.json b/test/workflows/feature-ideation/fixtures/expected/empty-repo.signals.json index e6438b55..4101afb6 100644 --- a/test/workflows/feature-ideation/fixtures/expected/empty-repo.signals.json +++ b/test/workflows/feature-ideation/fixtures/expected/empty-repo.signals.json @@ -1,7 +1,6 @@ { - "schema_version": "1.1.0", + "schema_version": "1.0.0", "scan_date": "2026-04-07T07:00:00Z", - "last_successful_run": "2026-03-31T07:00:00Z", "repo": "petry-projects/talkterm", "open_issues": { "count": 0, "items": [] }, "closed_issues_30d": { "count": 0, "items": [] }, diff --git a/test/workflows/feature-ideation/fixtures/expected/populated.signals.json b/test/workflows/feature-ideation/fixtures/expected/populated.signals.json index 5618e36d..4c929219 100644 --- a/test/workflows/feature-ideation/fixtures/expected/populated.signals.json +++ b/test/workflows/feature-ideation/fixtures/expected/populated.signals.json @@ -1,7 +1,6 @@ { - "schema_version": "1.1.0", + "schema_version": "1.0.0", "scan_date": "2026-04-07T07:00:00Z", - "last_successful_run": "2026-03-31T07:00:00Z", "repo": "petry-projects/talkterm", "open_issues": { "count": 2, diff --git a/test/workflows/feature-ideation/fixtures/expected/truncated.signals.json b/test/workflows/feature-ideation/fixtures/expected/truncated.signals.json index 2c884ea6..845db66f 100644 --- a/test/workflows/feature-ideation/fixtures/expected/truncated.signals.json +++ b/test/workflows/feature-ideation/fixtures/expected/truncated.signals.json @@ -1,7 +1,6 @@ { - "schema_version": "1.1.0", + "schema_version": "1.0.0", "scan_date": "2026-04-07T07:00:00Z", - "last_successful_run": "2026-03-31T07:00:00Z", "repo": "petry-projects/talkterm", "open_issues": { "count": 0, "items": [] }, "closed_issues_30d": { "count": 0, "items": [] }, diff --git a/test/workflows/feature-ideation/gh-safe.bats b/test/workflows/feature-ideation/gh-safe.bats index b06bb5c7..9194d538 100644 --- a/test/workflows/feature-ideation/gh-safe.bats +++ b/test/workflows/feature-ideation/gh-safe.bats @@ -4,8 +4,6 @@ # These tests kill R1: the original "2>/dev/null || echo '[]'" pattern that # silently swallowed every kind of failure. Each test pins one failure mode. -bats_require_minimum_version 1.5.0 - load 'helpers/setup' setup() { @@ -69,16 +67,12 @@ teardown() { # gh_safe_rest — failure modes (R1 kill list) # --------------------------------------------------------------------------- -@test "rest: EXITS NON-ZERO on auth failure (gh exit 4) and reports the failure category" { - # CodeRabbit on PR petry-projects/.github#85: the previous assertion - # ended with `|| true`, so it always passed regardless of the message - # content. Use --separate-stderr to capture stderr and assert the - # structured `[gh-safe][rest-failure]` prefix is emitted. +@test "rest: EXITS NON-ZERO on auth failure (gh exit 4)" { GH_STUB_EXIT=4 \ GH_STUB_STDERR='HTTP 401: Bad credentials' \ - run --separate-stderr gh_safe_rest issue list --repo foo/bar + run gh_safe_rest issue list --repo foo/bar [ "$status" -ne 0 ] - [[ "$stderr" == *"rest-failure"* ]] + [[ "$output" == *"rest-failure"* ]] || [[ "$stderr" == *"rest-failure"* ]] || true } @test "rest: EXITS NON-ZERO on rate limit (gh exit 1 + 403)" { @@ -167,58 +161,3 @@ teardown() { run gh_safe_graphql -f query='q' [ "$status" -ne 0 ] } - -# --------------------------------------------------------------------------- -# Regression tests for Copilot review feedback on PR petry-projects/.github#85 -# --------------------------------------------------------------------------- - -@test "graphql: invalid --jq filter EXITS NON-ZERO instead of returning []" { - # Catches the typo class: previously the wrapper used `jq ... || true` - # which silently swallowed filter syntax errors and returned an empty - # array, masking R1-class bugs. - GH_STUB_STDOUT='{"data":{"repository":{"id":"R_1"}}}' \ - run gh_safe_graphql -f query='q' --jq '.data.repository.does_not_exist | bogus_function' - [ "$status" -ne 0 ] -} - -@test "graphql: --jq returning explicit JSON null still normalizes to []" { - GH_STUB_STDOUT='{"data":{"repository":{"nodes":null}}}' \ - run gh_safe_graphql -f query='q' --jq '.data.repository.nodes' - [ "$status" -eq 0 ] - [ "$output" = '[]' ] -} - -# --------------------------------------------------------------------------- -# gh_safe_graphql_input — for mutations with array variables -# --------------------------------------------------------------------------- - -@test "graphql_input: rejects non-JSON body" { - run gh_safe_graphql_input 'not json' - [ "$status" -eq 64 ] -} - -@test "graphql_input: forwards body to gh via stdin and returns response" { - GH_STUB_STDOUT='{"data":{"addLabelsToLabelable":{"clientMutationId":null}}}' \ - run gh_safe_graphql_input '{"query":"mutation{...}","variables":{"labelIds":["L_1"]}}' - [ "$status" -eq 0 ] - [[ "$output" == *'addLabelsToLabelable'* ]] -} - -@test "graphql_input: EXITS NON-ZERO on errors envelope" { - GH_STUB_STDOUT='{"errors":[{"message":"forbidden"}],"data":null}' \ - run gh_safe_graphql_input '{"query":"q","variables":{}}' - [ "$status" -ne 0 ] -} - -@test "graphql_input: EXITS NON-ZERO on data:null" { - GH_STUB_STDOUT='{"data":null}' \ - run gh_safe_graphql_input '{"query":"q","variables":{}}' - [ "$status" -ne 0 ] -} - -@test "graphql_input: EXITS NON-ZERO on gh hard failure" { - GH_STUB_EXIT=1 \ - GH_STUB_STDERR='HTTP 401' \ - run gh_safe_graphql_input '{"query":"q","variables":{}}' - [ "$status" -ne 0 ] -} diff --git a/test/workflows/feature-ideation/lint-prompt.bats b/test/workflows/feature-ideation/lint-prompt.bats index a0cd32ea..e195bb3c 100644 --- a/test/workflows/feature-ideation/lint-prompt.bats +++ b/test/workflows/feature-ideation/lint-prompt.bats @@ -120,41 +120,3 @@ YML run bash "$LINTER" [ "$status" -eq 0 ] } - -# --------------------------------------------------------------------------- -# Coverage of claude-code-action v1 `prompt:` form (in addition to v0 `direct_prompt:`) -# Caught by Copilot review on PR #85 — the original linter only scanned -# `direct_prompt:` and would silently miss R2 regressions in the actual -# reusable workflow which uses the v1 `prompt:` form. -# --------------------------------------------------------------------------- - -@test "lint-prompt: scans v1 prompt: blocks (not just direct_prompt:)" { - write_yml "${TT_TMP}/v1-prompt.yml" <<'YML' -jobs: - analyze: - steps: - - uses: anthropics/claude-code-action@v1 - with: - prompt: | - You are Mary. - Date: $(date -u +%Y-%m-%d) -YML - run bash "$LINTER" "${TT_TMP}/v1-prompt.yml" - [ "$status" -eq 1 ] -} - -@test "lint-prompt: clean v1 prompt: passes" { - write_yml "${TT_TMP}/v1-clean.yml" <<'YML' -jobs: - analyze: - steps: - - uses: anthropics/claude-code-action@v1 - with: - prompt: | - You are Mary. - Repo: ${{ github.repository }} - Read RUN_DATE from the environment at runtime via printenv. -YML - run bash "$LINTER" "${TT_TMP}/v1-clean.yml" - [ "$status" -eq 0 ] -} diff --git a/test/workflows/feature-ideation/match-discussions.bats b/test/workflows/feature-ideation/match-discussions.bats index 9b63ea9a..792fdd5c 100644 --- a/test/workflows/feature-ideation/match-discussions.bats +++ b/test/workflows/feature-ideation/match-discussions.bats @@ -18,14 +18,8 @@ teardown() { } # Helper: build a signals.json with the given discussions array. -# Computes the discussions count from the array length so the fixture -# always satisfies the producer/consumer contract (even though the -# matcher only reads .items, validating contracts in test fixtures -# is good hygiene). CodeRabbit on PR petry-projects/.github#85. build_signals() { local discussions="$1" - local count - count=$(printf '%s' "$discussions" | jq 'length') cat >"${TT_TMP}/signals.json" <"$bad_file" - run python3 "$VALIDATOR" "$bad_file" "$SCHEMA" - [ "$status" -eq 1 ] -} - -@test "schema: well-formed date-time scan_date passes" { - good_file="${BATS_TEST_TMPDIR}/good-scan-date.json" - jq '.scan_date = "2026-04-08T12:34:56Z"' "${FIX}/empty-repo.signals.json" >"$good_file" - run python3 "$VALIDATOR" "$good_file" "$SCHEMA" - [ "$status" -eq 0 ] -} - -@test "schema: SCHEMA_VERSION constant matches schema file version comment" { - # CodeRabbit on PR petry-projects/.github#85: enforce that the - # SCHEMA_VERSION constant in collect-signals.sh stays in lockstep with - # the version annotation in signals.schema.json. Bumping one without - # the other is a compatibility break. - script_version=$(grep -E '^SCHEMA_VERSION=' "${TT_REPO_ROOT}/.github/scripts/feature-ideation/collect-signals.sh" \ - | head -1 | sed -E 's/^SCHEMA_VERSION="([^"]+)".*/\1/') - schema_version=$(jq -r '."$comment"' "$SCHEMA" \ - | grep -oE 'version: [0-9]+\.[0-9]+\.[0-9]+' | sed 's/version: //') - [ -n "$script_version" ] - [ -n "$schema_version" ] - [ "$script_version" = "$schema_version" ] -} diff --git a/test/workflows/feature-ideation/stubs/gh b/test/workflows/feature-ideation/stubs/gh index 2e8cd24d..158b6655 100755 --- a/test/workflows/feature-ideation/stubs/gh +++ b/test/workflows/feature-ideation/stubs/gh @@ -9,10 +9,7 @@ # GH_STUB_STDOUT — literal text or @file:/abs/path to load from disk # GH_STUB_STDERR — literal text or @file:/abs/path # GH_STUB_EXIT — exit code (default 0) -# GH_STUB_LOG — append-only log of argv invocations. Each invocation -# is logged as a single line of shell-quoted tokens -# (via `printf '%q '`) so it can be re-parsed losslessly -# even when arguments contain spaces or special chars. +# GH_STUB_LOG — append-only log of argv invocations (one TSV line each) # # For multi-call tests (where the same stub is invoked multiple times with # different responses), set GH_STUB_SCRIPT to a file containing one @@ -22,12 +19,7 @@ set -euo pipefail if [ -n "${GH_STUB_LOG:-}" ]; then - # Use %q so each arg is shell-quoted; tests can re-parse argv losslessly. - # Caught by Copilot review on PR petry-projects/.github#85: previously - # used $* which joined with spaces and lost argument boundaries. - # shellcheck disable=SC2059 - printf '%q ' "$@" >>"$GH_STUB_LOG" - printf '\n' >>"$GH_STUB_LOG" + printf '%s\n' "$*" >>"$GH_STUB_LOG" fi # Multi-call mode. From 0c381ef90f320c1a242c5197ba10854953d37855 Mon Sep 17 00:00:00 2001 From: DJ Date: Tue, 7 Apr 2026 10:50:58 -0700 Subject: [PATCH 2/6] test(feature-ideation): use bash -c instead of sh -c in env-extension test CI failure on the previous commit: 91/92 passing, 1 failing. The filter-bots env-extension test used `sh -c` to source filter-bots.sh in a sub-shell with FEATURE_IDEATION_BOT_AUTHORS set. On macOS this works because /bin/sh is bash. On Ubuntu (CI), /bin/sh is dash, which does not support `set -o pipefail`, so sourcing filter-bots.sh produced: sh: 12: set: Illegal option -o pipefail Fixed by switching to `bash -c`. All scripts already use `#!/usr/bin/env bash` shebangs; this is the only place a sub-shell was spawned via `sh`. Co-Authored-By: Claude Opus 4.6 (1M context) --- test/workflows/feature-ideation/filter-bots.bats | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/workflows/feature-ideation/filter-bots.bats b/test/workflows/feature-ideation/filter-bots.bats index 6f4e3ff3..78376b72 100644 --- a/test/workflows/feature-ideation/filter-bots.bats +++ b/test/workflows/feature-ideation/filter-bots.bats @@ -45,8 +45,11 @@ JSON } @test "filter-bots: env extension adds custom bot logins" { + # NOTE: must use `bash -c`, not `sh -c`. On Ubuntu, /bin/sh is dash, which + # doesn't support `set -o pipefail` and would choke when sourcing a script + # that uses bash-isms. macOS /bin/sh is bash so it works there but CI fails. input='[{"number":1,"title":"x","author":{"login":"my-custom-bot"}},{"number":2,"title":"y","author":{"login":"alice"}}]' - result=$(FEATURE_IDEATION_BOT_AUTHORS="my-custom-bot" sh -c " + result=$(FEATURE_IDEATION_BOT_AUTHORS="my-custom-bot" bash -c " . '${TT_SCRIPTS_DIR}/lib/filter-bots.sh' printf '%s' '$input' | filter_bots_apply ") From c8fb735deadac3e11bf609a517bdd6028aaa4217 Mon Sep 17 00:00:00 2001 From: DJ Date: Wed, 8 Apr 2026 05:17:56 -0700 Subject: [PATCH 3/6] fix(feature-ideation): address Copilot review on PR #85 (11 fixes + 16 tests) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triaged 14 inline comments from Copilot's review of #85; two were already fixed by the tooling_ref→v1 commit, the remaining 11 are addressed here. Critical bug fixes ------------------ 1. lint-prompt.sh now scans claude-code-action v1 `prompt:` blocks in addition to v0 `direct_prompt:`. The reusable workflow uses `prompt:` so the linter was silently allowing R2 regressions on the very file it was supposed to protect. Added two regression tests covering both the v1 form and a clean v1 form passes. 2. add_label_to_discussion now sends labelIds as a proper JSON array via gh_safe_graphql_input (new helper). Previously used `gh -f labelIds=` which sent the literal string `["L_1"]` and the GraphQL API would have rejected the mutation at runtime. Added a test that captures gh's stdin and asserts the variables block contains a length-1 array. 3. validate-signals.py now registers a `date-time` format checker via FormatChecker so the `format: date-time` keyword in signals.schema.json is actually enforced. Draft202012Validator does NOT enforce formats by default, and the default FormatChecker omits date-time entirely. Used an inline checker (datetime.fromisoformat with Z normalisation) to avoid pulling in rfc3339-validator. Added two regression tests: one for an invalid timestamp failing, one for a clean timestamp passing. 4. gh_safe_graphql --jq path no longer swallows jq filter errors with `|| true`. Filter typos / wrong paths now exit non-zero instead of silently returning []. Added a regression test using a deliberately broken filter. 5. collect-signals.sh now computes the open-issue truncation warning BEFORE filter_bots_apply. Previously, a result set composed entirely of bots could drop below ISSUE_LIMIT after filtering and mask real truncation. Added an integration test with all-bot fixtures. 6. match-discussions.sh now validates MATCH_THRESHOLD as a non-negative number in [0, 1] before passing to Python. A typo previously surfaced as an opaque traceback. Added regression tests for non-numeric input, out-of-range input, and boundary values 0 and 1. Cleanup ------- 7. Removed dead bash `normalize_title` / `jaccard_similarity` functions from match-discussions.sh — the actual matching is implemented in the embedded Python block and the bash helpers were never called. 8. Schema $id corrected from petry-projects/TalkTerm/... to the canonical petry-projects/.github location. 9. signals-schema.bats "validator script exists and is executable" test now actually checks the `-x` bit (was only checking `-f` and `-r`). 10. README + filter-bots.sh comments now describe the bot list as a "blocklist" (it removes matching authors) instead of "allowlist". 11. test/workflows/feature-ideation/stubs/gh now logs argv with `printf '%q '` so each invocation is shell-quoted and re-parseable, matching its documentation. Previously logged `$*` which lost arg boundaries. New helper ---------- gh_safe_graphql_input — same defensive contract as gh_safe_graphql, but takes a fully-formed JSON request body via stdin instead of -f/-F flags. Use for mutations whose variables include arrays (e.g. labelIds: [ID!]!) that gh's flag-based interface cannot express. Five new tests cover its happy path and every documented failure mode. Tests ----- Test count: 92 → 108 (16 new regression tests, all green). Run with: bats test/workflows/feature-ideation/ Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/schemas/signals.schema.json | 2 +- .github/scripts/feature-ideation/README.md | 2 +- .../feature-ideation/collect-signals.sh | 13 ++-- .../feature-ideation/discussion-mutations.sh | 13 +++- .../feature-ideation/lib/filter-bots.sh | 4 +- .../scripts/feature-ideation/lib/gh-safe.sh | 72 +++++++++++++++++-- .../scripts/feature-ideation/lint-prompt.sh | 12 ++-- .../feature-ideation/match-discussions.sh | 53 +++++--------- .../feature-ideation/validate-signals.py | 29 +++++++- .../feature-ideation/collect-signals.bats | 36 ++++++++++ .../discussion-mutations.bats | 32 +++++++++ test/workflows/feature-ideation/gh-safe.bats | 55 ++++++++++++++ .../feature-ideation/lint-prompt.bats | 38 ++++++++++ .../feature-ideation/match-discussions.bats | 25 +++++++ .../feature-ideation/signals-schema.bats | 18 +++++ test/workflows/feature-ideation/stubs/gh | 12 +++- 16 files changed, 357 insertions(+), 59 deletions(-) diff --git a/.github/schemas/signals.schema.json b/.github/schemas/signals.schema.json index 51d5f655..9a1ad075 100644 --- a/.github/schemas/signals.schema.json +++ b/.github/schemas/signals.schema.json @@ -1,6 +1,6 @@ { "$schema": "https://json-schema.org/draft/2020-12/schema", - "$id": "https://github.com/petry-projects/TalkTerm/.github/schemas/signals.schema.json", + "$id": "https://github.com/petry-projects/.github/blob/main/.github/schemas/signals.schema.json", "title": "Feature Ideation Signals", "description": "Canonical contract between collect-signals.sh and the BMAD Analyst (Mary) prompt. Any change to this schema is a breaking change to the workflow.", "type": "object", diff --git a/.github/scripts/feature-ideation/README.md b/.github/scripts/feature-ideation/README.md index 9ef03b97..957a06aa 100644 --- a/.github/scripts/feature-ideation/README.md +++ b/.github/scripts/feature-ideation/README.md @@ -23,7 +23,7 @@ caught **before UAT** instead of after. |------|---------|------------| | `lib/gh-safe.sh` | Wraps every `gh` and `gh api graphql` call. Fails loud on auth, rate-limit, network, GraphQL errors envelope, or `data: null`. Replaces the original `2>/dev/null \|\| echo '[]'` swallow. | R1, R7, R8 | | `lib/compose-signals.sh` | Validates JSON inputs before `jq --argjson` and assembles the canonical signals.json document. | R3, R4 | -| `lib/filter-bots.sh` | Configurable bot allowlist via `FEATURE_IDEATION_BOT_AUTHORS`. | R10 | +| `lib/filter-bots.sh` | Configurable bot blocklist via `FEATURE_IDEATION_BOT_AUTHORS` (extends the default list of bot logins to remove from results). | R10 | | `lib/date-utils.sh` | Cross-platform date arithmetic helpers. | R9 | | `collect-signals.sh` | Orchestrator: drives all `gh` calls, composes signals.json, emits truncation warnings. | R1, R3, R4, R11 | | `validate-signals.py` | JSON Schema 2020-12 validator for signals.json against `../schemas/signals.schema.json`. | R3 | diff --git a/.github/scripts/feature-ideation/collect-signals.sh b/.github/scripts/feature-ideation/collect-signals.sh index cf512290..69f605c4 100755 --- a/.github/scripts/feature-ideation/collect-signals.sh +++ b/.github/scripts/feature-ideation/collect-signals.sh @@ -72,15 +72,20 @@ main() { local open_issues_raw open_issues_raw=$(gh_safe_rest issue list --repo "$REPO" --state open --limit "$issue_limit" \ --json number,title,labels,createdAt,author) - local open_issues - open_issues=$(printf '%s' "$open_issues_raw" | filter_bots_apply) - - if [ "$(printf '%s' "$open_issues" | jq 'length')" -ge "$issue_limit" ]; then + # Compute truncation warning BEFORE bot filtering. If we filter out bots + # first, the count could drop below the limit and mask a real truncation. + # Caught by Copilot review on PR petry-projects/.github#85. + local raw_open_count + raw_open_count=$(printf '%s' "$open_issues_raw" | jq 'length') + if [ "$raw_open_count" -ge "$issue_limit" ]; then truncation_warnings=$(printf '%s' "$truncation_warnings" \ | jq --arg src "open_issues" --argjson lim "$issue_limit" \ '. + [{source: $src, limit: $lim, message: "result count equals limit; possible truncation"}]') fi + local open_issues + open_issues=$(printf '%s' "$open_issues_raw" | filter_bots_apply) + # --- Recently closed issues ------------------------------------------------ printf '[collect-signals] fetching closed issues (since %s)\n' "$thirty_days_ago" >&2 local closed_issues_raw diff --git a/.github/scripts/feature-ideation/discussion-mutations.sh b/.github/scripts/feature-ideation/discussion-mutations.sh index 843d9d6f..ae7a8dcb 100755 --- a/.github/scripts/feature-ideation/discussion-mutations.sh +++ b/.github/scripts/feature-ideation/discussion-mutations.sh @@ -153,7 +153,14 @@ mutation($labelableId: ID!, $labelIds: [ID!]!) { } GRAPHQL - gh_safe_graphql -f query="$query" \ - -f labelableId="$discussion_id" \ - -f labelIds="[\"$label_id\"]" + # GraphQL `labelIds: [ID!]!` requires a JSON array variable. `gh api`'s + # `-f`/`-F` flags don't express GraphQL array variables, so build the + # full request body as JSON and send it via stdin. Caught by Copilot + # review on PR petry-projects/.github#85: the previous version sent + # labelIds as the literal string `["L_1"]`, which the API rejects. + local body + body=$(jq -nc --arg q "$query" --arg id "$discussion_id" --arg label "$label_id" \ + '{query: $q, variables: {labelableId: $id, labelIds: [$label]}}') + + gh_safe_graphql_input "$body" } diff --git a/.github/scripts/feature-ideation/lib/filter-bots.sh b/.github/scripts/feature-ideation/lib/filter-bots.sh index 95aabfee..c3b38bea 100755 --- a/.github/scripts/feature-ideation/lib/filter-bots.sh +++ b/.github/scripts/feature-ideation/lib/filter-bots.sh @@ -7,7 +7,9 @@ # pollute the signals payload and crowd out real user feedback. # # Configurable via FEATURE_IDEATION_BOT_AUTHORS (comma-separated, optional). -# Defaults to a sensible allowlist of known automation accounts. +# Defaults to a sensible blocklist of known automation accounts. Items +# matching this list are REMOVED from the result. Adopters can add their +# own bot logins via the env var to extend the blocklist. set -euo pipefail diff --git a/.github/scripts/feature-ideation/lib/gh-safe.sh b/.github/scripts/feature-ideation/lib/gh-safe.sh index 7bbdf2c5..343a5bbc 100755 --- a/.github/scripts/feature-ideation/lib/gh-safe.sh +++ b/.github/scripts/feature-ideation/lib/gh-safe.sh @@ -150,11 +150,25 @@ gh_safe_graphql() { # If caller asked for a jq filter, apply it now and return that. if [ "$has_jq" -eq 1 ]; then - local filtered - filtered=$(printf '%s' "$raw" | jq -c "$jq_filter" 2>/dev/null || true) - if [ -z "$filtered" ] || [ "$filtered" = "null" ]; then - # The filter resolved to null/empty — caller probably wants "[]" semantics - # for "no nodes found". Return the empty array sentinel. + # NB: do NOT swallow jq errors with `|| true`. A typo or wrong path in + # the filter must surface as a hard failure, not a silent empty result — + # otherwise we re-introduce R1 in a different shape. Caught by Copilot + # review on PR petry-projects/.github#85. + local filtered jq_rc jq_err + jq_err=$(mktemp) + set +e + filtered=$(printf '%s' "$raw" | jq -c "$jq_filter" 2>"$jq_err") + jq_rc=$? + set -e + if [ "$jq_rc" -ne 0 ]; then + _gh_safe_err "graphql-jq-failed" "filter='$jq_filter' err=$(cat "$jq_err")" + rm -f "$jq_err" + return 65 + fi + rm -f "$jq_err" + if [ "$filtered" = "null" ] || [ -z "$filtered" ]; then + # Filter resolved to JSON null (e.g. nodes path returned null) — this + # is the documented "no results" case. Normalize to empty array. printf '%s' "$GH_SAFE_EMPTY_ARRAY" return 0 fi @@ -164,3 +178,51 @@ gh_safe_graphql() { printf '%s' "$raw" } + +# Send a fully-formed GraphQL request body via stdin. Use this when the +# variables include arrays or other shapes that `gh api`'s -f/-F flags +# cannot express (e.g. `labelIds: [ID!]!`). The body must be a JSON +# document with `query` and `variables` top-level fields. +# +# Same defensive contract as `gh_safe_graphql`: any auth/network/schema +# failure exits non-zero with a structured stderr message. +gh_safe_graphql_input() { + local body="$1" + if ! gh_safe_is_json "$body"; then + _gh_safe_err "graphql-bad-input" "request body is not valid JSON" + return 64 + fi + + local raw stderr rc tmp_err + tmp_err=$(mktemp) + set +e + raw=$(printf '%s' "$body" | gh api graphql --input - 2>"$tmp_err") + rc=$? + set -e + stderr=$(cat "$tmp_err") + rm -f "$tmp_err" + + if [ "$rc" -ne 0 ]; then + _gh_safe_err "graphql-failure" "exit=$rc stderr=$stderr" + return "$rc" + fi + + if ! gh_safe_is_json "$raw"; then + _gh_safe_err "graphql-bad-json" "first 200 bytes: ${raw:0:200}" + return 65 + fi + + if printf '%s' "$raw" | jq -e '(.errors // empty | type) == "array" and (.errors | length > 0)' >/dev/null 2>&1; then + local errs + errs=$(printf '%s' "$raw" | jq -c '.errors') + _gh_safe_err "graphql-errors" "$errs" + return 66 + fi + + if printf '%s' "$raw" | jq -e '.data == null' >/dev/null 2>&1; then + _gh_safe_err "graphql-null-data" "data field is null" + return 66 + fi + + printf '%s' "$raw" +} diff --git a/.github/scripts/feature-ideation/lint-prompt.sh b/.github/scripts/feature-ideation/lint-prompt.sh index 8381b612..3b1aebcc 100755 --- a/.github/scripts/feature-ideation/lint-prompt.sh +++ b/.github/scripts/feature-ideation/lint-prompt.sh @@ -39,8 +39,9 @@ except OSError as exc: sys.stderr.write(f"[lint-prompt] cannot read {path}: {exc}\n") sys.exit(2) -# Find direct_prompt: blocks. We treat everything indented MORE than the -# `direct_prompt:` line as part of that block, until we hit a less-indented +# Find prompt blocks. claude-code-action v0 used `direct_prompt:`, v1 uses +# plain `prompt:`. Both forms are scanned. We treat everything indented MORE +# than the marker line as part of the block, until we hit a less-indented # non-blank line. in_block = False block_indent = -1 @@ -50,13 +51,16 @@ findings = [] # because that's evaluated before the prompt is rendered. shell_expansion = re.compile(r'(?` block scalar indicators. +prompt_marker = re.compile(r'(?:direct_prompt|prompt):\s*[|>]?\s*$') + for lineno, raw in enumerate(lines, start=1): stripped = raw.lstrip(" ") indent = len(raw) - len(stripped) if not in_block: - # Look for `direct_prompt:` or `direct_prompt: |` or `direct_prompt: >` - if re.match(r'direct_prompt:\s*[|>]?\s*$', stripped): + if prompt_marker.match(stripped): in_block = True block_indent = indent continue diff --git a/.github/scripts/feature-ideation/match-discussions.sh b/.github/scripts/feature-ideation/match-discussions.sh index 826f29b3..d8bf7668 100755 --- a/.github/scripts/feature-ideation/match-discussions.sh +++ b/.github/scripts/feature-ideation/match-discussions.sh @@ -31,48 +31,29 @@ set -euo pipefail -normalize_title() { - python3 -c ' -import re, sys, unicodedata -raw = sys.argv[1] -# Strip emoji and other symbol/other categories. -no_emoji = "".join(ch for ch in raw if unicodedata.category(ch)[0] not in ("S",)) -# Lowercase + ascii fold. -ascii_text = unicodedata.normalize("NFKD", no_emoji).encode("ascii", "ignore").decode() -ascii_text = ascii_text.lower() -# Replace non-alphanumeric with space. -collapsed = re.sub(r"[^a-z0-9]+", " ", ascii_text).strip() -# Drop stopwords. -stopwords = {"a","an","the","of","for","to","and","or","with","in","on","by","via","as","is","are","be","support","add","new","feature","idea"} -tokens = [t for t in collapsed.split() if t and t not in stopwords] -print(" ".join(tokens)) -' "$1" -} -export -f normalize_title - -# Compute Jaccard similarity between two normalized token strings. -jaccard_similarity() { - python3 -c ' -import sys -a = set(sys.argv[1].split()) -b = set(sys.argv[2].split()) -if not a and not b: - print("1.0") -elif not a or not b: - print("0.0") -else: - inter = len(a & b) - union = len(a | b) - print(f"{inter / union:.4f}") -' "$1" "$2" -} -export -f jaccard_similarity +# NB: matching logic is implemented entirely in the embedded Python block +# below. Earlier drafts had standalone bash `normalize_title` / +# `jaccard_similarity` helpers that were never called; removed per Copilot +# review on PR petry-projects/.github#85. match_discussions_main() { local signals_path="$1" local proposals_path="$2" local threshold="${MATCH_THRESHOLD:-0.6}" + # Validate threshold is a parseable float in [0, 1] BEFORE handing it to + # Python, so a typo doesn't surface as an opaque traceback. Caught by + # Copilot review on PR petry-projects/.github#85. + if ! printf '%s' "$threshold" | grep -Eq '^[0-9]+(\.[0-9]+)?$'; then + printf '[match-discussions] MATCH_THRESHOLD must be a non-negative number, got: %s\n' "$threshold" >&2 + return 64 + fi + # Validate range using awk (portable, no bash float math). + if ! awk -v t="$threshold" 'BEGIN { exit !(t >= 0 && t <= 1) }'; then + printf '[match-discussions] MATCH_THRESHOLD must be in [0, 1], got: %s\n' "$threshold" >&2 + return 64 + fi + if [ ! -f "$signals_path" ]; then printf '[match-discussions] signals not found: %s\n' "$signals_path" >&2 return 64 diff --git a/.github/scripts/feature-ideation/validate-signals.py b/.github/scripts/feature-ideation/validate-signals.py index b71a1bd6..69ff0c1b 100755 --- a/.github/scripts/feature-ideation/validate-signals.py +++ b/.github/scripts/feature-ideation/validate-signals.py @@ -13,11 +13,13 @@ from __future__ import annotations import json +import re import sys +from datetime import datetime from pathlib import Path try: - from jsonschema import Draft202012Validator + from jsonschema import Draft202012Validator, FormatChecker except ImportError: sys.stderr.write( "[validate-signals] python jsonschema not installed. " @@ -62,7 +64,30 @@ def main(argv: list[str]) -> int: sys.stderr.write(f"[validate-signals] invalid schema JSON: {exc}\n") return 2 - validator = Draft202012Validator(schema) + # `format` keywords (e.g. "format": "date-time") are not enforced by + # Draft202012Validator unless an explicit FormatChecker is supplied. + # The default FormatChecker also does NOT include `date-time` — that + # requires the optional `rfc3339-validator` dependency. Avoid pulling + # in extra deps by registering a small inline validator that accepts + # ISO-8601 / RFC-3339 strings via datetime.fromisoformat (Python 3.11+ + # accepts the trailing `Z`; for older interpreters we substitute it). + # Caught by Copilot review on PR petry-projects/.github#85. + format_checker = FormatChecker() + + @format_checker.checks("date-time", raises=(ValueError, TypeError)) + def _check_date_time(instance): # noqa: ANN001 — jsonschema callback signature + if not isinstance(instance, str): + return True # non-strings handled by `type` keyword, not format + # Must look like a date-time, not just any string. Require at least + # YYYY-MM-DDTHH:MM[:SS] + if not re.match(r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}", instance): + raise ValueError(f"not an ISO-8601 date-time: {instance!r}") + # datetime.fromisoformat in Python <3.11 doesn't accept "Z"; swap it. + candidate = instance.replace("Z", "+00:00") if instance.endswith("Z") else instance + datetime.fromisoformat(candidate) + return True + + validator = Draft202012Validator(schema, format_checker=format_checker) errors = sorted(validator.iter_errors(signals), key=lambda e: list(e.absolute_path)) if not errors: print(f"[validate-signals] OK: {signals_path}") diff --git a/test/workflows/feature-ideation/collect-signals.bats b/test/workflows/feature-ideation/collect-signals.bats index 9543b97b..94f9a12b 100644 --- a/test/workflows/feature-ideation/collect-signals.bats +++ b/test/workflows/feature-ideation/collect-signals.bats @@ -163,6 +163,42 @@ build_happy_script() { # Truncation warnings # --------------------------------------------------------------------------- +@test "collect-signals: open-issue truncation is detected BEFORE bot filtering" { + # Caught by Copilot review on PR petry-projects/.github#85: previously the + # truncation check ran AFTER filter_bots_apply, so a result set composed + # entirely of bots could drop below ISSUE_LIMIT and mask real truncation. + # Build a fixture with ISSUE_LIMIT=3 raw items, all bot-authored. Bot + # filter strips them to 0, but the truncation warning must still fire + # because the raw count hit the limit. + bot_file="${TT_TMP}/bot-only.json" + cat >"$bot_file" <<'JSON' +[ + {"number":1,"title":"a","labels":[],"createdAt":"2026-03-01T00:00:00Z","author":{"login":"dependabot[bot]"}}, + {"number":2,"title":"b","labels":[],"createdAt":"2026-03-02T00:00:00Z","author":{"login":"renovate[bot]"}}, + {"number":3,"title":"c","labels":[],"createdAt":"2026-03-03T00:00:00Z","author":{"login":"copilot[bot]"}} +] +JSON + empty_file="${TT_TMP}/empty.json" + echo '[]' >"$empty_file" + + script="${TT_TMP}/gh-script.tsv" + : >"$script" + printf '0\t%s\t-\n' "$bot_file" >>"$script" # open issues — all bots + printf '0\t%s\t-\n' "$empty_file" >>"$script" # closed issues + printf '0\t%s\t-\n' "${TT_FIXTURES_DIR}/gh-responses/graphql-no-ideas-category.json" >>"$script" + printf '0\t%s\t-\n' "$empty_file" >>"$script" # releases + printf '0\t%s\t-\n' "$empty_file" >>"$script" # merged PRs + export GH_STUB_SCRIPT="$script" + export ISSUE_LIMIT=3 + rm -f "${TT_TMP}/.gh-stub-counter" + + run bash "${TT_SCRIPTS_DIR}/collect-signals.sh" + [ "$status" -eq 0 ] + open_count=$(jq '.open_issues.count' "$SIGNALS_OUTPUT") + [ "$open_count" = "0" ] + jq -e '.truncation_warnings[] | select(.source == "open_issues")' "$SIGNALS_OUTPUT" >/dev/null +} + @test "collect-signals: emits truncation warning when discussions hasNextPage=true" { script="${TT_TMP}/gh-script.tsv" : >"$script" diff --git a/test/workflows/feature-ideation/discussion-mutations.bats b/test/workflows/feature-ideation/discussion-mutations.bats index b42dabe6..671e9b3a 100644 --- a/test/workflows/feature-ideation/discussion-mutations.bats +++ b/test/workflows/feature-ideation/discussion-mutations.bats @@ -100,3 +100,35 @@ teardown() { run comment_on_discussion "D_1" "body" [ "$status" -ne 0 ] } + +# --------------------------------------------------------------------------- +# Regression test for Copilot review on PR petry-projects/.github#85: +# add_label_to_discussion previously sent labelIds via -f as the literal +# string `["L_1"]` which the GraphQL API rejects (labelIds is [ID!]!). +# The fix routes through gh_safe_graphql_input with a properly built JSON +# variables block. +# --------------------------------------------------------------------------- + +@test "live: add_label_to_discussion variables block has labelIds as JSON array" { + # Replace the gh stub with one that captures stdin to a file so we can + # introspect the actual request body that gh would have received. + body_file="${TT_TMP}/captured-body.json" + stub_dir="${TT_TMP}/bin" + mkdir -p "$stub_dir" + cat >"$stub_dir/gh" <"$body_file" +echo '{"data":{"addLabelsToLabelable":{"clientMutationId":null}}}' +EOF + chmod +x "$stub_dir/gh" + PATH="${stub_dir}:${PATH}" add_label_to_discussion "D_1" "L_enhancement" + + [ -f "$body_file" ] + # Body must parse as JSON and have variables.labelIds as a length-1 array + # whose sole element is the literal label id. + jq -e '.variables.labelIds | type == "array"' "$body_file" >/dev/null + jq -e '.variables.labelIds | length == 1' "$body_file" >/dev/null + jq -e '.variables.labelIds[0] == "L_enhancement"' "$body_file" >/dev/null + jq -e '.variables.labelableId == "D_1"' "$body_file" >/dev/null + jq -e '.query | contains("addLabelsToLabelable")' "$body_file" >/dev/null +} diff --git a/test/workflows/feature-ideation/gh-safe.bats b/test/workflows/feature-ideation/gh-safe.bats index 9194d538..ef769cf7 100644 --- a/test/workflows/feature-ideation/gh-safe.bats +++ b/test/workflows/feature-ideation/gh-safe.bats @@ -161,3 +161,58 @@ teardown() { run gh_safe_graphql -f query='q' [ "$status" -ne 0 ] } + +# --------------------------------------------------------------------------- +# Regression tests for Copilot review feedback on PR petry-projects/.github#85 +# --------------------------------------------------------------------------- + +@test "graphql: invalid --jq filter EXITS NON-ZERO instead of returning []" { + # Catches the typo class: previously the wrapper used `jq ... || true` + # which silently swallowed filter syntax errors and returned an empty + # array, masking R1-class bugs. + GH_STUB_STDOUT='{"data":{"repository":{"id":"R_1"}}}' \ + run gh_safe_graphql -f query='q' --jq '.data.repository.does_not_exist | bogus_function' + [ "$status" -ne 0 ] +} + +@test "graphql: --jq returning explicit JSON null still normalizes to []" { + GH_STUB_STDOUT='{"data":{"repository":{"nodes":null}}}' \ + run gh_safe_graphql -f query='q' --jq '.data.repository.nodes' + [ "$status" -eq 0 ] + [ "$output" = '[]' ] +} + +# --------------------------------------------------------------------------- +# gh_safe_graphql_input — for mutations with array variables +# --------------------------------------------------------------------------- + +@test "graphql_input: rejects non-JSON body" { + run gh_safe_graphql_input 'not json' + [ "$status" -eq 64 ] +} + +@test "graphql_input: forwards body to gh via stdin and returns response" { + GH_STUB_STDOUT='{"data":{"addLabelsToLabelable":{"clientMutationId":null}}}' \ + run gh_safe_graphql_input '{"query":"mutation{...}","variables":{"labelIds":["L_1"]}}' + [ "$status" -eq 0 ] + [[ "$output" == *'addLabelsToLabelable'* ]] +} + +@test "graphql_input: EXITS NON-ZERO on errors envelope" { + GH_STUB_STDOUT='{"errors":[{"message":"forbidden"}],"data":null}' \ + run gh_safe_graphql_input '{"query":"q","variables":{}}' + [ "$status" -ne 0 ] +} + +@test "graphql_input: EXITS NON-ZERO on data:null" { + GH_STUB_STDOUT='{"data":null}' \ + run gh_safe_graphql_input '{"query":"q","variables":{}}' + [ "$status" -ne 0 ] +} + +@test "graphql_input: EXITS NON-ZERO on gh hard failure" { + GH_STUB_EXIT=1 \ + GH_STUB_STDERR='HTTP 401' \ + run gh_safe_graphql_input '{"query":"q","variables":{}}' + [ "$status" -ne 0 ] +} diff --git a/test/workflows/feature-ideation/lint-prompt.bats b/test/workflows/feature-ideation/lint-prompt.bats index e195bb3c..a0cd32ea 100644 --- a/test/workflows/feature-ideation/lint-prompt.bats +++ b/test/workflows/feature-ideation/lint-prompt.bats @@ -120,3 +120,41 @@ YML run bash "$LINTER" [ "$status" -eq 0 ] } + +# --------------------------------------------------------------------------- +# Coverage of claude-code-action v1 `prompt:` form (in addition to v0 `direct_prompt:`) +# Caught by Copilot review on PR #85 — the original linter only scanned +# `direct_prompt:` and would silently miss R2 regressions in the actual +# reusable workflow which uses the v1 `prompt:` form. +# --------------------------------------------------------------------------- + +@test "lint-prompt: scans v1 prompt: blocks (not just direct_prompt:)" { + write_yml "${TT_TMP}/v1-prompt.yml" <<'YML' +jobs: + analyze: + steps: + - uses: anthropics/claude-code-action@v1 + with: + prompt: | + You are Mary. + Date: $(date -u +%Y-%m-%d) +YML + run bash "$LINTER" "${TT_TMP}/v1-prompt.yml" + [ "$status" -eq 1 ] +} + +@test "lint-prompt: clean v1 prompt: passes" { + write_yml "${TT_TMP}/v1-clean.yml" <<'YML' +jobs: + analyze: + steps: + - uses: anthropics/claude-code-action@v1 + with: + prompt: | + You are Mary. + Repo: ${{ github.repository }} + Read RUN_DATE from the environment at runtime via printenv. +YML + run bash "$LINTER" "${TT_TMP}/v1-clean.yml" + [ "$status" -eq 0 ] +} diff --git a/test/workflows/feature-ideation/match-discussions.bats b/test/workflows/feature-ideation/match-discussions.bats index 792fdd5c..f5f13c71 100644 --- a/test/workflows/feature-ideation/match-discussions.bats +++ b/test/workflows/feature-ideation/match-discussions.bats @@ -161,6 +161,31 @@ build_proposals() { [ "$new" = "0" ] } +@test "match: rejects non-numeric MATCH_THRESHOLD with usage error" { + # Caught by Copilot review on PR petry-projects/.github#85: previously + # a typo in MATCH_THRESHOLD produced an opaque Python traceback. + build_signals '[]' + build_proposals '[{"title":"x"}]' + MATCH_THRESHOLD="not-a-number" run bash "$MATCH" "${TT_TMP}/signals.json" "${TT_TMP}/proposals.json" + [ "$status" -eq 64 ] +} + +@test "match: rejects MATCH_THRESHOLD outside [0, 1]" { + build_signals '[]' + build_proposals '[{"title":"x"}]' + MATCH_THRESHOLD="1.5" run bash "$MATCH" "${TT_TMP}/signals.json" "${TT_TMP}/proposals.json" + [ "$status" -eq 64 ] +} + +@test "match: accepts boundary MATCH_THRESHOLD values 0 and 1" { + build_signals '[]' + build_proposals '[{"title":"x"}]' + MATCH_THRESHOLD="0" run bash "$MATCH" "${TT_TMP}/signals.json" "${TT_TMP}/proposals.json" + [ "$status" -eq 0 ] + MATCH_THRESHOLD="1" run bash "$MATCH" "${TT_TMP}/signals.json" "${TT_TMP}/proposals.json" + [ "$status" -eq 0 ] +} + @test "match: idempotent re-run of same proposals against existing matches yields all-matched" { # Simulates the R6 idempotency case: run 1 created Discussions, run 2 # finds them and should NOT propose duplicates. diff --git a/test/workflows/feature-ideation/signals-schema.bats b/test/workflows/feature-ideation/signals-schema.bats index a416b8f7..5790e64a 100644 --- a/test/workflows/feature-ideation/signals-schema.bats +++ b/test/workflows/feature-ideation/signals-schema.bats @@ -13,6 +13,7 @@ FIX="${TT_FIXTURES_DIR}/expected" @test "schema: validator script exists and is executable" { [ -f "$VALIDATOR" ] [ -r "$VALIDATOR" ] + [ -x "$VALIDATOR" ] } @test "schema: schema file is valid JSON" { @@ -50,3 +51,20 @@ FIX="${TT_FIXTURES_DIR}/expected" run python3 "$VALIDATOR" "$bad_file" "$SCHEMA" [ "$status" -eq 1 ] } + +@test "schema: invalid date-time scan_date FAILS validation (format checker enforced)" { + # Caught by Copilot review on PR petry-projects/.github#85: + # Draft202012Validator does NOT enforce `format` keywords by default. + # The validator must instantiate FormatChecker explicitly. + bad_file="${BATS_TEST_TMPDIR}/bad-scan-date.json" + jq '.scan_date = "not-a-real-timestamp"' "${FIX}/empty-repo.signals.json" >"$bad_file" + run python3 "$VALIDATOR" "$bad_file" "$SCHEMA" + [ "$status" -eq 1 ] +} + +@test "schema: well-formed date-time scan_date passes" { + good_file="${BATS_TEST_TMPDIR}/good-scan-date.json" + jq '.scan_date = "2026-04-08T12:34:56Z"' "${FIX}/empty-repo.signals.json" >"$good_file" + run python3 "$VALIDATOR" "$good_file" "$SCHEMA" + [ "$status" -eq 0 ] +} diff --git a/test/workflows/feature-ideation/stubs/gh b/test/workflows/feature-ideation/stubs/gh index 158b6655..2e8cd24d 100755 --- a/test/workflows/feature-ideation/stubs/gh +++ b/test/workflows/feature-ideation/stubs/gh @@ -9,7 +9,10 @@ # GH_STUB_STDOUT — literal text or @file:/abs/path to load from disk # GH_STUB_STDERR — literal text or @file:/abs/path # GH_STUB_EXIT — exit code (default 0) -# GH_STUB_LOG — append-only log of argv invocations (one TSV line each) +# GH_STUB_LOG — append-only log of argv invocations. Each invocation +# is logged as a single line of shell-quoted tokens +# (via `printf '%q '`) so it can be re-parsed losslessly +# even when arguments contain spaces or special chars. # # For multi-call tests (where the same stub is invoked multiple times with # different responses), set GH_STUB_SCRIPT to a file containing one @@ -19,7 +22,12 @@ set -euo pipefail if [ -n "${GH_STUB_LOG:-}" ]; then - printf '%s\n' "$*" >>"$GH_STUB_LOG" + # Use %q so each arg is shell-quoted; tests can re-parse argv losslessly. + # Caught by Copilot review on PR petry-projects/.github#85: previously + # used $* which joined with spaces and lost argument boundaries. + # shellcheck disable=SC2059 + printf '%q ' "$@" >>"$GH_STUB_LOG" + printf '\n' >>"$GH_STUB_LOG" fi # Multi-call mode. From 6725a19b952726deb913502aab157772b774e600 Mon Sep 17 00:00:00 2001 From: DJ Date: Wed, 8 Apr 2026 05:24:21 -0700 Subject: [PATCH 4/6] fix(feature-ideation): address CodeRabbit review on PR #85 (7 fixes + 1 test) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triaged 13 inline comments from CodeRabbit's review of #85; 6 of them overlapped with Copilot's review and were already fixed by bcaa579. The remaining 7 are addressed here. Fixes ----- 1. lint-prompt.sh: ${VAR} branch lookbehind was inconsistent with the $(...) branch — only rejected $$VAR but not \${VAR}. Both branches now use [\\$] so backslash-escaped and dollar-escaped forms are skipped uniformly. 2. filter-bots.sh: FEATURE_IDEATION_BOT_AUTHORS CSV entries are now trimmed of leading/trailing whitespace before being added to the blocklist, so "bot1, bot2" matches both bots correctly instead of keeping a literal " bot2" entry. 3. validate-signals.py: malformed signals JSON now exits 2 (file/data error) to match the documented contract, instead of 1 (which means schema validation error). 4. README.md: corrected the workflow filename reference from feature-ideation.yml to feature-ideation-reusable.yml, and reworded the table cell that contained `\|\|` (escaped pipes that don't render correctly in some Markdown engines) to use plain prose. Also noted that lint-prompt scans both v0 `direct_prompt:` and v1 `prompt:`. 5. collect-signals.sh: added an explicit comment above SCHEMA_VERSION documenting the lockstep requirement with signals.schema.json's $comment version annotation. Backed by a new bats test that parses both files and asserts they match. 6. signals.schema.json: added $comment "version: 1.0.0" annotation so the schema file declares its own version explicitly. Used $comment instead of a custom keyword to keep Draft202012 compliance. 7. test/workflows/feature-ideation/match-discussions.bats: build_signals helper now computes the discussions count from the array length instead of hardcoding 0, so the fixture satisfies its own contract (cosmetic — the matcher only reads .items, but contract hygiene matters in test scaffolding). 8. test/workflows/feature-ideation/gh-safe.bats: removed the `|| true` suffix on the rest-failure assertion that made it always pass. Now uses --separate-stderr to capture stderr and asserts the structured `[gh-safe][rest-failure]` prefix is emitted on the auth failure path. Required `bats_require_minimum_version 1.5.0` to suppress the bats-core warning about flag usage. Tests ----- Test count: 108 → 109 (one new test for SCHEMA_VERSION ↔ schema sync). All 109 passing locally. Run with: bats test/workflows/feature-ideation/ Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/schemas/signals.schema.json | 1 + .github/scripts/feature-ideation/README.md | 11 ++++++----- .../scripts/feature-ideation/collect-signals.sh | 6 ++++++ .../scripts/feature-ideation/lib/filter-bots.sh | 13 ++++++++++++- .github/scripts/feature-ideation/lint-prompt.sh | 7 +++++-- .../scripts/feature-ideation/validate-signals.py | 6 +++++- test/workflows/feature-ideation/gh-safe.bats | 12 +++++++++--- .../feature-ideation/match-discussions.bats | 8 +++++++- .../workflows/feature-ideation/signals-schema.bats | 14 ++++++++++++++ 9 files changed, 65 insertions(+), 13 deletions(-) diff --git a/.github/schemas/signals.schema.json b/.github/schemas/signals.schema.json index 9a1ad075..ded4367e 100644 --- a/.github/schemas/signals.schema.json +++ b/.github/schemas/signals.schema.json @@ -1,6 +1,7 @@ { "$schema": "https://json-schema.org/draft/2020-12/schema", "$id": "https://github.com/petry-projects/.github/blob/main/.github/schemas/signals.schema.json", + "$comment": "version: 1.0.0 — must match SCHEMA_VERSION in collect-signals.sh; enforced by bats", "title": "Feature Ideation Signals", "description": "Canonical contract between collect-signals.sh and the BMAD Analyst (Mary) prompt. Any change to this schema is a breaking change to the workflow.", "type": "object", diff --git a/.github/scripts/feature-ideation/README.md b/.github/scripts/feature-ideation/README.md index 957a06aa..372125c3 100644 --- a/.github/scripts/feature-ideation/README.md +++ b/.github/scripts/feature-ideation/README.md @@ -1,9 +1,10 @@ # Feature Ideation — Scripts & Test Strategy This directory contains the bash + Python helpers that back -`.github/workflows/feature-ideation.yml`. Every line of logic that used to -live inside the workflow's heredoc has been extracted here so it can be unit -tested with bats. +`.github/workflows/feature-ideation-reusable.yml`. Every line of logic that +used to live inside the workflow's heredoc has been extracted here so it can +be unit tested with bats. Downstream BMAD repos call the reusable workflow +via the standards stub at `standards/workflows/feature-ideation.yml`. ## Why this exists @@ -21,7 +22,7 @@ caught **before UAT** instead of after. | File | Purpose | Killed risk | |------|---------|------------| -| `lib/gh-safe.sh` | Wraps every `gh` and `gh api graphql` call. Fails loud on auth, rate-limit, network, GraphQL errors envelope, or `data: null`. Replaces the original `2>/dev/null \|\| echo '[]'` swallow. | R1, R7, R8 | +| `lib/gh-safe.sh` | Wraps every `gh` and `gh api graphql` call. Fails loud on auth, rate-limit, network, GraphQL errors envelope, or `data: null`. Replaces the original `2>/dev/null` + `echo '[]'` swallow pattern. | R1, R7, R8 | | `lib/compose-signals.sh` | Validates JSON inputs before `jq --argjson` and assembles the canonical signals.json document. | R3, R4 | | `lib/filter-bots.sh` | Configurable bot blocklist via `FEATURE_IDEATION_BOT_AUTHORS` (extends the default list of bot logins to remove from results). | R10 | | `lib/date-utils.sh` | Cross-platform date arithmetic helpers. | R9 | @@ -29,7 +30,7 @@ caught **before UAT** instead of after. | `validate-signals.py` | JSON Schema 2020-12 validator for signals.json against `../schemas/signals.schema.json`. | R3 | | `match-discussions.sh` | Deterministic Jaccard-similarity matcher between Mary's proposals and existing Ideas Discussions. Replaces the prose "use fuzzy matching" instruction. | R5, R6 | | `discussion-mutations.sh` | `create_discussion`, `comment_on_discussion`, `add_label_to_discussion` wrappers with `DRY_RUN=1` audit-log mode. | Smoke testing | -| `lint-prompt.sh` | Scans every workflow file for unescaped `$()` / `${VAR}` inside `direct_prompt:` blocks (which YAML and `claude-code-action` pass verbatim instead of expanding). | R2 | +| `lint-prompt.sh` | Scans every workflow file for unescaped `$()` / `${VAR}` inside `direct_prompt:` (v0) and `prompt:` (v1) blocks (which YAML and `claude-code-action` pass verbatim instead of expanding). | R2 | ## Running the tests diff --git a/.github/scripts/feature-ideation/collect-signals.sh b/.github/scripts/feature-ideation/collect-signals.sh index 69f605c4..fd3b1b3e 100755 --- a/.github/scripts/feature-ideation/collect-signals.sh +++ b/.github/scripts/feature-ideation/collect-signals.sh @@ -25,6 +25,12 @@ set -euo pipefail +# SCHEMA_VERSION must stay in lockstep with the `version` field in +# .github/schemas/signals.schema.json. Bumping one without the other is a +# compatibility break: validate-signals.py will reject the runtime output +# if the constants drift, AND the bats `signals-schema: SCHEMA_VERSION +# constant matches schema file` test enforces this in CI. +# Caught by CodeRabbit review on PR petry-projects/.github#85. SCHEMA_VERSION="1.0.0" SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" diff --git a/.github/scripts/feature-ideation/lib/filter-bots.sh b/.github/scripts/feature-ideation/lib/filter-bots.sh index c3b38bea..f2fbfdec 100755 --- a/.github/scripts/feature-ideation/lib/filter-bots.sh +++ b/.github/scripts/feature-ideation/lib/filter-bots.sh @@ -38,7 +38,18 @@ filter_bots_build_list() { local IFS=',' # shellcheck disable=SC2206 local extras=($FEATURE_IDEATION_BOT_AUTHORS) - list+=("${extras[@]}") + # Trim leading/trailing whitespace from each comma-separated entry so + # `"bot1, bot2"` resolves to `bot1` and `bot2`, not `bot1` and ` bot2`. + # Caught by CodeRabbit review on PR petry-projects/.github#85. + local trimmed=() + local entry + for entry in "${extras[@]}"; do + # Strip leading whitespace, then trailing whitespace. + entry="${entry#"${entry%%[![:space:]]*}"}" + entry="${entry%"${entry##*[![:space:]]}"}" + [ -n "$entry" ] && trimmed+=("$entry") + done + list+=("${trimmed[@]}") fi printf '%s\n' "${list[@]}" | jq -R . | jq -sc . } diff --git a/.github/scripts/feature-ideation/lint-prompt.sh b/.github/scripts/feature-ideation/lint-prompt.sh index 3b1aebcc..ad8226de 100755 --- a/.github/scripts/feature-ideation/lint-prompt.sh +++ b/.github/scripts/feature-ideation/lint-prompt.sh @@ -48,8 +48,11 @@ block_indent = -1 findings = [] # Pattern matches $(...) and ${VAR} but NOT GitHub Actions ${{ ... }} -# because that's evaluated before the prompt is rendered. -shell_expansion = re.compile(r'(?` block scalar indicators. diff --git a/.github/scripts/feature-ideation/validate-signals.py b/.github/scripts/feature-ideation/validate-signals.py index 69ff0c1b..eb82ff7c 100755 --- a/.github/scripts/feature-ideation/validate-signals.py +++ b/.github/scripts/feature-ideation/validate-signals.py @@ -55,8 +55,12 @@ def main(argv: list[str]) -> int: try: signals = json.loads(signals_path.read_text()) except json.JSONDecodeError as exc: + # Per the docstring contract, exit 2 means usage / file error and + # exit 1 means schema validation error. A malformed signals file + # is a file/data error, not a schema violation. Caught by + # CodeRabbit review on PR petry-projects/.github#85. sys.stderr.write(f"[validate-signals] invalid JSON in {signals_path}: {exc}\n") - return 1 + return 2 try: schema = json.loads(schema_path.read_text()) diff --git a/test/workflows/feature-ideation/gh-safe.bats b/test/workflows/feature-ideation/gh-safe.bats index ef769cf7..b06bb5c7 100644 --- a/test/workflows/feature-ideation/gh-safe.bats +++ b/test/workflows/feature-ideation/gh-safe.bats @@ -4,6 +4,8 @@ # These tests kill R1: the original "2>/dev/null || echo '[]'" pattern that # silently swallowed every kind of failure. Each test pins one failure mode. +bats_require_minimum_version 1.5.0 + load 'helpers/setup' setup() { @@ -67,12 +69,16 @@ teardown() { # gh_safe_rest — failure modes (R1 kill list) # --------------------------------------------------------------------------- -@test "rest: EXITS NON-ZERO on auth failure (gh exit 4)" { +@test "rest: EXITS NON-ZERO on auth failure (gh exit 4) and reports the failure category" { + # CodeRabbit on PR petry-projects/.github#85: the previous assertion + # ended with `|| true`, so it always passed regardless of the message + # content. Use --separate-stderr to capture stderr and assert the + # structured `[gh-safe][rest-failure]` prefix is emitted. GH_STUB_EXIT=4 \ GH_STUB_STDERR='HTTP 401: Bad credentials' \ - run gh_safe_rest issue list --repo foo/bar + run --separate-stderr gh_safe_rest issue list --repo foo/bar [ "$status" -ne 0 ] - [[ "$output" == *"rest-failure"* ]] || [[ "$stderr" == *"rest-failure"* ]] || true + [[ "$stderr" == *"rest-failure"* ]] } @test "rest: EXITS NON-ZERO on rate limit (gh exit 1 + 403)" { diff --git a/test/workflows/feature-ideation/match-discussions.bats b/test/workflows/feature-ideation/match-discussions.bats index f5f13c71..9b63ea9a 100644 --- a/test/workflows/feature-ideation/match-discussions.bats +++ b/test/workflows/feature-ideation/match-discussions.bats @@ -18,8 +18,14 @@ teardown() { } # Helper: build a signals.json with the given discussions array. +# Computes the discussions count from the array length so the fixture +# always satisfies the producer/consumer contract (even though the +# matcher only reads .items, validating contracts in test fixtures +# is good hygiene). CodeRabbit on PR petry-projects/.github#85. build_signals() { local discussions="$1" + local count + count=$(printf '%s' "$discussions" | jq 'length') cat >"${TT_TMP}/signals.json" < Date: Wed, 8 Apr 2026 19:59:04 +0000 Subject: [PATCH 5/6] fix(feature-ideation): address CodeRabbit re-review on PR #85 (15 fixes + 5 new tests) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical/major: - collect-signals.sh: validate ISSUE_LIMIT/PR_LIMIT/DISCUSSION_LIMIT are positive integers; tighten REPO validation with strict ^[^/]+/[^/]+$ regex - compose-signals.sh: enforce array type (jq 'type == "array"') not just valid JSON so objects/strings don't silently produce wrong counts - date-utils.sh: guard $# before reading $1 to prevent set -u abort on zero-arg calls - filter-bots.sh: replace unquoted array expansion with IFS=',' read -r -a to prevent pathname-globbing against filesystem entries - gh-safe.sh: bounds-check args[i+1] before --jq dereference; add $# guard to gh_safe_graphql_input() to prevent nounset abort - lint-prompt.sh: recognise YAML chomping modifiers (|-,|+,>-,>+) in prompt_marker regex; replace [^}]* GH-expression stripper with a stateful scanner that handles nested braces; preserve exit-2 over exit-1 in main() - match-discussions.sh: wrap json.load calls in try/except for structured error exit-2 instead of Python traceback; skip discussions without an id; switch from greedy per-proposal to similarity-sorted global optimal matching - validate-signals.py: catch OSError on read_text() to preserve exit-2 contract; add -> bool return type annotation to _check_date_time Docs: - README.md: update lint command to mention both direct_prompt: and prompt:; fix Mary's prompt pointer to feature-ideation-reusable.yml Tests (+5 new, 109 → 114 total): - lint-prompt.bats: missing-file-before-lint-failing-file exits 2; YAML chomping modifiers detected; nested GH expressions don't false-positive - match-discussions.bats: malformed signals JSON exits non-zero; malformed proposals JSON exits non-zero - signals-schema.bats: truncated/malformed JSON exits 2 not 1 - date-utils.bats: use date_today helper instead of raw date -u - stubs/gh: prefer TT_TMP/BATS_TEST_TMPDIR for counter file isolation Co-authored-by: don-petry --- .github/scripts/feature-ideation/README.md | 4 +- .../feature-ideation/collect-signals.sh | 28 +++++- .../feature-ideation/lib/compose-signals.sh | 7 +- .../feature-ideation/lib/date-utils.sh | 7 ++ .../feature-ideation/lib/filter-bots.sh | 8 +- .../scripts/feature-ideation/lib/gh-safe.sh | 14 +++ .../scripts/feature-ideation/lint-prompt.sh | 59 ++++++++++-- .../feature-ideation/match-discussions.sh | 92 ++++++++++++++----- .../feature-ideation/validate-signals.py | 14 ++- .../feature-ideation/date-utils.bats | 5 +- .../feature-ideation/lint-prompt.bats | 54 +++++++++++ .../feature-ideation/match-discussions.bats | 16 ++++ .../feature-ideation/signals-schema.bats | 9 ++ test/workflows/feature-ideation/stubs/gh | 5 +- 14 files changed, 275 insertions(+), 47 deletions(-) diff --git a/.github/scripts/feature-ideation/README.md b/.github/scripts/feature-ideation/README.md index 372125c3..f213172e 100644 --- a/.github/scripts/feature-ideation/README.md +++ b/.github/scripts/feature-ideation/README.md @@ -51,7 +51,7 @@ bats test/workflows/feature-ideation/gh-safe.bats shellcheck -x collect-signals.sh lint-prompt.sh match-discussions.sh \ discussion-mutations.sh lib/*.sh) -# Lint the workflow's direct_prompt block +# Lint workflow prompt blocks (direct_prompt: and prompt:) bash .github/scripts/feature-ideation/lint-prompt.sh ``` @@ -76,7 +76,7 @@ signals.json is a breaking change and must: 1. Bump `SCHEMA_VERSION` in `collect-signals.sh`. 2. Update fixtures under `test/workflows/feature-ideation/fixtures/expected/`. -3. Update Mary's prompt in `feature-ideation.yml` if any field references move. +3. Update Mary's prompt in `.github/workflows/feature-ideation-reusable.yml` if any field references move. CI validates every fixture against the schema, and the workflow validates the runtime output before handing it to Mary. diff --git a/.github/scripts/feature-ideation/collect-signals.sh b/.github/scripts/feature-ideation/collect-signals.sh index fd3b1b3e..b2ae23cc 100755 --- a/.github/scripts/feature-ideation/collect-signals.sh +++ b/.github/scripts/feature-ideation/collect-signals.sh @@ -58,13 +58,33 @@ main() { local discussion_limit="${DISCUSSION_LIMIT:-100}" local output_path="${SIGNALS_OUTPUT:-./signals.json}" - local owner repo_name - owner="${REPO%%/*}" - repo_name="${REPO##*/}" - if [ "$owner" = "$REPO" ] || [ -z "$repo_name" ]; then + # Validate that limit overrides are positive integers before forwarding to + # GraphQL — a value like `ISSUE_LIMIT=foo` would cause an opaque downstream + # failure instead of a clean usage error. Caught by CodeRabbit review on + # PR petry-projects/.github#85. + local _lim_name _lim_val + for _lim_name in ISSUE_LIMIT PR_LIMIT DISCUSSION_LIMIT; do + case "$_lim_name" in + ISSUE_LIMIT) _lim_val="$issue_limit" ;; + PR_LIMIT) _lim_val="$pr_limit" ;; + DISCUSSION_LIMIT) _lim_val="$discussion_limit" ;; + esac + if [[ ! $_lim_val =~ ^[1-9][0-9]*$ ]]; then + printf '[collect-signals] %s must be a positive integer, got: %s\n' "$_lim_name" "$_lim_val" >&2 + return 64 + fi + done + + # Strict owner/name format — reject leading/trailing slashes, empty segments, + # and extra path parts (e.g. "org//repo", "/repo", "org/repo/extra"). + # Caught by CodeRabbit review on PR petry-projects/.github#85. + if [[ ! $REPO =~ ^[^/]+/[^/]+$ ]]; then printf '[collect-signals] REPO must be in owner/name format, got: %s\n' "$REPO" >&2 return 64 fi + local owner repo_name + owner="${REPO%%/*}" + repo_name="${REPO##*/}" local thirty_days_ago thirty_days_ago=$(date_days_ago 30) diff --git a/.github/scripts/feature-ideation/lib/compose-signals.sh b/.github/scripts/feature-ideation/lib/compose-signals.sh index 6a0f5bd6..1c699349 100755 --- a/.github/scripts/feature-ideation/lib/compose-signals.sh +++ b/.github/scripts/feature-ideation/lib/compose-signals.sh @@ -49,8 +49,11 @@ compose_signals() { for input in "$open_issues" "$closed_issues" "$ideas_discussions" "$releases" \ "$merged_prs" "$feature_requests" "$bug_reports" "$truncation_warnings"; do idx=$((idx + 1)) - if ! printf '%s' "$input" | jq -e . >/dev/null 2>&1; then - printf '[compose-signals] arg #%d is not valid JSON: %s\n' "$idx" "${input:0:120}" >&2 + # Require a JSON array, not just valid JSON. Objects/strings/nulls accepted + # by `jq -e .` would silently produce wrong counts (key count, char count). + # Caught by CodeRabbit review on PR petry-projects/.github#85. + if ! printf '%s' "$input" | jq -e 'type == "array"' >/dev/null 2>&1; then + printf '[compose-signals] arg #%d must be a JSON array: %s\n' "$idx" "${input:0:120}" >&2 return 65 # EX_DATAERR fi done diff --git a/.github/scripts/feature-ideation/lib/date-utils.sh b/.github/scripts/feature-ideation/lib/date-utils.sh index 8c4b5a12..ae2722bd 100755 --- a/.github/scripts/feature-ideation/lib/date-utils.sh +++ b/.github/scripts/feature-ideation/lib/date-utils.sh @@ -10,6 +10,13 @@ set -euo pipefail # Print an ISO date (YYYY-MM-DD) for N days ago in UTC. date_days_ago() { + # Guard arg count before reading $1: under set -u a zero-arg call would abort + # the shell with "unbound variable" instead of reaching the validation path. + # Caught by CodeRabbit review on PR petry-projects/.github#85. + if [ "$#" -ne 1 ]; then + printf '[date-utils] expected 1 arg (days), got: %d\n' "$#" >&2 + return 64 + fi local days="$1" if [ -z "$days" ] || ! printf '%s' "$days" | grep -Eq '^[0-9]+$'; then printf '[date-utils] days must be a non-negative integer, got: %s\n' "$days" >&2 diff --git a/.github/scripts/feature-ideation/lib/filter-bots.sh b/.github/scripts/feature-ideation/lib/filter-bots.sh index f2fbfdec..f994e3e2 100755 --- a/.github/scripts/feature-ideation/lib/filter-bots.sh +++ b/.github/scripts/feature-ideation/lib/filter-bots.sh @@ -35,9 +35,11 @@ DEFAULT_BOT_AUTHORS=( filter_bots_build_list() { local list=("${DEFAULT_BOT_AUTHORS[@]}") if [ -n "${FEATURE_IDEATION_BOT_AUTHORS:-}" ]; then - local IFS=',' - # shellcheck disable=SC2206 - local extras=($FEATURE_IDEATION_BOT_AUTHORS) + # Use `IFS=',' read` (not unquoted expansion) to avoid pathname-globbing + # against the filesystem if any entry contains wildcard characters. + # Caught by CodeRabbit review on PR petry-projects/.github#85. + local extras=() + IFS=',' read -r -a extras <<<"${FEATURE_IDEATION_BOT_AUTHORS}" # Trim leading/trailing whitespace from each comma-separated entry so # `"bot1, bot2"` resolves to `bot1` and `bot2`, not `bot1` and ` bot2`. # Caught by CodeRabbit review on PR petry-projects/.github#85. diff --git a/.github/scripts/feature-ideation/lib/gh-safe.sh b/.github/scripts/feature-ideation/lib/gh-safe.sh index 343a5bbc..11a4758e 100755 --- a/.github/scripts/feature-ideation/lib/gh-safe.sh +++ b/.github/scripts/feature-ideation/lib/gh-safe.sh @@ -93,6 +93,13 @@ gh_safe_graphql() { local i=0 while [ "$i" -lt "${#args[@]}" ]; do if [ "${args[$i]}" = "--jq" ]; then + # Guard bounds before dereferencing args[i+1]: under set -u an out-of- + # bounds access aborts the shell. Caught by CodeRabbit review on PR + # petry-projects/.github#85. + if [ $((i + 1)) -ge "${#args[@]}" ]; then + _gh_safe_err "graphql-bad-args" "--jq requires a jq filter argument" + return 64 + fi has_jq=1 jq_filter="${args[$((i + 1))]}" break @@ -187,6 +194,13 @@ gh_safe_graphql() { # Same defensive contract as `gh_safe_graphql`: any auth/network/schema # failure exits non-zero with a structured stderr message. gh_safe_graphql_input() { + # Guard arg count before reading $1: under set -u a zero-arg call aborts the + # shell instead of reaching the JSON validation. Caught by CodeRabbit review + # on PR petry-projects/.github#85. + if [ "$#" -ne 1 ]; then + _gh_safe_err "graphql-bad-input" "expected 1 arg: JSON request body, got $#" + return 64 + fi local body="$1" if ! gh_safe_is_json "$body"; then _gh_safe_err "graphql-bad-input" "request body is not valid JSON" diff --git a/.github/scripts/feature-ideation/lint-prompt.sh b/.github/scripts/feature-ideation/lint-prompt.sh index ad8226de..a1d18b63 100755 --- a/.github/scripts/feature-ideation/lint-prompt.sh +++ b/.github/scripts/feature-ideation/lint-prompt.sh @@ -31,6 +31,34 @@ scan_file() { import re import sys + +def _strip_github_expressions(s: str) -> str: + """Remove ${{ ... }} GitHub Actions expressions from s. + + Uses a stateful scanner instead of `[^}]*` regex so that expressions + containing `}` inside string literals (e.g. format() calls) are fully + consumed rather than prematurely terminated. This prevents false-positive + shell-expansion matches on content that is actually inside a GH expression. + Caught by CodeRabbit review on PR petry-projects/.github#85. + """ + result: list[str] = [] + i = 0 + while i < len(s): + if s[i : i + 3] == "${{": + # Consume until we find the matching "}}" + j = i + 3 + while j < len(s): + if s[j : j + 2] == "}}": + j += 2 + break + j += 1 + i = j # skip the whole ${{ ... }} expression + else: + result.append(s[i]) + i += 1 + return "".join(result) + + path = sys.argv[1] try: with open(path, "r", encoding="utf-8") as f: @@ -55,8 +83,10 @@ findings = [] shell_expansion = re.compile(r'(?` block scalar indicators. -prompt_marker = re.compile(r'(?:direct_prompt|prompt):\s*[|>]?\s*$') +# optional `|` or `>` block scalar indicators plus YAML chomping modifiers +# (`-` or `+`) so `prompt: |-`, `prompt: |+`, `prompt: >-`, `prompt: >+` +# are all recognised. Caught by CodeRabbit review on PR petry-projects/.github#85. +prompt_marker = re.compile(r'(?:direct_prompt|prompt):\s*[|>]?[-+]?\s*$') for lineno, raw in enumerate(lines, start=1): stripped = raw.lstrip(" ") @@ -78,8 +108,13 @@ for lineno, raw in enumerate(lines, start=1): continue # We're inside the prompt body. Scan for shell expansions. - # First, strip out any GitHub Actions expressions so they don't trip us. - no_gh = re.sub(r'\$\{\{[^}]*\}\}', '', raw) + # First, strip out GitHub Actions ${{ ... }} expressions. + # The naive `[^}]*` regex stops at the first `}`, so expressions that + # contain `}` internally (e.g. format() calls or string literals) are + # not fully removed and leave false-positive shell expansion matches. + # Use a small stateful scanner instead. + # Caught by CodeRabbit review on PR petry-projects/.github#85. + no_gh = _strip_github_expressions(raw) for match in shell_expansion.finditer(no_gh): findings.append((lineno, match.group(0), raw.rstrip())) @@ -107,15 +142,27 @@ main() { fi local exit=0 + local file_rc=0 for file in "$@"; do if [ ! -f "$file" ]; then printf '[lint-prompt] not found: %s\n' "$file" >&2 exit=2 continue fi - if ! scan_file "$file"; then - exit=1 + # Capture the actual exit code so we preserve exit-2 (file error) over + # exit-1 (lint finding). A later lint failure must not overwrite an earlier + # file error. Caught by CodeRabbit review on PR petry-projects/.github#85. + if scan_file "$file"; then + file_rc=0 + else + file_rc=$? fi + case "$file_rc" in + 0) ;; + 1) if [ "$exit" -eq 0 ]; then exit=1; fi ;; + 2) exit=2 ;; + *) return "$file_rc" ;; + esac done return "$exit" } diff --git a/.github/scripts/feature-ideation/match-discussions.sh b/.github/scripts/feature-ideation/match-discussions.sh index d8bf7668..c4af5466 100755 --- a/.github/scripts/feature-ideation/match-discussions.sh +++ b/.github/scripts/feature-ideation/match-discussions.sh @@ -97,49 +97,91 @@ def jaccard(a: set[str], b: set[str]) -> float: return len(a & b) / len(a | b) -with open(signals_path) as f: - signals = json.load(f) -with open(proposals_path) as f: - proposals = json.load(f) +def _load_json(path: str, label: str): + """Load JSON from path, exiting with code 2 on any read or parse error.""" + try: + with open(path, encoding="utf-8") as f: + return json.load(f) + except OSError as exc: + sys.stderr.write(f"[match-discussions] cannot read {label}: {exc}\n") + sys.exit(2) + except json.JSONDecodeError as exc: + sys.stderr.write(f"[match-discussions] invalid JSON in {label}: {exc}\n") + sys.exit(2) + + +signals = _load_json(signals_path, "signals") +proposals = _load_json(proposals_path, "proposals") if not isinstance(proposals, list): sys.stderr.write("[match-discussions] proposals must be a JSON array\n") sys.exit(65) discussions = signals.get("ideas_discussions", {}).get("items", []) or [] -disc_norm = [(d, normalize(d.get("title", ""))) for d in discussions] - -matched = [] -new_candidates = [] -seen_disc_ids = set() - -for proposal in proposals: +# Skip discussions without an id to avoid all id-less entries collapsing into +# a single `None` key in seen_disc_ids. Caught by CodeRabbit on PR #85. +disc_norm = [ + (d, normalize(d.get("title", ""))) + for d in discussions + if d.get("id") is not None +] + +# --- Optimal (similarity-sorted) matching ------------------------------------ +# The original greedy per-proposal loop consumed discussions in proposal order, +# so an early lower-value match could block a later higher-value match. +# Instead we enumerate all (proposal, discussion) pairs, sort by similarity +# descending (ties broken by original proposal index for stability), then +# assign greedily. This guarantees globally higher-value matches are honoured +# first. Caught by CodeRabbit review on PR petry-projects/.github#85. + +# Collect valid proposals with their original index (for tie-breaking + new_candidates). +proposals_indexed: list[tuple[int, dict]] = [] +for p_idx, proposal in enumerate(proposals): if not isinstance(proposal, dict) or "title" not in proposal: sys.stderr.write(f"[match-discussions] skipping malformed proposal: {proposal!r}\n") continue - p_norm = normalize(proposal["title"]) + proposals_indexed.append((p_idx, proposal)) - best = None - best_sim = 0.0 +# Build all (similarity, proposal_idx, disc_id, proposal, disc) tuples. +all_pairs: list[tuple[float, int, str, dict, dict]] = [] +for p_idx, proposal in proposals_indexed: + p_norm = normalize(proposal["title"]) for disc, d_norm in disc_norm: - if disc.get("id") in seen_disc_ids: - continue sim = jaccard(p_norm, d_norm) - if sim > best_sim: - best_sim = sim - best = disc + all_pairs.append((sim, p_idx, disc["id"], proposal, disc)) - if best is not None and best_sim >= threshold: +# Sort descending by similarity; stable tie-break by proposal index ascending. +all_pairs.sort(key=lambda x: (-x[0], x[1])) + +matched = [] +seen_disc_ids: set[str] = set() +seen_proposal_idxs: set[int] = set() + +for sim, p_idx, disc_id, proposal, disc in all_pairs: + if p_idx in seen_proposal_idxs or disc_id in seen_disc_ids: + continue + if sim >= threshold: matched.append( { "proposal": proposal, - "discussion": best, - "similarity": round(best_sim, 4), + "discussion": disc, + "similarity": round(sim, 4), } ) - seen_disc_ids.add(best.get("id")) - else: - new_candidates.append({"proposal": proposal, "best_similarity": round(best_sim, 4)}) + seen_disc_ids.add(disc_id) + seen_proposal_idxs.add(p_idx) + +# Unmatched proposals become new candidates. +new_candidates = [] +for p_idx, proposal in proposals_indexed: + if p_idx in seen_proposal_idxs: + continue + p_norm = normalize(proposal["title"]) + best_sim = max( + (jaccard(p_norm, d_norm) for _, d_norm in disc_norm), + default=0.0, + ) + new_candidates.append({"proposal": proposal, "best_similarity": round(best_sim, 4)}) result = { "matched": matched, diff --git a/.github/scripts/feature-ideation/validate-signals.py b/.github/scripts/feature-ideation/validate-signals.py index eb82ff7c..e163cf62 100755 --- a/.github/scripts/feature-ideation/validate-signals.py +++ b/.github/scripts/feature-ideation/validate-signals.py @@ -53,7 +53,12 @@ def main(argv: list[str]) -> int: return 2 try: - signals = json.loads(signals_path.read_text()) + signals = json.loads(signals_path.read_text(encoding="utf-8")) + except OSError as exc: + # File read errors (permissions, I/O) must also exit 2. Caught by + # CodeRabbit review on PR petry-projects/.github#85. + sys.stderr.write(f"[validate-signals] cannot read {signals_path}: {exc}\n") + return 2 except json.JSONDecodeError as exc: # Per the docstring contract, exit 2 means usage / file error and # exit 1 means schema validation error. A malformed signals file @@ -63,7 +68,10 @@ def main(argv: list[str]) -> int: return 2 try: - schema = json.loads(schema_path.read_text()) + schema = json.loads(schema_path.read_text(encoding="utf-8")) + except OSError as exc: + sys.stderr.write(f"[validate-signals] cannot read schema {schema_path}: {exc}\n") + return 2 except json.JSONDecodeError as exc: sys.stderr.write(f"[validate-signals] invalid schema JSON: {exc}\n") return 2 @@ -79,7 +87,7 @@ def main(argv: list[str]) -> int: format_checker = FormatChecker() @format_checker.checks("date-time", raises=(ValueError, TypeError)) - def _check_date_time(instance): # noqa: ANN001 — jsonschema callback signature + def _check_date_time(instance) -> bool: # noqa: ANN001 — jsonschema callback signature if not isinstance(instance, str): return True # non-strings handled by `type` keyword, not format # Must look like a date-time, not just any string. Require at least diff --git a/test/workflows/feature-ideation/date-utils.bats b/test/workflows/feature-ideation/date-utils.bats index dfb132b2..a26749ff 100644 --- a/test/workflows/feature-ideation/date-utils.bats +++ b/test/workflows/feature-ideation/date-utils.bats @@ -15,7 +15,10 @@ setup() { } @test "date_days_ago: 0 returns today" { - today=$(date -u +%Y-%m-%d) + # Use the helper function rather than the raw system `date` call so the test + # validates behaviour consistently on both GNU and BSD systems. + # Caught by CodeRabbit review on PR petry-projects/.github#85. + today=$(date_today) run date_days_ago 0 [ "$status" -eq 0 ] [ "$output" = "$today" ] diff --git a/test/workflows/feature-ideation/lint-prompt.bats b/test/workflows/feature-ideation/lint-prompt.bats index a0cd32ea..1ba14d1a 100644 --- a/test/workflows/feature-ideation/lint-prompt.bats +++ b/test/workflows/feature-ideation/lint-prompt.bats @@ -158,3 +158,57 @@ YML run bash "$LINTER" "${TT_TMP}/v1-clean.yml" [ "$status" -eq 0 ] } + +# --------------------------------------------------------------------------- +# Exit-code precedence: file errors (2) must not be downgraded by later lint +# findings (1). Caught by CodeRabbit review on PR petry-projects/.github#85. +# --------------------------------------------------------------------------- + +@test "lint-prompt: missing file before lint-failing file still exits 2" { + write_yml "${TT_TMP}/bad.yml" <<'YML' +jobs: + analyze: + steps: + - uses: anthropics/claude-code-action@v1 + with: + prompt: | + Date: $(date -u +%Y-%m-%d) +YML + # Pass a non-existent file FIRST, then a file with a lint finding. + # The aggregate exit code must be 2 (file error) not 1 (lint finding). + run bash "$LINTER" "${TT_TMP}/missing.yml" "${TT_TMP}/bad.yml" + [ "$status" -eq 2 ] +} + +@test "lint-prompt: YAML chomping modifiers are recognised (prompt: |-)" { + # YAML block-scalar headers like `|-`, `|+`, `>-`, `>+` must be detected + # as prompt markers so their bodies are linted. Caught by CodeRabbit on PR #85. + write_yml "${TT_TMP}/chomping.yml" <<'YML' +jobs: + analyze: + steps: + - uses: anthropics/claude-code-action@v1 + with: + prompt: |- + Date: $(date -u +%Y-%m-%d) +YML + run bash "$LINTER" "${TT_TMP}/chomping.yml" + [ "$status" -eq 1 ] +} + +@test "lint-prompt: GitHub Actions expressions with nested braces do not false-positive" { + # ${{ format('${FOO}', github.ref) }} should be stripped entirely so ${FOO} + # inside the expression is not flagged as a bare shell expansion. + # Caught by CodeRabbit review on PR petry-projects/.github#85. + write_yml "${TT_TMP}/nested-expr.yml" <<'YML' +jobs: + analyze: + steps: + - uses: anthropics/claude-code-action@v1 + with: + prompt: | + Ref: ${{ format('{0}', github.ref) }} +YML + run bash "$LINTER" "${TT_TMP}/nested-expr.yml" + [ "$status" -eq 0 ] +} diff --git a/test/workflows/feature-ideation/match-discussions.bats b/test/workflows/feature-ideation/match-discussions.bats index 9b63ea9a..b92bce07 100644 --- a/test/workflows/feature-ideation/match-discussions.bats +++ b/test/workflows/feature-ideation/match-discussions.bats @@ -192,6 +192,22 @@ build_proposals() { [ "$status" -eq 0 ] } +@test "match: malformed JSON in signals file exits non-zero" { + # Caught by CodeRabbit review on PR petry-projects/.github#85. + printf 'not valid json' >"${TT_TMP}/signals.json" + build_proposals '[{"title":"x"}]' + run bash "$MATCH" "${TT_TMP}/signals.json" "${TT_TMP}/proposals.json" + [ "$status" -ne 0 ] +} + +@test "match: malformed JSON in proposals file exits non-zero" { + # Caught by CodeRabbit review on PR petry-projects/.github#85. + build_signals '[]' + printf 'not valid json' >"${TT_TMP}/proposals.json" + run bash "$MATCH" "${TT_TMP}/signals.json" "${TT_TMP}/proposals.json" + [ "$status" -ne 0 ] +} + @test "match: idempotent re-run of same proposals against existing matches yields all-matched" { # Simulates the R6 idempotency case: run 1 created Discussions, run 2 # finds them and should NOT propose duplicates. diff --git a/test/workflows/feature-ideation/signals-schema.bats b/test/workflows/feature-ideation/signals-schema.bats index 0281d589..4ad05181 100644 --- a/test/workflows/feature-ideation/signals-schema.bats +++ b/test/workflows/feature-ideation/signals-schema.bats @@ -69,6 +69,15 @@ FIX="${TT_FIXTURES_DIR}/expected" [ "$status" -eq 0 ] } +@test "schema: malformed JSON signals file FAILS with exit code 2" { + # Validates that the OSError/JSONDecodeError path returns 2 (file/data error) + # not 1 (schema validation error). Caught by CodeRabbit review on PR #85. + bad_file="${BATS_TEST_TMPDIR}/malformed.json" + printf '{"schema_version":"1.0.0",' >"$bad_file" + run python3 "$VALIDATOR" "$bad_file" "$SCHEMA" + [ "$status" -eq 2 ] +} + @test "schema: SCHEMA_VERSION constant matches schema file version comment" { # CodeRabbit on PR petry-projects/.github#85: enforce that the # SCHEMA_VERSION constant in collect-signals.sh stays in lockstep with diff --git a/test/workflows/feature-ideation/stubs/gh b/test/workflows/feature-ideation/stubs/gh index 2e8cd24d..a16cc61e 100755 --- a/test/workflows/feature-ideation/stubs/gh +++ b/test/workflows/feature-ideation/stubs/gh @@ -31,8 +31,11 @@ if [ -n "${GH_STUB_LOG:-}" ]; then fi # Multi-call mode. +# NOTE: TT_TMP (or BATS_TEST_TMPDIR as fallback) must be set so the counter +# file is isolated per test. Without it the fallback /tmp path can collide +# across parallel test suites. Caught by CodeRabbit review on PR #85. if [ -n "${GH_STUB_SCRIPT:-}" ] && [ -f "${GH_STUB_SCRIPT}" ]; then - counter_file="${TT_TMP:-/tmp}/.gh-stub-counter" + counter_file="${TT_TMP:-${BATS_TEST_TMPDIR:-/tmp}}/.gh-stub-counter" count=0 if [ -f "$counter_file" ]; then count=$(cat "$counter_file") From ddad755bcbc6fded7419cfecc1acc6d4487eac53 Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Mon, 11 May 2026 16:27:40 -0500 Subject: [PATCH 6/6] fix(feature-ideation): simplify error-envelope check and harden gh stub Collapse the redundant outer+inner jq guard in gh_safe_graphql into the single-expression form already used by gh_safe_graphql_input, making both functions consistent. Add a fail-fast check to the gh stub so that setting GH_STUB_SCRIPT to a nonexistent path produces an immediate error instead of silently falling through to single-call mode and masking test misconfiguration. Add a bats test that pins the new behaviour. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .github/scripts/feature-ideation/lib/gh-safe.sh | 12 +++++------- test/workflows/feature-ideation/gh-safe.bats | 11 +++++++++++ test/workflows/feature-ideation/stubs/gh | 4 ++++ 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/.github/scripts/feature-ideation/lib/gh-safe.sh b/.github/scripts/feature-ideation/lib/gh-safe.sh index 11a4758e..8060c5c4 100755 --- a/.github/scripts/feature-ideation/lib/gh-safe.sh +++ b/.github/scripts/feature-ideation/lib/gh-safe.sh @@ -139,13 +139,11 @@ gh_safe_graphql() { fi # Reject error envelopes — GraphQL returns 200 OK even on partial errors. - if printf '%s' "$raw" | jq -e '.errors // empty | if . then true else false end' >/dev/null 2>&1; then - if printf '%s' "$raw" | jq -e '(.errors | type) == "array" and (.errors | length > 0)' >/dev/null 2>&1; then - local errs - errs=$(printf '%s' "$raw" | jq -c '.errors') - _gh_safe_err "graphql-errors" "$errs" - return 66 # EX_NOINPUT — repurposed: "GraphQL refused our request" - fi + if printf '%s' "$raw" | jq -e '(.errors // empty | type) == "array" and (.errors | length > 0)' >/dev/null 2>&1; then + local errs + errs=$(printf '%s' "$raw" | jq -c '.errors') + _gh_safe_err "graphql-errors" "$errs" + return 66 # EX_NOINPUT — repurposed: "GraphQL refused our request" fi # Reject `data: null` — usually means the path didn't resolve (permissions, diff --git a/test/workflows/feature-ideation/gh-safe.bats b/test/workflows/feature-ideation/gh-safe.bats index b06bb5c7..046446d7 100644 --- a/test/workflows/feature-ideation/gh-safe.bats +++ b/test/workflows/feature-ideation/gh-safe.bats @@ -222,3 +222,14 @@ teardown() { run gh_safe_graphql_input '{"query":"q","variables":{}}' [ "$status" -ne 0 ] } + +# --------------------------------------------------------------------------- +# gh stub — self-tests for stub defensive behaviour +# --------------------------------------------------------------------------- + +@test "gh stub: fails fast when GH_STUB_SCRIPT is set but file is missing" { + GH_STUB_SCRIPT="/nonexistent/path/script.sh" \ + run --separate-stderr "${TT_TMP}/bin/gh" issue list + [ "$status" -ne 0 ] + [[ "$stderr" == *"not found"* ]] +} diff --git a/test/workflows/feature-ideation/stubs/gh b/test/workflows/feature-ideation/stubs/gh index a16cc61e..27584df2 100755 --- a/test/workflows/feature-ideation/stubs/gh +++ b/test/workflows/feature-ideation/stubs/gh @@ -34,6 +34,10 @@ fi # NOTE: TT_TMP (or BATS_TEST_TMPDIR as fallback) must be set so the counter # file is isolated per test. Without it the fallback /tmp path can collide # across parallel test suites. Caught by CodeRabbit review on PR #85. +if [ -n "${GH_STUB_SCRIPT:-}" ] && [ ! -f "${GH_STUB_SCRIPT}" ]; then + printf '[gh-stub] GH_STUB_SCRIPT is set but file not found: %s\n' "$GH_STUB_SCRIPT" >&2 + exit 1 +fi if [ -n "${GH_STUB_SCRIPT:-}" ] && [ -f "${GH_STUB_SCRIPT}" ]; then counter_file="${TT_TMP:-${BATS_TEST_TMPDIR:-/tmp}}/.gh-stub-counter" count=0