diff --git a/.github/workflows/prod-release-notes.yaml b/.github/workflows/prod-release-notes.yaml index d91234804e..a62e34fcbc 100644 --- a/.github/workflows/prod-release-notes.yaml +++ b/.github/workflows/prod-release-notes.yaml @@ -1,43 +1,16 @@ name: "Prod Release Notes" -# Fires when the prod rollout-complete AnalysisTemplate (see -# helm-chart/sefaria/templates/analysistemplate/rollout-complete.yaml) confirms -# Argo Rollouts' post-promotion analysis passed for prod — i.e. the new -# revision's pods are up and its health checks are green. That job relays a -# repository_dispatch here, carrying the deployed `version` and `chartVersion`. -# -# From there this workflow is a strict pipeline of deterministic scripts, with -# exactly one step handed to an LLM: -# 1. build/ci/shipped_stories.py resolves the prod/* tag range for that -# version (chartVersion disambiguates a chart-only rollout, where several -# tags share the same app version), walks git log between the tags, and -# hydrates the Shortcut stories those commits reference. -# 2. build/ci/mark_stories_deployed.py moves each of those stories from -# Deploy Ready -> Done via the Shortcut API. A failure here (missing -# token, API error, nothing to move) is surfaced as a warning but never -# blocks steps 3-4 — see "Mark shipped stories as deployed" below. -# 3. The sefaria-release-notes skill (.claude/skills/sefaria-release-notes/) -# reads that JSON and writes prose only — it does not talk to GitHub or -# Shortcut. -# 4. scripts/post_to_slack.py posts both generated files to Slack. -# -# Manual setup this depends on: see build/ci/README-prod-release-notes.md +# Fires when the prod rollout-complete AnalysisTemplate confirms the +# post-promotion analysis passed, via repository_dispatch carrying +# `version` and `chartVersion`. See build/ci/README-prod-release-notes.md. on: repository_dispatch: types: [prod-rollout-succeeded] - # repository_dispatch only ever runs the DEFAULT branch's copy of a - # workflow, so without workflow_dispatch there is no way to exercise this - # path at all before a change here merges, and no way to re-run it for a - # past release (e.g. after fixing a bug in one of the scripts below). + # workflow_dispatch lets this be re-run manually / tested off the default branch. workflow_dispatch: inputs: - # rollout-complete.yaml strips a leading "v" before building the - # repository_dispatch payload, so `inputs.version` is expected bare - # too (e.g. 6.111.0-prod.2, not v6.111.0-prod.2) -- both so the - # concurrency group below dedupes correctly across the two trigger - # paths, and for consistency with the dispatch payload. The Resolve - # step still strips a leading "v" defensively if one sneaks in. + # inputs.version is expected bare (no leading "v"), matching the dispatch payload. version: description: "Deployed prod version, e.g. 6.111.0-prod.2 (bare, no leading v)" required: true @@ -51,18 +24,13 @@ on: required: false type: boolean default: true + explain_triage: + description: "Opt-in: run the headless triage explainer over the reconcile sweep's triage bucket only (requires ANTHROPIC_API_KEY; never mutates Shortcut)" + required: false + type: boolean + default: false -# A pod retry (the AnalysisTemplate Job has backoffLimit: 1) can re-fire the -# repository_dispatch for the same version, and a manual workflow_dispatch -# re-run could race a real dispatch for that same version. Serialize on -# version so duplicate runs queue instead of double-posting to Slack. -# -# This group key is built from the RAW trigger value, before the Resolve -# step below can strip a leading "v" -- GitHub Actions expressions have no -# string-replace function, so normalization has to happen at the source -# instead (rollout-complete.yaml strips "v" in the dispatch payload) for -# repository_dispatch and workflow_dispatch to actually dedupe against each -# other for the same release. +# Serializes on version so a retry or manual re-run doesn't double-post to Slack. concurrency: group: prod-release-notes-${{ github.event.client_payload.version || inputs.version }} cancel-in-progress: false @@ -78,11 +46,18 @@ jobs: - uses: actions/checkout@v4 with: fetch-depth: 0 - # shipped_stories.py resolves the shipped commit range from prod/* - # git tags, so the checkout needs the tags themselves, not just full - # commit history. + # Needed for shipped_stories.py's prod/* tag resolution. fetch-tags: true + # Shared by both the release-notes and triage-explainer Claude Code steps. + - name: Set up Node + uses: actions/setup-node@v4 + with: + node-version: "20" + + - name: Install Claude Code CLI + run: npm install -g @anthropic-ai/claude-code@2.1.263 + - name: Resolve version id: resolve env: @@ -99,11 +74,7 @@ jobs: CHART_VERSION="$DISPATCH_CHART_VERSION" fi - # rollout-complete.yaml's `version` arg is expected bare (it strips - # a leading "v" before building the dispatch payload -- see the - # concurrency.group comment above), but this strip stays as a - # defensive fallback in case a leading "v" ever sneaks in (e.g. a - # manual workflow_dispatch typo). + # Defensive fallback in case a leading "v" sneaks in. VERSION="${RAW_VERSION#v}" if [[ -z "$VERSION" ]]; then @@ -131,12 +102,7 @@ jobs: - name: Mark shipped stories as deployed id: mark_deployed - # A Shortcut bookkeeping failure (missing token, API error, or the - # silent-no-op guard's sys.exit(2)) must never skip release-notes - # generation and Slack posting for an otherwise healthy rollout -- - # moving stories to Done and announcing the release are independent - # concerns. continue-on-error surfaces the failure via - # steps.mark_deployed.outcome below instead of failing the job. + # A bookkeeping failure must never block release-notes generation and Slack posting. continue-on-error: true env: SHORTCUT_API_TOKEN: ${{ secrets.SHORTCUT_API_TOKEN }} @@ -149,28 +115,135 @@ jobs: fi python3 build/ci/mark_stories_deployed.py "${ARGS[@]}" + - name: Reconcile Deploy Ready backlog + id: reconcile + # Report goes to $RUNNER_TEMP, not the checkout, so it can't leak into the prose step below. + continue-on-error: true + env: + SHORTCUT_API_TOKEN: ${{ secrets.SHORTCUT_API_TOKEN }} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + EVENT_NAME: ${{ github.event_name }} + DRY_RUN_INPUT: ${{ inputs.dry_run }} + run: | + CURRENT_TAG=$(python3 -c "import json; print(json.load(open('shipped-stories.json'))['range']['current_tag'])") + ARGS=(--out "$RUNNER_TEMP/reconcile-deploy-ready-report.json" --prod-tag "$CURRENT_TAG") + if [[ "$EVENT_NAME" != "workflow_dispatch" || "$DRY_RUN_INPUT" != "true" ]]; then + ARGS+=(--apply) + fi + python3 build/ci/reconcile_deploy_ready.py "${ARGS[@]}" + + - name: Merge current-release backfill into shipped-stories.json + id: merge_backfill + # Must run after "Reconcile Deploy Ready backlog" and before "Generate release notes". + continue-on-error: true + run: | + REPORT_FILE="$RUNNER_TEMP/reconcile-deploy-ready-report.json" + if [[ ! -s "$REPORT_FILE" ]]; then + echo "No reconcile report found at $REPORT_FILE (the reconcile step may have failed or produced nothing) -- nothing to merge." + exit 0 + fi + python3 build/ci/merge_release_backfill.py \ + --shipped-stories-out shipped-stories.json \ + --reconcile-report "$REPORT_FILE" + - name: Warn if Shortcut bookkeeping failed - if: steps.mark_deployed.outcome == 'failure' + if: steps.mark_deployed.outcome == 'failure' || steps.reconcile.outcome == 'failure' || steps.merge_backfill.outcome == 'failure' + env: + MARK_DEPLOYED_OUTCOME: ${{ steps.mark_deployed.outcome }} + RECONCILE_OUTCOME: ${{ steps.reconcile.outcome }} + MERGE_BACKFILL_OUTCOME: ${{ steps.merge_backfill.outcome }} run: | - echo "::warning::mark_stories_deployed.py failed -- shipped Shortcut stories were NOT moved Deploy Ready -> Done for this release. Release notes generation is continuing anyway. Check this run's 'Mark shipped stories as deployed' step and move the affected stories manually." + if [[ "$MARK_DEPLOYED_OUTCOME" == "failure" ]]; then + echo "::warning::mark_stories_deployed.py failed -- shipped Shortcut stories were NOT moved Deploy Ready -> Done for this release. Release notes generation is continuing anyway. Check this run's 'Mark shipped stories as deployed' step and move the affected stories manually." + fi + if [[ "$RECONCILE_OUTCOME" == "failure" ]]; then + echo "::warning::reconcile_deploy_ready.py failed -- the org-wide Deploy Ready backlog sweep did not complete. Release notes generation is continuing anyway. Check this run's 'Reconcile Deploy Ready backlog' step; the backlog will be retried on the next release." + fi + if [[ "$MERGE_BACKFILL_OUTCOME" == "failure" ]]; then + echo "::warning::merge_release_backfill.py failed -- a backfilled story that shipped in this release may be missing from the announcement. Check this run's 'Merge current-release backfill into shipped-stories.json' step." + fi - name: Alert Slack if Shortcut bookkeeping failed - if: steps.mark_deployed.outcome == 'failure' + if: steps.mark_deployed.outcome == 'failure' || steps.reconcile.outcome == 'failure' || steps.merge_backfill.outcome == 'failure' env: SLACK_WEBHOOK_URL: ${{ secrets.SLACK_DEPLOY_WEBHOOK }} VERSION: ${{ steps.resolve.outputs.version }} + MARK_DEPLOYED_OUTCOME: ${{ steps.mark_deployed.outcome }} + RECONCILE_OUTCOME: ${{ steps.reconcile.outcome }} + MERGE_BACKFILL_OUTCOME: ${{ steps.merge_backfill.outcome }} run: | + FAILURES="" + if [[ "$MARK_DEPLOYED_OUTCOME" == "failure" ]]; then + FAILURES="${FAILURES}mark_stories_deployed.py failed (this release's shipped stories NOT moved Deploy Ready -> Done). " + fi + if [[ "$RECONCILE_OUTCOME" == "failure" ]]; then + FAILURES="${FAILURES}reconcile_deploy_ready.py failed (org-wide Deploy Ready backlog sweep did not complete). " + fi + if [[ "$MERGE_BACKFILL_OUTCOME" == "failure" ]]; then + FAILURES="${FAILURES}merge_release_backfill.py failed (a backfilled story may be missing from the announcement). " + fi curl -s -X POST -H 'Content-type: application/json' \ - --data "{\"text\": \":warning: Prod release notes for version ${VERSION:-unknown}: mark_stories_deployed.py failed -- shipped Shortcut stories were NOT moved Deploy Ready -> Done. Release notes generation is continuing. Check the workflow run and move the affected stories manually.\"}" \ + --data "{\"text\": \":warning: Prod release notes for version ${VERSION:-unknown}: ${FAILURES}Release notes generation is continuing. Check the workflow run and move the affected stories manually.\"}" \ "$SLACK_WEBHOOK_URL" || true - - name: Set up Node - uses: actions/setup-node@v4 - with: - node-version: "20" + - name: Resolve triage explainer opt-in + id: triage_opt_in + env: + EVENT_NAME: ${{ github.event_name }} + EXPLAIN_TRIAGE_INPUT: ${{ inputs.explain_triage }} + # Repo-level Actions variable, not a secret. + ENABLE_TRIAGE_EXPLAINER_VAR: ${{ vars.ENABLE_TRIAGE_EXPLAINER }} + run: | + ENABLED=$(python3 build/ci/triage_explainer.py resolve-enabled \ + --event-name "$EVENT_NAME" \ + --explain-triage-input "$EXPLAIN_TRIAGE_INPUT" \ + --enable-var "$ENABLE_TRIAGE_EXPLAINER_VAR") + echo "enabled=${ENABLED}" >> "$GITHUB_OUTPUT" - - name: Install Claude Code CLI - run: npm install -g @anthropic-ai/claude-code + - name: Explain Deploy Ready triage backlog (optional) + id: explain_triage + if: steps.triage_opt_in.outputs.enabled == 'true' + # Opt-in and best-effort; never blocks the release announcement. + continue-on-error: true + # Input is contributor-controlled text; cwd here (not the checkout) + # is what actually bounds the step's Write grant. + working-directory: ${{ runner.temp }} + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + run: | + if [[ -z "$ANTHROPIC_API_KEY" ]]; then + echo "ANTHROPIC_API_KEY is empty -- skipping the triage explainer. This is an opt-in convenience feature and degrades cleanly without a key; it never blocks the release announcement." + exit 0 + fi + + REPORT_FILE="$RUNNER_TEMP/reconcile-deploy-ready-report.json" + if [[ ! -s "$REPORT_FILE" ]]; then + echo "No reconcile report found at $REPORT_FILE (the reconcile step may have failed or produced nothing) -- skipping the triage explainer." + exit 0 + fi + + TRIAGE_ONLY_FILE="$RUNNER_TEMP/reconcile-triage-only.json" + python3 "$GITHUB_WORKSPACE/build/ci/triage_explainer.py" extract --report "$REPORT_FILE" --out "$TRIAGE_ONLY_FILE" + + TRIAGE_COUNT=$(python3 -c "import json; print(json.load(open('$TRIAGE_ONLY_FILE'))['triage_count'])") + if [[ "$TRIAGE_COUNT" == "0" ]]; then + echo "No triage stories this run -- nothing for the explainer to explain." + exit 0 + fi + + ANNOTATED_FILE="$RUNNER_TEMP/reconcile-triage-annotated.json" + # --allowedTools, not --dangerously-skip-permissions: the input embeds + # contributor-controlled text, a prompt-injection path. No Bash/network tools. + claude -p "Read the JSON file at ${TRIAGE_ONLY_FILE}. It lists Shortcut stories an automated Deploy Ready reconciliation sweep could not classify as shipped or pending ('triage'), each with: name, description, comments, and (for a story with a linked PR that didn't qualify) linked_prs -- the PR number and exactly which shipping-evidence guard(s) it failed (not merged / wrong repo / wrong target branch), or (for a story on the wrong Shortcut workflow) its workflow_id/workflow_state_id. For EACH story in the 'triage' array, propose (a) a short hypothesis for why it is stuck, grounded ONLY in the fields given -- e.g. 'a linked PR targets preprod, not master -- likely a promotion PR was linked instead of the real feature PR', 'no PR is linked at all -- may have shipped via an unlinked PR, or a PR may still need to be opened', 'workflow/state ids do not match Standard -- this story may belong to a different team or process entirely', and (b) a short suggested next action for a human, e.g. 'search for the real feature PR and re-link it', 'no code artifact found -- consider closing manually', 'may belong to another repo's release train -- verify with that PR's author'. Do not invent PR numbers, dates, names, or any fact not present in the input. Do not recommend or imply any automatic action, transition, or comment -- every output is an unverified hypothesis for a human to check, never a finding. Prefix every hypothesis string with the literal text '[AI hypothesis, unverified] ' so it can never be mistaken for a verified cause. Write your ONLY output as JSON to ${ANNOTATED_FILE}: a list of objects, one per input triage story, each shaped {id, hypothesis, suggested_next_action}. Do not ask any clarifying questions -- proceed directly." \ + --allowedTools "Read,Write,Glob,Grep" + + if [[ -s "$ANNOTATED_FILE" ]]; then + echo "----- Triage explainer output (hypotheses only -- NOT verified findings) -----" + cat "$ANNOTATED_FILE" + echo "--------------------------------------------------------------------------------" + else + echo "Triage explainer produced no output file -- treating as a soft failure. Opt-in feature; this never blocks the run." + fi - name: Generate release notes (headless Claude Code) id: generate @@ -183,12 +256,8 @@ jobs: fi mkdir -p release-notes-output - # --allowedTools instead of --dangerously-skip-permissions: the - # input (shipped-stories.json) embeds contributor-controlled text - # (commit subjects, branch names, Shortcut free text), which is a - # prompt-injection path. This agent must never hold Bash or - # network tools, so the fix is least-privilege tool scoping at the - # provisioning layer, not content filtering. + # --allowedTools, not --dangerously-skip-permissions: shipped-stories.json + # embeds contributor-controlled text, a prompt-injection path. No Bash/network tools. claude -p "Use the sefaria-release-notes skill (.claude/skills/sefaria-release-notes/SKILL.md). Generate release notes from the shipped-stories JSON file at shipped-stories.json. Output directory: release-notes-output/. Do not ask any clarifying questions — proceed with defaults documented in the skill's Edge Cases table." \ --allowedTools "Read,Write,Glob,Grep" @@ -210,11 +279,7 @@ jobs: env: SLACK_WEBHOOK_URL: ${{ secrets.SLACK_PRODUCT_WEBHOOK }} run: | - # Guarded in the script body, not in `if:` — a step's own `env` block - # is not reliably visible to that same step's `if` expression, so - # `env.SLACK_PRODUCT_WEBHOOK != ''` there silently evaluates empty and - # the step never runs. Until the second webhook exists this is a - # deliberate no-op that must not fail the workflow. + # Guarded here, not in `if:` — a step's own `env` isn't reliably visible to its own `if`. if [[ -z "$SLACK_WEBHOOK_URL" ]]; then echo "SLACK_PRODUCT_WEBHOOK is not set — skipping the release announcement post." exit 0 diff --git a/build/ci/README-prod-release-notes.md b/build/ci/README-prod-release-notes.md index ebdee55e7c..7e4a759e53 100644 --- a/build/ci/README-prod-release-notes.md +++ b/build/ci/README-prod-release-notes.md @@ -1,81 +1,89 @@ # Prod rollout → Slack + release notes: manual setup -This repo's changes (below) are necessary but not sufficient — the following -still needs a human with real credentials, since none of it can be -generated or guessed by an agent. +This repo's changes are necessary but not sufficient — some setup below +requires a human with real credentials. -## How it works +## Pipeline ``` Argo post-promotion analysis (prod) -> repository_dispatch (prod-rollout-succeeded, carries `version` + `chartVersion`) - -> build/ci/shipped_stories.py — walks the prod/* tag range in git, - resolves Shortcut story codes from - commit subjects / merged-PR branch - names, hydrates story details - -> build/ci/mark_stories_deployed.py — moves each shipped story - Deploy Ready -> Done via the - Shortcut API. A failure here (missing - token, API error, nothing to move) is - logged and Slack-alerted but never - blocks the two steps below. - -> sefaria-release-notes skill — reads the JSON, writes prose only - -> scripts/post_to_slack.py — posts both files to Slack + -> build/ci/shipped_stories.py — resolves shipped Shortcut stories for the release + -> build/ci/mark_stories_deployed.py — moves those stories Deploy Ready -> Done + -> build/ci/reconcile_deploy_ready.py — org-wide Deploy Ready sweep (independent of this release) + -> build/ci/merge_release_backfill.py — folds any current-release backfill into shipped-stories.json + -> build/ci/triage_explainer.py — opt-in: proposes hypotheses for the reconcile sweep's triage bucket + + headless `claude -p` + -> sefaria-release-notes skill — reads shipped-stories.json, writes prose only + -> scripts/post_to_slack.py — posts both files to Slack ``` -Only the release-notes generation step is an LLM. Deciding which stories a given deploy closed -is a graph walk over git history and Shortcut IDs, and flipping their -workflow state is a for-loop over a REST API — neither of those is a job -for a model. The skill's only input is the JSON that -`shipped_stories.py` already produced; it does not call GitHub or Shortcut -itself, and it does not mutate any story. +Only the release-notes prose step and the opt-in triage explainer are LLM +steps; everything else is deterministic Python or a REST call. + +## Running the scripts + +``` +python3 build/ci/shipped_stories.py --version 6.111.0-prod.2 [--out shipped-stories.json] [--repo Sefaria/Sefaria-Project] [--chart-version 0.87.5-prod.1] +python3 build/ci/shipped_stories.py --range .. [--out shipped-stories.json] +``` +Requires `git` and `gh` on PATH. `SHORTCUT_API_TOKEN` is optional; without +it, story ids are still emitted but hydration and the PR-link fallback are +skipped. + +``` +python3 build/ci/mark_stories_deployed.py --input shipped-stories.json [--dry-run] \ + [--workflow-id 500000005] [--from-state-id 500000045] [--done-state-id 500000010] +``` +Requires `SHORTCUT_API_TOKEN` unless `--dry-run` is passed. + +``` +python3 build/ci/reconcile_deploy_ready.py [--dry-run] +python3 build/ci/reconcile_deploy_ready.py --apply +python3 build/ci/reconcile_deploy_ready.py --apply --prod-tag prod/6.111.0-prod.2+chart.0.87.5-prod.1 --out report.json +``` +`--dry-run` is the default; nothing is transitioned without `--apply`. +Requires `git`, `gh`, and `SHORTCUT_API_TOKEN` (required even for `--dry-run`). + +``` +python3 build/ci/merge_release_backfill.py \ + --shipped-stories-out shipped-stories.json \ + --reconcile-report reconcile-deploy-ready-report.json + # writes the merged result back to --shipped-stories-out by default; pass --out to write elsewhere +``` + +``` +python3 build/ci/triage_explainer.py extract --report reconcile-deploy-ready-report.json --out triage-only.json +python3 build/ci/triage_explainer.py resolve-enabled --event-name workflow_dispatch --explain-triage-input true --enable-var "" +``` ## What's already wired up in this repo - `helm-chart/sefaria/templates/analysistemplate/rollout-complete.yaml` — - a `notify-github` container, gated on `deployEnv == "production"` (the - prod HelmRelease sets `deployEnv: production`, not `"prod"`), fires a - `repository_dispatch` (`event_type: prod-rollout-succeeded`) once Argo's - post-promotion analysis confirms the rollout healthy. Reads a - `GH_DISPATCH_TOKEN` key from its OWN dedicated secret - (`.Values.secrets.ghDispatch.ref`, default `gh-dispatch-token`) — kept - separate from `local-settings-secrets` because that secret is mounted via - `envFrom` into every web/task/monitor/cronjob pod, which is far too broad - a blast radius for a GitHub PAT. + fires `repository_dispatch` (`prod-rollout-succeeded`) once Argo's + post-promotion analysis confirms the prod rollout healthy. Uses a + `GH_DISPATCH_TOKEN` from its own dedicated secret + (`.Values.secrets.ghDispatch.ref`, default `gh-dispatch-token`). - `.github/workflows/prod-release-notes.yaml` — listens for that dispatch - (or a manual `workflow_dispatch`), resolves the version (and optional - chart version, for disambiguating a chart-only rollout), runs - `shipped_stories.py` and `mark_stories_deployed.py`, runs the - `sefaria-release-notes` skill headlessly, and posts both output files to - Slack via `scripts/post_to_slack.py`. -- `.claude/skills/sefaria-release-notes/` — the skill, shipped in-repo, - now takes a shipped-stories JSON file as its only input and only writes - prose. It no longer talks to GitHub or Shortcut. -- **preprod needs no changes.** `rollout-complete-preprod` already exists - (same chart, templated per `deployEnv`) and already posts to Slack on a - successful preprod rollout — assuming its `SLACK_URL` is populated (see - below). + (or a manual `workflow_dispatch`), runs the pipeline above, and posts to + Slack. +- `.claude/skills/sefaria-release-notes/` — the release-notes skill, + shipped in-repo, takes a shipped-stories JSON file as its only input. +- Preprod needs no changes — `rollout-complete-preprod` already exists and + posts to Slack on a successful preprod rollout (as long as `SLACK_URL` + is populated; see below). ## Still required — infrastructure repo (SOPS-encrypted secret) -**This is the step that makes the whole pipeline live — without it, -`GH_DISPATCH_TOKEN` is simply absent, the dispatch curl gets a 401, falls -through `|| /bin/true`, and the entire feature is a silent no-op with -nothing failing anywhere.** - 1. Create a GitHub PAT scoped to `Sefaria/Sefaria-Project` only — - fine-grained, **Contents: read and write** permission (required for the - `repository_dispatch` API endpoint; this token never needs push/admin - access, it only fires a dispatch event). -2. SOPS-encrypt it into the `infrastructure` repo as its OWN dedicated - Secret (NOT `local-settings-secrets` — that secret is mounted into every - pod in the deployment; see above) under key `GH_DISPATCH_TOKEN`. This - repo's `envs/prod/helmrelease.yaml` already points - `secrets.ghDispatch.ref` at `gh-dispatch-token-production`; the - infrastructure repo needs to create a Secret with that exact name. -3. Confirm `flux reconcile` picks it up (or wait for the next 5-minute - poll) so the key exists on the `rollout-complete-production` Job's pod - before the next prod rollout. + fine-grained, **Contents: read and write** permission. +2. SOPS-encrypt it into the `infrastructure` repo as its own dedicated + Secret under key `GH_DISPATCH_TOKEN`, named `gh-dispatch-token-production` + to match `envs/prod/helmrelease.yaml`'s `secrets.ghDispatch.ref`. +3. Confirm `flux reconcile` picks it up before the next prod rollout. + +Without this, `GH_DISPATCH_TOKEN` is absent, the dispatch curl 401s and +falls through `|| /bin/true`, and the whole feature is a silent no-op. ## Still required — Sefaria-Project GitHub Actions secrets @@ -83,53 +91,55 @@ Add these under repo Settings → Secrets and variables → Actions: | Secret | Purpose | Notes | |---|---|---| -| `SHORTCUT_API_TOKEN` | `shipped_stories.py` story hydration and `mark_stories_deployed.py` state transitions | Shortcut → Settings → API Tokens. Not the same as the OAuth MCP connection used interactively. | -| `SLACK_PRODUCT_WEBHOOK` | Non-technical release announcement | A second Slack incoming webhook, pointed at whichever channel should get `release-announcement-product-slack.txt`. Until this is set, that post step is a guarded no-op (won't fail the workflow). | +| `SHORTCUT_API_TOKEN` | Story hydration, PR-link fallback, and state transitions for all three CI scripts | Shortcut → Settings → API Tokens | +| `SLACK_PRODUCT_WEBHOOK` | Non-technical release announcement | A second Slack incoming webhook. Until set, that post step is a guarded no-op | Already exist and are reused as-is: `SLACK_DEPLOY_WEBHOOK`, `GITHUB_TOKEN`, `ANTHROPIC_API_KEY`. +Optional — to have the triage explainer opt in automatically on the real +`repository_dispatch` trigger (a manual `workflow_dispatch` run can already +opt in per-run via its `explain_triage` input): add a repo-level Actions +**Variable** (not Secret) named `ENABLE_TRIAGE_EXPLAINER` set to `true`. + ## Worth verifying, not something this session could check `SLACK_URL` in `local-settings-secrets` — confirm it's actually populated -for **both** `preprod` and `prod` (not just present as a key). The existing -`rollout-complete` Slack ping silently no-ops if it's empty or missing -(`optional: true`), so a misconfigured value wouldn't surface as an error -anywhere — it would just be quiet. +for both `preprod` and `prod`. The existing `rollout-complete` Slack ping +silently no-ops if it's empty or missing (`optional: true`). -## End-to-end verification, once the above is done +## End-to-end verification -1. **Dry-run the whole pipeline without a real deploy.** Run: +1. Dry-run the whole pipeline without a real deploy: ``` gh workflow run "Prod Release Notes" -f version= -f dry_run=true ``` - Add `-f chart_version=` if that app version has more than - one `prod/*` tag (a chart-only rollout) and you need a specific one. + Add `-f chart_version=` for a chart-only rollout. - This exercises tag-range resolution, story hydration, release-notes - generation, and both Slack posts, with `mark_stories_deployed.py` run in - `--dry-run` mode so nothing in Shortcut actually moves. It's the fastest - way to validate a change to any of the scripts or the skill without - waiting on a real rollout. +2. Then confirm the real trigger path: promote something small through to + prod normally. +3. Watch for the existing terse Slack ping from `rollout-complete-production`. +4. Watch the `Prod Release Notes` GitHub Actions workflow run. If it + doesn't fire, check the `notify-github` container's logs on the + `rollout-complete-production` Job pod for the dispatch curl's + exit/response. +5. Confirm both Slack files post correctly, and confirm the shipped + stories actually moved Deploy Ready → Done in Shortcut. -2. **Then confirm the real trigger path.** Promote something small through - to prod normally. +## Running the tests -3. Watch for the existing terse Slack ping from `rollout-complete-production` - (confirms the AnalysisTemplate ran and Slack posting works at all). +``` +python3 -m pytest build/ci/tests/ -q -p no:django -c /dev/null +``` -4. Watch the `Prod Release Notes` GitHub Actions workflow run - (`repository_dispatch` → `prod-rollout-succeeded`). If it doesn't fire, - check the `notify-github` container's logs on the `rollout-complete-production` - Job pod (`kubectl logs -n default -l job-name=...`) for the dispatch - curl's exit/response. The curl runs with `-sS -f --max-time 30 - --connect-timeout 10`, so a bad/missing token (HTTP 401/403) or a - timeout now prints to stderr in the pod logs — but the call still falls - through `|| /bin/true` by design so it never blocks or fails the - rollout, which means a failure here still won't surface anywhere except - those logs unless you go look. +Both `-c /dev/null` and `-p no:django` are needed: the repo-root +`pytest.ini` sets `DJANGO_SETTINGS_MODULE`, which makes `pytest-django` try +to `django.setup()` the whole app even when only these standalone, +stdlib-only scripts' tests are selected. `-c /dev/null` stops that ini from +being read; `-p no:django` disables the plugin as a second line of defense. -5. Confirm both Slack files post correctly, and confirm the shipped - stories actually moved Deploy Ready → Done in Shortcut. +No network access, `git`, or `gh` binary is required — everything that +would otherwise shell out or call the Shortcut API is monkeypatched in the +tests. diff --git a/build/ci/mark_stories_deployed.py b/build/ci/mark_stories_deployed.py index 97e4ab09bd..f414dcef77 100755 --- a/build/ci/mark_stories_deployed.py +++ b/build/ci/mark_stories_deployed.py @@ -1,57 +1,12 @@ #!/usr/bin/env python3 """ -Deterministically transition shipped Shortcut stories from "Deploy Ready" to "Done". - -Reads a shipped-stories JSON file (as produced by shipped_stories.py) and, for -each entry in its "stories" list whose workflow_id equals --workflow-id AND -workflow_state_id equals --from-state-id, PUTs a workflow_state_id update to ---done-state-id via the Shortcut API. Stories already at the done state are -skipped and logged. Stories sitting in any other state of --workflow-id are -skipped and logged. Stories belonging to a DIFFERENT workflow than ---workflow-id are skipped and logged separately, because --from-state-id and ---done-state-id are workflow-specific: the Sefaria org has roughly ten -Shortcut workflows, and a state id that means "Deploy Ready" in one workflow -can be meaningless (or mean something else entirely) in another. Silently -lumping those stories into "skipped, some other state" is exactly the -unattended-run failure mode this script exists to avoid — a shipped story in -a non-Standard workflow must never look indistinguishable from "nothing -needed moving". No story is ever mutated in any of these three skip cases. +Transitions shipped Shortcut stories from "Deploy Ready" to "Done". Usage: python3 mark_stories_deployed.py --input shipped-stories.json [--dry-run] \ [--workflow-id 500000005] [--from-state-id 500000045] [--done-state-id 500000010] ---dry-run prints exactly what would be transitioned and mutates nothing; it -does not require SHORTCUT_API_TOKEN. Without --dry-run, a missing -SHORTCUT_API_TOKEN is a hard error (exit 1) raised before any story is -touched. A per-story API failure is logged and never aborts the loop and -never fails the process as a whole — the deploy already happened, so this -script must never be the thing that turns a green rollout red. - -Prints a summary JSON at the end with counts and ids for each bucket: -transitioned, already_done, skipped_other_state, skipped_different_workflow, -failed — plus a skipped_detail list carrying each skipped story's id, -workflow_id, workflow_state_id and the reason it was skipped, and hydrated / -unresolved_story_ids echoed straight from the input file so stage 3 doesn't -silently lose stories that shipped but that shipped_stories.py could not -look up. - -Whenever the input's "stories" list is non-empty and at least one story was -skipped for a reason other than already being Done, this script prints a -WARNING to stderr naming those stories -- even in a MIXED release where some -other stories transitioned fine, so a partial skip is never invisible just -because the run "worked". If, in addition, NOTHING was transitioned at all, -that combination is a silent no-op -- the exact failure mode this script -exists to prevent -- and, unless --dry-run was passed, the process exits -non-zero so an unattended run can't look successful when it silently did -nothing. (--dry-run never exits non-zero for this: it is a preview, and -dry-run "transitioned" is inherently just a list of candidates, not evidence -anything actually happened or failed to.) - -All ids shown in this file's docstring and comments (e.g. story id 22222) -are placeholders, not real Shortcut story ids. The workflow and state ids -(500000005, 500000045, 500000010, 500000728) are real Shortcut workflow/ -state ids, not story ids, and are not covered by that placeholder rule. +Requires SHORTCUT_API_TOKEN unless --dry-run is passed. """ import argparse @@ -64,12 +19,7 @@ SHORTCUT_API_BASE = "https://api.app.shortcut.com/api/v3" -# Sefaria's "Standard" Shortcut workflow: "Deploy Ready" -> "Done". These are -# used as defaults so the script is runnable without extra flags in the -# common case; pass --workflow-id/--from-state-id/--done-state-id explicitly -# to override for a different workflow. The org has roughly ten workflows, -# each with its own state ids — e.g. at least one other workflow has its own -# Done state, 500000728, which is NOT the same as DEFAULT_DONE_STATE_ID. +# Sefaria's "Standard" Shortcut workflow: "Deploy Ready" -> "Done". DEFAULT_WORKFLOW_ID = 500000005 DEFAULT_FROM_STATE_ID = 500000045 DEFAULT_DONE_STATE_ID = 500000010 @@ -85,17 +35,7 @@ def warn(message: str) -> None: def classify_stories(stories, workflow_id, from_state_id, done_state_id): - """Split stories into (to_transition, already_done, skipped_other_state, - skipped_different_workflow) buckets. Pure function, no I/O. - - Workflow membership is checked BEFORE state, so a story sitting at a - different workflow's own Done state (e.g. 500000728) is classified as - skipped_different_workflow, not already_done — those state ids are not - interchangeable across workflows. A story with no workflow_id on record - (e.g. from a shipped-stories.json produced before this field existed) - falls through to the state-only checks instead of being flagged as a - mismatch, so older input files don't spuriously warn on every story. - """ + """Split stories into (to_transition, already_done, skipped_other_state, skipped_different_workflow) buckets.""" to_transition = [] already_done = [] skipped_other_state = [] @@ -156,8 +96,7 @@ def _ids(stories): def _skipped_detail(skipped_other_state, skipped_different_workflow): - """Build the human-readable detail list for every skipped (non-already-done) - story: id, workflow_id, workflow_state_id and why it was skipped.""" + """Build the detail list for every skipped (non-already-done) story: id, workflow_id, workflow_state_id, and reason.""" detail = [] for story in skipped_different_workflow: detail.append({ @@ -254,13 +193,6 @@ def main(): print(json.dumps(summary, indent=2)) - # Make silent skips impossible: any story skipped for a reason other than - # already being Done must be visible, whether or not anything else in - # this run transitioned. A MIXED release -- some stories transition - # fine, others get skipped for a different workflow/state -- used to - # produce no WARNING at all as long as `transitioned` was non-empty; that - # silently lost visibility into exactly the stories this script exists - # to flag. Only the "nothing happened at all" case is fatal. if stories and skipped_detail: skipped_ids_and_context = [ f"{d['id']} (workflow_id={d['workflow_id']}, workflow_state_id={d['workflow_state_id']}, " diff --git a/build/ci/merge_release_backfill.py b/build/ci/merge_release_backfill.py new file mode 100644 index 0000000000..68a9f48092 --- /dev/null +++ b/build/ci/merge_release_backfill.py @@ -0,0 +1,98 @@ +#!/usr/bin/env python3 +""" +Merges reconcile_deploy_ready.py's "shipped in this release" backfill +stories into shipped_stories.py's own output, for the release-notes prose +step. Only reads the pre-filtered `hydrated_story` entries from the +reconcile report; never touches `shipped`/`pending`/`triage` wholesale. + +Usage: + python3 merge_release_backfill.py --shipped-stories-out shipped-stories.json \ + --reconcile-report reconcile-deploy-ready-report.json [--out shipped-stories.json] +""" + +import argparse +import json +import sys + + +def die(message: str) -> None: + print(f"ERROR: {message}", file=sys.stderr) + sys.exit(1) + + +def warn(message: str) -> None: + print(f"WARNING: {message}", file=sys.stderr) + + +def backfill_stories_from_report(report): + """Every `hydrated_story` already attached to report['shipped'] -- entries with none are skipped.""" + return [s["hydrated_story"] for s in (report.get("shipped") or []) if s.get("hydrated_story")] + + +def merge(shipped_stories_data, backfill_stories): + """Fold `backfill_stories` into `shipped_stories_data`'s `story_ids` / `stories`, deduplicated by story id. Returns (merged_data, added_ids).""" + existing_ids = set(str(sid) for sid in (shipped_stories_data.get("story_ids") or [])) + stories = list(shipped_stories_data.get("stories") or []) + added_ids = [] + + for story in backfill_stories: + sid = str(story.get("id")) + if sid in existing_ids: + continue + existing_ids.add(sid) + added_ids.append(sid) + stories.append(story) + + shipped_stories_data["story_ids"] = sorted(existing_ids, key=int) + shipped_stories_data["stories"] = stories + shipped_stories_data["stories_from_reconciliation_backfill"] = sorted(added_ids, key=int) + + return shipped_stories_data, added_ids + + +def build_arg_parser(): + parser = argparse.ArgumentParser( + description="Merge reconcile_deploy_ready.py's current-release backfill stories " + "into shipped_stories.py's own output, before the release-notes prose step runs.", + ) + parser.add_argument("--shipped-stories-out", required=True, help="Path to shipped_stories.py's own --out file") + parser.add_argument("--reconcile-report", required=True, help="Path to reconcile_deploy_ready.py's own --out file") + parser.add_argument( + "--out", + help="Where to write the merged result (default: overwrite --shipped-stories-out in place)", + ) + return parser + + +def main(): + args = build_arg_parser().parse_args() + + try: + with open(args.shipped_stories_out, encoding="utf-8") as f: + shipped_stories_data = json.load(f) + except (OSError, json.JSONDecodeError) as e: + die(f"Could not read/parse --shipped-stories-out {args.shipped_stories_out!r}: {e}") + + try: + with open(args.reconcile_report, encoding="utf-8") as f: + report = json.load(f) + except (OSError, json.JSONDecodeError) as e: + die(f"Could not read/parse --reconcile-report {args.reconcile_report!r}: {e}") + + backfill_stories = backfill_stories_from_report(report) + merged, added_ids = merge(shipped_stories_data, backfill_stories) + + out_path = args.out or args.shipped_stories_out + with open(out_path, "w", encoding="utf-8") as f: + json.dump(merged, f, indent=2, ensure_ascii=False) + f.write("\n") + + if added_ids: + print(f"Merged {len(added_ids)} backfilled stor{'y' if len(added_ids) == 1 else 'ies'} " + f"that shipped in the current release: {added_ids}") + else: + print("No backfilled stories belong to the current release -- nothing to merge.") + + +if __name__ == "__main__": + main() diff --git a/build/ci/reconcile_deploy_ready.py b/build/ci/reconcile_deploy_ready.py new file mode 100644 index 0000000000..c707414068 --- /dev/null +++ b/build/ci/reconcile_deploy_ready.py @@ -0,0 +1,475 @@ +#!/usr/bin/env python3 +""" +Reconciles "Deploy Ready" Shortcut stories against what has actually +reached prod, org-wide and regardless of which release shipped them. + +Enumerates every non-archived Deploy Ready story, resolves each one's +linked, merged PR(s), and checks whether any of those PRs' merge commits +landed in the current prod tag. Classifies each story as shipped +(transitioned Deploy Ready -> Done), pending (qualifying PR not yet in +prod), or triage (no qualifying PR, or a non-Standard workflow/state). + +Usage: + python3 reconcile_deploy_ready.py [--dry-run] + python3 reconcile_deploy_ready.py --apply + python3 reconcile_deploy_ready.py --apply --prod-tag prod/6.111.0-prod.2+chart.0.87.5-prod.1 --out reconcile-report.json + +Requires `git` and `gh` on PATH, and SHORTCUT_API_TOKEN (required even for --dry-run). +""" + +import argparse +import concurrent.futures +import json +import os +import subprocess +import sys +import urllib.error +import urllib.parse +import urllib.request + +# build/ci is not a package (see tests/conftest.py); a plain sibling-module +# import works both run directly and under pytest. +import shortcut_pr_guards + +SHORTCUT_API_BASE = "https://api.app.shortcut.com/api/v3" +SHORTCUT_API_ROOT = "https://api.app.shortcut.com" + +# Sefaria's "Standard" Shortcut workflow. +STANDARD_WORKFLOW_ID = 500000005 +DEPLOY_READY_STATE_ID = 500000045 +DONE_STATE_ID = 500000010 + +SEFARIA_PROJECT_REPO_ID = shortcut_pr_guards.SEFARIA_PROJECT_REPO_ID +DEFAULT_TARGET_BRANCH = shortcut_pr_guards.DEFAULT_TARGET_BRANCH +DEFAULT_REPO = "Sefaria/Sefaria-Project" + +DEPLOY_READY_SEARCH_QUERY = 'state:"Deploy Ready" !is:archived' + + +def die(message: str) -> None: + print(f"ERROR: {message}", file=sys.stderr) + sys.exit(1) + + +def warn(message: str) -> None: + print(f"WARNING: {message}", file=sys.stderr) + + +def run_git(args): + """Run a git subcommand, returning stdout. Exits the process on failure.""" + proc = subprocess.run(["git", *args], capture_output=True, text=True) + if proc.returncode != 0: + die(f"git {' '.join(args)} failed: {proc.stderr.strip()}") + return proc.stdout + + +def resolve_default_prod_tag(): + """Newest prod/* tag by creation date. Dies if there are no prod/* tags at all.""" + tags = [t for t in run_git(["tag", "--list", "prod/*", "--sort=-creatordate"]).splitlines() if t.strip()] + if not tags: + die("No 'prod/*' tags found in this checkout; pass --prod-tag explicitly.") + return tags[0] + + +def search_deploy_ready_stories(token): + """Enumerate all non-archived Deploy Ready stories via the token'd search endpoint, paginated via its `next` cursor. Deliberately not `iterations-get-active`, which is scoped to the calling token's own teams.""" + stories = [] + next_path = f"/api/v3/search/stories?query={urllib.parse.quote(DEPLOY_READY_SEARCH_QUERY)}" + while next_path: + url = next_path if next_path.startswith("http") else SHORTCUT_API_ROOT + next_path + req = urllib.request.Request(url, headers={"Shortcut-Token": token, "Accept": "application/json"}) + with urllib.request.urlopen(req, timeout=20) as resp: + body = json.loads(resp.read().decode("utf-8")) + stories.extend(body.get("data", [])) + next_path = body.get("next") + return stories + + +gather_linked_prs = shortcut_pr_guards.gather_linked_prs +qualifying_prs = shortcut_pr_guards.qualifying_prs + + +def fetch_pr_merge_oid(pr_number, repo): + """Resolve a merged PR's merge commit SHA via `gh pr view --json mergeCommit`. A failed lookup is logged and returns None.""" + proc = subprocess.run( + ["gh", "pr", "view", str(pr_number), "--repo", repo, "--json", "mergeCommit"], + capture_output=True, + text=True, + ) + if proc.returncode != 0: + warn(f"gh pr view {pr_number} --repo {repo} failed: {proc.stderr.strip()}") + return pr_number, None + try: + data = json.loads(proc.stdout) + except json.JSONDecodeError as e: + warn(f"gh pr view {pr_number} --repo {repo} returned invalid JSON: {e}") + return pr_number, None + oid = (data.get("mergeCommit") or {}).get("oid") + return pr_number, oid + + +def fetch_merge_oids(pr_numbers, repo, max_workers=8): + """Batch-resolve fetch_pr_merge_oid across every qualifying PR concurrently.""" + oid_by_pr = {} + if not pr_numbers: + return oid_by_pr + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor: + futures = [executor.submit(fetch_pr_merge_oid, n, repo) for n in pr_numbers] + for future in concurrent.futures.as_completed(futures): + try: + pr_number, oid = future.result() + except FileNotFoundError: + die("`gh` was not found on PATH. Install the GitHub CLI " + "(https://cli.github.com/) or ensure it's available in " + "this environment; PR merge-commit lookups cannot proceed without it.") + if oid: + oid_by_pr[pr_number] = oid + return oid_by_pr + + +def is_ancestor_of_prod(oid, prod_tag): + """True if commit `oid` is an ancestor of `prod_tag`, False if git can definitively say it is not, None if the check itself couldn't run (a local-repo problem, not proof either way).""" + proc = subprocess.run( + ["git", "merge-base", "--is-ancestor", oid, prod_tag], + capture_output=True, + text=True, + ) + if proc.returncode == 0: + return True + if proc.returncode == 1: + return False + warn(f"git merge-base --is-ancestor {oid} {prod_tag} could not run (rc={proc.returncode}): {proc.stderr.strip()}") + return None + + +def resolve_shipping_release_tag(oid): + """The first (earliest created) `prod/*` tag that contains commit `oid` -- the release that actually shipped it. + + `--sort=creatordate` (ascending) is deliberate and the opposite of + resolve_default_prod_tag's `-creatordate` above: that one wants the + newest tag, this one wants the oldest tag that still contains the + commit. Returns None if the lookup fails or no prod/* tag contains it.""" + proc = subprocess.run( + ["git", "tag", "--list", "prod/*", "--contains", oid, "--sort=creatordate"], + capture_output=True, + text=True, + ) + if proc.returncode != 0: + warn(f"git tag --list --contains {oid} failed: {proc.stderr.strip()}") + return None + tags = [t for t in proc.stdout.splitlines() if t.strip()] + return tags[0] if tags else None + + +def transition_story(story_id, done_state_id, token): + """PUT a workflow_state_id update to Shortcut.""" + url = f"{SHORTCUT_API_BASE}/stories/{story_id}" + body = json.dumps({"workflow_state_id": done_state_id}).encode("utf-8") + req = urllib.request.Request(url, data=body, method="PUT") + req.add_header("Shortcut-Token", token) + req.add_header("Content-Type", "application/json") + try: + with urllib.request.urlopen(req, timeout=20) as resp: + resp.read() + return story_id, True, None + except urllib.error.HTTPError as e: + return story_id, False, f"HTTP {e.code} {e.reason}" + except Exception as e: # noqa: BLE001 - a per-story failure must never abort the run + return story_id, False, str(e) + + +def transition_stories(story_ids, done_state_id, token, max_workers=8): + transitioned = [] + failed = [] + if not story_ids: + return transitioned, failed + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor: + futures = [executor.submit(transition_story, sid, done_state_id, token) for sid in story_ids] + for future in concurrent.futures.as_completed(futures): + sid, ok, err = future.result() + if ok: + transitioned.append(sid) + else: + warn(f"Failed to transition story {sid}: {err}") + failed.append((sid, err)) + return transitioned, failed + + +def _story_summary(story): + return {"id": story.get("id"), "name": story.get("name"), "url": story.get("app_url")} + + +def _triage_context(story): + """Extra context for a triage story only, already present in the search response (no extra API call).""" + return { + "description": story.get("description"), + "comments": [c.get("text") for c in (story.get("comments") or []) if c.get("text")], + } + + +def _diagnose_linked_pr(pr, repo_id, target_branch): + """Which PR-level guards this linked PR fails, if any. Diagnostic only -- classification never reads this.""" + failed = [] + if pr.get("merged") is not True: + failed.append("not merged") + if pr.get("repository_id") != repo_id: + failed.append(f"wrong repo (repository_id={pr.get('repository_id')}, expected {repo_id})") + if pr.get("target_branch_name") != target_branch: + failed.append(f"wrong target branch ({pr.get('target_branch_name')!r}, expected {target_branch!r})") + if pr.get("branch_name") in shortcut_pr_guards.LONG_LIVED_ENV_BRANCHES: + failed.append(f"promotion PR (head branch {pr.get('branch_name')!r} is a long-lived environment branch)") + return failed + + +def classify_stories(stories, repo_id, target_branch): + """Split stories into (triage, candidates), where candidates is a list of (story, qualifying_prs) pairs still needing a prod-ancestry check. A story not on the Standard workflow/state routes straight to triage.""" + triage = [] + candidates = [] + for story in stories: + if story.get("workflow_id") != STANDARD_WORKFLOW_ID or story.get("workflow_state_id") != DEPLOY_READY_STATE_ID: + entry = _story_summary(story) + entry["reason"] = "non_standard_workflow_or_state" + entry["workflow_id"] = story.get("workflow_id") + entry["workflow_state_id"] = story.get("workflow_state_id") + entry.update(_triage_context(story)) + triage.append(entry) + warn( + f"Story {story.get('id')} matched the Deploy Ready search but is on " + f"workflow_id={story.get('workflow_id')} / workflow_state_id={story.get('workflow_state_id')} " + f"(expected workflow_id={STANDARD_WORKFLOW_ID}, workflow_state_id={DEPLOY_READY_STATE_ID}); " + "routing to triage, not transitioning." + ) + continue + + linked = gather_linked_prs(story) + qualifying = qualifying_prs(linked, repo_id=repo_id, target_branch=target_branch) + if not qualifying: + entry = _story_summary(story) + entry["reason"] = "no_qualifying_pr" + entry["linked_pr_numbers"] = sorted( + pr.get("number") for pr in linked if pr.get("number") is not None + ) + entry["linked_prs"] = [ + {"number": pr.get("number"), "failed_guards": _diagnose_linked_pr(pr, repo_id, target_branch)} + for pr in sorted(linked, key=lambda p: (p.get("number") is None, p.get("number"))) + ] + entry.update(_triage_context(story)) + triage.append(entry) + continue + + candidates.append((story, qualifying)) + return triage, candidates + + +def _story_for_release_notes(story): + """Build a story entry in shipped_stories.py's own hydrated-story shape, from data already fetched (no extra API call).""" + return { + "id": story.get("id"), + "name": story.get("name"), + "description": story.get("description"), + "url": story.get("app_url"), + "workflow_id": story.get("workflow_id"), + "workflow_state_id": story.get("workflow_state_id"), + "story_type": story.get("story_type"), + } + + +def classify_candidates(candidates, oid_by_pr, prod_tag): + """For each (story, qualifying_prs) candidate, check every qualifying PR's merge commit for prod ancestry and split into shipped / pending. + A PR whose merge oid never resolved, or whose ancestry check couldn't run, counts as inconclusive, never as "in prod". + + Every shipped entry carries `shipping_release_tag`, the true release that shipped it, computed unconditionally regardless of + --apply/--dry-run. Only when it equals the current `prod_tag` does the entry also carry `hydrated_story` -- the only signal + merge_release_backfill.py uses to fold a backfilled story into shipped-stories.json.""" + shipped = [] + pending = [] + for story, prs in candidates: + shipped_via = [] + for pr in prs: + oid = oid_by_pr.get(pr["number"]) + if not oid: + continue + if is_ancestor_of_prod(oid, prod_tag) is True: + shipped_via.append(pr["number"]) + + entry = _story_summary(story) + entry["qualifying_prs"] = sorted(pr["number"] for pr in prs) + if shipped_via: + shipped_via_sorted = sorted(shipped_via) + entry["shipped_via_prs"] = shipped_via_sorted + shipping_tag = resolve_shipping_release_tag(oid_by_pr[shipped_via_sorted[0]]) + entry["shipping_release_tag"] = shipping_tag + if shipping_tag == prod_tag: + entry["hydrated_story"] = _story_for_release_notes(story) + shipped.append(entry) + else: + pending.append(entry) + return shipped, pending + + +def print_summary(report): + """Readable stdout summary. The triage list is printed in full, not just counted.""" + counts = report["counts"] + print(f"Prod tag: {report['prod_tag']}") + print(f"Mode: {'APPLY (mutating)' if report['applied'] else 'dry-run (no mutation)'}") + print( + f"shipped={counts['shipped']} pending={counts['pending']} " + f"triage={counts['triage']} (total Deploy Ready stories seen: {counts['total']})" + ) + print() + + print(f"--- shipped ({len(report['shipped'])}) ---") + for s in report["shipped"]: + if not report["applied"]: + status = "would transition" + elif s.get("transitioned"): + status = "transitioned" + else: + status = f"FAILED: {s.get('transition_error')}" + release_note = ( + "belongs in THIS release's notes" if s.get("hydrated_story") + else f"shipped in {s.get('shipping_release_tag')}" if s.get("shipping_release_tag") + else "shipping release could not be determined" + ) + print( + f" {s['id']} {s.get('name', '')!r} via PR(s) {s.get('shipped_via_prs')} " + f"[{status}] ({release_note})" + ) + + print(f"--- pending ({len(report['pending'])}) ---") + for s in report["pending"]: + print(f" {s['id']} {s.get('name', '')!r} qualifying PR(s) {s.get('qualifying_prs')} not yet in prod") + + print(f"--- triage ({len(report['triage'])}) ---") + for s in report["triage"]: + if s["reason"] == "non_standard_workflow_or_state": + print( + f" {s['id']} {s.get('name', '')!r} reason=non_standard_workflow_or_state " + f"workflow_id={s.get('workflow_id')} workflow_state_id={s.get('workflow_state_id')}" + ) + else: + print( + f" {s['id']} {s.get('name', '')!r} reason=no_qualifying_pr " + f"linked_pr_numbers={s.get('linked_pr_numbers')}" + ) + + +def build_arg_parser(): + parser = argparse.ArgumentParser( + description="Reconcile Deploy Ready Shortcut stories against what has actually reached prod.", + ) + parser.add_argument( + "--apply", action="store_true", + help="Actually transition qualifying stories Deploy Ready -> Done. Without this, " + "the sweep only classifies and reports; nothing is mutated.", + ) + parser.add_argument( + "--dry-run", action="store_true", + help="Explicit no-op flag for symmetry with mark_stories_deployed.py: classify and " + "report, mutate nothing. This is already the default without --apply, and " + "always wins if both are passed.", + ) + parser.add_argument( + "--prod-tag", + help="Prod tag to check ancestry against (default: newest 'prod/*' tag by creation date).", + ) + parser.add_argument("--repo", default=DEFAULT_REPO, help=f"GitHub repo for `gh pr view` lookups, default {DEFAULT_REPO}") + parser.add_argument( + "--target-branch", default=DEFAULT_TARGET_BRANCH, + help=f"Required PR target branch for a linked PR to count as shipping evidence (guard #2), " + f"default {DEFAULT_TARGET_BRANCH!r}", + ) + parser.add_argument("--out", help="Write the machine-readable JSON report to this path") + parser.add_argument("--max-workers", type=int, default=8) + return parser + + +def main(): + args = build_arg_parser().parse_args() + + token = os.environ.get("SHORTCUT_API_TOKEN") + if not token: + die( + "SHORTCUT_API_TOKEN is not set. Required even for --dry-run: enumerating Deploy " + "Ready stories and resolving their linked PRs both come from the Shortcut API, " + "not from git." + ) + + prod_tag = args.prod_tag or resolve_default_prod_tag() + + try: + stories = search_deploy_ready_stories(token) + except (urllib.error.URLError, urllib.error.HTTPError, json.JSONDecodeError, OSError) as e: + die(f"Failed to enumerate Deploy Ready stories from Shortcut: {e}") + + triage, candidates = classify_stories(stories, SEFARIA_PROJECT_REPO_ID, args.target_branch) + + all_qualifying_pr_numbers = sorted( + {pr["number"] for _, prs in candidates for pr in prs if pr.get("number") is not None} + ) + oid_by_pr = fetch_merge_oids(all_qualifying_pr_numbers, args.repo, args.max_workers) + + shipped, pending = classify_candidates(candidates, oid_by_pr, prod_tag) + + # --dry-run always wins over --apply if somehow both are passed -- a + # bulk mutation of shared team state should never be one flag-typo away + # from firing. + apply_mutations = args.apply and not args.dry_run + + failed_transitions = [] + if apply_mutations: + transitioned_ids, failed_transitions = transition_stories( + [s["id"] for s in shipped], DONE_STATE_ID, token, args.max_workers + ) + transitioned_set = set(transitioned_ids) + failed_by_id = dict(failed_transitions) + for s in shipped: + s["transitioned"] = s["id"] in transitioned_set + if s["id"] in failed_by_id: + s["transition_error"] = failed_by_id[s["id"]] + else: + for s in shipped: + s["transitioned"] = None # not attempted -- dry-run + + report = { + "prod_tag": prod_tag, + "applied": apply_mutations, + "counts": { + "total": len(stories), + "shipped": len(shipped), + "pending": len(pending), + "triage": len(triage), + }, + "shipped": shipped, + "pending": pending, + "triage": triage, + } + + print_summary(report) + + if args.out: + with open(args.out, "w", encoding="utf-8") as f: + json.dump(report, f, indent=2, ensure_ascii=False) + f.write("\n") + + if failed_transitions: + warn( + f"{len(failed_transitions)} stor{'y' if len(failed_transitions) == 1 else 'ies'} " + f"failed to transition: {failed_transitions}" + ) + sys.exit(1) + + # Nothing shipped but stories exist and some routed to triage: could be + # legitimate, or a guard misconfiguration (e.g. SEFARIA_PROJECT_REPO_ID + # drift) silently routing everything to triage forever. --dry-run is a + # preview by definition and never exits non-zero for this. + if apply_mutations and stories and not shipped and triage: + warn( + f"{len(stories)} Deploy Ready stor{'y' if len(stories) == 1 else 'ies'} seen, 0 shipped, " + f"{len(triage)} routed to triage. Check for guard drift before assuming the backlog is clean." + ) + sys.exit(2) + + +if __name__ == "__main__": + main() diff --git a/build/ci/shipped_stories.py b/build/ci/shipped_stories.py index 232c13f121..fb5343dc6b 100755 --- a/build/ci/shipped_stories.py +++ b/build/ci/shipped_stories.py @@ -1,40 +1,22 @@ #!/usr/bin/env python3 """ -Deterministically resolve which Shortcut stories shipped in a prod rollout. - -Walks the git tree between two prod tags (or an explicit commit range), -extracts Shortcut (SC) story ids from commit subjects and, for commits that -reference a merged PR, from that PR's branch name too. Revert commits -(`Revert "..."`, `Revert: ...`, `revert(...)`) never contribute story ids to -the shipped set; their suppressed ids are surfaced separately in -`reverted_commits` instead of being silently dropped. Optionally hydrates -each id via the Shortcut API (id, name, description, url, workflow id, -workflow state, story type) when SHORTCUT_API_TOKEN is set — `workflow_id` -is included because a workflow's Done state id is not universal across -Shortcut workflows, and downstream tooling (mark_stories_deployed.py) needs -it to tell "different workflow" apart from "different state". Emits a -single JSON document that downstream tooling (the sefaria-release-notes -skill, mark_stories_deployed.py) consumes — this script never writes prose -and never mutates a Shortcut story. +Deterministically resolves which Shortcut stories shipped in a prod rollout, +by walking the git range between two prod tags and extracting story ids +from commit subjects, PR branch names, and (as a fallback) Shortcut's own +PR<->story link. Usage: python3 shipped_stories.py --version 6.111.0-prod.2 [--out shipped-stories.json] [--repo Sefaria/Sefaria-Project] python3 shipped_stories.py --range .. [--out shipped-stories.json] [--repo Sefaria/Sefaria-Project] With --version V, the current tag is resolved as the single tag matching the -glob `prod/V+*` (an optional leading "v" on V is stripped first), and the -previous tag is whichever prod/* tag immediately precedes it by creation -date. With --range, the two endpoints are used verbatim as given. - -Requires `git` and `gh` on PATH. `gh` is only used to look up PR branch -names (`gh pr view --json headRefName,number`) and is never required to -succeed — a failing lookup for one PR is logged to stderr and skipped. -SHORTCUT_API_TOKEN is optional; without it, story ids are still emitted but -`stories` is empty and `unresolved_story_ids` is not populated (hydration -was never attempted, which is a different case from a failed lookup). - -All ids shown in this file's docstring and comments (e.g. story id 11111) -are placeholders, not real Shortcut story ids. +glob `prod/V+*`, and the previous tag is whichever prod/* tag immediately +precedes it by creation date. With --range, the two endpoints are used +verbatim as given. + +Requires `git` and `gh` on PATH. SHORTCUT_API_TOKEN is optional; without +it, story ids are still emitted but hydration and the PR-link fallback are +skipped. """ import argparse @@ -45,21 +27,18 @@ import subprocess import sys import urllib.error +import urllib.parse import urllib.request -# Shortcut (SC) story id patterns recognized in a commit subject or a PR -# branch name. Kept intentionally short: `\bsc[-_](\d+)\b` (pattern 1) has a -# TRAILING word boundary, so it already matches "sc-N"/"sc_N" wrapped in any -# punctuation (brackets, parens, a leading "chore:"/"feat:" etc.) -- adding a -# separate bracket/paren/prefix-scoped pattern for each of those shapes would -# just re-derive what pattern 1 already covers. The other two patterns here -# are kept because they are NOT subsumed by pattern 1: -# - `chore[:/\(].*?sc[-_](\d+)` / `feat[:/\(].*?sc[-_](\d+)` have no -# trailing boundary, so they still match when a word character follows -# the digits directly with no separator (e.g. a "chore(sc_123abc)"-shaped -# subject). -# - `feature/sc[-_ ](\d+)` allows a literal space after "sc", which -# pattern 1's `[-_]` does not. +# build/ci is not a package (see tests/conftest.py); a plain sibling-module +# import works both run directly and under pytest. +import shortcut_pr_guards + +LONG_LIVED_ENV_BRANCHES = shortcut_pr_guards.LONG_LIVED_ENV_BRANCHES + +# Pattern 1 has a trailing word boundary and covers "sc-N"/"sc_N" in most +# contexts; patterns 2-4 catch shapes it doesn't (no trailing boundary, or a +# literal space after "sc"). SC_PATTERNS = [ re.compile(r'\bsc[-_](\d+)\b', re.IGNORECASE), re.compile(r'feature/sc[-_ ](\d+)', re.IGNORECASE), @@ -69,38 +48,22 @@ PR_PATTERN = re.compile(r'\(#(\d+)\)') -# A real (non-squash) merge commit's subject never gets the parenthesized -# "(#N)" form -- GitHub writes it as a bare "Merge pull request #N from -# /". For a merge-commit PR this subject is often the ONLY -# place the PR number appears in the whole range (no matching child commit -# carries it), so PR_PATTERN alone would silently miss it. +# A real (non-squash) merge commit's subject uses the bare "Merge pull +# request #N from /" form instead of "(#N)". MERGE_PR_PATTERN = re.compile(r'^Merge pull request #(\d+)\s+from\s+\S+', re.IGNORECASE) -# Matches the double-quoted original subject inside a `Revert "..."` commit -# (e.g. a subject like `Revert "fix(sc-13): correct the thing"` captures -# `fix(sc-13): correct the thing`). Used to find that original commit -# elsewhere in the same range (see the reverted-original handling in -# main()) -- not applicable to the `Revert: ...` / `revert(...)` forms, -# which never quote a subject. +# Captures the quoted original subject inside a `Revert "..."` commit; not +# applicable to the `Revert: ...` / `revert(...)` forms. REVERT_QUOTE_PATTERN = re.compile(r'^revert\s+"(.+?)"', re.IGNORECASE) -# Auto-generated noise that should never count as "a real commit missing a -# story id" in commits_without_story: deploy() markers, "Merge pull -# request" / "Merge branch" / "Merge remote-tracking branch" subjects (the -# latter two are plain branch-sync merges -- now that merge commits are -# walked at all for ID/PR extraction, these show up in the range too and -# must not pollute that list), and any subject ending in a "[skip ci]" -# marker. Generalized from an enumerated (staging|preprod|prod) alternation -# so new deploy environments (e.g. deploy(sandbox)) are recognized without -# an edit here. +# Auto-generated noise excluded from commits_without_story. NOISE_PATTERN = re.compile( r'^(deploy\(\w+\)|Merge (pull request|branch|remote-tracking branch))|\[skip ci\]', re.IGNORECASE, ) # Matches a revert commit subject: `Revert "..."`, `Revert: ...`, or -# `revert(scope): ...`, case-insensitively. A revert's story ids must never -# be attributed as shipped — see the reverted_commits handling in main(). +# `revert(scope): ...`. REVERT_PATTERN = re.compile(r'^(revert\s+"|revert:\s|revert\()', re.IGNORECASE) SHORTCUT_API_BASE = "https://api.app.shortcut.com/api/v3" @@ -136,13 +99,7 @@ def extract_story_ids(text): def extract_pr_number(text): - """Return the LAST `(#NNN)` reference in text, matching the merge-commit - convention where a revert's own re-merge ref trails any PR ref quoted - from the original (reverted) subject, e.g. `Revert "x (#123)" (#456)`. - - Falls back to the bare "Merge pull request #N from ..." form when no - parenthesized ref is present -- that's the only form a real (non-squash) - merge commit ever gets.""" + """Return the LAST `(#NNN)` reference (e.g. `Revert "x (#123)" (#456)`), falling back to the bare "Merge pull request #N" form.""" if not text: return None matches = PR_PATTERN.findall(text) @@ -153,8 +110,7 @@ def extract_pr_number(text): def split_range_spec(spec): - """Split a PREV..CUR range string. Git refnames may never contain '..', - so the first occurrence is an unambiguous split point.""" + """Split a PREV..CUR range string on the first '..' (git refnames never contain '..').""" if ".." not in spec: die(f"--range must be of the form PREV..CUR, got: {spec!r}") idx = spec.index("..") @@ -167,20 +123,10 @@ def split_range_spec(spec): def resolve_range_from_version(version, chart_version=None): """Resolve the (prev, cur) prod/* tag pair for --version V [--chart-version CV]. - A prod app version can have MULTIPLE prod/* tags -- one per chart-only - rollout of the same app version (e.g. prod/6.100.0-prod.1+chart.0.85.8- - prod.{1,2,3}). With --chart-version, the exact tag `prod/V+chart.CV` is - preferred; if it doesn't exist (e.g. Argo's chartVersion arg and the tag - suffix drifted out of sync), that's logged and we fall through to the - no-chart-version path below rather than dying -- a chart-version mismatch - must never be the thing that kills a healthy rollout's release notes. - - Without a matching --chart-version, and when more than one tag matches - `prod/V+*`, the newest by tag creation date is chosen (never dies) and - the choice is logged. All tags are read once via `git tag --list - 'prod/*' --sort=-creatordate`, so "newest" and "previous tag" both use - that same deterministic ordering -- prev is simply the tag immediately - after the chosen one in that list. + Tags are read once via `git tag --list 'prod/*' --sort=-creatordate` + (newest first); with no matching --chart-version and multiple tags for + the same version, the newest is chosen and prev is the tag immediately + after it in that same ordering. """ v = version[1:] if version.startswith("v") else version @@ -226,20 +172,14 @@ def resolve_range_from_version(version, chart_version=None): def get_commit_subjects(range_spec): - # Deliberately NOT --no-merges: a merge-commit PR's bare "Merge pull - # request #N from .../hotfix/sc-.../..." subject can be the ONLY place - # its PR number and story id appear in the whole range (no squashed - # "(#N)" form, no matching child commit) -- see MERGE_PR_PATTERN and the - # NOISE_PATTERN handling for the plain branch-sync merges this also lets - # through. + # Deliberately not --no-merges: a merge commit's subject can be the only + # place its PR number and story id appear in the whole range. out = run_git(["log", range_spec, "--pretty=format:%s"]) return [line for line in out.split("\n") if line.strip()] def tag_creator_date_iso(tag): - """ISO 8601 creation date of an annotated tag (`%(creatordate:iso-strict)`), - or None if `tag` doesn't resolve to a real tag ref (e.g. an explicit - --range endpoint that's a branch or SHA, not a prod/* tag).""" + """ISO 8601 creation date of a tag, or None if `tag` isn't a real tag ref.""" out = run_git(["for-each-ref", "--format=%(creatordate:iso-strict)", f"refs/tags/{tag}"]).strip() return out or None @@ -271,10 +211,8 @@ def fetch_branches(pr_numbers, repo, max_workers=8): try: pr_number, branch = future.result() except FileNotFoundError: - # `gh` itself isn't on PATH -- every other in-flight lookup - # will hit the exact same error, so fail fast with one clear - # message instead of an unhandled traceback (or N identical - # per-PR warnings). + # gh not on PATH; every other in-flight lookup would fail + # identically, so fail fast with one clear message. die("`gh` was not found on PATH. Install the GitHub CLI " "(https://cli.github.com/) or ensure it's available in " "this environment; PR branch name lookups cannot proceed without it.") @@ -308,6 +246,73 @@ def fetch_story(story_id, token): } +def fetch_story_by_pr_link(pr_number, token): + """Look up the Shortcut story linked to a merged PR via `GET search/stories?query=pr:` + (only `pr:N` resolves this; `branch:`/`pull-request:` do not). Adopts the id only when + the search returns exactly one story and that story's matching PR passes passes_pr_guards; + ambiguous results or lookup failures are a warn-and-skip.""" + query = urllib.parse.quote(f"pr:{pr_number}") + url = f"{SHORTCUT_API_BASE}/search/stories?query={query}" + req = urllib.request.Request(url, headers={"Shortcut-Token": token, "Accept": "application/json"}) + try: + with urllib.request.urlopen(req, timeout=20) as resp: + body = resp.read().decode("utf-8") + data = json.loads(body) + except urllib.error.HTTPError as e: + warn(f"Shortcut PR-link lookup for PR #{pr_number} failed: HTTP {e.code} {e.reason}") + return pr_number, None + except Exception as e: # noqa: BLE001 - a lookup failure must never abort the run + warn(f"Shortcut PR-link lookup for PR #{pr_number} failed: {e}") + return pr_number, None + + results = data.get("data", []) + total = data.get("total", len(results)) + if not results: + return pr_number, None + if total > 1 or len(results) > 1: + warn(f"Shortcut PR-link lookup for PR #{pr_number} was ambiguous ({total} stories); skipping.") + return pr_number, None + + story = results[0] + story_id = story.get("id") + if story_id is None: + return pr_number, None + + try: + pr_number_int = int(pr_number) + except (TypeError, ValueError): + pr_number_int = None + + matching_pr = next( + (pr for pr in shortcut_pr_guards.gather_linked_prs(story) if pr.get("number") == pr_number_int), + None, + ) + if matching_pr is None or not shortcut_pr_guards.passes_pr_guards(matching_pr): + warn( + f"Shortcut PR-link lookup for PR #{pr_number} resolved to story {story_id}, but " + "that PR does not pass the shipping-evidence guards (merged / Sefaria-Project " + "repo / target branch master / head branch not long-lived) -- e.g. a promotion " + "or branch-sync merge rather than the real feature PR. Skipping." + ) + return pr_number, None + + return pr_number, str(story_id) + + +def fetch_stories_by_pr(pr_numbers, token, max_workers=8): + """Batch-resolve fetch_story_by_pr_link across PRs concurrently.""" + story_id_by_pr = {} + if not pr_numbers: + return story_id_by_pr + with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor: + futures = [executor.submit(fetch_story_by_pr_link, n, token) for n in pr_numbers] + for future in concurrent.futures.as_completed(futures): + pr_number, story_id = future.result() + if story_id: + story_id_by_pr[pr_number] = story_id + return story_id_by_pr + + def hydrate_stories(story_ids, token, max_workers=8): stories = [] unresolved = [] @@ -348,10 +353,7 @@ def build_arg_parser(): def chart_version_from_tag(tag): - """Prod tags carry the chart version after a '+', e.g. - prod/6.111.0-prod.2+chart.0.87.5-prod.1 -> 0.87.5-prod.1. - Returns None for a tag that does not follow that shape (e.g. an - explicit --range against arbitrary refs).""" + """Extract the chart version from a prod tag's '+chart.X' suffix, e.g. prod/6.111.0-prod.2+chart.0.87.5-prod.1 -> 0.87.5-prod.1. None if absent.""" if not tag: return None m = CHART_IN_TAG.search(tag) @@ -386,19 +388,51 @@ def main(): branch_by_pr = fetch_branches(sorted(pr_numbers), args.repo) - # Resolve each commit's full story_ids (subject ∪ its PR branch name) - # once, up front -- both the per-commit `commits[]` output and the - # aggregate shipped-set logic below read from this. + # Read once; used both for the PR-link fallback below and hydration later. + token = os.environ.get("SHORTCUT_API_TOKEN") + + # Resolve each commit's full story_ids (subject ∪ its PR branch name) once. for c in parsed_commits: branch = branch_by_pr.get(c["pr_number"]) if c["pr_number"] else None c["branch"] = branch c["story_ids"] = c["subject_story_ids"] | extract_story_ids(branch) - # Track, per story id, which NON-revert commit indices carry it. This is - # what lets a revert exclude ONLY the specific original commit it quotes - # from the shipped set -- not every commit that happens to share that id - # -- so an id independently carried by another, still-live commit keeps - # shipping (finding #7). + # Third discovery source: for a commit with a PR number but no story id + # yet, ask Shortcut whether that PR is linked to a story -- unless the + # subject is noise or the PR's head branch is a promotion merge. + def _eligible_for_pr_link_fallback(c): + return ( + c["pr_number"] + and not c["story_ids"] + and not c["is_revert"] + and not NOISE_PATTERN.search(c["subject"]) + and c["branch"] not in LONG_LIVED_ENV_BRANCHES + ) + + stories_from_shortcut_pr_link = set() + prs_needing_shortcut_lookup = sorted( + {c["pr_number"] for c in parsed_commits if _eligible_for_pr_link_fallback(c)}, + key=int, + ) + if prs_needing_shortcut_lookup: + if token: + story_id_by_pr = fetch_stories_by_pr(prs_needing_shortcut_lookup, token) + for c in parsed_commits: + if _eligible_for_pr_link_fallback(c): + sid = story_id_by_pr.get(c["pr_number"]) + if sid: + c["story_ids"] = {sid} + stories_from_shortcut_pr_link.add(sid) + else: + warn( + f"SHORTCUT_API_TOKEN is not set; skipping the Shortcut PR-link fallback " + f"lookup for {len(prs_needing_shortcut_lookup)} commit(s) whose PR carries " + "no sc-NNNNN id in its subject or branch name." + ) + + # Tracks which non-revert commits carry each story id, so a revert + # excludes only the specific commit it quotes, not every commit sharing + # that id. carrying_indices_by_id = {} for i, c in enumerate(parsed_commits): if c["is_revert"]: @@ -411,17 +445,15 @@ def main(): if not c["is_revert"]: continue - # A revert's OWN story ids must never be attributed as shipped, but - # they're too important to silently drop — surface them instead. + # A revert's own story ids are never attributed as shipped, but are surfaced separately. if c["story_ids"]: reverted_commits.append({ "subject": c["subject"], "suppressed_story_ids": sorted(c["story_ids"], key=int), }) - # If this revert quotes a commit subject that's ALSO in this same - # range, that original's ids must stop shipping too -- unless some - # other, still-live commit independently carries the same id. + # If the reverted commit is also in this range, stop its ids from + # shipping too, unless another commit still carries them. quote_match = REVERT_QUOTE_PATTERN.match(c["subject"]) if not quote_match: continue @@ -444,13 +476,9 @@ def main(): "story_ids": sorted(c["story_ids"], key=int), }) - # Reverts are excluded here too: a revert with a suppressed story id - # is already reported via reverted_commits, and a revert with no - # story id at all is ordinary noise, not a commit needing attention. if not c["is_revert"] and not c["story_ids"] and not NOISE_PATTERN.search(c["subject"]): commits_without_story.append(c["subject"]) - token = os.environ.get("SHORTCUT_API_TOKEN") hydrated = bool(token) if token: stories, unresolved_story_ids = hydrate_stories(sorted(all_story_ids, key=int), token) @@ -462,20 +490,16 @@ def main(): result = { "version": version, "chart_version": chart_version_from_tag(cur), - # ISO 8601 creation date of the resolved current tag, or null if - # `cur` isn't a real tag ref (e.g. an explicit --range against a - # branch/SHA). Source of truth for the release date shown in the - # generated Slack posts -- see sefaria-release-notes/SKILL.md. "release_date": tag_creator_date_iso(cur), "range": {"previous_tag": prev, "current_tag": cur, "spec": range_spec}, "commits": commits, "commits_without_story": commits_without_story, "reverted_commits": reverted_commits, "story_ids": sorted(all_story_ids, key=int), + # Ids adopted only via the PR-link fallback; already included in story_ids/stories. + "stories_from_shortcut_pr_link": sorted(stories_from_shortcut_pr_link, key=int), "stories": stories, - # False when SHORTCUT_API_TOKEN was absent, so `stories` being empty - # means "never looked up" rather than "looked up and found nothing". - # Without this the two cases are indistinguishable downstream. + # False when SHORTCUT_API_TOKEN was absent, distinguishing "never looked up" from "found nothing". "hydrated": hydrated, "unresolved_story_ids": unresolved_story_ids, } diff --git a/build/ci/shortcut_pr_guards.py b/build/ci/shortcut_pr_guards.py new file mode 100644 index 0000000000..2a37d0f6d5 --- /dev/null +++ b/build/ci/shortcut_pr_guards.py @@ -0,0 +1,44 @@ +#!/usr/bin/env python3 +""" +Shared guards for deciding whether a Shortcut-linked PR counts as evidence +that a story's change reached prod. +""" + +SEFARIA_PROJECT_REPO_ID = 500000103 + +DEFAULT_TARGET_BRANCH = "master" + +LONG_LIVED_ENV_BRANCHES = frozenset({"master", "preprod", "prod"}) + + +def passes_pr_guards(pr, repo_id=SEFARIA_PROJECT_REPO_ID, target_branch=DEFAULT_TARGET_BRANCH): + """True if a linked-PR object counts as shipping evidence: merged, right repo, right target branch, and not a promotion merge.""" + return ( + pr.get("merged") is True + and pr.get("repository_id") == repo_id + and pr.get("target_branch_name") == target_branch + and pr.get("branch_name") not in LONG_LIVED_ENV_BRANCHES + ) + + +def qualifying_prs(prs, repo_id=SEFARIA_PROJECT_REPO_ID, target_branch=DEFAULT_TARGET_BRANCH): + """Filter a list of linked-PR objects down to the ones that pass passes_pr_guards.""" + return [pr for pr in prs if passes_pr_guards(pr, repo_id=repo_id, target_branch=target_branch)] + + +def gather_linked_prs(story): + """Collect every PR linked to a story from both `pull_requests` and `branches[*].pull_requests`, deduplicated by PR number.""" + seen = set() + prs = [] + for pr in story.get("pull_requests") or []: + number = pr.get("number") + if number is not None and number not in seen: + seen.add(number) + prs.append(pr) + for branch in story.get("branches") or []: + for pr in branch.get("pull_requests") or []: + number = pr.get("number") + if number is not None and number not in seen: + seen.add(number) + prs.append(pr) + return prs diff --git a/build/ci/tests/test_mark_stories_deployed.py b/build/ci/tests/test_mark_stories_deployed.py index a1fd43b1ab..3704683dda 100644 --- a/build/ci/tests/test_mark_stories_deployed.py +++ b/build/ci/tests/test_mark_stories_deployed.py @@ -473,3 +473,4 @@ def _boom(*args, **kwargs): err = capsys.readouterr().err assert "silent no-op" not in err + diff --git a/build/ci/tests/test_merge_release_backfill.py b/build/ci/tests/test_merge_release_backfill.py new file mode 100644 index 0000000000..c986520c65 --- /dev/null +++ b/build/ci/tests/test_merge_release_backfill.py @@ -0,0 +1,261 @@ +"""Tests for build/ci/merge_release_backfill.py. + +No network calls, no `git`/`gh` calls: this script is a plain, deterministic +JSON merge with no I/O beyond reading --shipped-stories-out / +--reconcile-report and writing --out. + +All story ids used below (11111, 22222, ...) are placeholders, not real +Shortcut story ids. +""" + +import json + +import pytest + +import merge_release_backfill as mrb + +SHIPPED_STORIES_BASE = { + "version": "6.111.0-prod.2", + "chart_version": "0.87.5-prod.1", + "release_date": "2026-08-31T07:17:36Z", + "story_ids": ["11111"], + "stories": [{"id": 11111, "name": "Found by shipped_stories.py's own scan"}], +} + + +def _hydrated_story(story_id, name="Backfilled Story"): + return { + "id": story_id, + "name": name, + "description": "A description", + "url": f"https://app.shortcut.com/org/story/{story_id}", + "workflow_id": 500000005, + "workflow_state_id": 500000045, + "story_type": "feature", + } + + +def _reconcile_report(shipped): + return {"prod_tag": "prod/6.111.0-prod.2+chart.0.87.5-prod.1", "applied": True, + "counts": {"total": len(shipped), "shipped": len(shipped), "pending": 0, "triage": 0}, + "shipped": shipped, "pending": [], "triage": []} + + +# --- backfill_stories_from_report: trusts hydrated_story's presence only - + +def test_backfill_stories_from_report_returns_only_entries_with_hydrated_story(): + report = _reconcile_report([ + {"id": 22222, "hydrated_story": _hydrated_story(22222)}, + {"id": 33333, "shipping_release_tag": "prod/earlier"}, # no hydrated_story -- earlier release + {"id": 44444}, # unresolvable release -- also no hydrated_story + ]) + backfill = mrb.backfill_stories_from_report(report) + assert [s["id"] for s in backfill] == [22222] + + +def test_backfill_stories_from_report_empty_shipped_bucket(): + report = _reconcile_report([]) + assert mrb.backfill_stories_from_report(report) == [] + + +def test_backfill_stories_from_report_missing_shipped_key_degrades_to_empty(): + assert mrb.backfill_stories_from_report({"prod_tag": "prod/1.0"}) == [] + + +# --- merge: dedup, additive, provenance ----------------------------------- + +def test_merge_adds_a_story_whose_derived_release_is_the_current_tag(): + data = dict(SHIPPED_STORIES_BASE, story_ids=list(SHIPPED_STORIES_BASE["story_ids"]), + stories=list(SHIPPED_STORIES_BASE["stories"])) + backfill = [_hydrated_story(22222)] + merged, added_ids = mrb.merge(data, backfill) + assert added_ids == ["22222"] + assert merged["story_ids"] == ["11111", "22222"] + assert [s["id"] for s in merged["stories"]] == [11111, 22222] + assert merged["stories_from_reconciliation_backfill"] == ["22222"] + + +def test_merge_does_not_add_a_story_from_an_earlier_release(): + """The merge function itself only ever sees what backfill_stories_from_ + report already filtered down to -- this test drives that filtering + step too, confirming an earlier-release entry never reaches merge().""" + data = dict(SHIPPED_STORIES_BASE, story_ids=list(SHIPPED_STORIES_BASE["story_ids"]), + stories=list(SHIPPED_STORIES_BASE["stories"])) + report = _reconcile_report([{"id": 22222, "shipping_release_tag": "prod/6.100.0-prod.1+chart.0.85.8-prod.1"}]) + backfill = mrb.backfill_stories_from_report(report) + merged, added_ids = mrb.merge(data, backfill) + assert added_ids == [] + assert merged["story_ids"] == ["11111"] + + +def test_merge_does_not_add_a_story_with_unresolvable_release(): + data = dict(SHIPPED_STORIES_BASE, story_ids=list(SHIPPED_STORIES_BASE["story_ids"]), + stories=list(SHIPPED_STORIES_BASE["stories"])) + report = _reconcile_report([{"id": 22222, "shipping_release_tag": None}]) + backfill = mrb.backfill_stories_from_report(report) + merged, added_ids = mrb.merge(data, backfill) + assert added_ids == [] + assert merged["story_ids"] == ["11111"] + + +def test_merge_no_duplicates_when_both_discovery_paths_find_the_same_story(): + """A story shipped_stories.py's own git-range/RC1 discovery ALSO + found (already present in story_ids/stories) must not be duplicated + when the reconciliation sweep independently finds it too.""" + data = dict(SHIPPED_STORIES_BASE, story_ids=list(SHIPPED_STORIES_BASE["story_ids"]), + stories=list(SHIPPED_STORIES_BASE["stories"])) + backfill = [_hydrated_story(11111, name="Same story, found twice")] + merged, added_ids = mrb.merge(data, backfill) + assert added_ids == [] + assert merged["story_ids"] == ["11111"] + assert len(merged["stories"]) == 1 + # The pre-existing entry (shipped_stories.py's own hydration) wins -- + # never overwritten by the backfill's version of the same story. + assert merged["stories"][0]["name"] == "Found by shipped_stories.py's own scan" + + +def test_merge_multiple_backfill_stories_all_added(): + data = dict(SHIPPED_STORIES_BASE, story_ids=list(SHIPPED_STORIES_BASE["story_ids"]), + stories=list(SHIPPED_STORIES_BASE["stories"])) + backfill = [_hydrated_story(22222), _hydrated_story(33333)] + merged, added_ids = mrb.merge(data, backfill) + assert added_ids == ["22222", "33333"] + assert merged["story_ids"] == ["11111", "22222", "33333"] + + +def test_merge_empty_backfill_is_a_no_op(): + data = dict(SHIPPED_STORIES_BASE, story_ids=list(SHIPPED_STORIES_BASE["story_ids"]), + stories=list(SHIPPED_STORIES_BASE["stories"])) + merged, added_ids = mrb.merge(data, []) + assert added_ids == [] + assert merged["story_ids"] == ["11111"] + assert merged["stories_from_reconciliation_backfill"] == [] + + +def test_merge_preserves_other_shipped_stories_json_fields(): + """version/chart_version/release_date and any other existing field + must survive the merge untouched -- this script only ever touches + story_ids/stories/stories_from_reconciliation_backfill.""" + data = dict(SHIPPED_STORIES_BASE, story_ids=list(SHIPPED_STORIES_BASE["story_ids"]), + stories=list(SHIPPED_STORIES_BASE["stories"])) + merged, _ = mrb.merge(data, [_hydrated_story(22222)]) + assert merged["version"] == "6.111.0-prod.2" + assert merged["chart_version"] == "0.87.5-prod.1" + assert merged["release_date"] == "2026-08-31T07:17:36Z" + + +# --- main(): end to end, reads two files, writes merged result ----------- + +def test_main_merges_current_release_backfill_end_to_end(tmp_path): + shipped_path = tmp_path / "shipped-stories.json" + shipped_path.write_text(json.dumps(SHIPPED_STORIES_BASE), encoding="utf-8") + + report_path = tmp_path / "reconcile-report.json" + report_path.write_text(json.dumps(_reconcile_report([ + {"id": 22222, "hydrated_story": _hydrated_story(22222)}, + {"id": 33333, "shipping_release_tag": "prod/earlier"}, + ])), encoding="utf-8") + + out_path = tmp_path / "shipped-stories.json" # default: overwrite in place + + import sys + old_argv = sys.argv + try: + sys.argv = [ + "merge_release_backfill.py", + "--shipped-stories-out", str(shipped_path), + "--reconcile-report", str(report_path), + ] + mrb.main() + finally: + sys.argv = old_argv + + merged = json.loads(out_path.read_text(encoding="utf-8")) + assert merged["story_ids"] == ["11111", "22222"] + assert 33333 not in [s.get("id") for s in merged["stories"]] + + +def test_main_writes_to_explicit_out_path_when_given(tmp_path): + shipped_path = tmp_path / "shipped-stories.json" + shipped_path.write_text(json.dumps(SHIPPED_STORIES_BASE), encoding="utf-8") + report_path = tmp_path / "reconcile-report.json" + report_path.write_text(json.dumps(_reconcile_report([])), encoding="utf-8") + explicit_out = tmp_path / "merged.json" + + import sys + old_argv = sys.argv + try: + sys.argv = [ + "merge_release_backfill.py", + "--shipped-stories-out", str(shipped_path), + "--reconcile-report", str(report_path), + "--out", str(explicit_out), + ] + mrb.main() + finally: + sys.argv = old_argv + + assert explicit_out.exists() + # The original --shipped-stories-out is untouched when --out is given. + original = json.loads(shipped_path.read_text(encoding="utf-8")) + assert original == SHIPPED_STORIES_BASE + + +def test_main_missing_shipped_stories_file_dies_cleanly(tmp_path): + report_path = tmp_path / "reconcile-report.json" + report_path.write_text(json.dumps(_reconcile_report([])), encoding="utf-8") + + import sys + old_argv = sys.argv + try: + sys.argv = [ + "merge_release_backfill.py", + "--shipped-stories-out", str(tmp_path / "does-not-exist.json"), + "--reconcile-report", str(report_path), + ] + with pytest.raises(SystemExit): + mrb.main() + finally: + sys.argv = old_argv + + +def test_main_missing_reconcile_report_dies_cleanly(tmp_path): + shipped_path = tmp_path / "shipped-stories.json" + shipped_path.write_text(json.dumps(SHIPPED_STORIES_BASE), encoding="utf-8") + + import sys + old_argv = sys.argv + try: + sys.argv = [ + "merge_release_backfill.py", + "--shipped-stories-out", str(shipped_path), + "--reconcile-report", str(tmp_path / "does-not-exist.json"), + ] + with pytest.raises(SystemExit): + mrb.main() + finally: + sys.argv = old_argv + + +# --- isolation guarantee: this module never touches the reconcile --------- +# --- report's shipped/pending/triage wholesale, and never imports -------- +# --- anything that could reach the network or Shortcut ------------------- + +def test_merge_release_backfill_module_never_imports_network_or_subprocess(): + """This script only ever reads two JSON files and writes one -- it + must never import urllib, requests, or subprocess. It has no business + calling out to anything; if it ever needed to, that would be a sign + the isolation this script exists to preserve had already broken.""" + import ast + import inspect + + tree = ast.parse(inspect.getsource(mrb)) + imported_names = set() + for node in ast.walk(tree): + if isinstance(node, ast.Import): + imported_names.update(alias.name.split(".")[0] for alias in node.names) + elif isinstance(node, ast.ImportFrom) and node.module: + imported_names.add(node.module.split(".")[0]) + + forbidden = {"urllib", "requests", "subprocess", "socket", "http"} + assert not (imported_names & forbidden), imported_names & forbidden diff --git a/build/ci/tests/test_reconcile_deploy_ready.py b/build/ci/tests/test_reconcile_deploy_ready.py new file mode 100644 index 0000000000..a4dd44ef14 --- /dev/null +++ b/build/ci/tests/test_reconcile_deploy_ready.py @@ -0,0 +1,892 @@ +"""Tests for build/ci/reconcile_deploy_ready.py. + +No network, no real `git`/`gh` calls: anything that would call +urllib.request.urlopen, subprocess.run, or the module's own run_git is +monkeypatched, matching the style of test_shipped_stories.py and +test_mark_stories_deployed.py. + +All story ids used below (11111, 22222, ...) are placeholders, not real +Shortcut story ids. The repo/workflow/state ids (500000103, 500000005, +500000045, 500000010, 500000061) are real Shortcut/GitHub ids, not story +ids, and are not covered by that placeholder rule -- same convention as the +two existing test files. +""" + +import json + +import pytest + +import reconcile_deploy_ready as rdr + +STANDARD_WORKFLOW_ID = 500000005 +DEPLOY_READY_STATE_ID = 500000045 +DONE_STATE_ID = 500000010 +SEFARIA_REPO_ID = 500000103 +OTHER_REPO_ID = 500000124 # some other Shortcut-linked repo, not Sefaria-Project +# A non-Standard workflow (e.g. "Content") that also has some state it +# reports as workflow_state_id -- deliberately NOT 500000045, since that id +# means something else (or nothing) outside the Standard workflow. +OTHER_WORKFLOW_ID = 500000061 +OTHER_WORKFLOW_STATE_ID = 500000900 + + +def _pr(number, merged=True, repository_id=SEFARIA_REPO_ID, target_branch_name="master", + branch_name="feature/some-branch"): + return { + "number": number, + "merged": merged, + "repository_id": repository_id, + "target_branch_name": target_branch_name, + "branch_name": branch_name, + } + + +def _story(story_id, name="Story", workflow_id=STANDARD_WORKFLOW_ID, workflow_state_id=DEPLOY_READY_STATE_ID, + pull_requests=None, branches=None, description=None, comments=None, story_type="feature"): + return { + "id": story_id, + "name": name, + "app_url": f"https://app.shortcut.com/org/story/{story_id}", + "workflow_id": workflow_id, + "workflow_state_id": workflow_state_id, + "pull_requests": pull_requests or [], + "branches": branches or [], + "description": description, + "comments": comments or [], + "story_type": story_type, + } + + +# --- gather_linked_prs: dedup across pull_requests and branches[].pull_requests -- + +def test_gather_linked_prs_dedups_across_both_sources(): + """Shortcut duplicates the same PR object in story.pull_requests AND + story.branches[*].pull_requests -- the same PR number appearing in both + must not be double-counted.""" + pr = _pr(100) + story = _story(11111, pull_requests=[pr], branches=[{"pull_requests": [pr]}]) + prs = rdr.gather_linked_prs(story) + assert [p["number"] for p in prs] == [100] + + +def test_gather_linked_prs_collects_from_both_sources_when_numbers_differ(): + story = _story(11111, pull_requests=[_pr(100)], branches=[{"pull_requests": [_pr(200)]}]) + prs = rdr.gather_linked_prs(story) + assert sorted(p["number"] for p in prs) == [100, 200] + + +def test_gather_linked_prs_empty_story_returns_empty(): + story = _story(11111) + assert rdr.gather_linked_prs(story) == [] + + +def test_gather_linked_prs_ignores_prs_with_no_number(): + story = _story(11111, pull_requests=[{"merged": True}]) + assert rdr.gather_linked_prs(story) == [] + + +# --- qualifying_prs: the four PR-level guards (shortcut_pr_guards.py) ---- + +def test_qualifying_prs_wrong_repo_guard(): + """Guard: a PR linked from a DIFFERENT repo must never count as + evidence a Sefaria-Project story shipped, even if it's merged and + targets 'master' -- resolving it against Sefaria-Project would find an + unrelated PR that happens to share the number.""" + prs = [_pr(224, repository_id=OTHER_REPO_ID)] + assert rdr.qualifying_prs(prs) == [] + + +def test_qualifying_prs_promotion_pr_target_branch_guard(): + """Guard: a PR targeting anything other than 'master' (e.g. a + preprod->prod or master->preprod promotion PR some stories link instead + of the real feature PR) must not qualify.""" + prs = [_pr(3551, target_branch_name="prod")] + assert rdr.qualifying_prs(prs) == [] + prs2 = [_pr(3550, target_branch_name="preprod")] + assert rdr.qualifying_prs(prs2) == [] + + +def test_qualifying_prs_promotion_pr_head_branch_guard_preprod_to_master(): + """Guard (regression): a promotion PR merging preprod INTO master + passes the repo/merged/target-branch guards cleanly -- its target + really is 'master'. Only the HEAD branch (branch_name) reveals it's a + promotion merge, not a feature PR. This is the exact live false + positive that was found and fixed: a story was classified `shipped` on + the strength of a PR shaped exactly like this.""" + prs = [_pr(3677, target_branch_name="master", branch_name="preprod")] + assert rdr.qualifying_prs(prs) == [] + + +def test_qualifying_prs_promotion_pr_head_branch_guard_master_to_preprod(): + """The other promotion direction is already caught by the target-branch + guard, but the head-branch guard must reject it too, independently.""" + prs = [_pr(3698, target_branch_name="preprod", branch_name="master")] + assert rdr.qualifying_prs(prs) == [] + + +def test_qualifying_prs_unmerged_pr_guard(): + """Guard: an open or closed-without-merging PR proves nothing.""" + prs = [_pr(3397, merged=False)] + assert rdr.qualifying_prs(prs) == [] + + +def test_qualifying_prs_accepts_a_normal_feature_pr_passing_all_four_guards(): + prs = [_pr(3606, branch_name="feature/some-fix")] + assert rdr.qualifying_prs(prs) == prs + + +def test_qualifying_prs_filters_mixed_list_keeping_only_the_valid_one(): + prs = [ + _pr(224, repository_id=OTHER_REPO_ID), + _pr(3551, target_branch_name="prod"), + _pr(3397, merged=False), + _pr(3677, target_branch_name="master", branch_name="preprod"), + _pr(3606, branch_name="feature/some-fix"), + ] + assert [p["number"] for p in rdr.qualifying_prs(prs)] == [3606] + + +def test_qualifying_prs_custom_repo_and_target_branch_args(): + """--repo/--target-branch overrides are threaded through, not hardcoded.""" + prs = [_pr(1, repository_id=999, target_branch_name="main", branch_name="feature/some-fix")] + assert rdr.qualifying_prs(prs, repo_id=999, target_branch="main") == prs + assert rdr.qualifying_prs(prs, repo_id=SEFARIA_REPO_ID, target_branch="master") == [] + + +# --- classify_stories: guard #4 (workflow/state) + guard-filtered triage -- + +def test_classify_stories_non_standard_workflow_routes_to_triage_not_transition(): + """Guard #4: a story on a non-Standard workflow must be routed to + triage with its actual workflow/state ids reported -- never treated as + a shipping candidate, since 500000045/500000010 mean nothing there.""" + story = _story(11111, workflow_id=OTHER_WORKFLOW_ID, workflow_state_id=OTHER_WORKFLOW_STATE_ID, + pull_requests=[_pr(1)]) + triage, candidates = rdr.classify_stories([story], SEFARIA_REPO_ID, "master") + assert candidates == [] + assert len(triage) == 1 + assert triage[0]["id"] == 11111 + assert triage[0]["reason"] == "non_standard_workflow_or_state" + assert triage[0]["workflow_id"] == OTHER_WORKFLOW_ID + assert triage[0]["workflow_state_id"] == OTHER_WORKFLOW_STATE_ID + + +def test_classify_stories_unexpected_state_within_standard_workflow_routes_to_triage(): + """Guard #4 also keys on the NUMERIC state id, not just the workflow -- + a story on the Standard workflow but at some other state id must not + be treated as Deploy Ready just because it matched the search by name.""" + story = _story(11111, workflow_id=STANDARD_WORKFLOW_ID, workflow_state_id=500000099, + pull_requests=[_pr(1)]) + triage, candidates = rdr.classify_stories([story], SEFARIA_REPO_ID, "master") + assert candidates == [] + assert triage[0]["reason"] == "non_standard_workflow_or_state" + + +def test_classify_stories_no_qualifying_pr_routes_to_triage(): + story = _story(11111, pull_requests=[_pr(3397, merged=False)]) + triage, candidates = rdr.classify_stories([story], SEFARIA_REPO_ID, "master") + assert candidates == [] + assert triage[0]["reason"] == "no_qualifying_pr" + assert triage[0]["linked_pr_numbers"] == [3397] + + +# --- triage enrichment: description/comments/per-PR guard diagnostics --- +# --- (raw material for the opt-in triage-explainer step) ----------------- + +def test_diagnose_linked_pr_reports_every_failed_guard(): + pr = {"number": 1, "merged": False, "repository_id": 999, "target_branch_name": "preprod"} + failed = rdr._diagnose_linked_pr(pr, SEFARIA_REPO_ID, "master") + assert "not merged" in failed + assert any("wrong repo" in f for f in failed) + assert any("wrong target branch" in f for f in failed) + + +def test_diagnose_linked_pr_reports_no_failures_for_a_qualifying_pr(): + pr = _pr(3606) + assert rdr._diagnose_linked_pr(pr, SEFARIA_REPO_ID, "master") == [] + + +def test_diagnose_linked_pr_reports_only_the_specific_guard_that_failed(): + pr = _pr(3698, target_branch_name="preprod") # merged + right repo, wrong branch only + failed = rdr._diagnose_linked_pr(pr, SEFARIA_REPO_ID, "master") + assert failed == ["wrong target branch ('preprod', expected 'master')"] + + +def test_diagnose_linked_pr_reports_promotion_head_branch_guard(): + """A promotion PR (preprod -> master) passes merged/repo/target-branch + but must be flagged for its head branch -- the diagnostic must surface + the SAME reason qualifying_prs silently rejects it for.""" + pr = _pr(3677, target_branch_name="master", branch_name="preprod") + failed = rdr._diagnose_linked_pr(pr, SEFARIA_REPO_ID, "master") + assert failed == ["promotion PR (head branch 'preprod' is a long-lived environment branch)"] + + +def test_triage_context_extracts_description_and_comment_text(): + story = _story( + 11111, description="Some story description", + comments=[{"text": "first comment"}, {"text": "second comment"}, {"author_id": "x"}], + ) + ctx = rdr._triage_context(story) + assert ctx["description"] == "Some story description" + assert ctx["comments"] == ["first comment", "second comment"] + + +def test_triage_context_handles_missing_description_and_comments(): + story = {"id": 11111} + ctx = rdr._triage_context(story) + assert ctx["description"] is None + assert ctx["comments"] == [] + + +def test_classify_stories_no_qualifying_pr_entry_carries_full_context(monkeypatch): + """The raw material the opt-in explainer needs: description, comment + text, and -- per linked PR -- exactly which guard(s) it failed, not + just a bare number.""" + story = _story( + 11111, + pull_requests=[_pr(3698, target_branch_name="preprod"), _pr(3397, merged=False)], + description="A story description", + comments=[{"text": "why is this stuck?"}], + ) + triage, _ = rdr.classify_stories([story], SEFARIA_REPO_ID, "master") + entry = triage[0] + assert entry["description"] == "A story description" + assert entry["comments"] == ["why is this stuck?"] + assert entry["linked_pr_numbers"] == [3397, 3698] + by_number = {p["number"]: p["failed_guards"] for p in entry["linked_prs"]} + assert by_number[3698] == ["wrong target branch ('preprod', expected 'master')"] + assert by_number[3397] == ["not merged"] + + +def test_classify_stories_non_standard_workflow_entry_also_carries_context(): + story = _story( + 11111, workflow_id=500000061, workflow_state_id=500000900, + description="Lives on a different workflow", comments=[{"text": "not ours"}], + ) + triage, _ = rdr.classify_stories([story], SEFARIA_REPO_ID, "master") + entry = triage[0] + assert entry["description"] == "Lives on a different workflow" + assert entry["comments"] == ["not ours"] + + +def test_classify_stories_shipped_pending_candidates_do_not_carry_triage_context(monkeypatch): + """Shipped/pending stories must stay as lean as before this was added + -- _triage_context is triage-only, both by name and by application. + classify_stories only returns the raw (story, prs) pair for a + candidate; classify_candidates builds its own lean entry via + _story_summary, which never includes description/comments.""" + story = _story(11111, pull_requests=[_pr(3606)], description="should never appear") + triage, candidates = rdr.classify_stories([story], SEFARIA_REPO_ID, "master") + assert triage == [] + assert len(candidates) == 1 + + monkeypatch.setattr(rdr, "is_ancestor_of_prod", lambda oid, tag: True) + shipped, pending = rdr.classify_candidates(candidates, {3606: "abc"}, "prod/1.0") + assert "description" not in shipped[0] + assert "comments" not in shipped[0] + + +def test_classify_stories_story_with_no_linked_prs_at_all_routes_to_triage(): + story = _story(11111) + triage, candidates = rdr.classify_stories([story], SEFARIA_REPO_ID, "master") + assert triage[0]["reason"] == "no_qualifying_pr" + assert triage[0]["linked_pr_numbers"] == [] + + +def test_classify_stories_qualifying_story_becomes_a_candidate(): + story = _story(11111, pull_requests=[_pr(3606)]) + triage, candidates = rdr.classify_stories([story], SEFARIA_REPO_ID, "master") + assert triage == [] + assert len(candidates) == 1 + assert candidates[0][0]["id"] == 11111 + assert [p["number"] for p in candidates[0][1]] == [3606] + + +# --- classify_candidates: ancestor true vs false, unresolved oid --------- + +def test_classify_candidates_ancestor_true_is_shipped(monkeypatch): + monkeypatch.setattr(rdr, "is_ancestor_of_prod", lambda oid, tag: True) + monkeypatch.setattr(rdr, "resolve_shipping_release_tag", lambda oid: None) + story = _story(11111, pull_requests=[_pr(3606)]) + candidates = [(story, [_pr(3606)])] + shipped, pending = rdr.classify_candidates(candidates, {3606: "abc123"}, "prod/1.0") + assert pending == [] + assert len(shipped) == 1 + assert shipped[0]["id"] == 11111 + assert shipped[0]["shipped_via_prs"] == [3606] + assert shipped[0]["qualifying_prs"] == [3606] + + +def test_classify_candidates_ancestor_false_is_pending(monkeypatch): + monkeypatch.setattr(rdr, "is_ancestor_of_prod", lambda oid, tag: False) + story = _story(11111, pull_requests=[_pr(3670)]) + candidates = [(story, [_pr(3670)])] + shipped, pending = rdr.classify_candidates(candidates, {3670: "def456"}, "prod/1.0") + assert shipped == [] + assert len(pending) == 1 + assert pending[0]["id"] == 11111 + assert pending[0]["qualifying_prs"] == [3670] + + +def test_classify_candidates_unresolved_oid_never_counts_as_shipped(monkeypatch): + """A PR whose merge oid never resolved (gh lookup failed) must push a + story toward pending, never toward shipped -- a resolution failure must + never cause a wrong transition.""" + def _boom(oid, tag): + raise AssertionError("is_ancestor_of_prod must not be called for an unresolved oid") + + monkeypatch.setattr(rdr, "is_ancestor_of_prod", _boom) + story = _story(11111, pull_requests=[_pr(3606)]) + candidates = [(story, [_pr(3606)])] + shipped, pending = rdr.classify_candidates(candidates, {}, "prod/1.0") # oid_by_pr empty: unresolved + assert shipped == [] + assert len(pending) == 1 + + +def test_classify_candidates_inconclusive_ancestry_check_never_counts_as_shipped(monkeypatch): + """is_ancestor_of_prod returning None (the check itself couldn't run) + must also never be treated as "in prod".""" + monkeypatch.setattr(rdr, "is_ancestor_of_prod", lambda oid, tag: None) + story = _story(11111, pull_requests=[_pr(3606)]) + candidates = [(story, [_pr(3606)])] + shipped, pending = rdr.classify_candidates(candidates, {3606: "abc"}, "prod/1.0") + assert shipped == [] + assert len(pending) == 1 + + +def test_classify_candidates_any_qualifying_pr_in_prod_is_enough(monkeypatch): + """A story with two qualifying PRs, only one of which is in prod, still + ships -- 'at least one' per the spec.""" + monkeypatch.setattr(rdr, "is_ancestor_of_prod", lambda oid, tag: oid == "in-prod-oid") + monkeypatch.setattr(rdr, "resolve_shipping_release_tag", lambda oid: None) + + story = _story(11111, pull_requests=[_pr(1), _pr(2)]) + candidates = [(story, [_pr(1), _pr(2)])] + oid_by_pr = {1: "not-in-prod-oid", 2: "in-prod-oid"} + shipped, pending = rdr.classify_candidates(candidates, oid_by_pr, "prod/1.0") + assert len(shipped) == 1 + assert shipped[0]["shipped_via_prs"] == [2] + assert shipped[0]["qualifying_prs"] == [1, 2] + + +# --- classify_candidates: shipping_release_tag / hydrated_story ---------- +# --- (the "backfilled story that shipped in THIS release" fix) ----------- + +def test_classify_candidates_shipping_release_tag_equal_to_current_attaches_hydrated_story(monkeypatch): + """When the derived TRUE shipping release equals the CURRENT prod tag, + the entry gets a hydrated_story sub-object -- the ONLY signal + merge_release_backfill.py uses to fold a backfilled story into + shipped-stories.json.""" + monkeypatch.setattr(rdr, "is_ancestor_of_prod", lambda oid, tag: True) + monkeypatch.setattr(rdr, "resolve_shipping_release_tag", lambda oid: "prod/1.0") + story = _story(11111, name="Some Story", pull_requests=[_pr(3606)], description="A description") + candidates = [(story, [_pr(3606)])] + shipped, pending = rdr.classify_candidates(candidates, {3606: "abc"}, "prod/1.0") + assert shipped[0]["shipping_release_tag"] == "prod/1.0" + hydrated = shipped[0]["hydrated_story"] + assert hydrated == { + "id": 11111, + "name": "Some Story", + "description": "A description", + "url": f"https://app.shortcut.com/org/story/11111", + "workflow_id": STANDARD_WORKFLOW_ID, + "workflow_state_id": DEPLOY_READY_STATE_ID, + "story_type": "feature", + } + + +def test_classify_candidates_shipping_release_tag_earlier_than_current_excludes_hydrated_story(monkeypatch): + """A story that shipped in an EARLIER release must never carry + hydrated_story -- that's exactly the 'old features shipped today' + leak this whole design rejects.""" + monkeypatch.setattr(rdr, "is_ancestor_of_prod", lambda oid, tag: True) + monkeypatch.setattr(rdr, "resolve_shipping_release_tag", lambda oid: "prod/0.9-earlier") + story = _story(11111, pull_requests=[_pr(3606)]) + candidates = [(story, [_pr(3606)])] + shipped, pending = rdr.classify_candidates(candidates, {3606: "abc"}, "prod/1.0") + assert shipped[0]["shipping_release_tag"] == "prod/0.9-earlier" + assert "hydrated_story" not in shipped[0] + + +def test_classify_candidates_unresolvable_shipping_release_excludes_hydrated_story(monkeypatch): + """resolve_shipping_release_tag returning None (unresolvable) must + also never attach hydrated_story -- fail closed, never guess a story + into the announcement.""" + monkeypatch.setattr(rdr, "is_ancestor_of_prod", lambda oid, tag: True) + monkeypatch.setattr(rdr, "resolve_shipping_release_tag", lambda oid: None) + story = _story(11111, pull_requests=[_pr(3606)]) + candidates = [(story, [_pr(3606)])] + shipped, pending = rdr.classify_candidates(candidates, {3606: "abc"}, "prod/1.0") + assert shipped[0]["shipping_release_tag"] is None + assert "hydrated_story" not in shipped[0] + + +def test_classify_candidates_pending_entries_never_carry_shipping_release_tag(monkeypatch): + """shipping_release_tag/hydrated_story are shipped-only concepts -- a + pending story (nothing in prod yet) has no shipping release to derive + at all.""" + monkeypatch.setattr(rdr, "is_ancestor_of_prod", lambda oid, tag: False) + + def _boom(oid): + raise AssertionError("resolve_shipping_release_tag must not be called for a pending story") + + monkeypatch.setattr(rdr, "resolve_shipping_release_tag", _boom) + story = _story(11111, pull_requests=[_pr(3670)]) + candidates = [(story, [_pr(3670)])] + shipped, pending = rdr.classify_candidates(candidates, {3670: "def"}, "prod/1.0") + assert shipped == [] + assert "shipping_release_tag" not in pending[0] + assert "hydrated_story" not in pending[0] + + +def test_story_for_release_notes_matches_shipped_stories_hydrated_shape(): + story = _story(11111, name="A Story", description="Desc", story_type="bug") + hydrated = rdr._story_for_release_notes(story) + assert set(hydrated.keys()) == { + "id", "name", "description", "url", "workflow_id", "workflow_state_id", "story_type", + } + assert hydrated["url"] == story["app_url"] + + +# --- is_ancestor_of_prod: exit-code interpretation ----------------------- + +def test_is_ancestor_of_prod_true_on_exit_code_0(monkeypatch): + class _Proc: + returncode = 0 + stderr = "" + + monkeypatch.setattr(rdr.subprocess, "run", lambda *a, **k: _Proc()) + assert rdr.is_ancestor_of_prod("abc", "prod/1.0") is True + + +def test_is_ancestor_of_prod_false_on_exit_code_1(monkeypatch): + class _Proc: + returncode = 1 + stderr = "" + + monkeypatch.setattr(rdr.subprocess, "run", lambda *a, **k: _Proc()) + assert rdr.is_ancestor_of_prod("abc", "prod/1.0") is False + + +def test_is_ancestor_of_prod_none_on_other_exit_code(monkeypatch, capsys): + class _Proc: + returncode = 128 + stderr = "fatal: not a valid object name abc" + + monkeypatch.setattr(rdr.subprocess, "run", lambda *a, **k: _Proc()) + assert rdr.is_ancestor_of_prod("abc", "prod/1.0") is None + assert "WARNING" in capsys.readouterr().err + + +# --- search_deploy_ready_stories: pagination ------------------------------ + +class _FakeSearchResponse: + def __init__(self, payload): + self._body = json.dumps(payload).encode("utf-8") + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + def read(self): + return self._body + + +def test_search_deploy_ready_stories_paginates_via_next_cursor(monkeypatch): + """Two pages, exactly as verified live (20 stories, 10 per page): must + not assume a single page.""" + pages = [ + {"data": [{"id": 1}, {"id": 2}], "next": "/api/v3/search/stories?query=x&next=CURSOR1"}, + {"data": [{"id": 3}], "next": None}, + ] + calls = [] + + def _fake_urlopen(req, timeout=None): + calls.append(req.full_url) + return _FakeSearchResponse(pages[len(calls) - 1]) + + monkeypatch.setattr(rdr.urllib.request, "urlopen", _fake_urlopen) + stories = rdr.search_deploy_ready_stories("fake-token-for-tests") + assert [s["id"] for s in stories] == [1, 2, 3] + assert len(calls) == 2 + # The second call must follow the `next` cursor, not repeat page one. + assert "CURSOR1" in calls[1] + + +def test_search_deploy_ready_stories_single_page_stops_when_next_is_none(monkeypatch): + def _fake_urlopen(req, timeout=None): + return _FakeSearchResponse({"data": [{"id": 1}], "next": None}) + + monkeypatch.setattr(rdr.urllib.request, "urlopen", _fake_urlopen) + stories = rdr.search_deploy_ready_stories("fake-token-for-tests") + assert [s["id"] for s in stories] == [1] + + +# --- fetch_merge_oids: gh missing must not surface as a raw traceback ---- + +def test_fetch_merge_oids_dies_clearly_when_gh_is_missing(monkeypatch): + def _raise_missing_gh(*args, **kwargs): + raise FileNotFoundError("[Errno 2] No such file or directory: 'gh'") + + monkeypatch.setattr(rdr.subprocess, "run", _raise_missing_gh) + with pytest.raises(SystemExit): + rdr.fetch_merge_oids([3606], "Sefaria/Sefaria-Project") + + +def test_fetch_merge_oids_empty_input_makes_no_calls(monkeypatch): + def _boom(*args, **kwargs): + raise AssertionError("subprocess.run must not be called with no PR numbers") + + monkeypatch.setattr(rdr.subprocess, "run", _boom) + assert rdr.fetch_merge_oids([], "Sefaria/Sefaria-Project") == {} + + +def test_fetch_pr_merge_oid_failed_gh_call_returns_none(monkeypatch, capsys): + class _Proc: + returncode = 1 + stdout = "" + stderr = "PR not found" + + monkeypatch.setattr(rdr.subprocess, "run", lambda *a, **k: _Proc()) + pr_number, oid = rdr.fetch_pr_merge_oid(9999, "Sefaria/Sefaria-Project") + assert oid is None + assert "WARNING" in capsys.readouterr().err + + +# --- End-to-end main(): dry-run is the default and mutates nothing ------- + +def _make_main_env(monkeypatch, tmp_path, stories, prod_tag="prod/1.0", oid_by_pr=None, + ancestor_result=True, argv_extra=None, shipping_tag=None): + """Wire main() end-to-end with every I/O boundary mocked: Shortcut + search, gh merge-commit lookup, git ancestry, and the git-based + shipping-release lookup -- mirroring _run_main_with_commits in + test_shipped_stories.py. resolve_shipping_release_tag defaults to a + plain lambda returning `shipping_tag` (None unless overridden) rather + than shelling out to real git, so tests that don't care about that + specific behavior stay hermetic; the dedicated tests for it below + monkeypatch it themselves when they need to exercise its own + git-shelling logic.""" + monkeypatch.setenv("SHORTCUT_API_TOKEN", "fake-token-for-tests") + monkeypatch.setattr(rdr, "search_deploy_ready_stories", lambda token: stories) + monkeypatch.setattr(rdr, "resolve_default_prod_tag", lambda: prod_tag) + oid_map = oid_by_pr or {} + monkeypatch.setattr( + rdr, "fetch_merge_oids", + lambda pr_numbers, repo, max_workers=8: {n: oid_map[n] for n in pr_numbers if n in oid_map}, + ) + monkeypatch.setattr(rdr, "is_ancestor_of_prod", lambda oid, tag: ancestor_result) + monkeypatch.setattr(rdr, "resolve_shipping_release_tag", lambda oid: shipping_tag) + + argv = ["reconcile_deploy_ready.py"] + (argv_extra or []) + monkeypatch.setattr("sys.argv", argv) + + +def test_main_dry_run_is_the_default_and_never_calls_transition(monkeypatch, tmp_path, capsys): + story = _story(11111, pull_requests=[_pr(3606)]) + _make_main_env(monkeypatch, tmp_path, [story], oid_by_pr={3606: "abc"}, ancestor_result=True) + + def _boom(*args, **kwargs): + raise AssertionError("transition_story must never be called without --apply") + + monkeypatch.setattr(rdr, "transition_story", _boom) + rdr.main() # must not raise, must not exit non-zero + + out = capsys.readouterr().out + assert "shipped=1" in out + assert "would transition" in out + + +def test_main_apply_transitions_shipped_stories(monkeypatch, tmp_path, capsys): + story = _story(11111, pull_requests=[_pr(3606)]) + _make_main_env( + monkeypatch, tmp_path, [story], oid_by_pr={3606: "abc"}, ancestor_result=True, + argv_extra=["--apply"], + ) + + calls = [] + + def _fake_transition(story_id, done_state_id, token): + calls.append((story_id, done_state_id)) + return story_id, True, None + + monkeypatch.setattr(rdr, "transition_story", _fake_transition) + rdr.main() + + assert calls == [(11111, DONE_STATE_ID)] + + +def test_main_dry_run_flag_overrides_apply(monkeypatch, tmp_path, capsys): + """--dry-run always wins if somehow both --apply and --dry-run are + passed -- a bulk mutation across the whole org must never be one + flag-typo away from firing.""" + story = _story(11111, pull_requests=[_pr(3606)]) + _make_main_env( + monkeypatch, tmp_path, [story], oid_by_pr={3606: "abc"}, ancestor_result=True, + argv_extra=["--apply", "--dry-run"], + ) + + def _boom(*args, **kwargs): + raise AssertionError("transition_story must never be called when --dry-run is also passed") + + monkeypatch.setattr(rdr, "transition_story", _boom) + rdr.main() # must not raise + + +def test_main_writes_out_json_report(monkeypatch, tmp_path): + story_shipped = _story(11111, pull_requests=[_pr(3606)]) + story_triage = _story(22222) # no linked PRs at all + out_path = tmp_path / "report.json" + _make_main_env( + monkeypatch, tmp_path, [story_shipped, story_triage], + oid_by_pr={3606: "abc"}, ancestor_result=True, + argv_extra=["--out", str(out_path)], + ) + rdr.main() + + report = json.loads(out_path.read_text(encoding="utf-8")) + assert report["counts"] == {"total": 2, "shipped": 1, "pending": 0, "triage": 1} + assert report["applied"] is False + assert [s["id"] for s in report["shipped"]] == [11111] + assert [s["id"] for s in report["triage"]] == [22222] + + +def test_main_pending_bucket_when_qualifying_pr_not_yet_in_prod(monkeypatch, tmp_path): + story = _story(11111, pull_requests=[_pr(3670)]) + out_path = tmp_path / "report.json" + _make_main_env( + monkeypatch, tmp_path, [story], oid_by_pr={3670: "def"}, ancestor_result=False, + argv_extra=["--out", str(out_path)], + ) + rdr.main() + report = json.loads(out_path.read_text(encoding="utf-8")) + assert report["counts"] == {"total": 1, "shipped": 0, "pending": 1, "triage": 0} + assert report["pending"][0]["id"] == 11111 + + +def test_main_missing_token_exits_before_any_shortcut_call(monkeypatch, tmp_path): + monkeypatch.delenv("SHORTCUT_API_TOKEN", raising=False) + + def _boom(*args, **kwargs): + raise AssertionError("no Shortcut call should happen without a token") + + monkeypatch.setattr(rdr, "search_deploy_ready_stories", _boom) + monkeypatch.setattr("sys.argv", ["reconcile_deploy_ready.py"]) + with pytest.raises(SystemExit): + rdr.main() + + +def test_main_failed_transition_exits_non_zero_and_is_reported(monkeypatch, tmp_path, capsys): + """A story that fails to transition must be reported, not swallowed -- + and the process must exit non-zero so an unattended run can't look + successful.""" + story = _story(11111, pull_requests=[_pr(3606)]) + _make_main_env( + monkeypatch, tmp_path, [story], oid_by_pr={3606: "abc"}, ancestor_result=True, + argv_extra=["--apply"], + ) + monkeypatch.setattr(rdr, "transition_story", lambda sid, done, token: (sid, False, "HTTP 500 Internal Server Error")) + + with pytest.raises(SystemExit) as exc_info: + rdr.main() + assert exc_info.value.code != 0 + err = capsys.readouterr().err + assert "11111" in err + + +def test_main_apply_all_triage_no_shipped_warns_and_exits_nonzero(monkeypatch, tmp_path, capsys): + """Every story routes to triage, none shipped -- must not exit 0 as if + the backlog were clean.""" + story = _story(11111) # no linked PRs -> triage + _make_main_env(monkeypatch, tmp_path, [story], argv_extra=["--apply"]) + + with pytest.raises(SystemExit) as exc_info: + rdr.main() + assert exc_info.value.code != 0 + assert "WARNING" in capsys.readouterr().err + + +def test_main_dry_run_all_triage_no_shipped_does_not_exit_nonzero(monkeypatch, tmp_path): + story = _story(11111) + _make_main_env(monkeypatch, tmp_path, [story]) # dry-run default, no --apply + + rdr.main() # must not raise + + +def test_main_apply_all_triage_but_no_stories_at_all_does_not_exit_nonzero(monkeypatch, tmp_path): + """Empty backlog is not a silent no-op -- there's nothing to be silent about.""" + _make_main_env(monkeypatch, tmp_path, [], argv_extra=["--apply"]) + rdr.main() # must not raise + + +def test_main_prod_tag_override_is_used_instead_of_default(monkeypatch, tmp_path): + story = _story(11111, pull_requests=[_pr(3606)]) + out_path = tmp_path / "report.json" + + def _boom_default_tag(): + raise AssertionError("resolve_default_prod_tag must not be called when --prod-tag is given") + + monkeypatch.setenv("SHORTCUT_API_TOKEN", "fake-token-for-tests") + monkeypatch.setattr(rdr, "search_deploy_ready_stories", lambda token: [story]) + monkeypatch.setattr(rdr, "resolve_default_prod_tag", _boom_default_tag) + monkeypatch.setattr(rdr, "fetch_merge_oids", lambda pr_numbers, repo, max_workers=8: {3606: "abc"}) + monkeypatch.setattr(rdr, "is_ancestor_of_prod", lambda oid, tag: tag == "prod/explicit-tag") + monkeypatch.setattr(rdr, "resolve_shipping_release_tag", lambda oid: None) + monkeypatch.setattr( + "sys.argv", + ["reconcile_deploy_ready.py", "--prod-tag", "prod/explicit-tag", "--out", str(out_path)], + ) + rdr.main() + report = json.loads(out_path.read_text(encoding="utf-8")) + assert report["prod_tag"] == "prod/explicit-tag" + assert report["counts"]["shipped"] == 1 + + +def test_workflow_passes_prod_tag_to_reconcile_step(): + """reconcile_deploy_ready.py's own --prod-tag handling is correct + (tested above), but nothing at that level can catch the workflow + itself forgetting to pass the flag -- assert the step's run block + actually does.""" + import pathlib + + workflow = pathlib.Path(__file__).resolve().parents[3] / ".github" / "workflows" / "prod-release-notes.yaml" + text = workflow.read_text(encoding="utf-8") + start = text.index("Reconcile Deploy Ready backlog") + end = text.index("- name:", start + 1) + assert "--prod-tag" in text[start:end] + + +# --- resolve_default_prod_tag: newest by creation date ------------------- + +def test_resolve_default_prod_tag_picks_newest(monkeypatch): + monkeypatch.setattr( + rdr, "run_git", + lambda args: "prod/2.0+chart.1\nprod/1.0+chart.1\n", + ) + assert rdr.resolve_default_prod_tag() == "prod/2.0+chart.1" + + +def test_resolve_default_prod_tag_dies_with_no_tags(monkeypatch): + monkeypatch.setattr(rdr, "run_git", lambda args: "") + with pytest.raises(SystemExit): + rdr.resolve_default_prod_tag() + + +# --- resolve_shipping_release_tag: the TRUE (earliest) release, not the -- +# --- current --prod-tag -- see the module docstring for why this matters -- + +def test_resolve_shipping_release_tag_picks_earliest_containing_tag(monkeypatch): + """--sort=creatordate (ascending) here is the OPPOSITE of + resolve_default_prod_tag's -creatordate -- this wants the FIRST release + that ever contained the commit, not the newest tag overall.""" + class _Proc: + returncode = 0 + stdout = "prod/6.100.0-prod.1+chart.0.85.8-prod.1\nprod/6.111.0-prod.2+chart.0.87.5-prod.1\n" + stderr = "" + + captured_args = {} + + def _fake_run(args, **kwargs): + captured_args["args"] = args + return _Proc() + + monkeypatch.setattr(rdr.subprocess, "run", _fake_run) + tag = rdr.resolve_shipping_release_tag("abc123") + assert tag == "prod/6.100.0-prod.1+chart.0.85.8-prod.1" + assert "--contains" in captured_args["args"] + assert "abc123" in captured_args["args"] + assert "--sort=creatordate" in captured_args["args"] # ascending, not -creatordate + + +def test_resolve_shipping_release_tag_returns_none_when_no_tag_contains_it(monkeypatch): + class _Proc: + returncode = 0 + stdout = "" + stderr = "" + + monkeypatch.setattr(rdr.subprocess, "run", lambda *a, **k: _Proc()) + assert rdr.resolve_shipping_release_tag("abc123") is None + + +def test_resolve_shipping_release_tag_returns_none_and_warns_on_git_failure(monkeypatch, capsys): + class _Proc: + returncode = 128 + stdout = "" + stderr = "fatal: malformed object name abc123" + + monkeypatch.setattr(rdr.subprocess, "run", lambda *a, **k: _Proc()) + assert rdr.resolve_shipping_release_tag("abc123") is None + assert "WARNING" in capsys.readouterr().err + + + +# --- main(): shipping_release_tag / hydrated_story end-to-end ------------ + +def test_main_report_attaches_hydrated_story_when_shipping_release_matches_current(monkeypatch, tmp_path): + """End-to-end: a shipped story whose derived release equals the + CURRENT prod tag gets a hydrated_story entry in the report -- the + signal a separate merge step uses to fold it into today's release + notes.""" + story = _story(11111, name="Some Story", pull_requests=[_pr(3606)], description="A description") + out_path = tmp_path / "report.json" + _make_main_env( + monkeypatch, tmp_path, [story], oid_by_pr={3606: "abc"}, ancestor_result=True, + prod_tag="prod/1.0", shipping_tag="prod/1.0", + argv_extra=["--out", str(out_path)], + ) + rdr.main() + report = json.loads(out_path.read_text(encoding="utf-8")) + entry = report["shipped"][0] + assert entry["shipping_release_tag"] == "prod/1.0" + assert entry["hydrated_story"]["id"] == 11111 + assert entry["hydrated_story"]["name"] == "Some Story" + + +def test_main_report_excludes_hydrated_story_for_an_earlier_release(monkeypatch, tmp_path): + story = _story(11111, pull_requests=[_pr(3606)]) + out_path = tmp_path / "report.json" + _make_main_env( + monkeypatch, tmp_path, [story], oid_by_pr={3606: "abc"}, ancestor_result=True, + prod_tag="prod/9.9.9-prod.1+chart.9.9.9-prod.1", shipping_tag="prod/6.100.0-prod.1+chart.0.85.8-prod.1", + argv_extra=["--out", str(out_path)], + ) + rdr.main() + report = json.loads(out_path.read_text(encoding="utf-8")) + entry = report["shipped"][0] + assert entry["shipping_release_tag"] == "prod/6.100.0-prod.1+chart.0.85.8-prod.1" + assert "hydrated_story" not in entry + + +def test_main_report_excludes_hydrated_story_when_release_unresolvable(monkeypatch, tmp_path): + story = _story(11111, pull_requests=[_pr(3606)]) + out_path = tmp_path / "report.json" + _make_main_env( + monkeypatch, tmp_path, [story], oid_by_pr={3606: "abc"}, ancestor_result=True, + prod_tag="prod/9.9.9-prod.1+chart.9.9.9-prod.1", shipping_tag=None, + argv_extra=["--out", str(out_path)], + ) + rdr.main() + report = json.loads(out_path.read_text(encoding="utf-8")) + entry = report["shipped"][0] + assert entry["shipping_release_tag"] is None + assert "hydrated_story" not in entry + + +def test_main_never_posts_to_shortcut_beyond_the_state_transition(monkeypatch, tmp_path): + """Regression guard for the removed write-back comment feature: even + on a real --apply run, the only Shortcut mutation this script performs + is the workflow_state_id PUT (transition_story) -- nothing else calls + out to Shortcut.""" + story = _story(11111, pull_requests=[_pr(3606)]) + _make_main_env( + monkeypatch, tmp_path, [story], oid_by_pr={3606: "abc"}, ancestor_result=True, + argv_extra=["--apply"], + ) + calls = [] + monkeypatch.setattr( + rdr, "transition_story", + lambda sid, done, token: (calls.append(sid), sid, True, None)[1:], + ) + rdr.main() + assert calls == [11111] + assert not hasattr(rdr, "shortcut_comment") diff --git a/build/ci/tests/test_shipped_stories.py b/build/ci/tests/test_shipped_stories.py index 49e82900d0..0f1a8c3f9f 100644 --- a/build/ci/tests/test_shipped_stories.py +++ b/build/ci/tests/test_shipped_stories.py @@ -381,6 +381,207 @@ def test_fetch_story_workflow_id_defaults_to_none_when_absent(monkeypatch): assert data["workflow_id"] is None +# --- fetch_story_by_pr_link: RC1 Shortcut PR<->story fallback ---------- +# (story ids below, e.g. 66666, are placeholders, not real Shortcut ids, +# per repo convention. PR numbers like 3653 are real-shaped GitHub PR +# numbers used only as plausible test fixtures -- PR numbers are not +# Shortcut story ids and are not covered by that convention.) + + +def _story_with_linked_pr(story_id, pr_number, merged=True, repository_id=500000103, target_branch_name="master", + branch_name="feature/some-branch"): + """Minimal Shortcut search-result story payload carrying ONE linked PR + -- enough for fetch_story_by_pr_link's guard re-check + (shortcut_pr_guards.gather_linked_prs / passes_pr_guards) to find and + evaluate it. repository_id 500000103 is Sefaria-Project's real + Shortcut repository id, not a story id -- exempt from the + placeholder-id convention, same as elsewhere in this file.""" + return { + "id": story_id, + "pull_requests": [{ + "number": int(pr_number), + "merged": merged, + "repository_id": repository_id, + "target_branch_name": target_branch_name, + "branch_name": branch_name, + }], + } + + +def test_fetch_story_by_pr_link_adopts_single_match(monkeypatch): + monkeypatch.setattr( + ss.urllib.request, "urlopen", + lambda req, timeout=None: _FakeShortcutResponse( + {"data": [_story_with_linked_pr(66666, "3653")], "total": 1} + ), + ) + pr_number, story_id = ss.fetch_story_by_pr_link("3653", "fake-token-for-tests") + assert pr_number == "3653" + assert story_id == "66666" + + +def test_fetch_story_by_pr_link_wrong_repo_guard_rejects(monkeypatch, capsys): + """The story IS linked to this PR number, but that PR is against a + DIFFERENT repo -- resolving it against Sefaria-Project would be + exactly the wrong-repo false positive the guard exists to prevent.""" + monkeypatch.setattr( + ss.urllib.request, "urlopen", + lambda req, timeout=None: _FakeShortcutResponse( + {"data": [_story_with_linked_pr(66666, "224", repository_id=500000124)], "total": 1} + ), + ) + pr_number, story_id = ss.fetch_story_by_pr_link("224", "fake-token-for-tests") + assert story_id is None + err = capsys.readouterr().err + assert "does not pass the shipping-evidence guards" in err + assert "224" in err + + +def test_fetch_story_by_pr_link_non_master_target_branch_guard_rejects(monkeypatch, capsys): + """The linked PR merged, against the right repo, but targets `preprod` + (a promotion PR) instead of `master` -- must never be adopted as + evidence a story's own change shipped. This is the exact live failure + case: a promotion PR resolves via `pr:` to a real story just as + readily as that story's actual feature PR does.""" + monkeypatch.setattr( + ss.urllib.request, "urlopen", + lambda req, timeout=None: _FakeShortcutResponse( + {"data": [_story_with_linked_pr(66666, "3698", target_branch_name="preprod")], "total": 1} + ), + ) + pr_number, story_id = ss.fetch_story_by_pr_link("3698", "fake-token-for-tests") + assert story_id is None + assert "does not pass the shipping-evidence guards" in capsys.readouterr().err + + +def test_fetch_story_by_pr_link_unmerged_pr_guard_rejects(monkeypatch, capsys): + monkeypatch.setattr( + ss.urllib.request, "urlopen", + lambda req, timeout=None: _FakeShortcutResponse( + {"data": [_story_with_linked_pr(66666, "3606", merged=False)], "total": 1} + ), + ) + pr_number, story_id = ss.fetch_story_by_pr_link("3606", "fake-token-for-tests") + assert story_id is None + assert "does not pass the shipping-evidence guards" in capsys.readouterr().err + + +def test_fetch_story_by_pr_link_promotion_head_branch_guard_rejects_preprod_to_master(monkeypatch, capsys): + """Guard #4 regression: a promotion PR merging preprod INTO master + passes guards 1-3 cleanly (merged, right repo, target IS master) -- + only the HEAD branch reveals it's a promotion merge, not a feature PR. + This is the exact live false positive that was found: a story was + classified shipped on the strength of a PR shaped exactly like this.""" + monkeypatch.setattr( + ss.urllib.request, "urlopen", + lambda req, timeout=None: _FakeShortcutResponse( + {"data": [_story_with_linked_pr(66666, "3677", target_branch_name="master", branch_name="preprod")], + "total": 1} + ), + ) + pr_number, story_id = ss.fetch_story_by_pr_link("3677", "fake-token-for-tests") + assert story_id is None + assert "does not pass the shipping-evidence guards" in capsys.readouterr().err + + +def test_fetch_story_by_pr_link_promotion_head_branch_guard_rejects_master_to_preprod(monkeypatch, capsys): + """The other promotion direction (master -> preprod) is already caught + by guard #3 (target branch must be master) -- but guard #4 must reject + it too, independently, since the two checks are not redundant (a + linked PR could in principle target 'master' while also having a + long-lived head branch for some other reason).""" + monkeypatch.setattr( + ss.urllib.request, "urlopen", + lambda req, timeout=None: _FakeShortcutResponse( + {"data": [_story_with_linked_pr(66666, "3698", target_branch_name="master", branch_name="master")], + "total": 1} + ), + ) + pr_number, story_id = ss.fetch_story_by_pr_link("3698", "fake-token-for-tests") + assert story_id is None + assert "does not pass the shipping-evidence guards" in capsys.readouterr().err + + +def test_fetch_story_by_pr_link_normal_feature_pr_still_passes_all_four_guards(monkeypatch): + """A real feature PR (merged, right repo, targets master, head branch + is an ordinary feature branch) must still be adopted -- guard #4 must + not be so broad it rejects legitimate PRs.""" + monkeypatch.setattr( + ss.urllib.request, "urlopen", + lambda req, timeout=None: _FakeShortcutResponse( + {"data": [_story_with_linked_pr(66666, "3606", branch_name="feature/some-fix")], "total": 1} + ), + ) + pr_number, story_id = ss.fetch_story_by_pr_link("3606", "fake-token-for-tests") + assert story_id == "66666" + + +def test_fetch_story_by_pr_link_no_match_returns_none_quietly(monkeypatch, capsys): + monkeypatch.setattr( + ss.urllib.request, "urlopen", + lambda req, timeout=None: _FakeShortcutResponse({"data": [], "total": 0}), + ) + pr_number, story_id = ss.fetch_story_by_pr_link("9999", "fake-token-for-tests") + assert story_id is None + # No story linked is an ordinary, expected outcome -- not a warning. + assert capsys.readouterr().err == "" + + +def test_fetch_story_by_pr_link_ambiguous_match_warns_and_skips(monkeypatch, capsys): + """More than one story resolves for the same PR -- never guess which + one is right.""" + monkeypatch.setattr( + ss.urllib.request, "urlopen", + lambda req, timeout=None: _FakeShortcutResponse({"data": [{"id": 1}, {"id": 2}], "total": 2}), + ) + pr_number, story_id = ss.fetch_story_by_pr_link("3677", "fake-token-for-tests") + assert story_id is None + err = capsys.readouterr().err + assert "ambiguous" in err + assert "3677" in err + + +def test_fetch_story_by_pr_link_http_error_does_not_abort(monkeypatch, capsys): + def _boom(req, timeout=None): + raise ss.urllib.error.HTTPError(req.full_url, 500, "Internal Server Error", None, None) + + monkeypatch.setattr(ss.urllib.request, "urlopen", _boom) + pr_number, story_id = ss.fetch_story_by_pr_link("42", "fake-token-for-tests") + assert story_id is None + assert "WARNING" in capsys.readouterr().err + + +def test_fetch_story_by_pr_link_network_error_does_not_abort(monkeypatch, capsys): + def _boom(req, timeout=None): + raise ss.urllib.error.URLError("network is down") + + monkeypatch.setattr(ss.urllib.request, "urlopen", _boom) + pr_number, story_id = ss.fetch_story_by_pr_link("42", "fake-token-for-tests") + assert story_id is None + assert "WARNING" in capsys.readouterr().err + + +def test_fetch_stories_by_pr_skips_unresolved(monkeypatch): + monkeypatch.setattr(ss, "fetch_story_by_pr_link", lambda pr, token: (pr, None)) + assert ss.fetch_stories_by_pr(["1", "2"], "fake-token-for-tests") == {} + + +def test_fetch_stories_by_pr_collects_only_resolved_ids(monkeypatch): + def _fake(pr, token): + return (pr, "66666") if pr == "3653" else (pr, None) + + monkeypatch.setattr(ss, "fetch_story_by_pr_link", _fake) + assert ss.fetch_stories_by_pr(["1", "3653"], "fake-token-for-tests") == {"3653": "66666"} + + +def test_fetch_stories_by_pr_empty_input_makes_no_calls(monkeypatch): + def _boom(*args, **kwargs): + raise AssertionError("fetch_story_by_pr_link must not be called with no PR numbers") + + monkeypatch.setattr(ss, "fetch_story_by_pr_link", _boom) + assert ss.fetch_stories_by_pr([], "fake-token-for-tests") == {} + + # --- main(): revert commits are suppressed from story_ids and reported -- def _run_main_with_commits(monkeypatch, tmp_path, commit_subjects): @@ -556,6 +757,261 @@ def _fake_run_git(args): assert data["release_date"] == "2026-08-31T07:17:36Z" +# --- main(): the RC1 Shortcut PR-link fallback end-to-end --------------- + +def test_main_recovers_story_id_via_shortcut_pr_link_fallback(monkeypatch, tmp_path): + """Real-shaped regression case: PR #3653 merged with branch name + `feature/prod-rollout-slack-release-notes` -- no sc-NNNNN code anywhere + in the subject or the branch -- but Shortcut's own PR<->story link knew + it was story 66666. Both git-only discovery sources come up empty; only + the RC1 fallback recovers it, and it must be reported in + stories_from_shortcut_pr_link specifically (not just story_ids).""" + + def _fake_run_git(args): + if args[0] == "log": + return "Announce prod releases from Argo's post-promotion analysis (#3653)\n" + if args[0] == "for-each-ref": + return "" + raise AssertionError(f"unexpected git call: {args!r}") + + monkeypatch.setattr(ss, "run_git", _fake_run_git) + monkeypatch.setattr( + ss, "fetch_pr_branch", + lambda pr_number, repo: (pr_number, "feature/prod-rollout-slack-release-notes"), + ) + monkeypatch.setenv("SHORTCUT_API_TOKEN", "fake-token-for-tests") + + def _fake_urlopen(req, timeout=None): + if "search/stories" in req.full_url: + return _FakeShortcutResponse({"data": [_story_with_linked_pr(66666, "3653")], "total": 1}) + # hydrate_stories' fetch_story call for the id the fallback recovered. + return _FakeShortcutResponse({ + "id": 66666, "name": "Story", "description": "", "app_url": "u", + "workflow_id": 500000005, "workflow_state_id": 500000045, "story_type": "feature", + }) + + monkeypatch.setattr(ss.urllib.request, "urlopen", _fake_urlopen) + + out_path = tmp_path / "shipped-stories.json" + monkeypatch.setattr( + "sys.argv", + ["shipped_stories.py", "--range", "prev-tag..cur-tag", "--out", str(out_path)], + ) + ss.main() + data = json.loads(out_path.read_text(encoding="utf-8")) + assert data["stories_from_shortcut_pr_link"] == ["66666"] + assert data["story_ids"] == ["66666"] + + +# --- main(): promotion PRs must never be adopted via the RC1 fallback --- +# --- (regression for the live false-positive the coordinator found) ----- + +def test_main_pr_link_fallback_skips_bare_merge_commit_promotion_pr(monkeypatch, tmp_path): + """Real live evidence: a bare 'Merge pull request #N from + Sefaria/preprod' promotion-merge subject resolves via `pr:` to a + real story (exactly as readily as a real feature PR would) but proves + nothing about whether that story shipped. NOISE_PATTERN already + recognizes this exact subject shape (it's excluded from + commits_without_story for the same underlying reason) -- reused here + to filter it out of the fallback candidate set BEFORE any network call, + not merely relying on fetch_story_by_pr_link's own guard re-check.""" + + def _fake_run_git(args): + if args[0] == "log": + return "Merge pull request #3698 from Sefaria/preprod\n" + if args[0] == "for-each-ref": + return "" + raise AssertionError(f"unexpected git call: {args!r}") + + def _boom(*args, **kwargs): + raise AssertionError( + "urlopen must never be called for a commit whose subject is NOISE_PATTERN noise " + "-- the pre-filter must exclude it before any network call is attempted" + ) + + monkeypatch.setattr(ss, "run_git", _fake_run_git) + # A bare merge-commit subject like this has no `(#N)` form for + # fetch_pr_branch to key off of via extract_pr_number's parenthesized + # pattern -- MERGE_PR_PATTERN resolves the PR number from the subject + # itself, so the branch lookup below is irrelevant to reaching pr_number + # but is still stubbed defensively. + monkeypatch.setattr(ss, "fetch_pr_branch", lambda pr_number, repo: (pr_number, None)) + monkeypatch.setenv("SHORTCUT_API_TOKEN", "fake-token-for-tests") + monkeypatch.setattr(ss.urllib.request, "urlopen", _boom) + + out_path = tmp_path / "shipped-stories.json" + monkeypatch.setattr( + "sys.argv", + ["shipped_stories.py", "--range", "prev-tag..cur-tag", "--out", str(out_path)], + ) + ss.main() + data = json.loads(out_path.read_text(encoding="utf-8")) + assert data["stories_from_shortcut_pr_link"] == [] + assert data["story_ids"] == [] + + +def test_main_pr_link_fallback_skips_long_lived_env_head_branch(monkeypatch, tmp_path): + """Second layer of the same guard: even for a commit whose subject does + NOT match NOISE_PATTERN (e.g. a squash-merged promotion PR with an + ordinary-looking subject), a PR whose own HEAD branch is a long-lived + environment branch (master/preprod/prod) must still never reach the + Shortcut lookup -- that's the shape a promotion PR takes regardless of + how its merge commit's subject happens to read.""" + + def _fake_run_git(args): + if args[0] == "log": + return "chore: promote build (#3699)\n" + if args[0] == "for-each-ref": + return "" + raise AssertionError(f"unexpected git call: {args!r}") + + def _boom(*args, **kwargs): + raise AssertionError( + "urlopen must never be called for a commit whose PR's head branch is a " + "long-lived environment branch" + ) + + monkeypatch.setattr(ss, "run_git", _fake_run_git) + monkeypatch.setattr(ss, "fetch_pr_branch", lambda pr_number, repo: (pr_number, "master")) + monkeypatch.setenv("SHORTCUT_API_TOKEN", "fake-token-for-tests") + monkeypatch.setattr(ss.urllib.request, "urlopen", _boom) + + out_path = tmp_path / "shipped-stories.json" + monkeypatch.setattr( + "sys.argv", + ["shipped_stories.py", "--range", "prev-tag..cur-tag", "--out", str(out_path)], + ) + ss.main() + data = json.loads(out_path.read_text(encoding="utf-8")) + assert data["stories_from_shortcut_pr_link"] == [] + assert data["story_ids"] == [] + + +def test_main_pr_link_fallback_skips_revert_commits(monkeypatch, tmp_path): + """A revert's own merge-commit PR must never trigger the fallback -- + its story ids are suppressed from the shipped set regardless, so a + recovered id here would appear in stories_from_shortcut_pr_link while + never actually being in story_ids/stories.""" + + def _fake_run_git(args): + if args[0] == "log": + return 'Revert "feat: something (#123)" (#456)\n' + if args[0] == "for-each-ref": + return "" + raise AssertionError(f"unexpected git call: {args!r}") + + def _boom(*args, **kwargs): + raise AssertionError("urlopen must never be called for a revert commit's PR") + + monkeypatch.setattr(ss, "run_git", _fake_run_git) + monkeypatch.setattr(ss, "fetch_pr_branch", lambda pr_number, repo: (pr_number, "feature/some-branch")) + monkeypatch.setenv("SHORTCUT_API_TOKEN", "fake-token-for-tests") + monkeypatch.setattr(ss.urllib.request, "urlopen", _boom) + + out_path = tmp_path / "shipped-stories.json" + monkeypatch.setattr( + "sys.argv", + ["shipped_stories.py", "--range", "prev-tag..cur-tag", "--out", str(out_path)], + ) + ss.main() + data = json.loads(out_path.read_text(encoding="utf-8")) + assert data["stories_from_shortcut_pr_link"] == [] + + +def test_main_pr_link_fallback_not_triggered_when_subject_already_has_story_id(monkeypatch, tmp_path): + """A commit whose subject already carries a story id must never trigger + the fallback lookup at all -- it has nothing missing to recover.""" + + def _fake_run_git(args): + if args[0] == "log": + return "fix(sc-13): a change (#42)\n" + if args[0] == "for-each-ref": + return "" + raise AssertionError(f"unexpected git call: {args!r}") + + def _boom(*args, **kwargs): + raise AssertionError("urlopen must not be called when the commit already has a story id") + + monkeypatch.setattr(ss, "run_git", _fake_run_git) + monkeypatch.setattr(ss, "fetch_pr_branch", lambda pr_number, repo: (pr_number, None)) + monkeypatch.setenv("SHORTCUT_API_TOKEN", "fake-token-for-tests") + monkeypatch.setattr(ss.urllib.request, "urlopen", _boom) + + out_path = tmp_path / "shipped-stories.json" + monkeypatch.setattr( + "sys.argv", + ["shipped_stories.py", "--range", "prev-tag..cur-tag", "--out", str(out_path)], + ) + ss.main() + data = json.loads(out_path.read_text(encoding="utf-8")) + assert data["stories_from_shortcut_pr_link"] == [] + assert data["story_ids"] == ["13"] + + +def test_main_pr_link_fallback_skipped_without_token(monkeypatch, tmp_path, capsys): + """SHORTCUT_API_TOKEN gate: without a token, the fallback must be + skipped entirely (no network call attempted) and must not crash the + run -- it degrades to git-only discovery.""" + + def _fake_run_git(args): + if args[0] == "log": + return "Some subject (#42)\n" + if args[0] == "for-each-ref": + return "" + raise AssertionError(f"unexpected git call: {args!r}") + + def _boom(*args, **kwargs): + raise AssertionError("urlopen must never be called without a token") + + monkeypatch.setattr(ss, "run_git", _fake_run_git) + monkeypatch.setattr(ss, "fetch_pr_branch", lambda pr_number, repo: (pr_number, "no-story-code-branch")) + monkeypatch.delenv("SHORTCUT_API_TOKEN", raising=False) + monkeypatch.setattr(ss.urllib.request, "urlopen", _boom) + + out_path = tmp_path / "shipped-stories.json" + monkeypatch.setattr( + "sys.argv", + ["shipped_stories.py", "--range", "prev-tag..cur-tag", "--out", str(out_path)], + ) + ss.main() + data = json.loads(out_path.read_text(encoding="utf-8")) + assert data["stories_from_shortcut_pr_link"] == [] + assert data["story_ids"] == [] + assert "SHORTCUT_API_TOKEN is not set" in capsys.readouterr().err + + +def test_main_pr_link_fallback_lookup_failure_does_not_abort_run(monkeypatch, tmp_path): + """A Shortcut PR-link lookup failure (network error, HTTP error, ...) + must be a warn-and-skip, never something that aborts shipped_stories.py + entirely -- the whole point of finding/fixing RC1 must not introduce a + new way for the script to die.""" + + def _fake_run_git(args): + if args[0] == "log": + return "Some subject (#42)\n" + if args[0] == "for-each-ref": + return "" + raise AssertionError(f"unexpected git call: {args!r}") + + def _boom(req, timeout=None): + raise ss.urllib.error.URLError("network is down") + + monkeypatch.setattr(ss, "run_git", _fake_run_git) + monkeypatch.setattr(ss, "fetch_pr_branch", lambda pr_number, repo: (pr_number, "no-story-code-branch")) + monkeypatch.setenv("SHORTCUT_API_TOKEN", "fake-token-for-tests") + monkeypatch.setattr(ss.urllib.request, "urlopen", _boom) + + out_path = tmp_path / "shipped-stories.json" + monkeypatch.setattr( + "sys.argv", + ["shipped_stories.py", "--range", "prev-tag..cur-tag", "--out", str(out_path)], + ) + ss.main() # must return normally, not raise + data = json.loads(out_path.read_text(encoding="utf-8")) + assert data["stories_from_shortcut_pr_link"] == [] + assert data["story_ids"] == [] + + # --- fetch_branches: a missing `gh` binary must not surface as a raw ------ # --- traceback (finding #11) ---------------------------------------------- diff --git a/build/ci/tests/test_triage_explainer.py b/build/ci/tests/test_triage_explainer.py new file mode 100644 index 0000000000..9c7bac69cc --- /dev/null +++ b/build/ci/tests/test_triage_explainer.py @@ -0,0 +1,221 @@ +"""Tests for build/ci/triage_explainer.py. + +No network calls, no `git`/`gh` calls: this script is a plain, deterministic +JSON transform with no I/O beyond reading --report and writing --out. + +All story ids used below (11111, 22222, ...) are placeholders, not real +Shortcut story ids. +""" + +import json + +import pytest + +import triage_explainer as te + +FULL_REPORT = { + "prod_tag": "prod/7.1.3-prod.1+chart.0.88.2-prod.1", + "applied": False, + "counts": {"total": 3, "shipped": 1, "pending": 1, "triage": 1}, + "shipped": [ + {"id": 11111, "name": "Shipped story", "url": "https://app.shortcut.com/org/story/11111", + "shipped_via_prs": [3606], "qualifying_prs": [3606], "transitioned": True, + "shipping_release_tag": "prod/7.1.3-prod.1+chart.0.88.2-prod.1", + "hydrated_story": {"id": 11111, "name": "Shipped story", "description": "should never leak", + "url": "https://app.shortcut.com/org/story/11111"}}, + ], + "pending": [ + {"id": 22222, "name": "Pending story", "url": "https://app.shortcut.com/org/story/22222", + "qualifying_prs": [3670]}, + ], + "triage": [ + {"id": 33333, "name": "Triage story", "url": "https://app.shortcut.com/org/story/33333", + "reason": "no_qualifying_pr", "linked_pr_numbers": [3698], + "linked_prs": [{"number": 3698, "failed_guards": ["wrong target branch ('preprod', expected 'master')"]}], + "description": "A story description", "comments": ["why is this stuck?"]}, + ], +} + + +# --- extract_triage_only: the structural isolation guarantee ------------ + +def test_extract_triage_only_excludes_shipped_and_pending_entirely(): + result = te.extract_triage_only(FULL_REPORT) + assert "shipped" not in result + assert "pending" not in result + # Not just absent as top-level keys -- the shipped/pending story data + # itself (ids, names, hydrated_story text) must not appear anywhere in + # the serialized output. + serialized = json.dumps(result) + assert "11111" not in serialized + assert "22222" not in serialized + assert "should never leak" not in serialized + + +def test_extract_triage_only_excludes_applied_and_counts(): + result = te.extract_triage_only(FULL_REPORT) + assert "applied" not in result + assert "counts" not in result # counts.shipped/pending would otherwise leak bucket sizes + + +def test_extract_triage_only_includes_triage_verbatim_with_full_context(): + result = te.extract_triage_only(FULL_REPORT) + assert result["triage"] == FULL_REPORT["triage"] + assert result["triage"][0]["description"] == "A story description" + assert result["triage"][0]["comments"] == ["why is this stuck?"] + assert result["triage"][0]["linked_prs"][0]["failed_guards"] == [ + "wrong target branch ('preprod', expected 'master')" + ] + + +def test_extract_triage_only_includes_prod_tag_and_triage_count(): + result = te.extract_triage_only(FULL_REPORT) + assert result["prod_tag"] == "prod/7.1.3-prod.1+chart.0.88.2-prod.1" + assert result["triage_count"] == 1 + + +def test_extract_triage_only_empty_triage_bucket(): + report = {**FULL_REPORT, "triage": []} + result = te.extract_triage_only(report) + assert result["triage"] == [] + assert result["triage_count"] == 0 + + +def test_extract_triage_only_missing_triage_key_degrades_to_empty(): + """A report from a hypothetical older/different reconcile run without a + 'triage' key at all must not crash -- it's just zero triage stories.""" + report = {"prod_tag": "prod/1.0"} + result = te.extract_triage_only(report) + assert result["triage"] == [] + assert result["triage_count"] == 0 + + +# --- main(): reads --report, writes --out --------------------------------- + +def test_main_reads_report_and_writes_triage_only_document(tmp_path, monkeypatch): + report_path = tmp_path / "reconcile-deploy-ready-report.json" + report_path.write_text(json.dumps(FULL_REPORT), encoding="utf-8") + out_path = tmp_path / "triage-only.json" + + monkeypatch.setattr( + "sys.argv", + ["triage_explainer.py", "extract", "--report", str(report_path), "--out", str(out_path)], + ) + te.main() + + data = json.loads(out_path.read_text(encoding="utf-8")) + assert "shipped" not in data + assert "pending" not in data + assert data["triage_count"] == 1 + assert data["triage"][0]["id"] == 33333 + + +def test_main_missing_report_file_dies_cleanly(tmp_path, monkeypatch): + monkeypatch.setattr( + "sys.argv", + ["triage_explainer.py", "extract", "--report", str(tmp_path / "does-not-exist.json"), + "--out", str(tmp_path / "out.json")], + ) + with pytest.raises(SystemExit): + te.main() + + +def test_main_malformed_report_json_dies_cleanly(tmp_path, monkeypatch): + report_path = tmp_path / "bad.json" + report_path.write_text("not valid json{{{", encoding="utf-8") + monkeypatch.setattr( + "sys.argv", + ["triage_explainer.py", "extract", "--report", str(report_path), "--out", str(tmp_path / "out.json")], + ) + with pytest.raises(SystemExit): + te.main() + + +# --- resolve_enabled: the opt-in decision, single-sourced and tested ---- +# --- ("disabled path" coverage) ------------------------------------------- + +def test_resolve_enabled_defaults_to_disabled_with_no_input_at_all(): + assert te.resolve_enabled("workflow_dispatch", "", "") is False + assert te.resolve_enabled("repository_dispatch", "", "") is False + + +def test_resolve_enabled_workflow_dispatch_true_input_enables(): + assert te.resolve_enabled("workflow_dispatch", "true", "") is True + + +def test_resolve_enabled_workflow_dispatch_ignores_the_repo_variable(): + """A workflow_dispatch run opts in via ITS OWN input only -- the + repo-level variable is for the repository_dispatch path and must never + leak in and silently enable a manual run that didn't ask for it.""" + assert te.resolve_enabled("workflow_dispatch", "", "true") is False + + +def test_resolve_enabled_repository_dispatch_true_var_enables(): + assert te.resolve_enabled("repository_dispatch", "", "true") is True + + +def test_resolve_enabled_repository_dispatch_ignores_the_workflow_dispatch_input(): + """The real automatic trigger carries no explain_triage input at all + (it's a repository_dispatch, not a workflow_dispatch) -- even if that + field were somehow non-empty, it must never be read on this path.""" + assert te.resolve_enabled("repository_dispatch", "true", "") is False + + +@pytest.mark.parametrize("value", ["false", "1", "yes", "True ", " true", "TRUE", "truex", None]) +def test_resolve_enabled_only_exact_true_enables(value): + # "TRUE" / "True " / " true" are still accepted (case/whitespace + # tolerant); everything else in this list is not. + result = te.resolve_enabled("workflow_dispatch", value, "") + if value is not None and value.strip().lower() == "true": + assert result is True + else: + assert result is False + + +def test_resolve_enabled_cli_prints_true_or_false(capsys, monkeypatch): + monkeypatch.setattr( + "sys.argv", + ["triage_explainer.py", "resolve-enabled", "--event-name", "workflow_dispatch", + "--explain-triage-input", "true", "--enable-var", ""], + ) + te.main() + assert capsys.readouterr().out.strip() == "true" + + +def test_resolve_enabled_cli_prints_false_when_disabled(capsys, monkeypatch): + monkeypatch.setattr( + "sys.argv", + ["triage_explainer.py", "resolve-enabled", "--event-name", "repository_dispatch", + "--explain-triage-input", "", "--enable-var", ""], + ) + te.main() + assert capsys.readouterr().out.strip() == "false" + + +# --- structural no-mutation / no-network guarantee ("NO mutation occurs -- +# --- on any path" coverage for the Python side of this feature) ---------- + +def test_triage_explainer_module_never_imports_network_or_subprocess(): + """Architectural guarantee, not just a convention: this script is a + pure JSON transform plus a pure string-comparison decision function. It + must never import urllib, requests, subprocess, or anything else that + could reach a network or mutate external/process state -- the actual + LLM call (a `claude -p` CLI invocation) and any Shortcut mutation stay + entirely in the opt-in workflow step and the deterministic + reconcile_deploy_ready.py / mark_stories_deployed.py scripts, never + here. Checked by inspecting this module's own top-level imports + directly, not the source text (so a forbidden name appearing inside a + string or comment can't cause a false failure).""" + import ast + import inspect + + tree = ast.parse(inspect.getsource(te)) + imported_names = set() + for node in ast.walk(tree): + if isinstance(node, ast.Import): + imported_names.update(alias.name.split(".")[0] for alias in node.names) + elif isinstance(node, ast.ImportFrom) and node.module: + imported_names.add(node.module.split(".")[0]) + + forbidden = {"urllib", "requests", "subprocess", "socket", "http"} + assert not (imported_names & forbidden), imported_names & forbidden diff --git a/build/ci/triage_explainer.py b/build/ci/triage_explainer.py new file mode 100644 index 0000000000..c4bce63873 --- /dev/null +++ b/build/ci/triage_explainer.py @@ -0,0 +1,93 @@ +#!/usr/bin/env python3 +""" +Extracts the triage bucket from a reconcile_deploy_ready.py report into a +separate, isolated document, and resolves whether the opt-in triage +explainer should run. + +Usage: + python3 triage_explainer.py extract --report reconcile-deploy-ready-report.json --out triage-only.json + python3 triage_explainer.py resolve-enabled --event-name workflow_dispatch --explain-triage-input true --enable-var "" +""" + +import argparse +import json +import sys + +# Must never appear in the explainer's output. +EXCLUDED_REPORT_KEYS = frozenset({"shipped", "pending", "applied"}) + + +def die(message: str) -> None: + print(f"ERROR: {message}", file=sys.stderr) + sys.exit(1) + + +def extract_triage_only(report): + """Build the explainer's entire input via an explicit allow-list: triage list, prod_tag, and triage_count.""" + triage = report.get("triage") or [] + result = { + "prod_tag": report.get("prod_tag"), + "triage_count": len(triage), + "triage": triage, + } + # Defensive: should be unreachable given the allow-list above. + leaked = EXCLUDED_REPORT_KEYS & result.keys() + if leaked: + die(f"internal error: triage-only extraction would have leaked {sorted(leaked)} -- refusing to write it") + return result + + +def resolve_enabled(event_name, explain_triage_input, enable_var): + """Whether the opt-in triage explainer should run: workflow_dispatch opts in via its own input, other triggers via the ENABLE_TRIAGE_EXPLAINER repo variable.""" + value = explain_triage_input if event_name == "workflow_dispatch" else enable_var + return str(value).strip().lower() == "true" + + +def build_arg_parser(): + parser = argparse.ArgumentParser( + description="Extract the triage bucket from a reconcile_deploy_ready.py report, or " + "resolve whether the opt-in triage explainer should run -- for the " + "opt-in triage-explainer workflow step.", + ) + subparsers = parser.add_subparsers(dest="command", required=True) + + extract_parser = subparsers.add_parser( + "extract", help="Write the triage-only document for a reconcile report to --out.", + ) + extract_parser.add_argument("--report", required=True, help="Path to a reconcile_deploy_ready.py --out report JSON") + extract_parser.add_argument("--out", required=True, help="Output path for the triage-only document") + + enabled_parser = subparsers.add_parser( + "resolve-enabled", + help="Print 'true' or 'false' to stdout: should the explainer run for this trigger?", + ) + enabled_parser.add_argument("--event-name", required=True, help="github.event_name, e.g. workflow_dispatch") + enabled_parser.add_argument("--explain-triage-input", default="", help="inputs.explain_triage (workflow_dispatch only)") + enabled_parser.add_argument("--enable-var", default="", help="vars.ENABLE_TRIAGE_EXPLAINER") + + return parser + + +def main(): + args = build_arg_parser().parse_args() + + if args.command == "resolve-enabled": + print("true" if resolve_enabled(args.event_name, args.explain_triage_input, args.enable_var) else "false") + return + + # command == "extract" + try: + with open(args.report, encoding="utf-8") as f: + report = json.load(f) + except (OSError, json.JSONDecodeError) as e: + die(f"Could not read/parse --report {args.report!r}: {e}") + + triage_only = extract_triage_only(report) + + with open(args.out, "w", encoding="utf-8") as f: + json.dump(triage_only, f, indent=2, ensure_ascii=False) + f.write("\n") + + +if __name__ == "__main__": + main()