diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 4b8aa2a8fe..2cdac73f31 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -151,47 +151,6 @@ 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 deleted file mode 100644 index 8f95debc18..0000000000 --- a/.github/workflows/uidrift-scan.yml +++ /dev/null @@ -1,370 +0,0 @@ -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 deleted file mode 100644 index de75ced201..0000000000 --- a/.github/workflows/uidrift-tests.yml +++ /dev/null @@ -1,51 +0,0 @@ -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 a642aceca8..cfcacc4f58 100644 --- a/.mintignore +++ b/.mintignore @@ -8,11 +8,6 @@ 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 deleted file mode 100644 index 019ccdca1f..0000000000 --- a/scripts/uidrift/ADAPTING.md +++ /dev/null @@ -1,444 +0,0 @@ -# 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 deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/scripts/uidrift/_vendor/__init__.py b/scripts/uidrift/_vendor/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/scripts/uidrift/_vendor/commit_text.py b/scripts/uidrift/_vendor/commit_text.py deleted file mode 100644 index 4fabc3a73f..0000000000 --- a/scripts/uidrift/_vendor/commit_text.py +++ /dev/null @@ -1,118 +0,0 @@ -# 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 deleted file mode 100644 index 9e35a3a40b..0000000000 --- a/scripts/uidrift/_vendor/diff_signals.py +++ /dev/null @@ -1,125 +0,0 @@ -# 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 deleted file mode 100644 index 93a2fecc8d..0000000000 --- a/scripts/uidrift/_vendor/gitsource.py +++ /dev/null @@ -1,127 +0,0 @@ -# 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 deleted file mode 100644 index 844126f77b..0000000000 --- a/scripts/uidrift/build.py +++ /dev/null @@ -1,263 +0,0 @@ -"""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 deleted file mode 100644 index 0b20448e99..0000000000 --- a/scripts/uidrift/config.py +++ /dev/null @@ -1,139 +0,0 @@ -"""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 deleted file mode 100644 index 6a6eb37383..0000000000 --- a/scripts/uidrift/docsindex.py +++ /dev/null @@ -1,392 +0,0 @@ -"""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 deleted file mode 100644 index 9d5c196ce1..0000000000 --- a/scripts/uidrift/extract.py +++ /dev/null @@ -1,289 +0,0 @@ -"""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 deleted file mode 100644 index e01dc9b094..0000000000 --- a/scripts/uidrift/finding.py +++ /dev/null @@ -1,190 +0,0 @@ -"""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 deleted file mode 100644 index ed382c453d..0000000000 --- a/scripts/uidrift/ledger.py +++ /dev/null @@ -1,498 +0,0 @@ -"""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 deleted file mode 100644 index d770262fa7..0000000000 --- a/scripts/uidrift/ownership.py +++ /dev/null @@ -1,223 +0,0 @@ -"""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 deleted file mode 100644 index 9c2186c950..0000000000 --- a/scripts/uidrift/report.py +++ /dev/null @@ -1,315 +0,0 @@ -"""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 deleted file mode 100644 index 3835395bf3..0000000000 --- a/scripts/uidrift/scan.py +++ /dev/null @@ -1,420 +0,0 @@ -"""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 deleted file mode 100644 index 196a92db18..0000000000 --- a/scripts/uidrift/structure.py +++ /dev/null @@ -1,470 +0,0 @@ -"""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 deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/scripts/uidrift/tests/fixtures/9573f30.diff b/scripts/uidrift/tests/fixtures/9573f30.diff deleted file mode 100644 index 9a287ae84c..0000000000 --- a/scripts/uidrift/tests/fixtures/9573f30.diff +++ /dev/null @@ -1,2627 +0,0 @@ -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
--
-- --