diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 2cdac73f31..4b8aa2a8fe 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -151,6 +151,47 @@ git commit -m "Sync code examples from docs-code-eval" git push ``` +## UI label drift + +**Workflow**: `uidrift-scan.yml` + +Watches `wandb/core` for user-facing label changes that leave this repo's docs stale, and carries the resulting report in one rolling draft PR. The detector is `scripts/uidrift`; see [`scripts/uidrift/ADAPTING.md`](../../scripts/uidrift/ADAPTING.md) for what it looks for and why. This workflow is only the sink. + +### Setup required before the first run + +`wandb-docs-source-reader` is installed on `wandb/docs-code-eval` and `wandb/weave-internal` only, so **it cannot read `wandb/core` yet**. Pick one: + +- **Preferred**: install `wandb-docs-source-reader` on `wandb/core` with **Contents: read**. Needs a `wandb` org owner. No secret changes here; the workflow already asks for `repositories: core`. +- **Fallback**: add a repository secret `WANDB_CORE_TOKEN` holding a token that can read `wandb/core`. The workflow prefers the App and falls back to this, so adding the App install later needs no edit. + +With neither in place the first step fails immediately and names both options, rather than burning four minutes on a clone that cannot authenticate. + +### Triggers + +- **Scheduled**: weekdays at 13:00 UTC (6am PT), so a report is waiting at standup +- **Manual**: `workflow_dispatch` with `since` (window start for a non-incremental run), `seed` (ignore existing reports and rescan the whole window), and `dry-run` (report to the job summary, open no PR) + +### What it does + +1. Clones `wandb/core` — full history, single branch, no working tree. The ADAPTING.md table records why shallow and blobless clones were both rejected; do not "optimize" this without reading it. +2. Runs the scan. `--incremental` by default, taking its base from the head SHA in the newest report filename under `uidrift/reports/`; falls back to `--since` when no report exists yet. +3. Writes the report to the job summary, so a run is readable even when it opens no PR. +4. If there are findings (or a reopened decision), opens or updates a **draft PR** on the rolling branch `uidrift/drift-report` with the funnel counts, lane breakdown, and how to record a decision. +5. Fails the run — after the PR exists — if any stored decision reopened. That means a writer's earlier dismissal no longer matches the docs, which only a human can settle. + +Merging the PR advances the watermark. Closing it unmerged is also safe: the next run rescans the same range and supersedes the report. + +### Reviewing a report + +Each row lands in one of three lanes: **agent** (mechanical rename, safe to apply), **pair** (a writer scopes it, an agent applies it), **human** (prose has to be written). Rows that are wrong get recorded rather than deleted: + +```bash +PYTHONPATH=scripts python3 -m uidrift.scan decide \ + --status dismissed --by --agreement false_positive --note '' +``` + +`--agreement` is the detector's only feedback channel and cannot be reconstructed later. A dismissal reopens by itself if docs later start covering that surface, so it suppresses a row without hiding it forever. + ## Readability delta **Workflow**: `readability-delta.yml` diff --git a/.github/workflows/uidrift-scan.yml b/.github/workflows/uidrift-scan.yml new file mode 100644 index 0000000000..8f95debc18 --- /dev/null +++ b/.github/workflows/uidrift-scan.yml @@ -0,0 +1,370 @@ +name: UI label drift + +# Watches wandb/core for user-facing label changes that leave wandb/docs stale +# and carries the resulting report in one rolling draft PR. +# +# The detector is scripts/uidrift; scripts/uidrift/ADAPTING.md explains what it +# looks for and why. This workflow is only part 4 of that anatomy -- the sink. It +# supplies a wandb/core checkout, runs the scan, and turns the markdown report +# into something a writer can review and merge. + +on: + schedule: + # Weekday mornings at 13:00 UTC (6am PT), so a report is waiting at standup. + # Weekends are skipped: wandb/core barely moves and the watermark simply + # widens Monday's range, which costs seconds. + - cron: '0 13 * * 1-5' + workflow_dispatch: + inputs: + since: + description: 'Window start, as a git date. Used only when there is no report to resume from.' + type: string + default: '60 days ago' + seed: + description: 'Ignore existing reports and rescan the whole --since window' + type: boolean + default: false + dry-run: + description: 'Scan and print the report to the job summary; open no PR' + type: boolean + default: false + +permissions: + contents: write + pull-requests: write + +concurrency: + # Queue rather than cancel. Two runs must never write the rolling branch at + # once, and a cancelled run leaves no report -- the next one would just rescan + # the same range, so there is nothing to gain by killing one early. + group: uidrift + cancel-in-progress: false + +jobs: + scan: + runs-on: ubuntu-latest + # The wandb/core clone is the slow part (~3-4 min for 1.3 GB); the scan + # itself is seconds to ~2 minutes depending on the window. + timeout-minutes: 30 + + env: + # Only a manual dispatch can be dry or seeded: `inputs` is empty on schedule. + DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && inputs['dry-run'] || false }} + SEED: ${{ github.event_name == 'workflow_dispatch' && inputs.seed || false }} + SINCE: ${{ inputs.since || '60 days ago' }} + # The one variable that points the detector at a checkout; see + # config.SourceRepo.local_path_env. + CORE_REPO: ${{ runner.temp }}/core + SUMMARY_JSON: ${{ runner.temp }}/uidrift-summary.json + PR_BODY: ${{ runner.temp }}/uidrift-pr-body.md + + steps: + # Two ways to read a private wandb/core, checked before spending four + # minutes on a clone that cannot authenticate. + - name: Require a wandb/core read credential + id: creds + env: + APP_CLIENT_ID: ${{ vars.DOCS_SOURCE_READER_CLIENT_ID }} + APP_KEY: ${{ secrets.DOCS_SOURCE_READER_PRIVATE_KEY }} + CORE_PAT: ${{ secrets.WANDB_CORE_TOKEN }} + run: | + set -euo pipefail + if [ -n "$APP_CLIENT_ID" ] && [ -n "$APP_KEY" ]; then + echo "use_app=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + echo "use_app=false" >> "$GITHUB_OUTPUT" + if [ -n "$CORE_PAT" ]; then + echo "Using the WANDB_CORE_TOKEN secret; no App credentials configured." + exit 0 + fi + echo "::error::Nothing here can read wandb/core. Either install the wandb-docs-source-reader App on wandb/core with Contents: read (today it is installed only on docs-code-eval and weave-internal), or add a WANDB_CORE_TOKEN secret that can read wandb/core." + exit 1 + + - name: Create wandb/core read token + id: core_token + if: steps.creds.outputs.use_app == 'true' + # Not fatal. The App has to be installed on wandb/core for this to + # succeed, and WANDB_CORE_TOKEN is the documented fallback while that + # install is pending. The clone step reports the real failure. + continue-on-error: true + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3 + with: + client-id: ${{ vars.DOCS_SOURCE_READER_CLIENT_ID }} + private-key: ${{ secrets.DOCS_SOURCE_READER_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} + repositories: core + permission-contents: read + + - name: Require app credentials for the PR + if: env.DRY_RUN != 'true' + env: + PR_CLIENT_ID: ${{ vars.DOCS_PR_WRITER_CLIENT_ID }} + PR_KEY: ${{ secrets.DOCS_PR_WRITER_PRIVATE_KEY }} + run: | + if [ -z "$PR_CLIENT_ID" ] || [ -z "$PR_KEY" ]; then + echo "::error::DOCS_PR_WRITER_CLIENT_ID (variable) and DOCS_PR_WRITER_PRIVATE_KEY (secret) must be set in Settings → Secrets and variables → Actions." + exit 1 + fi + + # The default GITHUB_TOKEN would open the PR but would not trigger the + # pull_request checks on it, leaving required checks permanently pending. + # See "GitHub App authentication" in .github/workflows/README.md. + - name: Create token for wandb-docs-pr-writer + id: pr_token + if: env.DRY_RUN != 'true' + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3 + with: + client-id: ${{ vars.DOCS_PR_WRITER_CLIENT_ID }} + private-key: ${{ secrets.DOCS_PR_WRITER_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} + repositories: docs + + - name: Checkout docs + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + # Shallow is fine: the scan reads the docs tree as files, and its + # watermark comes from the report filenames, not from git history. + token: ${{ steps.pr_token.outputs.token || secrets.GITHUB_TOKEN }} + + - name: Set up Python + uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6 + with: + python-version: '3.11' + # No dependency step: scripts/uidrift is stdlib-only by design, and + # _vendor/ holds the three modules it borrows from another repo. + + - name: Clone wandb/core + env: + CORE_TOKEN: ${{ steps.core_token.outputs.token || secrets.WANDB_CORE_TOKEN }} + GIT_TERMINAL_PROMPT: "0" + run: | + set -euo pipefail + if [ -z "$CORE_TOKEN" ]; then + echo "::error::Could not mint a wandb/core App token and no WANDB_CORE_TOKEN secret is set. Install wandb-docs-source-reader on wandb/core with Contents: read, or add the secret." + exit 1 + fi + + # Run from RUNNER_TEMP, not the workspace. Inside the docs checkout git + # would apply that repo's persisted http.extraheader -- a GITHUB_TOKEN + # scoped to wandb/docs alone -- to this fetch, and a private wandb/core + # answers that with 404 rather than a permission error. + cd "$RUNNER_TEMP" + + header="Authorization: Basic $(printf 'x-access-token:%s' "$CORE_TOKEN" | base64 -w0)" + echo "::add-mask::${header#Authorization: Basic }" + + # Full history, single branch, no working tree. Each of those is + # measured rather than assumed: + # * Full, not shallow: ownership falls back to all-time authorship + # for files with thin recent history, which is exactly what a + # truncated clone cannot answer. A 7-month and an 18-month clone + # each named different reviewers than complete history did. + # * --no-checkout: every read is plumbing (log, show, rev-list), so + # materializing a 2 GB working tree buys nothing. + # * Not --filter=blob:none: iter_commits reads --numstat, which needs + # blob content. On a blobless clone a one-day window spent 82 + # seconds lazy-fetching and then died on the promisor remote. + git -c "http.extraheader=${header}" clone --quiet \ + --no-checkout --single-branch --branch master \ + https://github.com/wandb/core.git "$CORE_REPO" + + git -C "$CORE_REPO" log -1 --format='wandb/core master at %h (%ci)' origin/master + + - name: Scan for label drift + id: scan + run: | + set -uo pipefail + + # --incremental takes its base from the head SHA in the newest report + # already committed, so a first-ever run -- or a --seed dispatch -- has + # to name a window instead. + if [ "$SEED" != "true" ] && compgen -G 'uidrift/reports/*.md' > /dev/null; then + mode=(--incremental) + else + mode=(--since "$SINCE") + fi + echo "Scanning with: ${mode[*]}" + + set +e + PYTHONPATH=scripts python3 -m uidrift.scan scan "${mode[@]}" \ + --summary-json "$SUMMARY_JSON" + code=$? + set -e + + # 0 clean, 3 a decision reopened, anything else operator error. + if [ "$code" -ne 0 ] && [ "$code" -ne 3 ]; then + echo "::error::uidrift scan failed with exit $code. See scripts/uidrift/ADAPTING.md for the exit-code contract." + exit "$code" + fi + + { + echo "exit_code=$code" + echo "findings=$(jq -r '.findings' "$SUMMARY_JSON")" + echo "reopened=$(jq -r '.reopened' "$SUMMARY_JSON")" + echo "suppressed=$(jq -r '.suppressed' "$SUMMARY_JSON")" + echo "report=$(jq -r '.report // ""' "$SUMMARY_JSON")" + echo "head=$(jq -r '.head[0:12]' "$SUMMARY_JSON")" + echo "range=$(jq -r '.scanned_range' "$SUMMARY_JSON")" + # A reopened decision is drift too: a writer's earlier dismissal no + # longer matches the docs, so the row is live again and belongs in + # the PR even when nothing else was found. + echo "actionable=$(jq -r '.findings + .reopened' "$SUMMARY_JSON")" + } >> "$GITHUB_OUTPUT" + + - name: Publish the report to the job summary + env: + REPORT: ${{ steps.scan.outputs.report }} + run: | + set -euo pipefail + # The run is worth reading even when it opens no PR -- a dry run, or a + # quiet window whose only news is that there is no news. + if [ -n "$REPORT" ] && [ -f "$REPORT" ]; then + head -c 900000 "$REPORT" >> "$GITHUB_STEP_SUMMARY" + fi + { + echo + echo '
Run counts' + echo + echo '```json' + cat "$SUMMARY_JSON" + echo '```' + echo + echo '
' + } >> "$GITHUB_STEP_SUMMARY" + + - name: Compose the PR body + if: steps.scan.outputs.actionable != '0' && env.DRY_RUN != 'true' + # Every scan value arrives through env, never interpolated into the + # script. `range` echoes the --since input, which a dispatch controls, so + # `${{ }}` here would be a command-substitution hole. + env: + RANGE: ${{ steps.scan.outputs.range }} + HEAD_SHA: ${{ steps.scan.outputs.head }} + REPORT: ${{ steps.scan.outputs.report }} + REOPENED: ${{ steps.scan.outputs.reopened }} + run: | + set -euo pipefail + { + echo "UI labels changed in \`wandb/core\` that this repo may now describe wrongly." + echo + echo "**Range**: \`${RANGE}\` (head \`${HEAD_SHA}\`)" + echo "**Report**: \`${REPORT}\`" + echo + jq -r '"| Stage | Count |\n|---|---|\n" + + "| Commits in range | \(.commits) |\n" + + "| Touching UI paths | \(.ui_commits) |\n" + + "| Stage-1 candidates | \(.candidate_commits) (\(.candidate_deltas) changed strings) |\n" + + "| **Findings** | **\(.findings)** |\n" + + "| Suppressed by the ledger | \(.suppressed) |\n" + + "| Reopened | \(.reopened) |"' "$SUMMARY_JSON" + echo + + lanes=$(jq -r '.lanes | to_entries | map("\(.value) \(.key)") | join(", ")' "$SUMMARY_JSON") + # An `&&` one-liner would abort the whole body under `set -e` on the + # run that has no lanes to report. + if [ -n "$lanes" ]; then + echo "**Lanes**: $lanes" + fi + echo + + if [ "$REOPENED" != "0" ]; then + echo "> [!WARNING]" + echo "> ${REOPENED} stored decision(s) reopened: docs have started" + echo "> covering a surface that someone previously dismissed, so the dismissal is" + echo "> now hiding a real finding. Re-decide those rows rather than merging past them." + echo + fi + + echo "### How to review" + echo + echo "Read the report, then act per lane:" + echo + echo "- **agent** — mechanical rename; the label swap is safe to apply as-is." + echo "- **pair** — a writer scopes it first, then an agent applies it." + echo "- **human** — prose has to be written; the row only tells you where." + echo + echo "Rows that are wrong or not worth doing get recorded, not deleted:" + echo + echo '```bash' + echo "PYTHONPATH=scripts python3 -m uidrift.scan decide \\" + echo " --status dismissed --by --agreement false_positive --note ''" + echo '```' + echo + echo "\`--agreement\` is the detector's only feedback channel and cannot be" + echo "reconstructed later. A dismissal reopens by itself if docs later start" + echo "covering the surface, so it suppresses a row without hiding it forever." + echo + echo "Merging this PR advances the scan watermark: the next run resumes from" + echo "\`${HEAD_SHA}\`. Closing it without merging is also fine —" + echo "the next run rescans the same range and supersedes this report." + echo + echo "---" + echo "*Opened by \`.github/workflows/uidrift-scan.yml\`. Detector: \`scripts/uidrift\` ([how it works](https://github.com/wandb/docs/blob/main/scripts/uidrift/ADAPTING.md)).*" + } > "$PR_BODY" + + - name: Open or update the drift PR + id: pr + if: steps.scan.outputs.actionable != '0' && env.DRY_RUN != 'true' + uses: peter-evans/create-pull-request@5f6978faf089d4d20b00c7766989d076bb2fc7f1 # v8 + with: + token: ${{ steps.pr_token.outputs.token }} + base: main + # One rolling branch, not one per run. The watermark is the newest + # report on main, so every run regenerates the delta since the last + # MERGED report: a superseded report has nothing left to say, and one + # PR per weekday would bury the current one. + branch: uidrift/drift-report + delete-branch: true + draft: true + # Only the report. Nothing else should have changed, and the scan never + # writes the ledger -- only `decide` does. + add-paths: uidrift/reports + # Deliberately no `[skip ci]`, though the ADAPTING.md anatomy names it: + # a skipped run reports no status, so required pull_request checks would + # sit pending forever and the PR could never merge. Nothing expensive + # runs anyway -- Validate MDX sees no .mdx or .json in the diff and + # short-circuits, and .mintignore keeps uidrift/ out of the site build. + commit-message: | + Report UI label drift through ${{ steps.scan.outputs.head }} + + ${{ steps.scan.outputs.findings }} finding(s) over ${{ steps.scan.outputs.range }}. + Generated by .github/workflows/uidrift-scan.yml. + title: "UI label drift: ${{ steps.scan.outputs.findings }} finding(s) through ${{ steps.scan.outputs.head }}" + body-path: ${{ env.PR_BODY }} + labels: | + documentation + automated + uidrift + + - name: Report the outcome + env: + PR_NUMBER: ${{ steps.pr.outputs.pull-request-number }} + ACTIONABLE: ${{ steps.scan.outputs.actionable }} + FINDINGS: ${{ steps.scan.outputs.findings }} + RANGE: ${{ steps.scan.outputs.range }} + run: | + set -euo pipefail + if [ "$ACTIONABLE" = "0" ]; then + echo "::notice title=No drift to act on::Nothing in ${RANGE} left docs wrong. The watermark stays put, so the next run resumes from the same base and simply covers a wider window." + exit 0 + fi + if [ "$DRY_RUN" = "true" ]; then + echo "::notice title=Dry run::${FINDINGS} finding(s); no PR opened. The report is in the job summary." + exit 0 + fi + if [ -n "$PR_NUMBER" ]; then + echo "::notice title=Drift report ready::https://github.com/${{ github.repository }}/pull/${PR_NUMBER}" + fi + + # Last, so the report and the PR exist before the run goes red. A reopened + # decision is the one outcome the detector is allowed to fail a step over: + # a human's recorded call no longer matches the evidence it was made + # against, and only a human can settle that. + - name: Fail on reopened decisions + if: steps.scan.outputs.reopened != '0' + env: + REOPENED: ${{ steps.scan.outputs.reopened }} + run: | + echo "::error title=Reopened decisions::${REOPENED} stored decision(s) no longer match the docs they were made against. Re-run \`uidrift.scan decide\` on those ids to settle them." + exit 1 diff --git a/.github/workflows/uidrift-tests.yml b/.github/workflows/uidrift-tests.yml new file mode 100644 index 0000000000..de75ced201 --- /dev/null +++ b/.github/workflows/uidrift-tests.yml @@ -0,0 +1,51 @@ +name: UI label drift tests + +# Runs the detector's own test suite on every PR that touches it. +# +# The scan workflow (uidrift-scan.yml) triggers only on schedule and dispatch, +# so before this existed nothing ran scripts/uidrift/tests on a pull request: a +# regression could merge and first surface days later, in a rolling PR that a +# writer would reasonably read as drift rather than as a broken detector. +# +# The suite is stdlib unittest, needs no network and no wandb/core checkout, and +# finishes in seconds -- so it is cheap enough to gate every relevant PR. + +on: + pull_request: + paths: + - 'scripts/uidrift/**' + - '.github/workflows/uidrift-tests.yml' + push: + branches: [main] + paths: + - 'scripts/uidrift/**' + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: uidrift-tests-${{ github.ref }} + cancel-in-progress: true + +jobs: + test: + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + + - name: Set up Python + uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6 + with: + # Matches the scan workflow. No dependency step: scripts/uidrift is + # stdlib-only by design. + python-version: '3.11' + + - name: Run the detector test suite + # Discovery runs from scripts/ so the tests' relative imports (`from .. + # import config`) resolve against the uidrift package, which is how they + # are written and how they run locally. + run: | + python3 -m unittest discover -s scripts/uidrift/tests -t scripts -p 'test_*.py' -v diff --git a/.mintignore b/.mintignore index cfcacc4f58..a642aceca8 100644 --- a/.mintignore +++ b/.mintignore @@ -8,6 +8,11 @@ runbooks/ node_modules/ .github/ +# UI label drift reports and the decision ledger. Committed as a record for the +# docs team, never published: only /*.md is ignored above, so a nested +# uidrift/reports/*.md would otherwise become a page. +uidrift/ + # Top-level config and assets (not doc pages) # Note: Do not exclude .js or .css. Mintlify loads # these automatically if present in the content root. diff --git a/scripts/uidrift/ADAPTING.md b/scripts/uidrift/ADAPTING.md new file mode 100644 index 0000000000..019ccdca1f --- /dev/null +++ b/scripts/uidrift/ADAPTING.md @@ -0,0 +1,444 @@ +# Adapting this detector to another repo + +This is a worked example, not a framework. There is no plugin interface to +implement and no abstract base class to subclass — those would require guessing +the second implementation's shape before anyone has built one. Instead, this +file states what is generic, what is `wandb/core`-specific, and what surprised +us, so that adapting it is a reading exercise rather than an archaeology one. + +If you are an agent being told *"do this, but watch `coreweave/sunk` for +releases"* — read this file first, then `config.py`, then `extract.py`. The +other modules follow from those. + +## The four-part anatomy + +Every repo-watching doc-drift detector has the same four parts. Only the second +column changes. + +| Part | Generic — reuse as-is | Repo-specific — expect to rewrite | +|---|---|---| +| **1. Event detector** | commit iteration, diff splitting, set-equality dedupe | which paths are user-facing; which literal patterns are labels | +| **2. Evidence** | structural signals, gate scope, ownership | gate registry location and mechanism; CODEOWNERS shape | +| **3. Triage** | the agent/pair/human decision procedure | thresholds; which paths are immutable | +| **4. Sink** | markdown table renderer, ledger, rolling report PR | report location; JIRA project and component | + +Part 1 is where nearly all the adaptation cost lives. Parts 3 and 4 usually +transfer unchanged. + +## Step zero for a new repo: is there an i18n catalog? + +**Ask this before anything else.** It determines whether the job takes two days +or two weeks. + +- **A catalog exists** (`en.json`, `messages.po`, `.ftl`, an i18next/Lingui/ + react-intl setup): you are diffing structured key-value pairs. Keys give you + free stable identity across renames, and you can skip most of `extract.py`. + This is the easy case. +- **No catalog** — `wandb/core`'s situation: strings are inline in JSX and you + are parsing source. Everything below applies. + +How to check, quickly: + +```bash +git -C grep -lE 'useTranslation|defineMessages|FormattedMessage|i18next|@lingui' | head +git -C ls-tree -r --name-only | grep -iE 'locales?/|translations?/|/en\.json$' +``` + +For `wandb/core` at `origin/master` both return nothing. There is no i18n +toolkit in the repo at all. (There is a Locadex/gt-react localization pilot, but +it runs against a *fork* — `wandb/mattcore` — and does not affect `wandb/core`.) + +## What surprised us on wandb/core + +These are the findings that cost real time. They are the reason this file exists. + +### 1. Enumerating attribute names guarantees silent misses + +The first extractor listed the attributes that carry copy: `aria-label`, +`placeholder`, `title`, `tooltip`. It scored **zero** on a drawer-consolidation +commit, because that component library takes its copy as `saveLabel=`, +`cancelLabel=`, `isPendingAriaLabel=`. A design system invents new label props +as it grows, and the list is open-ended. + +**Match on suffix, not on membership.** See `_ATTR_SUFFIX` in `extract.py`. +The cost is that `name=` becomes ambiguous (`` is an +identifier, `` is copy), handled by +rejecting slug-shaped values for ambiguous keys only. + +### 2. Prettier reflow is the dominant false positive + +Re-indentation shows up as `-aria-label="X"` / `+ aria-label="X"` — a removal +and an addition of the same string. Killed deterministically by per-file +set-equality, with no model and no heuristic. Roughly 25 of 233 label-touching +commits in a 60-day window are pure reflow. + +Generalized from `diff_signals.graphql_contract_change`, which uses the same +trick to decide whether a `.graphql` change is client-visible. + +### 3. Refactor-titled commits are the dominant false negative + +**Never filter on conventional-commit type.** The single richest real finding in +the corpus — eight table headers title-cased, still wrong in published docs six +weeks later — arrived half under `feat(app): migrate ... to Table` and half +under `refactor(app): migrate OrgDashboard UsersTable`. Neither subject line +suggests user-visible copy changed. Both changed it. + +The commit type is recorded as metadata and used by nothing. + +### 4. Never normalize case + +`normalize()` collapses whitespace and stops there. Lowercasing would make +`MODELS SEAT` and `Models Seat` identical, so the case-only rename would cancel +itself out in the set arithmetic and vanish without a trace. There is a test +pinning this (`test_case_is_never_normalized`) precisely because the failure is +silent. + +### 5. Move detection needs a *looser* identity than reflow detection + +These are two different questions and they want two different keys: + +- *Did this file's copy change?* → strict identity `(kind, key, string)`. + Reflow preserves the expression form exactly, so form must participate. +- *Did this string leave the product?* → the string alone. + +A drawer consolidation moved `Add secret` out of `Add secret` and +into `saveLabel="Add secret"`. Same string, different form, still on screen. +Keyed strictly, that commit reports 23 phantom removals — 23 false rows in the +very first report anyone reads. See `LabelDelta.ident` vs `.moved_ident`. + +### 6. `wrapped` means "not a complete literal", not "Prettier moved it" + +Easy to conflate, and conflating it disqualifies good findings from the agent +lane for no reason. Interpolation (`` `Allow ${AGENT_NAME} to ...` ``) and +ternary branches are genuinely unsafe to find-and-replace. Text that Prettier +pushed onto its own line is captured exactly and is perfectly safe. + +### 7. Merged ≠ visible, and flag *presence* is a decayed signal + +New UI ships behind Statsig ramp flags. But engineers rarely remove a flag once +it reaches 100% — leaving it is safer — so a gate being present tells you almost +nothing. Do not suppress on it. + +The **lifecycle events** are the signal: + +| Event | Meaning | +|---|---| +| Flag added in the same commit as the copy | not visible yet | +| Flag removal commit | GA | +| Flag merely present, added long ago | no signal — ignore | + +Gating is also **per-surface, not per-feature**: one gate governs three surfaces +in `APIKeysTabContent.tsx`, with different answers for each. The diff shows +whether the changed element sits inside the conditional; use that, not the flag +name. + +Deployment semantics for `wandb/core` specifically are involved enough to live +in their own skill — see `beta-deployment-availability` in `coreweave/docs-skills`. +Do not re-derive them here. + +### 8. The docs oracle runs in one direction only + +Docs presence **raises** confidence that a surface is live, so drift on it is +real. Docs absence must **never** lower it — "available but undocumented" is +precisely the gap being hunted, and using absence to suppress closes a loop the +detector never escapes: looks unreleased → suppress → nobody writes docs → still +no docs → still suppressed. + +This is enforced structurally rather than by convention: `docsindex` exposes no +function that returns a negative score, so the loop is unrepresentable. + +Also: naive substring matching is useless. `search` appears on 215 docs pages. +Require UI-emphasis context (`**bold**`, backticks, quotes, or "the X button"), +a ≥2-token-or-ALL-CAPS specificity gate, and a page-count cap. + +### 9. Match the literal case-sensitively, or you report already-fixed drift + +Non-obvious and easy to get backwards. The lookup asks "does the OLD string +still appear in docs?" If docs say `MODELS SEAT` and the code now says +`Models Seat`, that is drift. If docs already say `Models Seat`, there is +nothing to do. A case-insensitive match cannot tell those apart, so it reports +the fixed page as broken — and the case-only rename is exactly the class where +this matters most. + +Surrounding words (`the`, the noun) can be case-insensitive via a scoped +`(?i:...)`. The literal itself must not be. + +### 10. Blank frontmatter; do not delete it + +Deleting YAML frontmatter shifts every line number after it, so a reported +`page:line` stops resolving to what a reader sees — off by five, in our corpus. +Replace it with an equal number of newlines instead. Cheap, and it keeps +citations exact while still preventing frontmatter keys from matching as prose. + +### 11. Published release notes are immutable, and they are a big share of hits + +Roughly half the docs hits in a 60-day window land in `release-notes/**`. Those +are a historical record of what shipped under the name it shipped under. +Rewriting them would be falsifying a changelog. Report them for awareness, never +propose an edit, and never count them toward agent eligibility. + +### 12. Include reusable fragments; exclude worktrees + +Two corpus-selection mistakes with opposite signs: + +- **`snippets/`** carries real UI prose (`go to the **Service Accounts** tab`) + and renders into many pages, so a label there has *wider* blast radius than + one in a single page. Excluding it creates blind spots. +- **`.claude/`** contains git worktrees — full copies of the tree. Indexing it + double-counts every occurrence and silently inflates page counts, which then + trips the too-generic cap and suppresses real findings. + +### 13. Pair renames by position before you consider similarity + +The obvious approach — match a removed string to the added string it most +resembles — fails on the most important case. A label that was genuinely +reworded shares almost no characters with its replacement: + +| Old | New | Similarity | +|---|---|---| +| `Hide manually hidden runs` | `List only visible runs` | 0.55 | +| `Only show visualized` | `Hide manually hidden runs` | 0.22 | + +No threshold catches those and still refuses to pair two unrelated column +headers. But git already answers the question: an in-place edit appears as a `-` +line and the `+` line that replaced it at the same offset in one change block. +Position is stronger evidence than string similarity, and it has no false-pair +failure mode. + +Similarity is still worth a second pass, for renames that are *not* in-place — +`header: 'WEAVE ACCESS'` became `name: 'Weave Access'` on a different line and a +different field. Group by (path, kind), not (path, kind, key), or that one is +invisible. + +### 14. Not every conditional is a feature gate + +Walking up from a changed line to the enclosing `if` finds plenty of blocks that +have nothing to do with visibility. `if (hideManuallyHidden)` is UI state. +Reporting it as a gate would mark half the app "not yet visible" and destroy +trust in the one signal that should mean something. + +Require the conditional's variable to resolve to a gate hook — +`const shouldShowX = useStatsigGateX(orgName)` — and report nothing when it does +not. The chain is fully readable inside a single diff. The Statsig key itself +usually is not; it lives in the ramp registry, so treat it as optional +enrichment rather than a precondition. + +### 15. A change to an undocumented label is not drift + +The first report rendered 22 rows for three commits, of which 2 were real. The +rest were `new **Loading members**`, `new **Invited**`, `PROFILE removed` — every +changed string that matched no doc page, each filed as a "coverage gap". + +Drift requires docs to drift *from*. Renaming a label that no page mentions makes +nothing incorrect, so it is not a finding; it is at most a statistic. Count those +and print the count. Enumerating them buries the rows that matter, which is the +one failure this report cannot survive — a reviewer who skims past the real +finding will not come back. + +This is not the suppression the one-directional rule forbids. Absence of docs +must never *hide a finding that exists*; it just must not *manufacture findings +that do not*. + +Two related shapes fell out of the same pass: + +- **Aggregate new copy per surface.** A new settings panel adds a heading, a + description, two field labels and a button. That is one docs task, not five. +- **Key findings on the docs task, not the code surface.** Three member tables + render the same column, and the docs page names it once. Including the surface + in the finding id showed one edit as three rows. + +### 16. Freeze real diffs as fixtures, immediately + +Six frozen `git show` outputs in `tests/fixtures/` are the entire regression +surface, and they caught three bugs that survived design review: the inline-JSX +miss, the enumerated-attribute miss, and the move-identity bug. None of these +were visible in the plan. All three were obvious within a minute of running +against real diffs. + +When someone reports a miss, add it as a fixture before fixing it. + +### 17. Ownership is per-run data, so pay for it once + +Reviewers and owning team look like per-finding lookups and are not. CODEOWNERS +is one file that does not change mid-run, and authorship comes from one +`git log --name-only` over the UI roots, parsed into a path → author index in +memory. The naive version — `git show` for CODEOWNERS plus one or two `git log` +calls per finding — is roughly three subprocesses per row for data that is +identical across the whole scan. + +Cache it per run and *not* across runs. Team membership changes, and a stale +owner cache is a wrong @-mention in a PR nobody can explain. + +The docs oracle wants the same treatment for the same reason. `docsindex.find` +is memoized on the index, because the corpus does not change mid-run and the +repeats are structural: `build_findings` probes a literal once to decide whether +it is documented and again to attach the evidence, and one label routinely +changes in several commits across a window. Over 60 days of `wandb/core` that +is 1180 changed strings collapsing to far fewer distinct lookups. + +### 18. Almost nothing needs to be stored between runs + +The obvious design for a re-scanning detector is a ledger that remembers every +finding it has ever emitted. Resist it. Ask of each field: *can a fresh scan +recompute this?* For finding identity, dedupe, settledness, triage, ownership and +docs coverage, the answer is yes — every input is in the commit history or the +docs tree. Storing them only creates a second copy that can disagree with the +first, and the second copy is the one nobody notices is wrong. + +What genuinely cannot be recomputed is a human having said "I looked at this and +it is fine." That is the whole contents of `ledger.json`. + +The best version of this trick needs no file at all. A sibling project mirrors +upstream SDK release notes into a docs page and finds its watermark by reading +the newest `` out of the page it maintains — its state *is* +the published artifact, so the two cannot drift apart. Look for that shape first. + +A corollary worth stating: derived-not-stored means a re-scan is the fix for a +bad run. There is no cache to invalidate and no migration to write when a signal +changes, which is what makes it safe to keep changing the signals. + +### 19. Suppression is one-directional too + +Lesson 8 governs the docs oracle. The same rule has to govern stored decisions, +and the failure mode is subtler: a writer dismisses a finding as a false +positive, and six weeks later a page starts documenting that very surface. The +finding is now real, and the stored dismissal would silently hide it — a +suppression that gets *more* wrong over time, and that nobody can detect from +the report. + +So a decision records the docs evidence it was made against, and expanding +evidence reopens it. Only expansion counts: a page that stops mentioning the +string means somebody did the work, which is not a reason to reopen anything. + +Evidence is pages and *counts per page*, never line numbers. Line numbers churn +on every unrelated docs edit, so reopening on them would be pure noise — but a +bare set of page names is too coarse in the other direction. It cannot tell one +editable occurrence on a page from three, so a dismissed finding that grew a +second occurrence on a page already in the set stayed suppressed while the docs +got further out of date. Counts sit between the two: immune to churn, sensitive +to growth. If you change this shape again, make old evidence still compare equal +to an unchanged corpus, or every stored decision reopens at once — which is the +fastest way to teach a writer that the ledger cries wolf. + +Two consequences: + +- **Never auto-delete a decision.** An orphaned decision is ambiguous — the + drift may be resolved, or the scan window may simply not reach its commit. + List them and let a human choose. +- **Account for suppression in the report.** A count of what was held back is + the only way a reader can tell "no drift" from "all drift already dismissed". + A detector that quietly drops rows cannot be audited. + +## Running it + +```bash +PYTHONPATH=scripts python3 -m uidrift.scan scan --since "60 days ago" +PYTHONPATH=scripts python3 -m uidrift.scan scan --incremental +PYTHONPATH=scripts python3 -m uidrift.scan decide --status dismissed \ + --by matt --agreement false_positive --note "why" +``` + +`--incremental` takes its base from the head SHA in the newest report filename +under `uidrift/reports/`, which is lesson 18 applied to the watermark: the +reports are the record, so there is no state file to disagree with them. It is +the difference between 96 seconds and 1.3 seconds, and it is what makes a +frequent cron affordable. + +Reports are named `YYYY-MM-DDTHHMMSS-.md`, in UTC. The time is not +decoration: "newest" was once decided by date alone, which left two reports +merged on one day to be ordered by their head SHA — content-addressed, so +effectively random. Half the time that picks the older one, and a watermark +that moves backwards re-reports drift a writer already dismissed. Sort reports +by name, never by SHA. Untimed names still parse and sort before timestamped +ones from the same day, which is the safe direction: a range gets rescanned, +never skipped. + +The scan never writes the ledger; only `decide` does. `decide` re-derives the +finding by scanning rather than reading a report, because the evidence +fingerprint has to reflect the corpus as it is now — a decision stamped with +stale evidence would never reopen. + +Exit codes: `0` clean, `1` operator error (bad range, missing checkout), `2` no +subcommand, `3` at least one decision reopened. `3` is separate because a +reopened decision is the one outcome that should be able to fail a CI step. + +`--summary-json PATH` writes the run's counts for a caller that has to decide +something. A CI step choosing whether to open a PR should read that, not grep the +rendered report: the prose exists to be read, and coupling a workflow to a +sentence like "No drift to act on in this window" makes the wording load-bearing. + +## Running it in CI + +`.github/workflows/uidrift-scan.yml` is the sink. Three things in it are +measurements rather than preferences, and are the parts to keep when adapting: + +**Clone the watched repo in full, single-branch, with `--no-checkout`.** Each +half of that was tested against `wandb/core` (2.4 GB, 50k commits): + +| Clone | Cost | Verdict | +|---|---|---| +| Full, `--no-checkout` | 1.3 GB, ~3.5 min | **What we use** | +| `--shallow-since=7 months` | 136 MB, ~10 s | Wrong reviewers | +| `--shallow-since=18 months` | 348 MB, ~15 s | Still wrong reviewers | +| `--filter=blob:none` | 96 MB, ~17 s | Unusable | + +Shallow is the tempting one and it is wrong for a specific reason: ownership +falls back to all-time authorship when a file has fewer than +`MIN_RECENT_AUTHORS` recent authors, which is exactly the history a truncated +clone does not have. Both windows above named different reviewers than complete +history did, and no window is safe — the fallback exists *for* old, quiet files. + +A blobless partial clone looks ideal (smallest, complete commit history, and +ownership only needs trees) but `iter_commits` reads `--numstat`, which needs +blob content. A one-day window spent 82 seconds lazy-fetching and then failed on +the promisor remote. If you make that clone work, `iter_commits` never uses the +add/delete counts it parses — only `cols[2]`, the path — so `--name-only` would +be enough. That is a change to vendored code; make it deliberately. + +**No `[skip ci]` on the commit.** The anatomy table used to say otherwise. A +skipped workflow reports no status at all, so required `pull_request` checks sit +pending forever and the PR can never merge. Keep the report cheap for CI instead: +`.mintignore` excludes `uidrift/`, and Validate MDX sees no `.mdx` or `.json` in +the diff and short-circuits. + +**One rolling branch, and only commit when there is something to say.** Because +the watermark is the newest report *on the default branch*, a run regenerates the +delta since the last **merged** report — so a superseded report has nothing left +to say, and a fresh branch per weekday would bury the current one. A run with no +findings commits nothing and leaves the watermark where it was; the next run +rescans the same range over a slightly wider window, which costs seconds. The +alternative — committing a "nothing found" report daily to advance the +watermark — buys a cheaper scan with a PR nobody wants to read. + +## Volume expectations + +Calibrate before building. For `wandb/core` over 60 days: + +| Stage | Count | +|---|---| +| Commits on `origin/master` | 2,990 | +| Touching `frontends/app/src/**/*.tsx` | 592 | +| **Stage-1 candidates** | **170** (~20/week) | +| …with a published-docs occurrence | **12** (~1.5/week) | +| Reduction | 71% at stage 1; 93% after the docs join | + +The docs join is the real filter, and it is deterministic. Do not reach for a +model until after it: judging 170 commits costs an order of magnitude more than +judging the 12 that actually touch published copy. + +Full scan runs in ~27 seconds with no network and no token, because +`gitsource.commit_diff` takes a pathspec and never fetches diffs outside the UI +roots. Keep that property: without it, a 2,600-line commit is unaffordable. + +If your stage-1 count exceeds ~250/60d, tighten before adding stage 2 — a model +pass over the whole `.tsx` stream is mostly waste. + +## The vendored modules + +`_vendor/` holds copies of `gitsource.py`, `diff_signals.py`, and +`commit_text.py` from `wandb/release-note-genie`, each with a provenance header +naming the source commit. They are copies rather than imports on purpose: this +detector must run inside `wandb/docs` with no dependency on another repo being +checked out, and all three are stdlib-only and stable. + +Re-vendor deliberately, never automatically. diff --git a/scripts/uidrift/__init__.py b/scripts/uidrift/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/scripts/uidrift/_vendor/__init__.py b/scripts/uidrift/_vendor/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/scripts/uidrift/_vendor/commit_text.py b/scripts/uidrift/_vendor/commit_text.py new file mode 100644 index 0000000000..4fabc3a73f --- /dev/null +++ b/scripts/uidrift/_vendor/commit_text.py @@ -0,0 +1,118 @@ +# Vendored from wandb/release-note-genie:scripts/commit_text.py at 5846dd1 +# Do not edit here. Upstream changes must be re-vendored deliberately. +# See scripts/uidrift/ADAPTING.md for why this is a copy and not an import. + +#!/usr/bin/env python3 +"""Normalize a commit message down to the prose that actually describes the change. + +Scoring and flag detection used to run keyword scans over the raw commit message. In +wandb/core that message is dominated by PR-template boilerplate, and the boilerplate was +moving scores more than the change itself: + + - Every PR has a ``## Testing`` section, so the low-signal keyword "test" fired on + *every* commit — a universal -1 dressed up as signal. + - Go identifiers inside code spans leaked into prose matching: ``internalKeyInfo`` + matched the low-signal keyword "internal" and cost a real perf fix a point. + - Cherry-pick preambles name the target branch (``server-release-0.83.x``), which + supplied the "release" half of a bogus GA boost, and their conflict-resolution + narrative describes *the port* rather than the change. + - Co-author trailers and Devin session URLs contribute nothing but match on substrings. + +``clean_for_scoring`` strips all of that while always preserving the subject line, which is +the highest-signal text in the message. +""" + +from __future__ import annotations + +import re + +# Fenced code blocks and inline code spans. Identifiers are implementation detail, not a +# description of user-visible behavior, and they are the main source of false keyword hits. +_FENCED_RE = re.compile(r"```.*?```", re.S) +_INLINE_CODE_RE = re.compile(r"`[^`\n]*`") + +# Markdown headings whose contents never describe user-visible behavior. +_DROP_SECTION_TITLES = ( + "testing", + "test plan", + "how to test", + "checklist", + "conflict resolution", + "screenshots", +) +_HEADING_RE = re.compile(r"^\s{0,3}#{1,6}\s*(.+?)\s*#*\s*$") + +# Commit trailers and generated links. +_TRAILER_RE = re.compile( + r"^\s*(co-authored-by|signed-off-by|reviewed-by|acked-by|tested-by|cc|" + r"link to devin session|devin session|generated with|reported-by|fixes|closes)\s*:", + re.I, +) +_URL_RE = re.compile(r"https?://\S+") + +# Markdown task-list rows ("- [x] Added unit tests"). +_CHECKBOX_RE = re.compile(r"^\s*[-*]\s*\[[ xX]\]\s*") + +# Cherry-pick bookkeeping: names the target release branch and describes the port. +_CHERRY_LINE_RE = re.compile(r"cherry[- ]pick", re.I) +_RELEASE_BRANCH_RE = re.compile(r"\b(?:server-)?release[-/][\w.\-]*\b", re.I) + + +def _is_dropped_heading(text: str) -> bool: + lowered = text.strip().lower().rstrip(":").strip() + return any(lowered.startswith(title) for title in _DROP_SECTION_TITLES) + + +def clean_for_scoring(message: str) -> str: + """Return subject + description prose, with boilerplate and code identifiers removed. + + The result is intended for keyword/regex scanning only. It is lossy by design and must + never be shown to a human or used as draft text. + """ + if not message: + return "" + + raw_lines = message.splitlines() + subject = raw_lines[0] if raw_lines else "" + body = "\n".join(raw_lines[1:]) + + body = _FENCED_RE.sub(" ", body) + body = _INLINE_CODE_RE.sub(" ", body) + + kept: list[str] = [] + skipping = False + for line in body.splitlines(): + heading = _HEADING_RE.match(line) + if heading: + # A new heading always ends any section we were skipping. + skipping = _is_dropped_heading(heading.group(1)) + continue + if skipping: + continue + if _TRAILER_RE.match(line) or _CHERRY_LINE_RE.search(line): + continue + line = _CHECKBOX_RE.sub("", line) + line = _URL_RE.sub(" ", line) + kept.append(line) + + cleaned = f"{subject}\n" + "\n".join(kept) + # Release-branch names survive outside cherry-pick lines too (e.g. "onto release-0.83.x"). + cleaned = _RELEASE_BRANCH_RE.sub(" ", cleaned) + # Collapse the blank lines left behind, but keep newlines: several regexes are + # deliberately sentence-local and rely on line breaks as boundaries. + cleaned = re.sub(r"[ \t]+", " ", cleaned) + cleaned = re.sub(r"\n{2,}", "\n", cleaned) + return cleaned.strip() + + +def unwrap_paragraphs(text: str) -> str: + """Join hard-wrapped lines within a paragraph into single logical lines. + + PR bodies wrap at ~72 columns, which splits phrases the sentence-local flag regexes + need to see whole ("gated by the new Statsig gate\\n`some_gate_name`"). + """ + if not text: + return "" + paragraphs = re.split(r"\n\s*\n", text) + joined = [re.sub(r"\s*\n\s*", " ", p).strip() for p in paragraphs] + return "\n".join(p for p in joined if p) diff --git a/scripts/uidrift/_vendor/diff_signals.py b/scripts/uidrift/_vendor/diff_signals.py new file mode 100644 index 0000000000..9e35a3a40b --- /dev/null +++ b/scripts/uidrift/_vendor/diff_signals.py @@ -0,0 +1,125 @@ +# Vendored from wandb/release-note-genie:scripts/diff_signals.py at 5846dd1 +# Do not edit here. Upstream changes must be re-vendored deliberately. +# See scripts/uidrift/ADAPTING.md for why this is a copy and not an import. + +#!/usr/bin/env python3 +"""Signals that can only be read from a commit's diff, not its message or file list. + +Classification runs on the commit message plus the changed-path list. Several judgments have +turned out to need more than that, and the honest response to "the deciding evidence isn't +visible" has been to route to review. This module narrows that set where the diff *can* decide +deterministically — no model, no heuristics over prose. + +Currently one signal, because it is the one where the diff genuinely resolves the ambiguity: + + graphql_contract_change + Editing a published ``.graphql`` file covers two unrelated things, and the path cannot + tell them apart: + + contract change ``+ limit: Int`` / ``+ pattern: String`` on ``historyKeys`` — + new arguments clients can send. Note-worthy. + server annotation ``- id: ID!`` / ``+ id: ID! @skipFieldTrace`` across 34 fields — + cuts ~129M Datadog spans/day. Clients observe nothing. + + The diff separates them exactly: strip the internal directives and a directive-only + change has identical removed and added line sets, while a contract change does not. + +Deliberately NOT here: whether a changed default is note-worthy. That looked like a diff +question and isn't. ``ConnMaxLifetime`` carries a ``schema:"conn_max_lifetime"`` tag, so the +diff says "operator-settable" — yet the parameter appears nowhere in W&B's public docs, so no +admin can act on it and reviewers rejected the note. The deciding fact lives in the docs, not +the diff, so changed defaults still go to a human. A docs cross-reference (does the parameter +name appear in the published docs?) would be the capability that resolves it. +""" + +from __future__ import annotations + +import re +from typing import Optional + +# Directives that are server-side implementation detail: adding or removing one changes +# nothing a client can observe. Anything NOT listed here is treated as contract-relevant, so an +# unfamiliar directive fails toward "contract change" (a human looks) rather than being +# silently ignored. @deprecated is intentionally absent — deprecating a field IS user-facing. +INTERNAL_DIRECTIVES = frozenset({ + "skipFieldTrace", # tracing suppression (wandb/core) + "goField", # gqlgen codegen binding + "goModel", + "goTag", + "goExtraField", + "goEnum", +}) + +_DIFF_FILE_RE = re.compile(r"^diff --git a/(\S+) b/(\S+)", re.M) +_HUNK_RE = re.compile(r"^@@") +_DIRECTIVE_RE = re.compile(r"@(\w+)(\s*\([^)]*\))?") + + +def _iter_file_diffs(diff: str): + """Yield ``(path, body)`` for each file section of a unified diff.""" + matches = list(_DIFF_FILE_RE.finditer(diff or "")) + for i, m in enumerate(matches): + end = matches[i + 1].start() if i + 1 < len(matches) else len(diff) + # Prefer the b/ path (post-image); falls back to a/ for deletions. + yield m.group(2) or m.group(1), diff[m.end():end] + + +def graphql_files_in_diff(diff: str) -> list[str]: + return [p for p, _ in _iter_file_diffs(diff) if p.lower().endswith(".graphql")] + + +def _strip_internal_directives(line: str) -> str: + """Remove only the directives listed in INTERNAL_DIRECTIVES.""" + def sub(m: re.Match) -> str: + return "" if m.group(1) in INTERNAL_DIRECTIVES else m.group(0) + return _DIRECTIVE_RE.sub(sub, line) + + +def _normalize(line: str) -> Optional[str]: + """Normalize a schema line for comparison, or None if it carries no contract meaning.""" + body = line[1:] # drop the +/- marker + body = _strip_internal_directives(body) + body = re.sub(r"\s+", " ", body).strip() + if not body: + return None + if body.startswith("#"): + return None # a comment-only edit is not a contract change + return body + + +def graphql_contract_change(diff: str) -> Optional[bool]: + """Did a diff change a published GraphQL contract? + + Returns True when at least one ``.graphql`` file gained or lost real schema content, + False when every ``.graphql`` change is internal-directive or comment noise, and None + when the diff contains no ``.graphql`` files at all (nothing to say). + """ + if not diff: + return None + + saw_graphql = False + for path, body in _iter_file_diffs(diff): + if not path.lower().endswith(".graphql"): + continue + saw_graphql = True + + removed: list[str] = [] + added: list[str] = [] + for raw in body.splitlines(): + if raw.startswith("---") or raw.startswith("+++") or _HUNK_RE.match(raw): + continue + if raw.startswith("-"): + n = _normalize(raw) + if n is not None: + removed.append(n) + elif raw.startswith("+"): + n = _normalize(raw) + if n is not None: + added.append(n) + + # Identical multisets => every surviving difference was an internal directive or a + # comment, so no client-visible schema content moved. + if sorted(removed) != sorted(added): + return True + + return False if saw_graphql else None diff --git a/scripts/uidrift/_vendor/gitsource.py b/scripts/uidrift/_vendor/gitsource.py new file mode 100644 index 0000000000..93a2fecc8d --- /dev/null +++ b/scripts/uidrift/_vendor/gitsource.py @@ -0,0 +1,127 @@ +# Vendored from wandb/release-note-genie:scripts/rolling/gitsource.py at 5846dd1 +# Do not edit here. Upstream changes must be re-vendored deliberately. +# See scripts/uidrift/ADAPTING.md for why this is a copy and not an import. + +#!/usr/bin/env python3 +"""Read commit deltas from a local wandb/core clone. + +The rolling watcher analyzes the delta between the last SHA it saw and master. In CI the +runner checks out wandb/core; locally the clone lives at ~/core. Reading history from git +(rather than the GitHub compare API) means the watcher needs no token and is fast for the +incremental delta. + +``iter_commits`` returns commit dicts shaped like the GitHub API objects the existing +scoring code expects (``commit['commit']['message']``, ``commit['files']``, ...), so +score_commit_impact / extract_pr_number can be reused unchanged. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from typing import Optional + +_REC_SEP = "\x1e" +_FIELD_SEP = "\x1f" + + +def _git(core: Path, *args: str) -> subprocess.CompletedProcess: + return subprocess.run(["git", "-C", str(core), *args], capture_output=True, text=True) + + +def resolve_sha(core: Path, ref: str) -> Optional[str]: + """Resolve a ref to a concrete SHA, trying a few common fallbacks.""" + for candidate in (ref, f"origin/{ref}"): + r = _git(core, "rev-parse", "--verify", "--quiet", f"{candidate}^{{commit}}") + if r.returncode == 0 and r.stdout.strip(): + return r.stdout.strip() + return None + + +def ref_exists(core: Path, ref: str) -> bool: + return resolve_sha(core, ref) is not None + + +def commit_diff(core: Path, sha: str, *pathspec: str) -> Optional[str]: + """Return a commit's unified diff, optionally limited to a pathspec. + + Used for signals that need the diff's content rather than its file list (see + scripts/diff_signals.py). Always pass a pathspec when only certain files matter: some + wandb/core commits touch hundreds of files and the full patch is large enough to be worth + not materializing. Returns None when the commit or repo is unavailable, so callers degrade + to message/path-only classification instead of failing. + """ + args = ["show", "--format=", "--no-color", "-M", sha] + if pathspec: + args += ["--", *pathspec] + r = _git(core, *args) + if r.returncode != 0: + return None + return r.stdout or None + + +def is_ancestor(core: Path, sha: str, ref: str) -> Optional[bool]: + """True if sha is an ancestor of ref; None if ref cannot be resolved.""" + if not ref_exists(core, ref): + return None + target = resolve_sha(core, ref) + r = _git(core, "merge-base", "--is-ancestor", sha, target or ref) + if r.returncode == 0: + return True + if r.returncode == 1: + return False + return None + + +def iter_commits( + core: Path, + base: str, + head: str, + *, + owner_repo: str = "wandb/core", + limit: Optional[int] = None, + include_merges: bool = False, +) -> list[dict]: + """Return API-shaped commit dicts for ``base..head`` (commits in head not in base).""" + fmt = _REC_SEP + "%H" + _FIELD_SEP + "%an" + _FIELD_SEP + "%aI" + _FIELD_SEP + "%B" + _FIELD_SEP + args = ["log", f"--format={fmt}", "--numstat", "--date=iso-strict"] + if not include_merges: + args.append("--no-merges") + args.append(f"{base}..{head}") + r = _git(core, *args) + if r.returncode != 0: + raise RuntimeError(f"git log {base}..{head} failed: {r.stderr.strip()}") + + commits: list[dict] = [] + chunks = r.stdout.split(_REC_SEP) + for chunk in chunks: + if not chunk.strip(): + continue + parts = chunk.split(_FIELD_SEP) + if len(parts) < 5: + continue + sha, author, date_iso, body, numstat_block = parts[0], parts[1], parts[2], parts[3], parts[4] + sha = sha.strip() + if not sha: + continue + paths: list[str] = [] + for line in numstat_block.splitlines(): + line = line.strip() + if not line: + continue + cols = line.split("\t") + if len(cols) == 3 and cols[2]: + paths.append(cols[2]) + commit = { + "sha": sha, + "commit": { + "message": body.strip("\n"), + "author": {"name": author, "date": date_iso}, + }, + "html_url": f"https://github.com/{owner_repo}/commit/{sha}", + "files": [{"filename": p} for p in paths], + } + commits.append(commit) + if limit and len(commits) >= limit: + break + return commits diff --git a/scripts/uidrift/build.py b/scripts/uidrift/build.py new file mode 100644 index 0000000000..844126f77b --- /dev/null +++ b/scripts/uidrift/build.py @@ -0,0 +1,263 @@ +"""Assemble Findings from one commit's analysis. + +Everything here is deterministic. The model pass (step 7) refines `surface` and +sanity-checks the classification; it does not discover anything this file +missed, which is why the table is useful before it exists. +""" + +from __future__ import annotations + +import re +from datetime import date, datetime, timedelta +from pathlib import Path +from typing import Optional, Sequence + +from . import config, docsindex, ownership, structure +from .extract import LabelDelta +from .finding import ( + COVERAGE_COVERED, + COVERAGE_NONE, + KIND_ADDED, + KIND_MOVED, + KIND_NEW_SETTING, + KIND_REMOVED, + KIND_RENAME, + CommitRef, + Finding, + action_for, + triage, +) + +_PR = re.compile(r"\(#(\d+)\)") +# Split camelCase without shredding acronyms: LLMAsAJudgeScorerForm becomes +# "LLM as a judge scorer form", not "L L M As A Judge Scorer Form". +_CAMEL = re.compile(r"(?<=[a-z0-9])(?=[A-Z])|(?<=[A-Z])(?=[A-Z][a-z])") + + +def surface_from_path(path: str) -> str: + """A human-readable name for the screen a string lives on. + + Derived from the component filename, which is a decent proxy and costs + nothing. The stage-2 model pass replaces this with something a reader would + recognize ("Organization dashboard -> Members table"); until then this is + honest about being mechanical. + """ + stem = Path(path).stem + for suffix in ("Content", "Component", "Container", "Tab", "Page"): + if stem.endswith(suffix) and len(stem) > len(suffix): + stem = stem[: -len(suffix)] + words = _CAMEL.sub(" ", stem).replace("_", " ").strip() + return words[:1].upper() + words[1:] if words else path + + +def _docs_payload(lookup: docsindex.DocsLookup) -> dict: + targets = lookup.replace_targets + return { + "coverage": COVERAGE_COVERED if lookup.ui_occurrences else COVERAGE_NONE, + "eligible": lookup.eligible, + "ineligible_reason": lookup.reason, + "pages": [ + {"page": o.page, "line": o.line, "context": o.context, "immutable": o.immutable} + for o in lookup.ui_occurrences + ], + "replace_targets": [ + {"page": o.page, "line": o.line, "context": o.context} for o in targets + ], + "translations_affected": lookup.translations_affected, + "corpus_frequency": lookup.corpus_frequency, + "all_occurrences_emphasized": lookup.all_occurrences_emphasized, + "match_confidence": lookup.match_confidence, + "code_context_only": bool(targets) and all( + t["context"] == docsindex.CTX_CODE + for t in ({"context": o.context} for o in targets) + ), + "touches_immutable": lookup.touches_immutable, + } + + +def _landed_date(commit: dict) -> str: + """When the commit landed on the watched branch, not when it was written. + + Settledness asks "has this stopped moving on master", and the author date + cannot answer that: rebase and cherry-pick both preserve it, so a commit + authored in March and landed today arrives already older than SETTLED_DAYS + and skips the churn protection entirely -- straight into the unattended + agent lane. + + The committer date is the landing date. It is read from the GitHub API's + own shape (`commit.committer.date`), which `scan` fills in from the local + clone, so this keeps working unchanged if the vendored reader ever starts + supplying it. Falls back to the author date when it is absent, because a + slightly-too-settled finding is a better failure than a crash. + """ + committer = (commit.get("commit") or {}).get("committer") or {} + return committer.get("date") or commit["commit"]["author"]["date"] + + +def _commit_ref(commit: dict, delta: LabelDelta) -> CommitRef: + message = commit["commit"]["message"] + subject = message.splitlines()[0] + m = _PR.search(subject) + return CommitRef( + sha=commit["sha"], + date=_landed_date(commit), + subject=subject, + author=commit["commit"]["author"].get("name", ""), + file=delta.path, + line=delta.line_no, + pr=int(m.group(1)) if m else None, + ) + + +def _is_settled(commit_date: str, today: date) -> bool: + try: + when = datetime.fromisoformat(commit_date).date() + except ValueError: + return False + return (today - when) >= timedelta(days=config.SETTLED_DAYS) + + +def build_findings( + commit: dict, + added: Sequence[LabelDelta], + removed: Sequence[LabelDelta], + moved: Sequence[LabelDelta], + diff: str, + index: docsindex.DocsIndex, + *, + today: date, + core: Optional[Path] = None, + resolve_owners: bool = True, +) -> tuple[list[Finding], list[str]]: + streams = structure.parse_streams(diff) + lifecycle = structure.flag_lifecycle(diff) + pairs, unpaired_add, unpaired_rem = structure.pair_renames(added, removed, streams) + + commit_date = _landed_date(commit) + settled = _is_settled(commit_date, today) + findings: dict[str, Finding] = {} + + def emit(kind: str, old: str, new: str, probe: str, delta: LabelDelta, + extra_signals: Sequence[str] = ()) -> None: + lookup = docsindex.find(index, probe) + gate = structure.gate_scope(streams, delta) + signals = list(extra_signals) + + if structure.testid_corroboration(streams, delta): + signals.append("testid_unchanged") + if gate: + signals.append(f"gate:{gate.name}") + if gate.conditional_added: + signals.append("conditional_added") + + # Gated only when this commit says so: a newly-added conditional around + # the string, or the gate itself entering the registry here. Static + # presence is never enough. + gate_added_here = bool(gate and gate.key and lifecycle.get(gate.key) == "added") + not_yet_visible = bool(gate and (gate.conditional_added or gate_added_here)) + for name, event in lifecycle.items(): + signals.append(f"flag_{event}:{name}") + + f = Finding( + kind=kind, + surface=surface_from_path(delta.path), + old_string=old, + new_string=new, + literal_kind=delta.kind, + literal_key=delta.key, + commits=[_commit_ref(commit, delta)], + first_seen_date=commit_date, + last_changed_date=commit_date, + settled=settled, + gate={"name": gate.name, "hook": gate.hook, + "conditional_added": gate.conditional_added} if gate else None, + not_yet_visible=not_yet_visible, + docs=_docs_payload(lookup), + signals=signals, + ) + f.surfaces = [f.surface] + if delta.wrapped: + f.degradations.append("literal is interpolated or a ternary branch") + + if f.id in findings: + existing = findings[f.id] + if f.surface not in existing.surfaces: + existing.surfaces.append(f.surface) + known = {c.sha for c in existing.commits} + existing.commits.extend(c for c in f.commits if c.sha not in known) + return + lane, reason = triage(f) + f.triage, f.triage_reason = lane, reason + f.action = action_for(lane, kind) + if resolve_owners: + own = ownership.resolve( + delta.path, core=core, commit_author=f.commits[0].author + ) + f.reviewers, f.owning_team = own.reviewers, own.team or "" + findings[f.id] = f + + # A change to an UNDOCUMENTED label is not drift. Nothing in the docs became + # wrong, because nothing in the docs said it. Emitting a row per such string + # buried the two real findings under twenty rows of `Loading members` and + # `Invited` the first time this ran. + # + # The coverage gap is still real, so it is counted and reported in aggregate. + # That is not the suppression the direction-discipline rule forbids: absence + # of docs never hides a finding that exists, it just stops manufacturing + # findings that do not. + gaps: list[str] = [] + + def emit_if_documented(kind, old, new, probe, delta, extra=()): + if not docsindex.find(index, probe).ui_occurrences: + gaps.append(probe) + return + emit(kind, old, new, probe, delta, extra) + + for p in pairs: + extra = ["case_only_change"] if p.case_only else [] + if p.ambiguous: + extra.append("ambiguous_pairing") + stability = structure.slug_stability(streams, p) + if stability != "n/a": + extra.append(stability) + # Probe the OLD string: the question is whether docs still say the thing + # the product stopped saying. + emit_if_documented(KIND_RENAME, p.old.norm, p.new.norm, p.old.norm, p.old, extra) + + for d in unpaired_rem: + emit_if_documented(KIND_REMOVED, d.norm, "", d.norm, d) + + for d in moved: + emit_if_documented(KIND_MOVED, d.norm, d.norm, d.norm, d) + + # New copy is aggregated to ONE row per surface, not one per string. A new + # settings panel adds a heading, a description, two field labels and a + # button; that is one docs task, not five. Incidental additions (an empty + # state, a spinner's aria-label) are not reported at all -- nobody documents + # "Loading members". + for path in {d.path for d in unpaired_add}: + in_path = [d for d in unpaired_add if d.path == path] + new_setting = structure.is_new_setting(unpaired_add, path) + gate_added = any(ev == "added" for ev in lifecycle.values()) + if not (new_setting or gate_added): + gaps.extend(d.norm for d in in_path) + continue + # Lead with the most descriptive string on the surface -- a heading or + # title beats a bare field label like "Path", which tells a reader + # nothing about what was added. + lead = max( + in_path, + key=lambda d: ( + d.key in ("title", "heading", "header", "Header"), + d.kind == "obj", + min(len(d.norm), 60), + ), + ) + others = [d.norm for d in in_path if d.norm != lead.norm] + emit( + KIND_NEW_SETTING if new_setting else KIND_ADDED, + "", lead.norm, lead.norm, lead, + [f"and {len(others)} more new string(s) on this surface"] if others else [], + ) + + return list(findings.values()), gaps diff --git a/scripts/uidrift/config.py b/scripts/uidrift/config.py new file mode 100644 index 0000000000..0b20448e99 --- /dev/null +++ b/scripts/uidrift/config.py @@ -0,0 +1,139 @@ +"""The adaptation surface. + +This is the only module that names a repository, a path, or a token. Everything +else in this package is a pure function over the shapes defined here. + +Pointing the detector at a different repo means editing this file and the +checkout steps in the workflow. Nothing else. That is the whole portability +story -- see ADAPTING.md for what actually varies between repos and what +surprised us about wandb/core. +""" + +from __future__ import annotations + +import os +from dataclasses import dataclass +from pathlib import Path + + +@dataclass(frozen=True) +class SourceRepo: + """The repo being watched for user-facing change.""" + + owner_repo: str + local_path_env: str + local_path_default: str + default_head: str + token_env: str + # Only diffs under these roots are fetched. This is what keeps a + # 2,600-line commit affordable: commit_diff() takes them as a pathspec. + ui_roots: tuple[str, ...] + ui_exts: tuple[str, ...] + exclude_globs: tuple[str, ...] + # The hand-maintained union of frontend-reachable gate names. Diffing this + # file across a commit is the flag-lifecycle signal; static presence of a + # flag is deliberately NOT consulted, because engineers leave gates at 100% + # forever and the signal has decayed to noise. + flag_registry: str + flag_hooks: str + codeowners: tuple[str, ...] + + @property + def path(self) -> Path: + return Path(os.environ.get(self.local_path_env) or self.local_path_default).expanduser() + + +@dataclass(frozen=True) +class DocsRepo: + """The docs corpus searched for occurrences of a changed label.""" + + local_path_env: str + local_path_default: str + content_exts: tuple[str, ...] + primary_locale: str + # Indexed and counted for blast radius, but never the find-and-replace target. + mirror_locales: tuple[str, ...] + exclude_dirs: tuple[str, ...] + # Published history. Occurrences here are reported for awareness and never + # proposed as edits -- rewriting a changelog is falsifying a record. + immutable_globs: tuple[str, ...] + nav_manifest: str + + @property + def path(self) -> Path: + return Path(os.environ.get(self.local_path_env) or self.local_path_default).expanduser() + + +SOURCE = SourceRepo( + owner_repo="wandb/core", + local_path_env="CORE_REPO", + local_path_default="~/core", + default_head="origin/master", + token_env="WANDB_CORE_TOKEN", + ui_roots=("frontends/app/src",), + ui_exts=(".tsx", ".jsx"), + exclude_globs=( + "*.test.tsx", "*.test.ts", "*.spec.tsx", + "*.stories.tsx", "*/__mocks__/*", "*/__tests__/*", + "*/wandb-admin/*", # internal admin UI, not a customer surface + ), + flag_registry="frontends/app/src/util/useRampFlag.ts", + flag_hooks="frontends/app/src/util/rampFeatureFlags/rampFeatureFlags.ts", + codeowners=(".github/CODEOWNERS", "CODEOWNERS"), +) + +DOCS = DocsRepo( + local_path_env="DOCS_REPO", + # The docs corpus is native -- this package lives at /scripts/uidrift, + # so the repo root is two levels up. Resolved from the module's own location + # rather than the cwd, so the detector works the same from a test runner, a + # subdirectory, and a CI step. + local_path_default=str(Path(__file__).resolve().parents[2]), + content_exts=(".mdx",), + primary_locale="en", + mirror_locales=("ja", "ko", "fr"), + # NB: snippets/ is deliberately NOT excluded. Reusable fragments carry real + # UI prose ("go to the **Service Accounts** tab") and render into many + # pages, so a label there has wider blast radius than one in a single page. + # .claude is excluded because it holds git worktrees -- indexing it would + # double-count every occurrence. + # uidrift is excluded to break a feedback loop: a report quotes the very + # labels it is reporting on, so indexing our own output would make every + # finding look documented. Reports are .md and only .mdx is indexed today, + # which makes this insurance rather than a fix. + exclude_dirs=( + ".git", "node_modules", ".claude", "docengine-site", "docengine", + "scripts", "images", "assets", "media", "css", "icons", "layouts", + "uidrift", + ), + immutable_globs=("release-notes/*",), + nav_manifest="docs.json", +) + +# Where state and output land, relative to the host repo root. +LEDGER_PATH = Path("uidrift/ledger.json") +REPORT_DIR = Path("uidrift/reports") + +# --- Tunables ------------------------------------------------------------- +# A string that has not changed for this long, and has not been re-changed, is +# safe to act on. Observed re-churn interval in wandb/core is 7 days: a label +# renamed on 2026-06-03 was renamed again on 2026-06-10. Filing docs work on +# day 2 wastes a PR and teaches the group the detector generates churn. +# +# Counted from when the commit LANDED on the watched branch, not when it was +# authored -- see build._landed_date. Rebase and cherry-pick preserve the author +# date, so measuring from that would let a months-old commit arrive already +# "settled" and skip this protection entirely. +SETTLED_DAYS = 7 + +# A literal appearing on more pages than this is too generic to be a UI label. +# Calibration: naive substring matching puts "search" on 215 pages. +MAX_DOCS_PAGES = 15 + +# There is deliberately no confidence threshold here. Agent eligibility is +# decided by `triage()` from structural facts -- literal kind, docs coverage, +# whether every occurrence is emphasized -- not by a score. A +# MIN_AGENT_CONFIDENCE tunable used to sit here and was never read by anything, +# which advertised a safety floor that did not exist. If the model pass later +# produces a real confidence value, wire it into triage() in the same change +# that introduces it. diff --git a/scripts/uidrift/docsindex.py b/scripts/uidrift/docsindex.py new file mode 100644 index 0000000000..6a6eb37383 --- /dev/null +++ b/scripts/uidrift/docsindex.py @@ -0,0 +1,392 @@ +"""The docs corpus, indexed for one question: where does this UI label appear? + +DIRECTION DISCIPLINE. This module can raise confidence that a finding is real. +It can never lower it. There is deliberately no function here that returns a +score, a penalty, or a "not documented" verdict that a caller could subtract. + +The reason is a feedback loop, not style. If missing docs were allowed to +suppress a finding, then: a surface looks unreleased -> we suppress the row -> +nobody writes the page -> there are still no docs -> it is still suppressed. +Forever. And "available but undocumented" is precisely the gap this whole +project exists to find. So absence of docs is routed to a human as a coverage +gap; it is never evidence of anything. + +The asymmetry is enforced by what this module refuses to expose, so a future +caller cannot reintroduce the loop by accident. +""" + +from __future__ import annotations + +import fnmatch +import re +from dataclasses import dataclass, field +from pathlib import Path +from typing import Optional + +from . import config + +# Context in which a literal appears on a page. Everything except `prose` is a +# UI reference -- the writer marked it up as a thing on screen. +CTX_BOLD = "bold" +CTX_CODE = "code" +CTX_QUOTED = "quoted" +CTX_DEICTIC = "deictic" +CTX_PROSE = "prose" + +UI_CONTEXTS = frozenset({CTX_BOLD, CTX_CODE, CTX_QUOTED, CTX_DEICTIC}) + +# Nouns that mark "the X " as a reference to a control rather than prose. +_UI_NOUNS = ( + "button|tab|toggle|field|menu|dropdown|drop-down|column|setting|settings" + "|page|panel|section|checkbox|dialog|modal|drawer|link|icon|header|option" +) + +_TOKEN = re.compile(r"[a-z0-9]+") +_FRONTMATTER = re.compile(r"\A---\n.*?\n---\n", re.S) + + +@dataclass(frozen=True) +class DocsOccurrence: + page: str # repo-relative + line: int + context: str + locale: str + immutable: bool # published history; report, never propose an edit + + @property + def is_ui_reference(self) -> bool: + return self.context in UI_CONTEXTS + + +@dataclass +class DocsLookup: + """Result of looking one literal up in the corpus. + + Carries no score. `eligible=False` means the literal was too generic to + search for meaningfully -- which is NOT the same as "this surface is + undocumented", and callers must not conflate them. + """ + + literal: str + eligible: bool + reason: str + occurrences: list[DocsOccurrence] = field(default_factory=list) + translations_affected: dict[str, int] = field(default_factory=dict) + + @property + def ui_occurrences(self) -> list[DocsOccurrence]: + return [o for o in self.occurrences if o.is_ui_reference] + + @property + def pages(self) -> list[str]: + seen, out = set(), [] + for o in self.ui_occurrences: + if o.page not in seen: + seen.add(o.page) + out.append(o.page) + return out + + @property + def corpus_frequency(self) -> int: + """Pages the literal appears on at all, emphasized or not.""" + return len({o.page for o in self.occurrences}) + + @property + def all_occurrences_emphasized(self) -> bool: + """Every appearance is inside UI markup. + + The predicate that decides whether a find-and-replace is safe. + `**Add panel**` inside a numbered step is a token substitution with no + grammatical consequence. "add a panel to your workspace" is a verb + phrase that happens to contain the same words, and a substitution would + mangle it. + """ + return bool(self.occurrences) and all(o.is_ui_reference for o in self.occurrences) + + @property + def touches_immutable(self) -> bool: + return any(o.immutable for o in self.occurrences) + + @property + def match_confidence(self) -> str: + """How sure are we that these occurrences are references to the control? + + Deliberately three coarse buckets, not a taxonomy. The docs corpus has + more context shapes than are worth classifying -- SDK output, MDX + component props, headings, CSV enum values -- and chasing each one buys + less than reporting the edge case at low confidence and letting a human + glance at it. + + The rule this encodes: text a writer marked up as a control must match + the UI exactly, so a rename is real drift. Text in a run of prose is + governed by the style guide, not by the UI, so a rename usually means + nothing there. Note this cuts both ways -- prose is not free-form + either, it just answers to a different authority. Product surfaces stay + capitalized in prose ("a Models seat") whatever the UI does. + + NB: this grades MATCH QUALITY, not coverage. Absence of docs still + cannot lower anything -- see the module docstring. + """ + if not self.occurrences: + return "low" + if not self.ui_occurrences: + return "low" # prose only: probably style-governed, not a control + if all(o.context == CTX_CODE for o in self.ui_occurrences): + # A backticked string is as often an API value or identifier as a + # UI label -- `"Models Seat"` in org_dashboard.mdx is a CSV column + # value, not a button. + return "medium" + if self.all_occurrences_emphasized: + return "high" + return "medium" + + @property + def replace_targets(self) -> list[DocsOccurrence]: + """The occurrences a fix may touch: marked-up, mutable ones only. + + Prose is never a target. If the UI renames the `Add panel` button, the + bold reference must follow, but "add a panel to your workspace" stays as + it is -- it is a verb phrase, not a reference to the control. Leaving + the page mixed is correct, not inconsistent. + """ + return [o for o in self.ui_occurrences if not o.immutable] + + +@dataclass +class DocsIndex: + root: Path + primary_locale: str + pages: list[str] + text: list[str] + lines: list[list[str]] + immutable: list[bool] + token_pages: dict[str, set[int]] + # locale -> page -> raw text, kept apart so mirrors never become + # find-and-replace targets; they are blast-radius reporting only. + mirrors: dict[str, dict[str, str]] + + # literal -> lookup, for the life of this index. The corpus does not change + # mid-run, so a repeated literal has a repeated answer. Two things make the + # repeats add up: `build_findings` probes a literal once to decide whether + # it is documented and again to attach the evidence, and the same label + # routinely changes in several commits across one scan window. + _memo: dict[str, "DocsLookup"] = field(default_factory=dict, repr=False) + + def __len__(self) -> int: + return len(self.pages) + + +def _is_excluded(rel: Path, cfg: config.DocsRepo) -> bool: + return any(part in cfg.exclude_dirs for part in rel.parts) + + +def _locale_of(rel: Path, cfg: config.DocsRepo) -> str: + head = rel.parts[0] if rel.parts else "" + return head if head in cfg.mirror_locales else cfg.primary_locale + + +def _strip_frontmatter(text: str) -> str: + """Blank out YAML frontmatter, preserving the line count. + + Deleting it outright shifts every line number in the file, so a reported + `page:line` no longer resolves to what a reader sees. Replacing it with the + same number of newlines keeps citations exact while stopping frontmatter + keywords from matching as prose. + """ + m = _FRONTMATTER.match(text) + if not m: + return text + return "\n" * m.group(0).count("\n") + text[m.end():] + + +def build_index(cfg: config.DocsRepo = config.DOCS) -> DocsIndex: + """Load the primary-locale corpus into memory, plus mirror text for counts.""" + root = cfg.path.resolve() + pages: list[str] = [] + text: list[str] = [] + lines: list[list[str]] = [] + immutable: list[bool] = [] + token_pages: dict[str, set[int]] = {} + mirrors: dict[str, dict[str, str]] = {loc: {} for loc in cfg.mirror_locales} + + for ext in cfg.content_exts: + for path in root.rglob(f"*{ext}"): + rel = path.relative_to(root) + if _is_excluded(rel, cfg): + continue + try: + body = _strip_frontmatter(path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError): + continue + + locale = _locale_of(rel, cfg) + relstr = rel.as_posix() + if locale != cfg.primary_locale: + mirrors[locale][relstr] = body + continue + + idx = len(pages) + pages.append(relstr) + text.append(body) + lines.append(body.splitlines()) + immutable.append( + any(fnmatch.fnmatch(relstr, g) for g in cfg.immutable_globs) + ) + for tok in set(_TOKEN.findall(body.lower())): + token_pages.setdefault(tok, set()).add(idx) + + return DocsIndex( + root, cfg.primary_locale, pages, text, lines, immutable, token_pages, mirrors + ) + + +def is_specific_enough(literal: str) -> tuple[bool, str]: + """Is this literal distinctive enough to look up? + + Calibration: naive substring matching puts `search` on 215 pages and + `delete` on 140. A single Title-Case token in backticks in docs is + overwhelmingly a code identifier, not a UI label. Requiring two words OR + all-caps keeps `MODELS SEAT` and `Add reference bucket` while dropping + `Inference`, `Threshold`, `download`. + """ + s = literal.strip() + if len(s) < 3: + return False, "literal too short" + if len(s.split()) >= 2: + return True, "" + if s.isupper(): + return True, "" + return False, "single token and not all-caps: too generic to attribute" + + +def _literal_regex(literal: str) -> re.Pattern[str]: + """Match the literal as a whole term, not as a substring. + + `Add panel` occurs inside `Add panels`, and without boundaries that one + plural inflates the literal from 2 pages to 16 -- enough to trip the + too-generic cap and suppress a real finding -- while also misfiling every + bold `**Add panels**` as unemphasized prose, because the bold matcher then + fails on the trailing `s`. + + Lookarounds rather than \\b so literals that begin or end with punctuation + (`+ Add panel`, `Save...`) still match. + """ + return re.compile(r"(? list[tuple[str, re.Pattern[str]]]: + """Build the UI-emphasis matchers for one literal. + + The literal itself is matched CASE-SENSITIVELY, on purpose. The whole + case-only rename class depends on it: if docs say `MODELS SEAT` and the code + now says `Models Seat`, that is real drift. If docs already say + `Models Seat`, there is nothing to fix. A case-insensitive match cannot tell + those apart and would report drift that has already been fixed. + + Surrounding words use a scoped (?i:...) so `The`/`the` both work. + """ + e = r"(? set[int]: + """Narrow to pages that could contain the literal, using the token index.""" + tokens = set(_TOKEN.findall(literal.lower())) + if not tokens: + return set() + sets = [index.token_pages.get(t, set()) for t in tokens] + if any(not s for s in sets): + return set() + return set.intersection(*sets) + + +def find(index: DocsIndex, literal: str) -> DocsLookup: + """Locate every appearance of `literal` in the primary-locale corpus. + + Memoized on the index. Callers treat the result as read-only; it is shared. + """ + cached = index._memo.get(literal) + if cached is not None: + return cached + found = _find_uncached(index, literal) + index._memo[literal] = found + return found + + +def _find_uncached(index: DocsIndex, literal: str) -> DocsLookup: + eligible, reason = is_specific_enough(literal) + if not eligible: + return DocsLookup(literal, False, reason) + + matchers = _context_regexes(literal) + term = _literal_regex(literal) + occurrences: list[DocsOccurrence] = [] + + for page_idx in _candidate_pages(index, literal): + if not term.search(index.text[page_idx]): + continue + for line_no, line in enumerate(index.lines[page_idx], start=1): + hits = list(term.finditer(line)) + if not hits: + continue + # Classify each occurrence by the emphasis that ENCLOSES it, not by + # whether the line contains emphasis somewhere. In MDX a paragraph + # is usually one line, so "Click **Add panel** to begin. The Add + # panel button..." is a single line holding one emphasized and one + # prose occurrence. Marking the whole line from the first matcher + # collapsed those to one `bold` occurrence, which makes + # `all_occurrences_emphasized` true and can send a finding to the + # agent lane on the strength of prose it never saw. + spans = [(name, [m.span() for m in rx.finditer(line)]) for name, rx in matchers] + for hit in hits: + context = CTX_PROSE + for name, extents in spans: + if any(s <= hit.start() and hit.end() <= e for s, e in extents): + context = name + break + occurrences.append( + DocsOccurrence( + page=index.pages[page_idx], + line=line_no, + context=context, + locale=index.primary_locale, + immutable=index.immutable[page_idx], + ) + ) + + lookup = DocsLookup(literal, True, "", occurrences) + lookup.translations_affected = { + loc: sum(1 for body in pages.values() if term.search(body)) + for loc, pages in index.mirrors.items() + } + lookup.translations_affected = { + k: v for k, v in lookup.translations_affected.items() if v + } + return lookup + + +def coverage(lookup: DocsLookup, cfg_max_pages: Optional[int] = None) -> str: + """`covered` or `none`. + + Returning `none` is a FINDING, not a failure and not a penalty: it means + this surface exists in the product and nothing documents it. It routes to a + human as a coverage gap. It must never be read as "this change does not + matter" -- see the module docstring. + """ + from .finding import COVERAGE_COVERED, COVERAGE_NONE + + max_pages = config.MAX_DOCS_PAGES if cfg_max_pages is None else cfg_max_pages + if not lookup.eligible: + return COVERAGE_NONE + if not lookup.ui_occurrences: + return COVERAGE_NONE + if len({o.page for o in lookup.ui_occurrences}) > max_pages: + # Too widespread to be a specific control. Not a coverage claim either + # way; the triage rule routes this to a pair for a human read. + return COVERAGE_COVERED + return COVERAGE_COVERED diff --git a/scripts/uidrift/extract.py b/scripts/uidrift/extract.py new file mode 100644 index 0000000000..9d5c196ce1 --- /dev/null +++ b/scripts/uidrift/extract.py @@ -0,0 +1,289 @@ +"""Stage 1: a unified diff in, label deltas out. + +Deterministic, no model, no network, no token. This is the module that must +produce identical output for identical input forever -- the ledger's dedupe and +`settled` logic both assume a given commit always yields the same finding id. + +Two rules here were learned the expensive way and are load-bearing: + +1. Normalize whitespace, NEVER case. Lowercasing collapses `MODELS SEAT` into + `Models Seat`, which silently destroys the single richest real finding in the + corpus -- an eight-header title-casing that has been wrong in published docs + for six weeks. + +2. Never filter on conventional-commit type. `refactor(app):` and `chore(ui):` + carried real user-visible label changes in every sampled window. The commit + type is recorded as metadata and used by nothing. +""" + +from __future__ import annotations + +import fnmatch +import re +from dataclasses import dataclass +from typing import Iterator, Sequence + +from . import config +from ._vendor.diff_signals import _iter_file_diffs + +# --- Extractors ----------------------------------------------------------- +# Four shapes, because wandb/core has no i18n catalog. Strings are inline in +# JSX, so there is no single file to watch and no key to diff. + +# Attribute names that carry copy are open-ended -- a component library invents +# `saveLabel`, `cancelLabel`, `emptyText`, `confirmText` as it grows. Enumerating +# them guarantees silent misses: the secret-drawer consolidation moved its copy +# into `saveLabel=` and an enumerated matcher scored zero on it. Match on suffix. +_ATTR_SUFFIX = r"[Ll]abel|[Tt]ext|[Tt]itle|[Hh]eader|[Pp]laceholder|[Tt]ooltip|[Hh]eading" +ATTR = re.compile( + r'\b((?:[a-zA-Z][a-zA-Z0-9]*)?(?:' + _ATTR_SUFFIX + r')|aria-label|name)' + r'\s*=\s*"([^"]{2,})"' +) + +# `name=` is genuinely ambiguous: `` is +# copy, `` is an identifier. Reject all-lowercase slug-shaped +# values for these keys only, so unambiguous keys keep values like +# `placeholder="my-reference-bucket"`. +_AMBIGUOUS_ATTR_KEYS = frozenset({"name", "text"}) +_IDENTIFIER_VALUE = re.compile(r"^[a-z][a-z0-9_-]*$") + +# A label reached through an expression rather than a plain string, e.g. +# saveLabel={drawerMode === 'edit' ? 'Replace secret' : 'Add secret'} +# Common wherever one component serves two modes. Each literal is emitted +# separately; `wrapped` marks them, since replacing one blind would be wrong. +ATTR_EXPR = re.compile( + r'\b((?:[a-zA-Z][a-zA-Z0-9]*)?(?:' + _ATTR_SUFFIX + r'))\s*=\s*\{([^}]{2,200})\}' +) +_EXPR_LITERAL = re.compile(r"['\"]([A-Z][^'\"]{1,80})['\"]") + +_OBJ_KEYS = ( + "name|title|slug|label|description|header|Header|tooltip" + "|placeholder|text|subtitle" +) +# Plain literal value: safe for find-and-replace. +OBJ = re.compile( + r"^\s*(" + _OBJ_KEYS + r")\s*:\s*['\"`]([^'\"`$]{2,})['\"`]\s*,?\s*$" +) +# Template literal carrying interpolation, e.g. `Allow ${AGENT_NAME} to ...`. +# Captured so a new setting is still detected, but marked wrapped=True so it can +# never become an unattended find-and-replace target. +OBJ_INTERP = re.compile( + r"^\s*(" + _OBJ_KEYS + r")\s*:\s*`([^`]*\$\{[^`]*)`\s*,?\s*$" +) + +# Text between tags on one line: List only visible runs +# Requiring a real closing `` would otherwise capture `Promise`). +JSX_INLINE = re.compile(r">\s*([A-Z][A-Za-z0-9 ,'’.\-?!:%()/]{1,80}?)\s* tuple[str, str, str]: + """Strict identity, for detecting reflow within a file. + + Prettier re-indentation preserves the expression form exactly, so kind + and key must participate: only an identical string in an identical + position is reflow. + """ + return (self.kind, self.key, self.norm) + + @property + def moved_ident(self) -> str: + """Loose identity, for detecting a string relocating across the commit. + + Deliberately just the string. A label can move from JSX text into a + prop -- the secret-drawer consolidation moved `Add secret` from + `Add secret` into `saveLabel="Add secret"`. The user still + sees it, so it is a move, not a removal. Keying on (kind, key) here + would report 23 phantom removals for that one commit. + """ + return self.norm + + +def normalize(raw: str) -> str: + """Collapse whitespace. Case is preserved deliberately -- see module docstring.""" + return re.sub(r"\s+", " ", raw).strip() + + +def path_is_ui(path: str, cfg: config.SourceRepo = config.SOURCE) -> bool: + if not any(path.startswith(root) for root in cfg.ui_roots): + return False + if not any(path.endswith(ext) for ext in cfg.ui_exts): + return False + return not any(fnmatch.fnmatch(path, pat) for pat in cfg.exclude_globs) + + +def _reject_ownline(text: str) -> bool: + # ` Avatar,` is an import specifier, not a label. + if text.endswith(","): + return True + # A single word with no space is overwhelmingly an identifier. + return " " not in text + + +def _reject_inline(text: str) -> bool: + if not _PASCAL_IDENT.match(text): + return False + # Keep `Save`, `Cancel`, `Delete`; drop `Promise`, `ReactNode`, `WBTable`. + return not _SIMPLE_WORD.match(text) + + +def _scan_line(line: str, sign: str, path: str, line_no: int, in_import: bool) -> Iterator[LabelDelta]: + body = line[1:] + + for m in ATTR.finditer(body): + key, raw = m.group(1), m.group(2) + if key in _AMBIGUOUS_ATTR_KEYS and _IDENTIFIER_VALUE.match(raw): + continue + yield LabelDelta(sign, path, "attr", key, raw, normalize(raw), line_no, False) + + for m in ATTR_EXPR.finditer(body): + key = m.group(1) + for lit in _EXPR_LITERAL.finditer(m.group(2)): + raw = lit.group(1) + yield LabelDelta(sign, path, "attr", key, raw, normalize(raw), line_no, True) + + m = OBJ.match(body) + if m: + raw = m.group(2) + yield LabelDelta(sign, path, "obj", m.group(1), raw, normalize(raw), line_no, False) + return + + m = OBJ_INTERP.match(body) + if m: + raw = m.group(2) + yield LabelDelta(sign, path, "obj", m.group(1), raw, normalize(raw), line_no, True) + return + + for m in JSX_INLINE.finditer(body): + raw = m.group(1) + if _reject_inline(raw): + continue + yield LabelDelta(sign, path, "jsx", "_", raw, normalize(raw), line_no, False) + + stripped = body.strip() + if not in_import and JSX_OWNLINE.match(stripped) and not _reject_ownline(stripped): + # Prettier reflowed this out of its element. The literal itself is + # complete, so it stays a valid replace target. + yield LabelDelta(sign, path, "jsx", "_", stripped, normalize(stripped), line_no, False) + + +def extract_deltas( + diff: str, cfg: config.SourceRepo = config.SOURCE +) -> list[LabelDelta]: + """Walk a unified diff and return every added/removed user-facing string.""" + out: list[LabelDelta] = [] + if not diff: + return out + + for path, body in _iter_file_diffs(diff): + if not path_is_ui(path, cfg): + continue + + old_ln = new_ln = 0 + in_import = False + + for line in body.splitlines(): + m = _HUNK.match(line) + if m: + old_ln, new_ln = int(m.group(1)), int(m.group(2)) + in_import = False + continue + if line.startswith("+++") or line.startswith("---"): + continue + + payload = line[1:] if line and line[0] in "+- " else line + if re.match(r"^\s*import\s", payload) and " from " not in payload: + in_import = True + elif in_import and ("from " in payload or payload.rstrip().endswith(";")): + in_import = False + + if line.startswith("+"): + out.extend(_scan_line(line, "+", path, new_ln, in_import)) + new_ln += 1 + elif line.startswith("-"): + out.extend(_scan_line(line, "-", path, old_ln, in_import)) + old_ln += 1 + else: + old_ln += 1 + new_ln += 1 + + return out + + +# --- Set arithmetic ------------------------------------------------------- +# Generalized from diff_signals.graphql_contract_change, which uses the same +# trick to decide whether a .graphql change is client-visible. + + +def file_has_net_change(deltas: Sequence[LabelDelta]) -> bool: + """Did this file's copy actually change? + + Prettier reflow shows up as `-aria-label="X"` / `+ aria-label="X"`: the same + string on both sides. Set-equality kills it deterministically, with no model + and no heuristic. This is the dominant false positive. + """ + added = sorted(d.ident for d in deltas if d.sign == "+") + removed = sorted(d.ident for d in deltas if d.sign == "-") + return added != removed + + +def commit_net_change( + deltas: Sequence[LabelDelta], +) -> tuple[list[LabelDelta], list[LabelDelta], list[LabelDelta]]: + """Split commit-wide deltas into (added, removed, moved). + + A string removed from file X and added in file Y cancels globally: the user + still sees it, it just lives somewhere else now. Without this, a drawer + consolidation that relocates 23 strings files 23 false "removed" findings. + """ + added = [d for d in deltas if d.sign == "+"] + removed = [d for d in deltas if d.sign == "-"] + both = {d.moved_ident for d in added} & {d.moved_ident for d in removed} + + moved = [d for d in added if d.moved_ident in both] + return ( + [d for d in added if d.moved_ident not in both], + [d for d in removed if d.moved_ident not in both], + moved, + ) + + +def surviving_deltas(deltas: Sequence[LabelDelta]) -> list[LabelDelta]: + """Drop files whose copy did not net-change, then return what remains.""" + by_path: dict[str, list[LabelDelta]] = {} + for d in deltas: + by_path.setdefault(d.path, []).append(d) + out: list[LabelDelta] = [] + for path_deltas in by_path.values(): + if file_has_net_change(path_deltas): + out.extend(path_deltas) + return out diff --git a/scripts/uidrift/finding.py b/scripts/uidrift/finding.py new file mode 100644 index 0000000000..e01dc9b094 --- /dev/null +++ b/scripts/uidrift/finding.py @@ -0,0 +1,190 @@ +"""The Finding record. + +Step 1 defines the schema and the identity rule only. The triage decision +procedure, docs coverage, and ownership arrive in later steps -- the fields are +declared here so the shape is reviewable before anything depends on it. +""" + +from __future__ import annotations + +import hashlib +from dataclasses import asdict, dataclass, field +from typing import Any, Optional + +from .config import MAX_DOCS_PAGES, SETTLED_DAYS + +# Findings are keyed by CONTENT, not by commit SHA. +# +# Two commits routinely produce one documentation problem: `f4861ad` and +# `17d7bf7` title-cased the same family of table headers two weeks apart, and a +# SHA-keyed store would file two rows for one fix. The inverse is just as real: +# `c99e959` renamed a toggle and `ccd66e2` renamed it again seven days later, so +# a SHA-keyed store files two rows for a string that only ever needed one. +# +# SHA does not disappear -- it becomes an array, and remains the join key into +# release-note-genie's cycles//ledger.json. + +KIND_RENAME = "rename" +KIND_ADDED = "added" +KIND_REMOVED = "removed" +KIND_MOVED = "moved" +KIND_NEW_SETTING = "new_setting" + +TRIAGE_AGENT = "agent" +TRIAGE_PAIR = "pair" +TRIAGE_HUMAN = "human" + +COVERAGE_COVERED = "covered" +COVERAGE_NONE = "none" + + +@dataclass +class CommitRef: + """One commit that contributed to this finding. Append-only.""" + + sha: str + date: str + subject: str + author: str + file: str + line: int + pr: Optional[int] = None + + +@dataclass +class Finding: + kind: str + surface: str + old_string: str + new_string: str + literal_kind: str # attr | obj | jsx + literal_key: str + + # Every code surface that made this same change. One docs page says + # "MODELS SEAT" once; renaming it in three different member tables is still + # one edit, so the id must not include the surface or the report shows the + # same fix three times. + surfaces: list[str] = field(default_factory=list) + + commits: list[CommitRef] = field(default_factory=list) + first_seen_date: str = "" + last_changed_date: str = "" + settled: bool = False + + # Reported, never suppressed. A gated change is advance warning: draft the + # docs while the change is fresh and hold the PR. + gate: Optional[dict[str, Any]] = None + not_yet_visible: bool = False + + docs: dict[str, Any] = field(default_factory=dict) + signals: list[str] = field(default_factory=list) + confidence: float = 0.0 + degradations: list[str] = field(default_factory=list) + + triage: str = "" + triage_reason: str = "" + action: str = "" + reviewers: list[str] = field(default_factory=list) + owning_team: str = "" + jira: dict[str, Any] = field(default_factory=dict) + + # --- human fields; a re-scan must never clobber these ----------------- + status: str = "" + assignee: str = "" + decided_by: str = "" + decided_at: str = "" + docs_pr: Optional[int] = None + jira_key: Optional[str] = None + # Captured at decision time because it cannot be reconstructed later. + detection_agreement: str = "" # "" | detected | missed | false_positive + + first_seen: str = "" + last_updated: str = "" + + @property + def id(self) -> str: + raw = f"{self.kind}|{self.old_string}|{self.new_string}" + return hashlib.sha1(raw.encode("utf-8")).hexdigest()[:12] + + def to_dict(self) -> dict[str, Any]: + d = asdict(self) + d["id"] = self.id + return d + + +HUMAN_FIELDS = ( + "status", "assignee", "decided_by", "decided_at", + "docs_pr", "jira_key", "detection_agreement", +) + +MACHINE_REFRESH = ( + "kind", "surface", "surfaces", "old_string", "new_string", "literal_kind", "literal_key", + "commits", "last_changed_date", "settled", "gate", "not_yet_visible", + "docs", "signals", "confidence", "degradations", + "triage", "triage_reason", "action", "reviewers", "owning_team", "jira", +) + + +# --- triage --------------------------------------------------------------- +# +# Deliberately asymmetric: easy to fall out of the agent lane, hard to fall in. +# A wrong `agent` call puts a false statement into published docs with nobody +# watching, which ends the project's credibility in one PR. A wrong `pair` call +# costs a writer fifteen minutes. So every uncertainty routes down. + + +def triage(f: "Finding") -> tuple[str, str]: + """Return (lane, reason). First match wins.""" + docs = f.docs or {} + targets = docs.get("replace_targets") or [] + + # --- human: prose has to be written, not substituted ------------------ + if f.kind in (KIND_ADDED, KIND_NEW_SETTING): + return TRIAGE_HUMAN, "new copy on screen; there is no old string to swap" + if f.kind == KIND_REMOVED and docs.get("coverage") == COVERAGE_COVERED: + return TRIAGE_HUMAN, "docs describe a control that is gone; deprecation is a judgment" + if docs.get("coverage") == COVERAGE_NONE: + return TRIAGE_HUMAN, "no page covers this surface (coverage gap, not a dead end)" + if f.degradations: + return TRIAGE_HUMAN, f"incomplete evidence: {', '.join(f.degradations)}" + + # --- pair: a mechanical edit exists, but its blast radius is unclear --- + if f.not_yet_visible: + return TRIAGE_PAIR, "gated: draft the change now, hold the PR until it ships" + if f.kind == KIND_MOVED: + return TRIAGE_PAIR, "string relocated rather than changed; it may still render" + if not f.settled: + return TRIAGE_PAIR, f"changed within {SETTLED_DAYS}d or changed twice; still moving" + if "ambiguous_pairing" in f.signals: + return TRIAGE_PAIR, "several equally good replacements; cannot tell which is which" + if "ambiguous_chain" in f.signals: + # The string was renamed again later, but to more than one thing, so the + # current target cannot be established. Substituting the intermediate + # would publish a label the product no longer uses. + return TRIAGE_PAIR, "renamed again in a later commit; cannot tell which target is current" + if "url_changed" in f.signals: + return TRIAGE_PAIR, "slug changed, so links and anchors moved too, not just words" + if not targets: + return TRIAGE_PAIR, "only occurrences are in published release notes; nothing to edit" + if docs.get("code_context_only"): + return TRIAGE_PAIR, "only appears in code spans; may be an API value, not a label" + if docs.get("corpus_frequency", 0) > MAX_DOCS_PAGES: + return TRIAGE_PAIR, f"appears on {docs['corpus_frequency']} pages; too broad to be one control" + + # --- agent ------------------------------------------------------------ + n = len(targets) + where = "page" if len({t["page"] for t in targets}) == 1 else "pages" + return TRIAGE_AGENT, ( + f"settled 1:1 rename, {n} marked-up occurrence{'s' if n != 1 else ''} " + f"across {len({t['page'] for t in targets})} {where}" + ) + + +def action_for(lane: str, kind: str) -> str: + if lane == TRIAGE_AGENT: + return "cut a docs PR (find-and-replace on marked-up occurrences)" + if lane == TRIAGE_PAIR: + return "writer confirms scope, then an agent applies it" + if kind in (KIND_ADDED, KIND_NEW_SETTING): + return "write new docs" + return "review and decide" diff --git a/scripts/uidrift/ledger.py b/scripts/uidrift/ledger.py new file mode 100644 index 0000000000..ed382c453d --- /dev/null +++ b/scripts/uidrift/ledger.py @@ -0,0 +1,498 @@ +"""State that survives a re-scan. + +Two jobs live here and only one of them needs a file on disk. + +`merge_findings` collapses the per-commit finding lists a scan produces into one +row per docs task. It looks like the ledger's job and is not: identity is already +content-addressed, and every input is in the commit history, so a re-scan +recomputes it identically. Storing it would only create something that can go +stale. This is what finally resolves the two-commit `ORG ROLE` cluster -- and it +also fixes a real bug in doing so. `settled` was computed from the date of +whichever commit happened to be in front of the loop; it has to come from the +LAST change, or a label renamed 60 days ago and renamed again yesterday reports +as safe to act on. + +`apply_decisions` is the part that genuinely needs a file, because "a human +looked at this and it is fine" cannot be derived from anything. That is the only +thing the ledger stores. Dedupe, settledness, triage, ownership and docs +coverage are all recomputed every run. + +The suppression here obeys the same one-directional rule as the rest of the +detector: a stored decision must never hide a finding that has since become +real. So a decision records the docs evidence it was made against, and if that +evidence later EXPANDS -- a page starts mentioning the string, a new editable +occurrence appears -- the decision is surfaced as stale rather than honored. +Evidence shrinking is not a reopen; that is the work getting done. +""" + +from __future__ import annotations + +import copy +import json +from dataclasses import asdict, dataclass, field +from datetime import date, datetime, timedelta +from pathlib import Path +from typing import Any, Iterable, Optional, Sequence + +from . import config +from .finding import ( + COVERAGE_COVERED, + COVERAGE_NONE, + HUMAN_FIELDS, + KIND_RENAME, + Finding, + action_for, + triage, +) + +LEDGER_VERSION = 1 + +# A human looked at this and decided it needs no docs change. Suppressed. +STATUS_DISMISSED = "dismissed" +# Real, and queued -- jira_key or assignee says where. Still reported. +STATUS_ACCEPTED = "accepted" +# Docs were updated. Normally self-extinguishing: the next scan probes the old +# string, does not find it in docs, and never emits the finding. So a `fixed` +# decision that still matches a finding means the fix did not land or was +# reverted, and that is worth saying out loud rather than suppressing. +STATUS_FIXED = "fixed" + +STATUSES = (STATUS_DISMISSED, STATUS_ACCEPTED, STATUS_FIXED) + + +class LedgerError(Exception): + """The ledger file is unreadable or malformed. + + Always raised, never swallowed. This file is hand-edited and holds the only + unrecoverable state in the system; silently starting from an empty ledger + would discard human decisions and re-report everything already settled. + """ + + +@dataclass +class Decision: + """One human judgment about one finding.""" + + status: str + assignee: str = "" + decided_by: str = "" + decided_at: str = "" + docs_pr: Optional[int] = None + jira_key: Optional[str] = None + detection_agreement: str = "" # "" | detected | missed | false_positive + note: str = "" + # The docs evidence this decision was made against. See evidence_expanded. + evidence: dict[str, Any] = field(default_factory=dict) + # Decorative: a 12-character id tells a human nothing when they open the + # file to add a note. Never read back, so it cannot drift into a lie. + change: str = "" + + def human_fields(self) -> dict[str, Any]: + return { + "status": self.status, + "assignee": self.assignee, + "decided_by": self.decided_by, + "decided_at": self.decided_at, + "docs_pr": self.docs_pr, + "jira_key": self.jira_key, + "detection_agreement": self.detection_agreement, + } + + +@dataclass +class Merged: + """One run's findings, collapsed to one row per docs task.""" + + findings: list[Finding] = field(default_factory=list) + # A->B->A. Net zero change, so docs are already correct and there is nothing + # to report -- but counted, never silently dropped. + reverted: list[Finding] = field(default_factory=list) + + +@dataclass +class Applied: + """What the ledger did to this run's findings.""" + + findings: list[Finding] = field(default_factory=list) + suppressed: list[Finding] = field(default_factory=list) + reopened: list[Finding] = field(default_factory=list) + unresolved: list[Finding] = field(default_factory=list) + # Decision ids that matched nothing this run. Never auto-deleted -- see + # prunable() for why that is a human's call. + orphans: list[str] = field(default_factory=list) + + +# --- merge (recomputed, never stored) ------------------------------------- + + +def _is_settled(when: str, today: date) -> bool: + try: + parsed = datetime.fromisoformat(when).date() + except ValueError: + return False + return (today - parsed) >= timedelta(days=config.SETTLED_DAYS) + + +def _dedup(values: Iterable[str]) -> list[str]: + """Order-preserving dedupe. Order is what makes the report diff readably.""" + seen: set[str] = set() + out: list[str] = [] + for v in values: + if v not in seen: + seen.add(v) + out.append(v) + return out + + +def _last_date(f: Finding) -> str: + return max((c.date for c in f.commits), default="") + + +def _first_date(f: Finding) -> str: + return min((c.date for c in f.commits), default="") + + +def _absorb(base: Finding, parts: Sequence[Finding], *, today: date) -> Finding: + """Fold the commits, surfaces and evidence of `parts` into a copy of `base`.""" + out = copy.deepcopy(base) + + commits: dict[str, Any] = {} + for inst in parts: + for c in inst.commits: + commits.setdefault(c.sha, c) + out.commits = sorted(commits.values(), key=lambda c: c.date) + + dates = [c.date for c in out.commits if c.date] + out.first_seen_date = min(dates) if dates else "" + out.last_changed_date = max(dates) if dates else "" + # The whole point of settledness is "has stopped moving", so it comes from + # the most recent change, not whichever one the loop happened to be on. + out.settled = _is_settled(out.last_changed_date, today) if dates else False + + out.surfaces = _dedup(s for inst in parts for s in (inst.surfaces or [inst.surface])) + out.signals = _dedup(s for inst in parts for s in inst.signals) + # Any instance with incomplete evidence routes the merged row down. + out.degradations = _dedup(d for inst in parts for d in inst.degradations) + return out + + +def _chain_links(renames: Sequence[Finding]) -> dict[int, Optional[int]]: + """Map each rename to the rename that supersedes it, by index. + + A link exists only when it is unambiguous in both directions: exactly one + later rename starts from this one's new string, and exactly one rename ends + at that string. A fork or a join is left unlinked and flagged, because + guessing which target is current is precisely the mistake that would put a + stale string into published docs. + """ + by_old: dict[str, list[int]] = {} + by_new: dict[str, list[int]] = {} + for i, f in enumerate(renames): + by_old.setdefault(f.old_string, []).append(i) + by_new.setdefault(f.new_string, []).append(i) + + links: dict[int, Optional[int]] = {} + for i, f in enumerate(renames): + nxt = [ + j for j in by_old.get(f.new_string, []) + if j != i and _first_date(renames[j]) >= _last_date(f) + ] + prev = [j for j in by_new.get(f.new_string, []) if j != i] + links[i] = nxt[0] if len(nxt) == 1 and len(prev) == 0 else None + if len(nxt) > 1: + f.signals.append("ambiguous_chain") + return links + + +def _chain_renames(renames: Sequence[Finding], *, today: date) -> Merged: + """Collapse A->B->C into A->C. + + The head's docs payload is kept, not the tail's: the probe that found docs + evidence ran against the head's OLD string, which is what the pages actually + say. Only the replacement target moves to the end of the chain. + """ + links = _chain_links(renames) + successors = {j for j in links.values() if j is not None} + out = Merged() + walked: set[int] = set() + + for i in range(len(renames)): + if i in successors or i in walked: + continue + chain = [i] + walked.add(i) + nxt = links[i] + while nxt is not None and nxt not in walked: + chain.append(nxt) + walked.add(nxt) + nxt = links[nxt] + + parts = [renames[k] for k in chain] + if len(parts) == 1: + out.findings.append(_absorb(parts[0], parts, today=today)) + continue + + head, tail = parts[0], parts[-1] + folded = _absorb(head, parts, today=today) + folded.new_string = tail.new_string + folded.signals = _dedup([*folded.signals, f"rename_chain:{len(parts)}"]) + if folded.old_string == folded.new_string: + # Renamed and renamed back. Docs were right all along. + out.reverted.append(folded) + else: + out.findings.append(folded) + + # Anything a cycle kept us from reaching still has to be reported. + for i, f in enumerate(renames): + if i not in walked: + out.findings.append(_absorb(f, [f], today=today)) + return out + + +def merge_findings(findings: Sequence[Finding], *, today: date) -> Merged: + """Collapse findings that describe the same docs task. + + Two passes, because two different things produce duplicate rows. + `build_findings` dedupes within one commit; the first pass here dedupes the + same change appearing in several commits, and the second follows renames + that were themselves renamed later. Triage is recomputed at the end so it + sees the merged picture rather than one commit's slice of it. + """ + groups: dict[str, list[Finding]] = {} + for f in findings: + groups.setdefault(f.id, []).append(f) + + by_id: list[Finding] = [] + for instances in groups.values(): + ordered = sorted(instances, key=_last_date) + by_id.append(_absorb(ordered[-1], ordered, today=today)) + + renames = [f for f in by_id if f.kind == KIND_RENAME and f.old_string and f.new_string] + chainable = {id(f) for f in renames} + out = _chain_renames(renames, today=today) + out.findings.extend(f for f in by_id if id(f) not in chainable) + + for f in (*out.findings, *out.reverted): + f.triage, f.triage_reason = triage(f) + f.action = action_for(f.triage, f.kind) + return out + + +# --- decisions (the only persisted state) --------------------------------- + + +def evidence_of(f: Finding) -> dict[str, Any]: + """The docs evidence a decision is made against. + + Pages, not line numbers: lines churn on every unrelated docs edit, and a + reopen on that would be pure noise. + """ + docs = f.docs or {} + return { + "coverage": docs.get("coverage") or COVERAGE_NONE, + "pages": sorted({p["page"] for p in (docs.get("pages") or [])}), + # Counts per page, not a set of page names. A set cannot tell "one + # editable occurrence on this page" from "three", so a dismissed + # finding that gained a second occurrence on a page already in the set + # stayed suppressed -- the docs grew and the decision did not notice. + # Still no line numbers: those churn on every unrelated docs edit, and + # reopening on that would be pure noise. + "targets": _page_counts(docs.get("replace_targets") or []), + } + + +def _page_counts(entries: Iterable[dict[str, Any]]) -> dict[str, int]: + counts: dict[str, int] = {} + for entry in entries: + page = entry.get("page") + if page: + counts[page] = counts.get(page, 0) + 1 + return dict(sorted(counts.items())) + + +def _as_counts(value: Any) -> dict[str, int]: + """Read either evidence shape. + + Decisions written before targets carried counts stored a list of page + names, and a hand-written decision may still do that. Treating each name as + a single occurrence makes an old decision compare equal to an unchanged + corpus, so upgrading the shape does not reopen every stored decision at + once. + """ + if isinstance(value, dict): + return {str(k): int(v) for k, v in value.items()} + return {str(page): 1 for page in (value or [])} + + +def evidence_expanded(stored: dict[str, Any], current: dict[str, Any]) -> bool: + """Has the docs evidence grown since the decision was made? + + Only growth counts. A page that stopped mentioning the string means someone + did the work; a page that started mentioning it means the decision was made + on a smaller picture than the one we have now. + """ + # A hand-written decision with no evidence block is taken at face value. + # Requiring the fingerprint would mean a writer cannot dismiss something by + # editing the file, which is the main way this file gets used. + if not stored: + return False + if stored.get("coverage") == COVERAGE_NONE and current.get("coverage") == COVERAGE_COVERED: + return True + if set(current.get("pages") or []) - set(stored.get("pages") or []): + return True + # Growth means a new page OR more editable occurrences on a page already + # known. Shrinkage still counts for nothing: fewer occurrences means + # somebody did the work. + now = _as_counts(current.get("targets")) + before = _as_counts(stored.get("targets")) + return any(count > before.get(page, 0) for page, count in now.items()) + + +def apply_decisions( + findings: Sequence[Finding], decisions: dict[str, Decision] +) -> Applied: + """Overlay stored human decisions onto this run's findings.""" + out = Applied() + seen: set[str] = set() + + for f in findings: + decision = decisions.get(f.id) + if decision is None: + out.findings.append(f) + continue + + seen.add(f.id) + for name, value in decision.human_fields().items(): + if name in HUMAN_FIELDS: + setattr(f, name, value) + + if evidence_expanded(decision.evidence, evidence_of(f)): + f.signals.append(f"reopened:{decision.status}") + out.reopened.append(f) + out.findings.append(f) + elif decision.status == STATUS_DISMISSED: + out.suppressed.append(f) + elif decision.status == STATUS_FIXED: + f.signals.append("marked_fixed_still_detected") + out.unresolved.append(f) + out.findings.append(f) + else: + out.findings.append(f) + + out.orphans = sorted(set(decisions) - seen) + return out + + +def record( + finding: Finding, + status: str, + *, + today: date, + decided_by: str = "", + assignee: str = "", + docs_pr: Optional[int] = None, + jira_key: Optional[str] = None, + detection_agreement: str = "", + note: str = "", +) -> Decision: + """Build a Decision, capturing the evidence it was made against. + + Going through here rather than constructing a Decision directly is what + makes the reopen check work, so it is the only supported way to add one + programmatically. + """ + if status not in STATUSES: + raise LedgerError(f"unknown status {status!r}; expected one of {', '.join(STATUSES)}") + return Decision( + status=status, + assignee=assignee, + decided_by=decided_by, + decided_at=today.isoformat(), + docs_pr=docs_pr, + jira_key=jira_key, + detection_agreement=detection_agreement, + note=note, + evidence=evidence_of(finding), + change=f"{finding.old_string or '(new)'} -> {finding.new_string or '(removed)'}", + ) + + +def prunable(applied: Applied, decisions: dict[str, Decision]) -> list[str]: + """Orphaned decisions that look finished. + + Reported, never acted on automatically. An orphan is ambiguous: the finding + may be genuinely resolved, or the scan window may simply not reach back far + enough to see its commit. Deleting a human's record on that guess is not a + call this code gets to make. + """ + return [ + did for did in applied.orphans + if decisions[did].status in (STATUS_FIXED, STATUS_DISMISSED) + ] + + +# --- persistence ---------------------------------------------------------- + +_DECISION_KEYS = set(Decision.__dataclass_fields__) + + +def _parse_decision(finding_id: str, raw: Any) -> Decision: + if not isinstance(raw, dict): + raise LedgerError(f"decision {finding_id!r} is {type(raw).__name__}, expected an object") + + unknown = sorted(set(raw) - _DECISION_KEYS) + if unknown: + # A typo'd key silently doing nothing is the worst outcome for a + # hand-edited file -- the writer thinks they recorded a decision. + raise LedgerError( + f"decision {finding_id!r} has unknown field(s): {', '.join(unknown)}. " + f"Valid fields: {', '.join(sorted(_DECISION_KEYS))}" + ) + + status = raw.get("status") + if status not in STATUSES: + raise LedgerError( + f"decision {finding_id!r} has status {status!r}; " + f"expected one of {', '.join(STATUSES)}" + ) + return Decision(**raw) + + +def load(path: Optional[Path] = None) -> dict[str, Decision]: + """Read the ledger. A missing file is a normal first run, not an error.""" + target = path or (config.DOCS.path / config.LEDGER_PATH) + if not target.exists(): + return {} + try: + raw = json.loads(target.read_text(encoding="utf-8")) + except (OSError, ValueError) as exc: + raise LedgerError(f"cannot read ledger at {target}: {exc}") from exc + + if not isinstance(raw, dict): + raise LedgerError(f"ledger at {target} is not an object") + version = raw.get("version") + if version != LEDGER_VERSION: + raise LedgerError( + f"ledger at {target} is version {version!r}, expected {LEDGER_VERSION}" + ) + decisions = raw.get("decisions") or {} + if not isinstance(decisions, dict): + raise LedgerError(f"ledger at {target}: 'decisions' is not an object") + + return {fid: _parse_decision(fid, d) for fid, d in decisions.items()} + + +def save(decisions: dict[str, Decision], path: Optional[Path] = None) -> Path: + """Write the ledger. + + Sorted and indented because this file is read and edited by hand, and its + diffs land in review. + """ + target = path or (config.DOCS.path / config.LEDGER_PATH) + target.parent.mkdir(parents=True, exist_ok=True) + payload = { + "version": LEDGER_VERSION, + "decisions": {fid: asdict(decisions[fid]) for fid in sorted(decisions)}, + } + target.write_text(json.dumps(payload, indent=2, sort_keys=True) + "\n", encoding="utf-8") + return target diff --git a/scripts/uidrift/ownership.py b/scripts/uidrift/ownership.py new file mode 100644 index 0000000000..d770262fa7 --- /dev/null +++ b/scripts/uidrift/ownership.py @@ -0,0 +1,223 @@ +"""Who should look at this? + +Two sources with different strengths. CODEOWNERS gives the accountable team but +is coarse -- `/frontends/app/ @wandb/frontend-reviewers` covers most of the app. +Git authorship names actual humans but says nothing about accountability. Report +both; neither is a substitute for the other. + +Both answers are the same for every finding in a run, so both are computed once +per run rather than once per finding. The naive version shelled out `git show` +for CODEOWNERS and one-to-two `git log` invocations per path, which is ~3 +subprocesses per finding for data that never changes mid-run. See ADAPTING.md. +""" + +from __future__ import annotations + +import re +import subprocess +from dataclasses import dataclass +from pathlib import Path +from typing import Optional + +from . import config + +# Below this many distinct recent authors the window is too thin to rank, so it +# widens to full history. Measured: the flagship file has 5 authors with 1-2 +# commits each over six months, but one clear owner (23 commits) over all time. +MIN_RECENT_AUTHORS = 3 + +_BOT = re.compile(r"\[bot\]$|\bbot\b|-agent$|^wandbot|devin-ai", re.I) + +# Author lines are prefixed so they cannot be confused with a path. \x01 cannot +# appear in a git author name or a filename. +_AUTHOR_MARK = "\x01" + +# (root, head, since) -> {path: {author: commits}}. Populated once per run. +_AUTHOR_INDEX: dict[tuple[str, str, Optional[str]], dict[str, dict[str, int]]] = {} +# (root, head) -> parsed CODEOWNERS rules, most general first. +_CODEOWNERS: dict[tuple[str, str], list[tuple[re.Pattern[str], str]]] = {} + + +def reset_caches() -> None: + """Drop the per-run caches. Tests call this; a cron process is short-lived.""" + _AUTHOR_INDEX.clear() + _CODEOWNERS.clear() + + +@dataclass(frozen=True) +class Ownership: + reviewers: list[str] + team: Optional[str] + source: str # "recent" | "all-time" | "none" + + +def _build_author_index(core: Path, since: Optional[str]) -> dict[str, dict[str, int]]: + """One `git log` over the UI roots, yielding every (path, author) pair. + + Scoped to `ui_roots` by pathspec because that is the only place a finding's + path can come from, and unscoped history over wandb/core is far larger than + anything this needs. + """ + cmd = [ + "git", "-C", str(core), "log", config.SOURCE.default_head, + f"--format={_AUTHOR_MARK}%an", "--name-only", + ] + if since: + cmd.append(f"--since={since}") + cmd += ["--", *config.SOURCE.ui_roots] + try: + out = subprocess.run(cmd, capture_output=True, text=True, timeout=180) + except (OSError, subprocess.SubprocessError): + return {} + if out.returncode != 0: + return {} + + index: dict[str, dict[str, int]] = {} + author = "" + for line in out.stdout.splitlines(): + if line.startswith(_AUTHOR_MARK): + author = line[len(_AUTHOR_MARK):].strip() + continue + path = line.strip() + # Cherry-pick and codegen bots are not reviewers. + if not path or not author or _BOT.search(author): + continue + counts = index.setdefault(path, {}) + counts[author] = counts.get(author, 0) + 1 + return index + + +def _author_index(core: Path, since: Optional[str]) -> dict[str, dict[str, int]]: + key = (str(core), config.SOURCE.default_head, since) + if key not in _AUTHOR_INDEX: + _AUTHOR_INDEX[key] = _build_author_index(core, since) + return _AUTHOR_INDEX[key] + + +def _git_authors(core: Path, path: str, since: Optional[str]) -> list[str]: + counts = _author_index(core, since).get(path, {}) + return [n for n, _ in sorted(counts.items(), key=lambda kv: (-kv[1], kv[0]))] + + +def suggest_reviewers( + path: str, + *, + core: Optional[Path] = None, + since: str = "6 months ago", + top: int = 3, +) -> tuple[list[str], str]: + """Rank humans who have touched this file, most commits first.""" + root = core or config.SOURCE.path + recent = _git_authors(root, path, since) + if len(recent) >= MIN_RECENT_AUTHORS: + return recent[:top], "recent" + # Too few recent commits to rank meaningfully -- widen rather than report a + # single drive-by contributor as the owner. + all_time = _git_authors(root, path, None) + if all_time: + return all_time[:top], "all-time" + return recent[:top], "none" if not recent else "recent" + + +def _codeowners_regex(pattern: str) -> re.Pattern[str]: + """Translate a CODEOWNERS glob into a regex. + + Supports the subset that appears in wandb/core: a leading `/` anchor, + `**` across segments, `*` within a segment, and a trailing `/` for + directories. + """ + anchored = pattern.startswith("/") + p = pattern.lstrip("/") + directory = p.endswith("/") + p = p.rstrip("/") + + out: list[str] = [] + i = 0 + while i < len(p): + if p.startswith("**/", i): + # `**/` spans ZERO or more directories, so it has to consume the + # slash too. Emitting `.*` and then the literal `/` requires at + # least one directory, which makes `/src/**/*ramp*` miss + # `src/ramp.tsx` -- and a missed pattern is a missed last-match + # override, which names the wrong team rather than no team. + out.append("(?:.*/)?") + i += 3 + elif p.startswith("**", i): + out.append(".*") + i += 2 + elif p[i] == "*": + out.append("[^/]*") + i += 1 + else: + out.append(re.escape(p[i])) + i += 1 + + body = "".join(out) + prefix = "^" if anchored else "^(?:.*/)?" + suffix = "(?:/.*)?$" if directory else "(?:/.*)?$" + return re.compile(prefix + body + suffix) + + +def _codeowners_rules(core: Path) -> list[tuple[re.Pattern[str], str]]: + """Read and compile CODEOWNERS once per run. + + The file is identical for every finding in a scan, so this is read once and + the globs are compiled once. Rules stay in file order because matching + depends on it. + """ + key = (str(core), config.SOURCE.default_head) + if key in _CODEOWNERS: + return _CODEOWNERS[key] + + content = None + for candidate in config.SOURCE.codeowners: + try: + out = subprocess.run( + ["git", "-C", str(core), "show", + f"{config.SOURCE.default_head}:{candidate}"], + capture_output=True, text=True, timeout=20, + ) + except (OSError, subprocess.SubprocessError): + continue + if out.returncode == 0: + content = out.stdout + break + + rules: list[tuple[re.Pattern[str], str]] = [] + for line in (content or "").splitlines(): + line = line.split("#", 1)[0].strip() + if not line: + continue + parts = line.split() + if len(parts) < 2: + continue + rules.append((_codeowners_regex(parts[0]), " ".join(parts[1:]))) + _CODEOWNERS[key] = rules + return rules + + +def owning_team(path: str, *, core: Optional[Path] = None) -> Optional[str]: + """The CODEOWNERS entry for a path. + + LAST match wins, not first -- that is the GitHub rule, and wandb/core relies + on it: `/frontends/app/` is overridden by `/frontends/app/src/weave` and by + `/frontends/app/**/*ramp**` further down the file. + """ + root = core or config.SOURCE.path + winner = None + for pattern, owners in _codeowners_rules(root): + if pattern.match(path): + winner = owners + return winner + + +def resolve( + path: str, *, core: Optional[Path] = None, commit_author: str = "" +) -> Ownership: + """Reviewers and owning team for one path.""" + reviewers, source = suggest_reviewers(path, core=core) + if commit_author and not _BOT.search(commit_author): + # The person who made the change is the most relevant reviewer, and a + # cherry-pick bot never is. + reviewers = [commit_author] + [r for r in reviewers if r != commit_author] + return Ownership(reviewers[:3], owning_team(path, core=core), source) diff --git a/scripts/uidrift/report.py b/scripts/uidrift/report.py new file mode 100644 index 0000000000..9c2186c950 --- /dev/null +++ b/scripts/uidrift/report.py @@ -0,0 +1,315 @@ +"""Render findings as the markdown table. + +This is the v1 deliverable the working group agreed on: a table, not Jira +tickets. Jira metadata rides along in a column so the day someone flips filing +on, nothing has to be recomputed. +""" + +from __future__ import annotations + +from datetime import date +from typing import Sequence + +from . import config +from .finding import ( + KIND_MOVED, + KIND_NEW_SETTING, + TRIAGE_AGENT, + TRIAGE_HUMAN, + TRIAGE_PAIR, + Finding, +) + +_LANE_ORDER = (TRIAGE_AGENT, TRIAGE_PAIR, TRIAGE_HUMAN) +_LANE_TITLE = { + TRIAGE_AGENT: "Agent can fix unattended", + TRIAGE_PAIR: "Needs a writer's call first", + TRIAGE_HUMAN: "Needs a human to write", +} +# Derived, not hard-coded: config.SOURCE is the one place that names the watched +# repo, and a report that links to a repo the scan did not read is worse than no +# link at all. +_REPO_URL = f"https://github.com/{config.SOURCE.owner_repo}/commit/" + + +def _gaps_section(gaps: int) -> list[str]: + """The undocumented-surface count. + + Shared by both paths on purpose. A run whose findings are all gaps used to + print "No drift to act on" and then omit the one number that explains why, + which reads as "nothing happened" when what happened is that every changed + label was undocumented. + """ + if not gaps: + return [] + return [ + "### Undocumented surfaces", + "", + f"{gaps} changed label(s) match no documentation at all, so nothing in the", + "docs became wrong and they are not listed above. Most are incidental copy", + "— empty states, spinner labels, status pills. The count is here because a", + "sustained rise in it is worth noticing, not because each one needs a row.", + "", + ] + + +def _escape(text: str) -> str: + return text.replace("|", "\\|").replace("\n", " ") + + +def _commit_links(f: Finding) -> str: + return " ".join( + f"[`{c.sha[:7]}`]({_REPO_URL}{c.sha})" for c in f.commits + ) + + +def _change_cell(f: Finding) -> str: + if f.kind == KIND_NEW_SETTING: + return f"new setting **{_escape(f.new_string)}**" + if f.kind == KIND_MOVED: + return f"`{_escape(f.old_string)}` moved" + if not f.old_string: + return f"new **{_escape(f.new_string)}**" + if not f.new_string: + return f"`{_escape(f.old_string)}` removed" + return f"`{_escape(f.old_string)}` → `{_escape(f.new_string)}`" + + +def _docs_cell(f: Finding) -> str: + pages = (f.docs or {}).get("pages") or [] + if not pages: + return "*no page covers this surface*" + shown = [f"`{p['page']}:{p['line']}`" + (" **(release notes)**" if p["immutable"] else "") + for p in pages[:3]] + if len(pages) > 3: + shown.append(f"+{len(pages) - 3} more") + tr = (f.docs or {}).get("translations_affected") or {} + if tr: + shown.append("mirrors: " + ", ".join(f"{n}×{loc}" for loc, n in sorted(tr.items()))) + return "
".join(shown) + + +def _decision_bits(f: Finding) -> list[str]: + """Whatever a human already said about this finding.""" + bits: list[str] = [] + if any(s.startswith("reopened:") for s in f.signals): + prior = next(s.split(":", 1)[1] for s in f.signals if s.startswith("reopened:")) + bits.append(f"**reopened** — was `{prior}`, docs evidence has grown since") + if "marked_fixed_still_detected" in f.signals: + bits.append("**marked fixed but still detected**") + elif f.status and not any(s.startswith("reopened:") for s in f.signals): + bits.append(f"status `{f.status}`") + where = [] + if f.jira_key: + where.append(f.jira_key) + if f.docs_pr: + where.append(f"docs#{f.docs_pr}") + if where: + bits.append(" / ".join(where)) + if f.assignee: + bits.append(f"assigned {_escape(f.assignee)}") + return bits + + +def _notes_cell(f: Finding) -> str: + bits = [_commit_links(f)] + bits.extend(_decision_bits(f)) + if f.gate: + state = "not yet visible" if f.not_yet_visible else "gated (already ramped)" + bits.append(f"gate `{f.gate['name']}` — {state}") + if "case_only_change" in f.signals: + bits.append("case-only change") + if "testid_unchanged" in f.signals: + bits.append("`data-test` unchanged") + if "url_stable" in f.signals: + bits.append("URL stable") + if "url_changed" in f.signals: + bits.append("**URL changed**") + if not f.settled: + bits.append(f"**unsettled** (<{config.SETTLED_DAYS}d)") + if len(f.commits) > 1: + bits.append(f"changed {len(f.commits)}× since {f.first_seen_date[:10]}") + return "
".join(bits) + + +def _row(f: Finding) -> str: + reviewers = ", ".join(f.reviewers) if f.reviewers else "—" + team = f.owning_team or "—" + return "| " + " | ".join([ + f"`{f.id}`", + _escape(f.surface + (f" (+{len(f.surfaces) - 1} more)" if len(f.surfaces) > 1 else "")), + _change_cell(f), + _escape(f.triage_reason), + _docs_cell(f), + f"{_escape(reviewers)}
{_escape(team)}", + _notes_cell(f), + ]) + " |" + + +def _ledger_sections( + suppressed: Sequence[Finding], + reopened: Sequence[Finding], + unresolved: Sequence[Finding], + orphans: Sequence[str], + reverted: Sequence[Finding], +) -> list[str]: + """What the stored decisions did to this run. + + Suppression is always accounted for in the report. A detector that quietly + drops rows is one nobody can audit, and the count is the only way a reader + can tell "no drift" from "all drift already dismissed". + """ + out: list[str] = [] + a = out.append + + if reopened: + a(f"### Reopened decisions ({len(reopened)})") + a("") + a("These were decided once, but the docs evidence has grown since — a page") + a("now mentions the string, or a new editable occurrence appeared. The prior") + a("decision is shown in the table above rather than applied.") + a("") + + if unresolved: + a(f"### Marked fixed, still detected ({len(unresolved)})") + a("") + a("A `fixed` finding normally disappears on its own: the next scan looks for") + a("the old string in docs and does not find it. These are still detected, so") + a("the fix did not land, did not cover every occurrence, or was reverted.") + a("") + + if suppressed: + a(f"### Held back by earlier decisions ({len(suppressed)})") + a("") + a("| ID | Change | Decision | Who | When |") + a("|---|---|---|---|---|") + for f in sorted(suppressed, key=lambda x: x.decided_at): + a("| " + " | ".join([ + f"`{f.id}`", + _change_cell(f), + f"`{f.status}`" + (f" ({f.detection_agreement})" if f.detection_agreement else ""), + _escape(f.decided_by or "—"), + f.decided_at or "—", + ]) + " |") + a("") + + if reverted: + a(f"### Renamed and renamed back ({len(reverted)})") + a("") + a("These labels changed and then changed back within the window, so the docs") + a("were never wrong and there is nothing to edit. Counted rather than listed,") + a("for the same reason as undocumented surfaces.") + a("") + + if orphans: + a(f"### Stored decisions that matched nothing ({len(orphans)})") + a("") + a("Either the drift is genuinely gone, or this scan's window does not reach") + a("back to the commit that caused it. Ambiguous, so nothing was deleted:") + a("") + a("```") + for did in orphans: + a(did) + a("```") + a("") + + return out + + +def render( + findings: Sequence[Finding], + *, + scanned_range: str, + today: date, + commits: int, + ui_commits: int, + candidates: int, + docs_pages: int, + gaps: int = 0, + candidate_deltas: int = 0, + suppressed: Sequence[Finding] = (), + reopened: Sequence[Finding] = (), + unresolved: Sequence[Finding] = (), + orphans: Sequence[str] = (), + reverted: Sequence[Finding] = (), +) -> str: + lines: list[str] = [] + a = lines.append + + a(f"# UI label drift — {config.SOURCE.owner_repo} → docs") + a("") + a(f"Scanned `{scanned_range}` on {today.isoformat()}.") + detail = f" ({candidate_deltas} changed strings)" if candidate_deltas else "" + funnel = ( + f"{commits} commits → {ui_commits} touching UI → {candidates} stage-1 " + f"candidates{detail} → **{len(findings)} findings**, against " + f"{docs_pages} indexed doc pages." + ) + if suppressed: + funnel += f" {len(suppressed)} previously decided finding(s) held back." + a(funnel) + a("") + + if reopened: + a(f"> **{len(reopened)} decided finding(s) reopened.** The docs evidence behind " + f"the original call has grown, so the decision was surfaced instead of " + f"honored. See *Reopened decisions* below.") + a("") + + if not findings: + a("No drift to act on in this window.") + a("") + a("An empty table is a real result, not a broken run: every UI label change in") + a("the window is already reflected in docs, touches no documented surface, or") + if suppressed: + # Saying "nothing found" when findings were held back would be a + # lie of omission, and the reader has no way to catch it. + a("was already decided on. See the sections below for what was held back.") + else: + a("was renamed back before anyone had to act on it.") + a("") + lines.extend(_gaps_section(gaps)) + lines.extend(_ledger_sections(suppressed, reopened, unresolved, orphans, reverted)) + return "\n".join(lines) + "\n" + + by_lane: dict[str, list[Finding]] = {lane: [] for lane in _LANE_ORDER} + for f in findings: + by_lane.setdefault(f.triage, []).append(f) + + a("| Lane | Findings | What it means |") + a("|---|---|---|") + a(f"| **agent** | {len(by_lane[TRIAGE_AGENT])} | mechanical; safe to apply unattended |") + a(f"| **pair** | {len(by_lane[TRIAGE_PAIR])} | a writer scopes it, then an agent applies |") + a(f"| **human** | {len(by_lane[TRIAGE_HUMAN])} | prose has to be written |") + a("") + + # Known lanes first, then anything unrecognized. A finding with an + # unexpected lane must still appear: the header has already counted it, and + # a row that is tallied but not shown is the one failure mode a reader + # cannot detect. + for lane in (*_LANE_ORDER, *(l for l in by_lane if l not in _LANE_ORDER)): + rows = by_lane[lane] + if not rows: + continue + title = _LANE_TITLE.get(lane) or f"Unclassified ({lane or 'no lane'})" + a(f"## {title} ({len(rows)})") + a("") + a("| ID | Surface | Change | Why this lane | Docs | Reviewers / team | Evidence |") + a("|---|---|---|---|---|---|---|") + for f in sorted(rows, key=lambda x: (-len((x.docs or {}).get("pages") or []), x.surface)): + a(_row(f)) + a("") + + gated = [f for f in findings if f.not_yet_visible] + if gated: + a("### Advance warning") + a("") + a(f"{len(gated)} finding(s) are behind a gate that this commit added or newly") + a("wrapped, so users cannot see the change yet. Draft the docs while the change") + a("is fresh and hold the PR — this is lead time, not noise.") + a("") + + lines.extend(_gaps_section(gaps)) + + lines.extend(_ledger_sections(suppressed, reopened, unresolved, orphans, reverted)) + return "\n".join(lines) + "\n" diff --git a/scripts/uidrift/scan.py b/scripts/uidrift/scan.py new file mode 100644 index 0000000000..3835395bf3 --- /dev/null +++ b/scripts/uidrift/scan.py @@ -0,0 +1,420 @@ +"""The entrypoint. Wires the pipeline together and writes the report. + + python3 -m uidrift.scan --since "60 days ago" + python3 -m uidrift.scan --incremental # since the last report + python3 -m uidrift.scan decide --status dismissed --by matt + +Incremental mode takes its base from the newest report already in +`uidrift/reports/`, whose filename carries the head SHA it scanned. There is +deliberately no "last scanned" state file: the reports ARE the record, so the +watermark cannot drift away from what was actually published. See lesson 18 in +ADAPTING.md. + +The scan never writes the ledger. Only `decide` does. A scan that could modify +decisions is a scan that could lose them. +""" + +from __future__ import annotations + +import argparse +import json +import re +import subprocess +import sys +from dataclasses import dataclass +from datetime import date, datetime, timezone +from pathlib import Path +from typing import Optional, Sequence + +from . import build, config, docsindex, extract, ledger, ownership, report +from ._vendor import gitsource + +# uidrift/reports/2026-08-13T131502-71fa9d10412a.md +# +# The time is what makes the watermark deterministic. Reports used to be named +# by date alone and picked with max() over (date, sha, path), so two reports +# merged on one UTC date were ordered by SHA text -- effectively at random. The +# loser could be the newer one, and a watermark that goes backwards re-reports +# drift a writer has already dismissed. +# +# The time group stays optional so a hand-named or older report still parses. +# Those sort before any timestamped report from the same day, which is the safe +# direction: at worst the range is rescanned, never skipped. +_REPORT_NAME = re.compile( + r"^(\d{4}-\d{2}-\d{2})(?:T(\d{6}))?-([0-9a-f]{7,40})\.md$" +) + + +class ScanError(Exception): + """Something the operator has to fix, reported without a traceback.""" + + +@dataclass +class ScanResult: + markdown: str + stats: dict + applied: ledger.Applied + merged: ledger.Merged + + def by_id(self, finding_id: str): + """Any finding this scan saw, reported or not. + + Suppressed and reverted rows are searchable on purpose: changing your + mind about a dismissal is the main reason to reach for `decide` twice. + """ + for f in (*self.applied.findings, *self.applied.suppressed, + *self.applied.unresolved, *self.merged.reverted): + if f.id == finding_id: + return f + return None + + +def _resolve_base(core: Path, *, since: Optional[str], base: Optional[str], + head: str) -> tuple[str, str]: + """Return (base_ref, description) for the range to scan.""" + if base: + resolved = gitsource.resolve_sha(core, base) + if not resolved: + raise ScanError(f"cannot resolve --base {base!r} in {core}") + return resolved, f"{resolved[:12]}..{head}" + + # `git log --since` would drop commits whose author date predates the window + # but which landed inside it. Pinning a base SHA by date and diffing forward + # keeps the range a contiguous range of history. + r = subprocess.run( + ["git", "-C", str(core), "rev-list", "-1", f"--before={since}", head], + capture_output=True, text=True, + ) + if r.returncode != 0 or not r.stdout.strip(): + raise ScanError(f"no commit in {core} before {since!r} on {head}") + return r.stdout.strip(), f"{since} .. {head}" + + +def _add_landing_dates( + core: Path, base: str, head: str, commits: list[dict] +) -> None: + """Annotate commits with their committer date, in place. + + `_vendor/gitsource` reads `%aI`, the author date, which rebase and + cherry-pick preserve -- so it says when a change was written, not when it + reached master. Settledness needs the latter (see `build._landed_date`). + + Done here rather than in the vendored reader on purpose: `_vendor/` is a + faithful copy of another repo's module, and carrying a local edit there + makes every future re-vendor a merge. The key written is the one the real + GitHub API already uses for this, so nothing downstream has to know where + it came from. One extra `git log` with no `--numstat` -- cheap next to the + walk that already happened. + """ + r = subprocess.run( + ["git", "-C", str(core), "log", "--no-merges", "--format=%H %cI", f"{base}..{head}"], + capture_output=True, text=True, + ) + if r.returncode != 0: + return # Non-fatal: build() falls back to the author date. + landed = dict( + line.split(" ", 1) for line in r.stdout.splitlines() if " " in line + ) + for commit in commits: + when = landed.get(commit.get("sha", "")) + if when: + commit.setdefault("commit", {}).setdefault("committer", {})["date"] = when + + +def _last_report(report_dir: Path) -> Optional[tuple[date, str, Path]]: + """The newest report on disk, as (date, head_sha, path). + + Ordered by (date, time), never by SHA: the head SHA is content-addressed + and carries no chronology, so letting it break a tie is a coin flip on + which way the watermark moves. + """ + found: list[tuple[date, str, str, Path]] = [] + for path in report_dir.glob("*.md"): + m = _REPORT_NAME.match(path.name) + if not m: + continue + try: + when = datetime.strptime(m.group(1), "%Y-%m-%d").date() + except ValueError: + continue + found.append((when, m.group(2) or "", m.group(3), path)) + if not found: + return None + when, _time, sha, path = max(found, key=lambda r: (r[0], r[1])) + return when, sha, path + + +def scan( + *, + since: Optional[str] = "60 days ago", + base: Optional[str] = None, + head: Optional[str] = None, + today: Optional[date] = None, + limit: Optional[int] = None, + resolve_owners: bool = True, + core: Optional[Path] = None, + report_dir: Optional[Path] = None, + incremental: bool = False, + progress=lambda msg: None, +) -> ScanResult: + """Run the pipeline.""" + root = core or config.SOURCE.path + if not (root / ".git").exists(): + raise ScanError( + f"{root} is not a git checkout. Set {config.SOURCE.local_path_env} " + f"or clone {config.SOURCE.owner_repo} there." + ) + head = head or config.SOURCE.default_head + today = today or date.today() + reports = report_dir or (config.DOCS.path / config.REPORT_DIR) + + if incremental: + previous = _last_report(reports) + if not previous: + raise ScanError( + f"--incremental needs a previous report in {reports}; " + f"run once with --since first" + ) + base = base or previous[1] + progress(f"incremental from {previous[2].name}") + + base_sha, scanned_range = _resolve_base(root, since=since, base=base, head=head) + + commits = gitsource.iter_commits( + root, base_sha, head, owner_repo=config.SOURCE.owner_repo, limit=limit + ) + _add_landing_dates(root, base_sha, head, commits) + progress(f"{len(commits)} commits in range") + + ui_commits = [ + c for c in commits + if any(extract.path_is_ui(f["filename"]) for f in c.get("files", [])) + ] + progress(f"{len(ui_commits)} touch UI paths") + + index = docsindex.build_index() + progress(f"docs index: {len(index)} pages") + + # Resolved once for the whole run rather than per finding; the caches are + # process-local, so a fresh run always re-reads them. + ownership.reset_caches() + + raw: list = [] + gaps: list[str] = [] + # Two different counts, because the established funnel reports commits while + # the useful calibration number is deltas. + candidate_commits = 0 + candidate_deltas = 0 + for i, commit in enumerate(ui_commits, 1): + diff = gitsource.commit_diff(root, commit["sha"], *config.SOURCE.ui_roots) + if not diff: + continue + surviving = extract.surviving_deltas(extract.extract_deltas(diff)) + if not surviving: + continue + candidate_commits += 1 + candidate_deltas += len(surviving) + added, removed, moved = extract.commit_net_change(surviving) + found, commit_gaps = build.build_findings( + commit, added, removed, moved, diff, index, + today=today, core=root, resolve_owners=resolve_owners, + ) + raw.extend(found) + gaps.extend(commit_gaps) + if i % 50 == 0: + progress(f" {i}/{len(ui_commits)} commits, {len(raw)} raw findings") + + progress(f"{candidate_commits} commits with candidate strings " + f"({candidate_deltas} deltas) -> {len(raw)} raw findings") + + merged = ledger.merge_findings(raw, today=today) + applied = ledger.apply_decisions(merged.findings, ledger.load()) + progress( + f"{len(applied.findings)} findings " + f"({len(applied.suppressed)} suppressed, {len(applied.reopened)} reopened, " + f"{len(merged.reverted)} reverted)" + ) + + markdown = report.render( + applied.findings, + scanned_range=scanned_range, + today=today, + commits=len(commits), + ui_commits=len(ui_commits), + candidates=candidate_commits, + candidate_deltas=candidate_deltas, + docs_pages=len(index), + gaps=len(gaps), + suppressed=applied.suppressed, + reopened=applied.reopened, + unresolved=applied.unresolved, + orphans=applied.orphans, + reverted=merged.reverted, + ) + lanes: dict[str, int] = {} + for f in applied.findings: + lanes[f.triage] = lanes.get(f.triage, 0) + 1 + stats = { + "base": base_sha, + "head": gitsource.resolve_sha(root, head) or head, + "scanned_range": scanned_range, + "commits": len(commits), + "ui_commits": len(ui_commits), + "candidate_commits": candidate_commits, + "candidate_deltas": candidate_deltas, + "findings": len(applied.findings), + "lanes": lanes, + "suppressed": len(applied.suppressed), + "reopened": len(applied.reopened), + "unresolved": len(applied.unresolved), + "reverted": len(merged.reverted), + "gaps": len(gaps), + "orphans": applied.orphans, + } + return ScanResult(markdown=markdown, stats=stats, applied=applied, merged=merged) + + +def _cmd_scan(args: argparse.Namespace) -> int: + def progress(msg: str) -> None: + if not args.quiet: + print(msg, file=sys.stderr) + + result = scan( + since=args.since, + base=args.base, + head=args.head, + limit=args.limit, + resolve_owners=not args.no_owners, + incremental=args.incremental, + progress=progress, + ) + stats = dict(result.stats) + + if args.stdout: + print(result.markdown) + else: + reports = config.DOCS.path / config.REPORT_DIR + reports.mkdir(parents=True, exist_ok=True) + # The head SHA in the name is what makes --incremental work. + # UTC, so the ordering a scheduled run relies on cannot be reshuffled by + # a runner's local timezone. + stamp = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H%M%S") + out = reports / f"{stamp}-{stats['head'][:12]}.md" + out.write_text(result.markdown, encoding="utf-8") + # Relative to the docs repo root, which is where a CI step's cwd is and + # what a PR body has to name. + stats["report"] = str(out.relative_to(config.DOCS.path)) + progress(f"wrote {out}") + + if args.summary_json: + # A caller that has to decide whether to open a PR needs counts, not + # prose. Grepping the rendered report for "No drift to act on" would + # couple a workflow to wording that exists to be readable, not parsed. + Path(args.summary_json).write_text( + json.dumps(stats, indent=2, sort_keys=True) + "\n", encoding="utf-8" + ) + progress(f"wrote {args.summary_json}") + + if stats["reopened"]: + # Worth a distinct exit code: a reopened decision means a human's + # earlier call no longer matches the evidence, which is the one outcome + # that should be able to fail a CI step. + return 3 + return 0 + + +def _cmd_decide(args: argparse.Namespace) -> int: + """Record a human decision against a finding. + + The finding is re-derived by scanning rather than read out of a report, + because the evidence fingerprint has to reflect the corpus as it is now. A + decision stamped with stale evidence would never reopen. + """ + def progress(msg: str) -> None: + if not args.quiet: + print(msg, file=sys.stderr) + + result = scan(since=args.since, base=args.base, resolve_owners=False, + progress=progress) + finding = result.by_id(args.finding_id) + if finding is None: + raise ScanError( + f"no finding {args.finding_id!r} in this window. Widen --since, or " + f"check the id against the newest report." + ) + + decisions = ledger.load() + existing = decisions.get(args.finding_id) + if existing and not args.force: + raise ScanError( + f"{args.finding_id} is already {existing.status!r} " + f"(by {existing.decided_by or 'unknown'} on " + f"{existing.decided_at or 'unknown date'}). Pass --force to replace it." + ) + + decisions[args.finding_id] = ledger.record( + finding, args.status, today=date.today(), + decided_by=args.by, assignee=args.assignee, docs_pr=args.docs_pr, + jira_key=args.jira, detection_agreement=args.agreement, note=args.note, + ) + path = ledger.save(decisions) + print(f"{args.finding_id} -> {args.status} ({path})") + return 0 + + +def main(argv: Optional[Sequence[str]] = None) -> int: + parser = argparse.ArgumentParser( + prog="uidrift.scan", + description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + sub = parser.add_subparsers(dest="command") + + s = sub.add_parser("scan", help="scan a commit range and write a report") + s.add_argument("--since", default="60 days ago", + help="window start, as a git date (default: %(default)r)") + s.add_argument("--base", help="explicit base SHA, overrides --since") + s.add_argument("--head", help=f"default: {config.SOURCE.default_head}") + s.add_argument("--incremental", action="store_true", + help="base on the head SHA of the newest existing report") + s.add_argument("--limit", type=int, help="stop after N commits (for testing)") + s.add_argument("--no-owners", action="store_true", + help="skip reviewer/team resolution") + s.add_argument("--stdout", action="store_true", + help="print the report instead of writing it") + s.add_argument("--summary-json", metavar="PATH", + help="write the run's counts as JSON, for a CI step to read") + s.add_argument("--quiet", action="store_true", help="no progress on stderr") + s.set_defaults(func=_cmd_scan) + + d = sub.add_parser("decide", help="record a human decision") + d.add_argument("finding_id") + d.add_argument("--status", required=True, choices=ledger.STATUSES) + d.add_argument("--by", default="", help="who decided") + d.add_argument("--note", default="", help="why") + d.add_argument("--assignee", default="") + d.add_argument("--jira", help="JIRA key, e.g. DOCS-1234") + d.add_argument("--docs-pr", type=int, help="docs PR number") + d.add_argument("--agreement", default="", + choices=("", "detected", "missed", "false_positive"), + help="was the detector right? captured now, unreconstructable later") + d.add_argument("--force", action="store_true", + help="replace an existing decision for this id") + d.add_argument("--since", default="60 days ago") + d.add_argument("--base", help="explicit base SHA, overrides --since") + d.add_argument("--quiet", action="store_true") + d.set_defaults(func=_cmd_decide) + + args = parser.parse_args(argv) + if not args.command: + parser.print_help() + return 2 + try: + return args.func(args) + except ScanError as exc: + print(f"error: {exc}", file=sys.stderr) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/uidrift/structure.py b/scripts/uidrift/structure.py new file mode 100644 index 0000000000..196a92db18 --- /dev/null +++ b/scripts/uidrift/structure.py @@ -0,0 +1,470 @@ +"""Structural signals: what kind of change is this, and is anyone seeing it yet? + +Pure functions over a diff and the deltas extracted from it. No I/O, no model, +no network -- everything here is readable off the patch, which is what makes it +testable against frozen fixtures and cheap enough to run on every candidate. + +The one exception is `resolve_gate_key`, which reads the ramp registry out of +the watched repo. It is separate, optional, and degrades to None so the rest of +the module stays hermetic. +""" + +from __future__ import annotations + +import difflib +import re +from dataclasses import dataclass +from pathlib import Path +from typing import Iterable, Optional, Sequence + +from . import config +from ._vendor.diff_signals import _iter_file_diffs +from .extract import LabelDelta + +# Two strings pair as a rename above this casefolded similarity. Chosen so that +# a pure case change (ratio 1.0 after casefold) and a light reword both pair, +# while two unrelated column headers do not. +RENAME_RATIO = 0.6 + +# How far back to look for the conditional that governs a line. +GATE_LOOKBACK = 40 + +_HUNK = re.compile(r"^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@") + +# `if (shouldShowX) {`, `} else if (canY) {` +_IF_COND = re.compile(r"\bif\s*\(\s*([A-Za-z_$][\w$]*)\s*\)") +# `{showX && (` -- the JSX short-circuit form +_JSX_COND = re.compile(r"\{\s*([A-Za-z_$][\w$]*)\s*&&") +# `const shouldShowX = useStatsigGateFoo(orgName);` +_GATE_ASSIGN = re.compile( + r"\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*" + r"((?:use)[A-Za-z_$][\w$]*(?:Gate|RampFlag|Flag)[\w$]*)\s*\(" +) +# Only the assigned form above is matched. A gate hook called inline -- no +# intermediate variable -- is not detected: measured against core's UI tree, +# _GATE_ASSIGN finds 261 call sites and an inline matcher would add ~110 more, +# but ~19 of those are `useGatedValue`, an unrelated Weave utility that any +# `use*Gate*` pattern also matches. Widening this needs a name filter and its +# own tests, so it is a follow-up rather than a regex. + +# The hand-maintained union of frontend-reachable gate names in useRampFlag.ts. +_RAMP_KEY_LINE = re.compile(r"^\s*\|\s*'([a-z0-9_.-]+)'\s*$") + +# Hook definition in rampFeatureFlags.ts, for resolving a hook to its Statsig key. +_HOOK_TO_KEY = r"{hook}\s*=[\s\S]{{0,400}}?['\"]([a-z0-9_.-]+)['\"]" + +_TESTID = re.compile(r"\b(data-test|data-testid|data-dd-action-name)\s*=") +_SLUG = re.compile(r"^\s*slug\s*:\s*['\"`]([^'\"`]*)['\"`]") + +# Where a "new setting" plausibly lives. +_SETTINGS_PATH = re.compile(r"(Settings|Privacy|Preferences|Profile)", re.I) + + +@dataclass(frozen=True) +class DiffLine: + sign: str # "+" | "-" | " " + old_no: int + new_no: int + text: str + + @property + def indent(self) -> int: + return len(self.text) - len(self.text.lstrip()) + + +@dataclass(frozen=True) +class RenamePair: + old: LabelDelta + new: LabelDelta + ratio: float + ambiguous: bool + + @property + def case_only(self) -> bool: + return self.old.norm != self.new.norm and self.old.norm.casefold() == self.new.norm.casefold() + + +@dataclass(frozen=True) +class GateScope: + """The conditional governing a changed line, if there is one.""" + + variable: str + hook: Optional[str] + key: Optional[str] # the Statsig key, when resolvable + conditional_added: bool # the `if` itself is a + line in this commit + + @property + def name(self) -> str: + return self.key or self.hook or self.variable + + +def parse_streams(diff: str) -> dict[str, list[DiffLine]]: + """Turn a unified diff into per-file positioned line streams.""" + streams: dict[str, list[DiffLine]] = {} + for path, body in _iter_file_diffs(diff): + lines: list[DiffLine] = [] + old_no = new_no = 0 + for raw in body.splitlines(): + m = _HUNK.match(raw) + if m: + old_no, new_no = int(m.group(1)), int(m.group(2)) + continue + if raw.startswith("+++") or raw.startswith("---"): + continue + if raw.startswith("+"): + lines.append(DiffLine("+", 0, new_no, raw[1:])) + new_no += 1 + elif raw.startswith("-"): + lines.append(DiffLine("-", old_no, 0, raw[1:])) + old_no += 1 + elif raw.startswith(" ") or not raw: + lines.append(DiffLine(" ", old_no, new_no, raw[1:] if raw else "")) + old_no += 1 + new_no += 1 + streams[path] = lines + return streams + + +# --- rename pairing ------------------------------------------------------- + + +def _change_blocks(lines: Sequence[DiffLine]) -> list[tuple[list[DiffLine], list[DiffLine]]]: + """Contiguous runs of changed lines, split into their removed and added halves.""" + blocks: list[tuple[list[DiffLine], list[DiffLine]]] = [] + rem: list[DiffLine] = [] + add: list[DiffLine] = [] + for ln in lines: + if ln.sign == "-": + rem.append(ln) + elif ln.sign == "+": + add.append(ln) + else: + if rem or add: + blocks.append((rem, add)) + rem, add = [], [] + if rem or add: + blocks.append((rem, add)) + return blocks + + +def _positional_pairs( + streams: dict[str, list[DiffLine]], + added: Sequence[LabelDelta], + removed: Sequence[LabelDelta], +) -> list[tuple[int, int]]: + """Pair by position in the diff, before considering similarity at all. + + A complete reword shares almost no characters -- 'Hide manually hidden runs' + and 'List only visible runs' score 0.55, under any threshold loose enough to + be safe elsewhere. But git presents an in-place edit as a `-` line and the + `+` line that replaced it at the same offset in the same change block, which + is far stronger evidence than string similarity ever is. + + Lowering the similarity threshold to catch these would manufacture false + pairs between unrelated labels. Position does not have that failure mode. + """ + rem_at: dict[tuple[str, int], list[int]] = {} + add_at: dict[tuple[str, int], list[int]] = {} + for i, d in enumerate(removed): + rem_at.setdefault((d.path, d.line_no), []).append(i) + for j, d in enumerate(added): + add_at.setdefault((d.path, d.line_no), []).append(j) + + out: list[tuple[int, int]] = [] + for path, lines in streams.items(): + for rem_lines, add_lines in _change_blocks(lines): + for rl, al in zip(rem_lines, add_lines): + r_cands = rem_at.get((path, rl.old_no), []) + a_cands = add_at.get((path, al.new_no), []) + for i in r_cands: + for j in a_cands: + if removed[i].kind == added[j].kind and removed[i].key == added[j].key: + out.append((i, j)) + break + return out + + +def pair_renames( + added: Sequence[LabelDelta], + removed: Sequence[LabelDelta], + streams: Optional[dict[str, list[DiffLine]]] = None, +) -> tuple[list[RenamePair], list[LabelDelta], list[LabelDelta]]: + """Match removed strings to the strings that replaced them. + + Two passes, strongest evidence first: + + 1. Position -- a `-`/`+` at the same offset in one change block is an + in-place edit, whatever the strings look like. + 2. Similarity -- grouped by (path, kind) rather than (path, kind, key), + because a rename can move between fields: `header: 'WEAVE ACCESS'` became + `name: 'Weave Access'` in the same commit, and keying on the field name + would miss it. Greedy on descending ratio, so a file renaming eight + column headers at once pairs all eight instead of collapsing into one + ambiguous blob. + """ + pairs: list[RenamePair] = [] + used_add: set[int] = set() + used_rem: set[int] = set() + + if streams: + for i, j in _positional_pairs(streams, added, removed): + if i in used_rem or j in used_add: + continue + used_rem.add(i) + used_add.add(j) + ratio = difflib.SequenceMatcher( + None, removed[i].norm.casefold(), added[j].norm.casefold() + ).ratio() + pairs.append(RenamePair(removed[i], added[j], ratio, ambiguous=False)) + + groups: dict[tuple[str, str], tuple[list[int], list[int]]] = {} + for i, d in enumerate(removed): + if i not in used_rem: + groups.setdefault((d.path, d.kind), ([], []))[0].append(i) + for j, d in enumerate(added): + if j not in used_add: + groups.setdefault((d.path, d.kind), ([], []))[1].append(j) + + for (_path, _kind), (rem_idx, add_idx) in groups.items(): + if not rem_idx or not add_idx: + continue + + scored: list[tuple[float, int, int]] = [] + for i in rem_idx: + for j in add_idx: + ratio = difflib.SequenceMatcher( + None, removed[i].norm.casefold(), added[j].norm.casefold() + ).ratio() + if ratio >= RENAME_RATIO: + scored.append((ratio, i, j)) + scored.sort(key=lambda t: (-t[0], t[1], t[2])) + + # A removal with several equally-good candidates cannot be resolved from + # the diff alone, so it is paired but flagged -- never agent-eligible. + best: dict[int, float] = {} + for ratio, i, _j in scored: + best[i] = max(best.get(i, 0.0), ratio) + tie_count: dict[int, int] = {} + for ratio, i, _j in scored: + if ratio == best[i]: + tie_count[i] = tie_count.get(i, 0) + 1 + + for ratio, i, j in scored: + if i in used_rem or j in used_add: + continue + used_rem.add(i) + used_add.add(j) + pairs.append( + RenamePair(removed[i], added[j], ratio, ambiguous=tie_count.get(i, 1) > 1) + ) + + return ( + pairs, + [d for j, d in enumerate(added) if j not in used_add], + [d for i, d in enumerate(removed) if i not in used_rem], + ) + + +# --- gating --------------------------------------------------------------- + + +def gate_scope( + streams: dict[str, list[DiffLine]], delta: LabelDelta +) -> Optional[GateScope]: + """Find the conditional that governs this line, if any. + + Per-surface, not per-feature. One gate can govern three surfaces in a single + component with a different answer for each, so the question is always + "is *this string* behind a conditional", answered by reading the enclosing + block -- never by matching a flag name against the commit message. + """ + lines = streams.get(delta.path) + if not lines: + return None + + anchor = None + for idx, ln in enumerate(lines): + pos = ln.new_no if delta.sign == "+" else ln.old_no + if ln.sign == delta.sign and pos == delta.line_no and delta.raw in ln.text: + anchor = idx + break + if anchor is None: + return None + + # A ceiling, not a fixed indent. Every closing delimiter we pass on the way + # up ends a block that CANNOT contain the anchor, so nothing at that depth + # or deeper is an ancestor any more. Without this, a sibling of an + # already-closed gate inherits it: prettier puts a JSX attribute one level + # deeper than its own element, so + # + # {showBeta && ( + # + # )} + # + # + # scanned past `)}` and reported the label as gated by showBeta. + ceiling = lines[anchor].indent + for idx in range(anchor - 1, max(-1, anchor - GATE_LOOKBACK) - 1, -1): + ln = lines[idx] + if ln.sign == "-": + continue + if ln.indent >= ceiling: + continue + + stripped = ln.text.lstrip() + if stripped[:1] in (")", "}", "]"): + # Closes a sibling block. Anything from here up must be shallower + # still to count as enclosing. + ceiling = ln.indent + continue + + m = _IF_COND.search(ln.text) or _JSX_COND.search(ln.text) + if not m: + # Dedented past the enclosing block without finding a conditional. + if ln.text.strip() and ln.indent == 0: + break + continue + + variable = m.group(1) + hook = _resolve_hook(lines, variable) + if hook is None: + # An ordinary conditional, not a gate. `hideManuallyHidden` is UI + # state; treating every `if` as gating would mark most of the app + # "not yet visible". No resolvable hook means no claim. + return None + return GateScope( + variable=variable, + hook=hook, + key=None, + conditional_added=ln.sign == "+", + ) + return None + + +def _resolve_hook(lines: Iterable[DiffLine], variable: str) -> Optional[str]: + """Walk from the conditional variable back to the gate hook that set it.""" + for ln in lines: + if ln.sign == "-": + continue + m = _GATE_ASSIGN.search(ln.text) + if m and m.group(1) == variable: + return m.group(2) + return None + + +def resolve_gate_key( + hook: str, core: Optional[Path] = None, cfg: config.SourceRepo = config.SOURCE +) -> Optional[str]: + """Map a gate hook to its Statsig key by reading the watched repo. + + Optional enrichment: the hook definition usually lives outside the commit + being examined, so this is the one function here that touches the repo. + Returns None on any failure -- a missing key degrades the finding's + confidence, it does not break the scan. + """ + import subprocess + + root = core or cfg.path + try: + out = subprocess.run( + ["git", "-C", str(root), "show", f"{cfg.default_head}:{cfg.flag_hooks}"], + capture_output=True, text=True, timeout=20, + ) + except (OSError, subprocess.SubprocessError): + return None + if out.returncode != 0: + return None + m = re.search(_HOOK_TO_KEY.format(hook=re.escape(hook)), out.stdout) + return m.group(1) if m else None + + +def flag_lifecycle(diff: str, cfg: config.SourceRepo = config.SOURCE) -> dict[str, str]: + """Gate names added or removed from the frontend ramp registry by this commit. + + This is the whole flag signal. Static presence is deliberately not consulted: + engineers leave a gate in place after ramping it to 100% because removing it + is riskier than keeping it, so "there is a flag" has decayed to noise. The + lifecycle events have not: + + added in the same commit as the copy -> not visible yet + removal commit -> GA + present, added long ago -> no signal at all + """ + out: dict[str, str] = {} + for path, body in _iter_file_diffs(diff): + if path != cfg.flag_registry: + continue + for raw in body.splitlines(): + if raw.startswith("+++") or raw.startswith("---"): + continue + if not raw or raw[0] not in "+-": + continue + m = _RAMP_KEY_LINE.match(raw[1:]) + if m: + out[m.group(1)] = "added" if raw[0] == "+" else "removed" + return out + + +# --- corroborating signals ------------------------------------------------ + + +def testid_corroboration( + streams: dict[str, list[DiffLine]], delta: LabelDelta, window: int = 6 +) -> bool: + """Is there an unchanged test id next to this string? + + A `data-test` that stayed put while the adjacent text changed is close to + proof of a pure rename: the element is the same, only its copy moved. Purely + a confidence booster -- absence means nothing, since only a minority of + files carry test ids at all. + """ + lines = streams.get(delta.path) + if not lines: + return False + + for idx, ln in enumerate(lines): + pos = ln.new_no if delta.sign == "+" else ln.old_no + if ln.sign == delta.sign and pos == delta.line_no and delta.raw in ln.text: + lo, hi = max(0, idx - window), min(len(lines), idx + window + 1) + return any( + _TESTID.search(n.text) and n.sign == " " for n in lines[lo:hi] + ) + return False + + +def slug_stability(streams: dict[str, list[DiffLine]], pair: RenamePair) -> str: + """Did the URL survive the rename? + + `name:` changed while a sibling `slug:` was left alone means the tab was + renamed but its route did not move: docs prose is stale, docs links are + fine. A changed slug is a different and worse problem, because anchors and + deep links break too. + """ + if pair.new.kind != "obj": + return "n/a" + lines = streams.get(pair.new.path) + if not lines: + return "n/a" + + changed = [ln for ln in lines if ln.sign in "+-" and _SLUG.match(ln.text)] + if not changed: + return "url_stable" if any(_SLUG.match(ln.text) for ln in lines) else "n/a" + + added = {_SLUG.match(ln.text).group(1) for ln in changed if ln.sign == "+"} + removed = {_SLUG.match(ln.text).group(1) for ln in changed if ln.sign == "-"} + return "url_stable" if added == removed else "url_changed" + + +def is_new_setting(added: Sequence[LabelDelta], path: str) -> bool: + """A newly-added object literal carrying both a title and a description. + + The shape a settings row takes in this codebase, and the reason a new + setting is always a human finding: there is no old string to swap, so + somebody has to write prose. + """ + if not _SETTINGS_PATH.search(path): + return False + keys = {d.key for d in added if d.path == path and d.kind == "obj"} + return "title" in keys and "description" in keys diff --git a/scripts/uidrift/tests/__init__.py b/scripts/uidrift/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/scripts/uidrift/tests/fixtures/9573f30.diff b/scripts/uidrift/tests/fixtures/9573f30.diff new file mode 100644 index 0000000000..9a287ae84c --- /dev/null +++ b/scripts/uidrift/tests/fixtures/9573f30.diff @@ -0,0 +1,2627 @@ +diff --git a/frontends/app/src/agents.md b/frontends/app/src/agents.md +index ecd12d76eda..48af2ffdf3c 100644 +--- a/frontends/app/src/agents.md ++++ b/frontends/app/src/agents.md +@@ -271,9 +271,9 @@ bun run generate:workspace-templates + + - **Redact sensitive data from Meticulous recordings.** We use [Meticulous](https://meticulous.ai) to record user sessions and run visual regression tests. When adding or modifying UI that displays API keys, secrets, tokens, or other sensitive data, you **must** redact it from recordings. There are two layers: + +- **DOM redaction** — Wrap any element that renders sensitive text with the `` component from `src/util/SensitiveInfoWrapper.tsx`. This applies the `meticulous-redact-recording` CSS class with `display: contents` so layout is unaffected: ++ **DOM redaction** — Wrap any element that renders sensitive text with the `` component from `@wandb/weave/components/SensitiveInfoWrapper`. This applies the `meticulous-redact-recording` CSS class with `display: contents` so layout is unaffected: + ```tsx +- import {SensitiveInfoWrapper} from '../util/SensitiveInfoWrapper'; ++ import {SensitiveInfoWrapper} from '@wandb/weave/components/SensitiveInfoWrapper'; + + + +diff --git a/frontends/app/src/components/ApiKeyViewer.tsx b/frontends/app/src/components/ApiKeyViewer.tsx +index 29a691d42c7..9e7ae41b7fb 100644 +--- a/frontends/app/src/components/ApiKeyViewer.tsx ++++ b/frontends/app/src/components/ApiKeyViewer.tsx +@@ -1,4 +1,5 @@ + import {Icon, IconButton} from '@wandb/components'; ++import {SensitiveInfoWrapper} from '@wandb/weave/components/SensitiveInfoWrapper'; + import React, {type FC, useMemo} from 'react'; + import {Button, Divider, Header} from 'semantic-ui-react'; + +@@ -7,7 +8,6 @@ import { + useGenerateApiKeyMutation, + useViewerApiKeysQuery, + } from '../generated/graphql'; +-import {SensitiveInfoWrapper} from '../util/SensitiveInfoWrapper'; + import CopyableText from './CopyableText'; + import {SemanticLoader as Loader} from './utility/InstrumentedLoader'; + +diff --git a/frontends/app/src/components/ApiKeyViewerWithSearch.tsx b/frontends/app/src/components/ApiKeyViewerWithSearch.tsx +index 5f5b7a8ec83..4dc472a89d0 100644 +--- a/frontends/app/src/components/ApiKeyViewerWithSearch.tsx ++++ b/frontends/app/src/components/ApiKeyViewerWithSearch.tsx +@@ -2,6 +2,7 @@ import {Banner, Button, DropdownMenu, IconButton} from '@wandb/components'; + import {fuzzyMatchWithMapping} from '@wandb/weave/common/util/fuzzyMatch'; + import {TextField} from '@wandb/weave/components/Form/TextField'; + import {Icon as WeaveIcon} from '@wandb/weave/components/Icon'; ++import {SensitiveInfoWrapper} from '@wandb/weave/components/SensitiveInfoWrapper'; + import {Tailwind} from '@wandb/weave/components/Tailwind'; + import _ from 'lodash'; + import type {FC} from 'react'; +@@ -16,7 +17,6 @@ import {DeleteApiKeyModal} from '../pages/Billing/AccountSettings/APIKeysTab/Del + import {EditApiKeyDrawer} from '../pages/Billing/AccountSettings/APIKeysTab/EditApiKeyDrawer'; + import {MAX_API_KEYS_PER_SCOPE} from '../util/constants'; + import {DateFormat, format} from '../util/date'; +-import {SensitiveInfoWrapper} from '../util/SensitiveInfoWrapper'; + import {CreateApiKeyDrawer} from './CreateApiKeyDrawer'; + import {SemanticLoader as Loader} from './utility/InstrumentedLoader'; + +diff --git a/frontends/app/src/components/CreateApiKeyDrawer.tsx b/frontends/app/src/components/CreateApiKeyDrawer.tsx +index 6ae908108be..7ea4df507fb 100644 +--- a/frontends/app/src/components/CreateApiKeyDrawer.tsx ++++ b/frontends/app/src/components/CreateApiKeyDrawer.tsx +@@ -2,6 +2,7 @@ import {Drawer} from '@mui/material'; + import {Button} from '@wandb/components'; + import {TextField} from '@wandb/weave/components/Form/TextField'; + import {Icon} from '@wandb/weave/components/Icon'; ++import {SensitiveInfoWrapper} from '@wandb/weave/components/SensitiveInfoWrapper'; + import {Tailwind} from '@wandb/weave/components/Tailwind'; + import type {FC} from 'react'; + import React, {useState} from 'react'; +@@ -16,7 +17,6 @@ import type {GenerateApiKeyMutationData} from '../graphql/users'; + import {useHandleScroll} from '../pages/HomePage/HomePageSidebar/useHandleScroll'; + import {DateFormat, format} from '../util/date'; + import {extractErrorMessageFromApolloError} from '../util/errors'; +-import {SensitiveInfoWrapper} from '../util/SensitiveInfoWrapper'; + import {Banner, BannerFlexWrapper, BannerTextWrapper} from './Banner'; + import {ResizableDrawer} from './common/ResizableDrawer/ResizableDrawer'; + +diff --git a/frontends/app/src/components/Launch/JobDrawer/JobDrawerContent.tsx b/frontends/app/src/components/Launch/JobDrawer/JobDrawerContent.tsx +index 7f7a0a467da..eee64bfb765 100644 +--- a/frontends/app/src/components/Launch/JobDrawer/JobDrawerContent.tsx ++++ b/frontends/app/src/components/Launch/JobDrawer/JobDrawerContent.tsx +@@ -61,7 +61,7 @@ const getViewConfig = (view: JobDrawerView): ViewConfig => { + }; + case JobDrawerView.Secrets: + return { +- title: '', ++ title: 'Add secret', + showBackButton: true, + }; + } +diff --git a/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.test.tsx b/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.test.tsx +new file mode 100644 +index 00000000000..8f656e49a0d +--- /dev/null ++++ b/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.test.tsx +@@ -0,0 +1,113 @@ ++import {render, screen, waitFor} from '@testing-library/react'; ++import userEvent from '@testing-library/user-event'; ++import React from 'react'; ++import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; ++ ++import {type Secret} from '../../../generated/graphql'; ++import { ++ JobDrawerSecretsContext, ++ type JobDrawerSecretsContextType, ++} from './JobDrawerSecretsContext'; ++import {JobDrawerSecretsSubview} from './JobDrawerSecretsSubview'; ++ ++const mockUpsertTeamSecret = vi.hoisted(() => vi.fn()); ++ ++vi.mock('../../../generated/graphql', () => ({ ++ useUpsertTeamSecretMutation: () => [mockUpsertTeamSecret, {loading: false}], ++})); ++ ++const makeSecret = (name: string): Secret => ({name}) as Secret; ++ ++const renderSubview = ({ ++ contextOverrides, ++ onClose = vi.fn(), ++}: { ++ contextOverrides?: Partial; ++ onClose?: () => void; ++} = {}) => { ++ const contextValue: JobDrawerSecretsContextType = { ++ secretsData: [], ++ secretsLoading: false, ++ secretsError: false, ++ refetchSecrets: vi.fn(), ++ ...contextOverrides, ++ }; ++ ++ render( ++ ++ ++ ++ ); ++ ++ return {contextValue, onClose}; ++}; ++ ++const fillValidSecretForm = async () => { ++ const user = userEvent.setup(); ++ ++ await user.type( ++ screen.getByPlaceholderText('Ex. OPENAI_API_KEY'), ++ 'OPENAI_API_KEY' ++ ); ++ await user.type( ++ screen.getByPlaceholderText('Paste your secret value'), ++ 'secret-value' ++ ); ++ ++ return user; ++}; ++ ++describe('JobDrawerSecretsSubview', () => { ++ beforeEach(() => { ++ mockUpsertTeamSecret.mockResolvedValue({ ++ data: {upsertTeamSecret: {secret: {name: 'OPENAI_API_KEY'}}}, ++ }); ++ }); ++ ++ afterEach(() => { ++ vi.clearAllMocks(); ++ }); ++ ++ it('warns on duplicate team secret names case-insensitively and blocks creation', async () => { ++ renderSubview({ ++ contextOverrides: {secretsData: [makeSecret('openai_api_key')]}, ++ }); ++ ++ await fillValidSecretForm(); ++ ++ expect( ++ screen.getByText( ++ 'A secret with this name already exists. Choose a different name or edit the existing secret.' ++ ) ++ ).toBeInTheDocument(); ++ expect(screen.getByRole('button', {name: 'Add secret'})).toBeDisabled(); ++ expect(mockUpsertTeamSecret).not.toHaveBeenCalled(); ++ }); ++ ++ it('creates a team secret when the name is valid and not duplicated', async () => { ++ const refetchSecrets = vi.fn(); ++ const onClose = vi.fn(); ++ renderSubview({ ++ contextOverrides: { ++ secretsData: [makeSecret('OTHER_SECRET')], ++ refetchSecrets, ++ }, ++ onClose, ++ }); ++ ++ const user = await fillValidSecretForm(); ++ await user.click(screen.getByRole('button', {name: 'Add secret'})); ++ ++ await waitFor(() => ++ expect(mockUpsertTeamSecret).toHaveBeenCalledWith({ ++ variables: { ++ entityName: 'test-team', ++ secretName: 'OPENAI_API_KEY', ++ secretValue: 'secret-value', ++ }, ++ }) ++ ); ++ expect(refetchSecrets).toHaveBeenCalled(); ++ expect(onClose).toHaveBeenCalled(); ++ }); ++}); +diff --git a/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.tsx b/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.tsx +index f23a2e79b12..9fd16093c14 100644 +--- a/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.tsx ++++ b/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.tsx +@@ -1,23 +1,12 @@ +-import {Button} from '@wandb/components'; +-import {Checkbox} from '@wandb/weave/components/Checkbox'; +-import {TextArea} from '@wandb/weave/components/Form/TextArea'; +-import {TextField} from '@wandb/weave/components/Form/TextField'; +-import {TailwindContents} from '@wandb/weave/components/Tailwind'; +-import React, {useContext, useState} from 'react'; ++import {Alert} from '@wandb/weave/components/Alert'; ++import {CreateSecretFormContent} from '@wandb/weave/components/Secrets'; ++import React, {useContext} from 'react'; + import {toast} from 'react-toastify'; + +-import {useInsertSecretMutation} from '../../../generated/graphql'; ++import {useUpsertTeamSecretMutation} from '../../../generated/graphql'; + import {extractErrorMessageFromApolloError} from '../../../util/errors'; +-import {SensitiveInfoWrapper} from '../../../util/SensitiveInfoWrapper'; +-import * as TeamSettingsStyles from '../../TeamSettings/TeamSettingsSecrets.styles'; + import {JobDrawerSecretsContext} from './JobDrawerSecretsContext'; + +-const validateSecretName = (name: string) => { +- // Any alphanumeric+underscore string starting with an alphabet character or underscore +- const re = /^[a-zA-Z_][a-zA-Z0-9_]*$/; +- return re.test(String(name)); +-}; +- + export const JobDrawerSecretsSubview = ({ + entityName, + onClose, +@@ -25,115 +14,46 @@ export const JobDrawerSecretsSubview = ({ + entityName: string; + onClose?: () => void; + }) => { +- const [secretName, setSecretName] = useState(''); +- const [secretValue, setSecretValue] = useState(''); +- const [showSecret, setShowSecret] = useState(false); +- +- const {refetchSecrets} = useContext(JobDrawerSecretsContext); ++ const {refetchSecrets, secretsData} = useContext(JobDrawerSecretsContext); + +- // Prevent initial input error when the secret name is empty +- const [isSecretNameModified, setIsSecretNameModified] = useState(false); ++ const [upsertTeamSecret, {loading: isSaving}] = useUpsertTeamSecretMutation(); + +- const [insertSecret] = useInsertSecretMutation(); +- +- const onClickAddSecret = async () => { ++ const handleSave = async (input: { ++ secretName: string; ++ secretValue: string; ++ }) => { + try { +- await insertSecret({ +- variables: {entityName, secretName, secretValue}, ++ await upsertTeamSecret({ ++ variables: { ++ entityName, ++ secretName: input.secretName, ++ secretValue: input.secretValue, ++ }, + }); +- toast.success( +- +- Added secret {secretName} +- +- ); ++ toast.success(`Added secret ${input.secretName}`); + refetchSecrets(); + onClose?.(); + } catch (err) { + const errMsg = extractErrorMessageFromApolloError(err); + console.error(errMsg); +- toast.error(`Problem adding secret ${secretName}: ${errMsg}`); ++ toast.error(`Problem adding secret ${input.secretName}: ${errMsg}`); + } + }; + +- const inputError = isSecretNameModified && !validateSecretName(secretName); +- const canAddSecret = +- !inputError && secretValue.length > 0 && secretName.length > 0; +- + return ( +- +-
+-
+-
+- Add secret +-
+-
+-
+- This secret will be created for {entityName}, +- owner of the destination project for your launch. Team secrets can +- be managed in team settings by team admins. +-
+-
+-
+- Secret name +-
+-
+- { +- setSecretName(e); +- setIsSecretNameModified(true); +- }} +- /> +- {inputError && ( +-
+- Secret names can only contain alphanumeric characters ([a-z], +- [A-Z], [0-9]) or underscores (_) and start with a letter ([a-z], +- [A-Z]) or underscores (_). +-
+- )} +-
+-
Secret
+-
+- +-