From 9b486a39caa29402a26c4e0823ec68ebce6cc3d8 Mon Sep 17 00:00:00 2001 From: Matt Linville Date: Wed, 12 Aug 2026 14:03:36 -0700 Subject: [PATCH 01/16] Add stage 1 of the UI label drift detector Detects user-facing label changes in wandb/core that may have left docs stale. This is step 1 of 9: the deterministic funnel and its regression tests. No ledger, docs index, model, CI, or secrets yet. Over 60 days of origin/master it reduces 2,990 commits to 170 candidates (~20/week) in 27 seconds, with no network or token. Three things worth knowing: - Case is never normalized. Lowercasing would collapse MODELS SEAT into Models Seat, silently cancelling a real drift finding that has been wrong in manage-organization.mdx for six weeks. - Commit type is never filtered on. Half that finding arrived under refactor(app):. - Label-carrying attribute names are matched by suffix, not enumerated. A component library invents saveLabel/isPendingAriaLabel as it grows. The six fixtures are frozen `git show` output from real commits. They caught three bugs that design review missed, so they are the regression surface, not decoration. ADAPTING.md records what is generic and what is wandb/core-specific, for pointing at another repo later. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/uidrift/ADAPTING.md | 190 ++ scripts/uidrift/__init__.py | 0 scripts/uidrift/_vendor/__init__.py | 0 scripts/uidrift/_vendor/commit_text.py | 118 + scripts/uidrift/_vendor/diff_signals.py | 125 + scripts/uidrift/_vendor/gitsource.py | 127 + scripts/uidrift/config.py | 115 + scripts/uidrift/extract.py | 289 ++ scripts/uidrift/finding.py | 117 + scripts/uidrift/tests/__init__.py | 0 scripts/uidrift/tests/fixtures/9573f30.diff | 2627 +++++++++++++++++++ scripts/uidrift/tests/fixtures/c99e959.diff | 467 ++++ scripts/uidrift/tests/fixtures/cb100df.diff | 928 +++++++ scripts/uidrift/tests/fixtures/ccd66e2.diff | 50 + scripts/uidrift/tests/fixtures/e1bc1e6.diff | 126 + scripts/uidrift/tests/fixtures/f4861ad.diff | 1082 ++++++++ scripts/uidrift/tests/test_extract.py | 284 ++ scripts/uidrift_watch.py | 121 + 18 files changed, 6766 insertions(+) create mode 100644 scripts/uidrift/ADAPTING.md create mode 100644 scripts/uidrift/__init__.py create mode 100644 scripts/uidrift/_vendor/__init__.py create mode 100644 scripts/uidrift/_vendor/commit_text.py create mode 100644 scripts/uidrift/_vendor/diff_signals.py create mode 100644 scripts/uidrift/_vendor/gitsource.py create mode 100644 scripts/uidrift/config.py create mode 100644 scripts/uidrift/extract.py create mode 100644 scripts/uidrift/finding.py create mode 100644 scripts/uidrift/tests/__init__.py create mode 100644 scripts/uidrift/tests/fixtures/9573f30.diff create mode 100644 scripts/uidrift/tests/fixtures/c99e959.diff create mode 100644 scripts/uidrift/tests/fixtures/cb100df.diff create mode 100644 scripts/uidrift/tests/fixtures/ccd66e2.diff create mode 100644 scripts/uidrift/tests/fixtures/e1bc1e6.diff create mode 100644 scripts/uidrift/tests/fixtures/f4861ad.diff create mode 100644 scripts/uidrift/tests/test_extract.py create mode 100644 scripts/uidrift_watch.py diff --git a/scripts/uidrift/ADAPTING.md b/scripts/uidrift/ADAPTING.md new file mode 100644 index 0000000000..82c0d944a3 --- /dev/null +++ b/scripts/uidrift/ADAPTING.md @@ -0,0 +1,190 @@ +# 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, `[skip ci]` commit-back | 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. 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. + +## 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) | +| Reduction | 71% | + +Full scan runs in ~27 seconds with no network and no token, because +`gitsource.commit_diff` takes a pathspec and never fetches diffs outside the UI +roots. Keep that property: without it, a 2,600-line commit is unaffordable. + +If your stage-1 count exceeds ~250/60d, tighten before adding stage 2 — a model +pass over the whole `.tsx` stream is mostly waste. + +## The vendored modules + +`_vendor/` holds copies of `gitsource.py`, `diff_signals.py`, and +`commit_text.py` from `wandb/release-note-genie`, each with a provenance header +naming the source commit. They are copies rather than imports on purpose: this +detector must run inside `wandb/docs` with no dependency on another repo being +checked out, and all three are stdlib-only and stable. + +Re-vendor deliberately, never automatically. diff --git a/scripts/uidrift/__init__.py b/scripts/uidrift/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/scripts/uidrift/_vendor/__init__.py b/scripts/uidrift/_vendor/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/scripts/uidrift/_vendor/commit_text.py b/scripts/uidrift/_vendor/commit_text.py new file mode 100644 index 0000000000..4fabc3a73f --- /dev/null +++ b/scripts/uidrift/_vendor/commit_text.py @@ -0,0 +1,118 @@ +# Vendored from wandb/release-note-genie:scripts/commit_text.py at 5846dd1 +# Do not edit here. Upstream changes must be re-vendored deliberately. +# See scripts/uidrift/ADAPTING.md for why this is a copy and not an import. + +#!/usr/bin/env python3 +"""Normalize a commit message down to the prose that actually describes the change. + +Scoring and flag detection used to run keyword scans over the raw commit message. In +wandb/core that message is dominated by PR-template boilerplate, and the boilerplate was +moving scores more than the change itself: + + - Every PR has a ``## Testing`` section, so the low-signal keyword "test" fired on + *every* commit — a universal -1 dressed up as signal. + - Go identifiers inside code spans leaked into prose matching: ``internalKeyInfo`` + matched the low-signal keyword "internal" and cost a real perf fix a point. + - Cherry-pick preambles name the target branch (``server-release-0.83.x``), which + supplied the "release" half of a bogus GA boost, and their conflict-resolution + narrative describes *the port* rather than the change. + - Co-author trailers and Devin session URLs contribute nothing but match on substrings. + +``clean_for_scoring`` strips all of that while always preserving the subject line, which is +the highest-signal text in the message. +""" + +from __future__ import annotations + +import re + +# Fenced code blocks and inline code spans. Identifiers are implementation detail, not a +# description of user-visible behavior, and they are the main source of false keyword hits. +_FENCED_RE = re.compile(r"```.*?```", re.S) +_INLINE_CODE_RE = re.compile(r"`[^`\n]*`") + +# Markdown headings whose contents never describe user-visible behavior. +_DROP_SECTION_TITLES = ( + "testing", + "test plan", + "how to test", + "checklist", + "conflict resolution", + "screenshots", +) +_HEADING_RE = re.compile(r"^\s{0,3}#{1,6}\s*(.+?)\s*#*\s*$") + +# Commit trailers and generated links. +_TRAILER_RE = re.compile( + r"^\s*(co-authored-by|signed-off-by|reviewed-by|acked-by|tested-by|cc|" + r"link to devin session|devin session|generated with|reported-by|fixes|closes)\s*:", + re.I, +) +_URL_RE = re.compile(r"https?://\S+") + +# Markdown task-list rows ("- [x] Added unit tests"). +_CHECKBOX_RE = re.compile(r"^\s*[-*]\s*\[[ xX]\]\s*") + +# Cherry-pick bookkeeping: names the target release branch and describes the port. +_CHERRY_LINE_RE = re.compile(r"cherry[- ]pick", re.I) +_RELEASE_BRANCH_RE = re.compile(r"\b(?:server-)?release[-/][\w.\-]*\b", re.I) + + +def _is_dropped_heading(text: str) -> bool: + lowered = text.strip().lower().rstrip(":").strip() + return any(lowered.startswith(title) for title in _DROP_SECTION_TITLES) + + +def clean_for_scoring(message: str) -> str: + """Return subject + description prose, with boilerplate and code identifiers removed. + + The result is intended for keyword/regex scanning only. It is lossy by design and must + never be shown to a human or used as draft text. + """ + if not message: + return "" + + raw_lines = message.splitlines() + subject = raw_lines[0] if raw_lines else "" + body = "\n".join(raw_lines[1:]) + + body = _FENCED_RE.sub(" ", body) + body = _INLINE_CODE_RE.sub(" ", body) + + kept: list[str] = [] + skipping = False + for line in body.splitlines(): + heading = _HEADING_RE.match(line) + if heading: + # A new heading always ends any section we were skipping. + skipping = _is_dropped_heading(heading.group(1)) + continue + if skipping: + continue + if _TRAILER_RE.match(line) or _CHERRY_LINE_RE.search(line): + continue + line = _CHECKBOX_RE.sub("", line) + line = _URL_RE.sub(" ", line) + kept.append(line) + + cleaned = f"{subject}\n" + "\n".join(kept) + # Release-branch names survive outside cherry-pick lines too (e.g. "onto release-0.83.x"). + cleaned = _RELEASE_BRANCH_RE.sub(" ", cleaned) + # Collapse the blank lines left behind, but keep newlines: several regexes are + # deliberately sentence-local and rely on line breaks as boundaries. + cleaned = re.sub(r"[ \t]+", " ", cleaned) + cleaned = re.sub(r"\n{2,}", "\n", cleaned) + return cleaned.strip() + + +def unwrap_paragraphs(text: str) -> str: + """Join hard-wrapped lines within a paragraph into single logical lines. + + PR bodies wrap at ~72 columns, which splits phrases the sentence-local flag regexes + need to see whole ("gated by the new Statsig gate\\n`some_gate_name`"). + """ + if not text: + return "" + paragraphs = re.split(r"\n\s*\n", text) + joined = [re.sub(r"\s*\n\s*", " ", p).strip() for p in paragraphs] + return "\n".join(p for p in joined if p) diff --git a/scripts/uidrift/_vendor/diff_signals.py b/scripts/uidrift/_vendor/diff_signals.py new file mode 100644 index 0000000000..9e35a3a40b --- /dev/null +++ b/scripts/uidrift/_vendor/diff_signals.py @@ -0,0 +1,125 @@ +# Vendored from wandb/release-note-genie:scripts/diff_signals.py at 5846dd1 +# Do not edit here. Upstream changes must be re-vendored deliberately. +# See scripts/uidrift/ADAPTING.md for why this is a copy and not an import. + +#!/usr/bin/env python3 +"""Signals that can only be read from a commit's diff, not its message or file list. + +Classification runs on the commit message plus the changed-path list. Several judgments have +turned out to need more than that, and the honest response to "the deciding evidence isn't +visible" has been to route to review. This module narrows that set where the diff *can* decide +deterministically — no model, no heuristics over prose. + +Currently one signal, because it is the one where the diff genuinely resolves the ambiguity: + + graphql_contract_change + Editing a published ``.graphql`` file covers two unrelated things, and the path cannot + tell them apart: + + contract change ``+ limit: Int`` / ``+ pattern: String`` on ``historyKeys`` — + new arguments clients can send. Note-worthy. + server annotation ``- id: ID!`` / ``+ id: ID! @skipFieldTrace`` across 34 fields — + cuts ~129M Datadog spans/day. Clients observe nothing. + + The diff separates them exactly: strip the internal directives and a directive-only + change has identical removed and added line sets, while a contract change does not. + +Deliberately NOT here: whether a changed default is note-worthy. That looked like a diff +question and isn't. ``ConnMaxLifetime`` carries a ``schema:"conn_max_lifetime"`` tag, so the +diff says "operator-settable" — yet the parameter appears nowhere in W&B's public docs, so no +admin can act on it and reviewers rejected the note. The deciding fact lives in the docs, not +the diff, so changed defaults still go to a human. A docs cross-reference (does the parameter +name appear in the published docs?) would be the capability that resolves it. +""" + +from __future__ import annotations + +import re +from typing import Optional + +# Directives that are server-side implementation detail: adding or removing one changes +# nothing a client can observe. Anything NOT listed here is treated as contract-relevant, so an +# unfamiliar directive fails toward "contract change" (a human looks) rather than being +# silently ignored. @deprecated is intentionally absent — deprecating a field IS user-facing. +INTERNAL_DIRECTIVES = frozenset({ + "skipFieldTrace", # tracing suppression (wandb/core) + "goField", # gqlgen codegen binding + "goModel", + "goTag", + "goExtraField", + "goEnum", +}) + +_DIFF_FILE_RE = re.compile(r"^diff --git a/(\S+) b/(\S+)", re.M) +_HUNK_RE = re.compile(r"^@@") +_DIRECTIVE_RE = re.compile(r"@(\w+)(\s*\([^)]*\))?") + + +def _iter_file_diffs(diff: str): + """Yield ``(path, body)`` for each file section of a unified diff.""" + matches = list(_DIFF_FILE_RE.finditer(diff or "")) + for i, m in enumerate(matches): + end = matches[i + 1].start() if i + 1 < len(matches) else len(diff) + # Prefer the b/ path (post-image); falls back to a/ for deletions. + yield m.group(2) or m.group(1), diff[m.end():end] + + +def graphql_files_in_diff(diff: str) -> list[str]: + return [p for p, _ in _iter_file_diffs(diff) if p.lower().endswith(".graphql")] + + +def _strip_internal_directives(line: str) -> str: + """Remove only the directives listed in INTERNAL_DIRECTIVES.""" + def sub(m: re.Match) -> str: + return "" if m.group(1) in INTERNAL_DIRECTIVES else m.group(0) + return _DIRECTIVE_RE.sub(sub, line) + + +def _normalize(line: str) -> Optional[str]: + """Normalize a schema line for comparison, or None if it carries no contract meaning.""" + body = line[1:] # drop the +/- marker + body = _strip_internal_directives(body) + body = re.sub(r"\s+", " ", body).strip() + if not body: + return None + if body.startswith("#"): + return None # a comment-only edit is not a contract change + return body + + +def graphql_contract_change(diff: str) -> Optional[bool]: + """Did a diff change a published GraphQL contract? + + Returns True when at least one ``.graphql`` file gained or lost real schema content, + False when every ``.graphql`` change is internal-directive or comment noise, and None + when the diff contains no ``.graphql`` files at all (nothing to say). + """ + if not diff: + return None + + saw_graphql = False + for path, body in _iter_file_diffs(diff): + if not path.lower().endswith(".graphql"): + continue + saw_graphql = True + + removed: list[str] = [] + added: list[str] = [] + for raw in body.splitlines(): + if raw.startswith("---") or raw.startswith("+++") or _HUNK_RE.match(raw): + continue + if raw.startswith("-"): + n = _normalize(raw) + if n is not None: + removed.append(n) + elif raw.startswith("+"): + n = _normalize(raw) + if n is not None: + added.append(n) + + # Identical multisets => every surviving difference was an internal directive or a + # comment, so no client-visible schema content moved. + if sorted(removed) != sorted(added): + return True + + return False if saw_graphql else None diff --git a/scripts/uidrift/_vendor/gitsource.py b/scripts/uidrift/_vendor/gitsource.py new file mode 100644 index 0000000000..93a2fecc8d --- /dev/null +++ b/scripts/uidrift/_vendor/gitsource.py @@ -0,0 +1,127 @@ +# Vendored from wandb/release-note-genie:scripts/rolling/gitsource.py at 5846dd1 +# Do not edit here. Upstream changes must be re-vendored deliberately. +# See scripts/uidrift/ADAPTING.md for why this is a copy and not an import. + +#!/usr/bin/env python3 +"""Read commit deltas from a local wandb/core clone. + +The rolling watcher analyzes the delta between the last SHA it saw and master. In CI the +runner checks out wandb/core; locally the clone lives at ~/core. Reading history from git +(rather than the GitHub compare API) means the watcher needs no token and is fast for the +incremental delta. + +``iter_commits`` returns commit dicts shaped like the GitHub API objects the existing +scoring code expects (``commit['commit']['message']``, ``commit['files']``, ...), so +score_commit_impact / extract_pr_number can be reused unchanged. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from typing import Optional + +_REC_SEP = "\x1e" +_FIELD_SEP = "\x1f" + + +def _git(core: Path, *args: str) -> subprocess.CompletedProcess: + return subprocess.run(["git", "-C", str(core), *args], capture_output=True, text=True) + + +def resolve_sha(core: Path, ref: str) -> Optional[str]: + """Resolve a ref to a concrete SHA, trying a few common fallbacks.""" + for candidate in (ref, f"origin/{ref}"): + r = _git(core, "rev-parse", "--verify", "--quiet", f"{candidate}^{{commit}}") + if r.returncode == 0 and r.stdout.strip(): + return r.stdout.strip() + return None + + +def ref_exists(core: Path, ref: str) -> bool: + return resolve_sha(core, ref) is not None + + +def commit_diff(core: Path, sha: str, *pathspec: str) -> Optional[str]: + """Return a commit's unified diff, optionally limited to a pathspec. + + Used for signals that need the diff's content rather than its file list (see + scripts/diff_signals.py). Always pass a pathspec when only certain files matter: some + wandb/core commits touch hundreds of files and the full patch is large enough to be worth + not materializing. Returns None when the commit or repo is unavailable, so callers degrade + to message/path-only classification instead of failing. + """ + args = ["show", "--format=", "--no-color", "-M", sha] + if pathspec: + args += ["--", *pathspec] + r = _git(core, *args) + if r.returncode != 0: + return None + return r.stdout or None + + +def is_ancestor(core: Path, sha: str, ref: str) -> Optional[bool]: + """True if sha is an ancestor of ref; None if ref cannot be resolved.""" + if not ref_exists(core, ref): + return None + target = resolve_sha(core, ref) + r = _git(core, "merge-base", "--is-ancestor", sha, target or ref) + if r.returncode == 0: + return True + if r.returncode == 1: + return False + return None + + +def iter_commits( + core: Path, + base: str, + head: str, + *, + owner_repo: str = "wandb/core", + limit: Optional[int] = None, + include_merges: bool = False, +) -> list[dict]: + """Return API-shaped commit dicts for ``base..head`` (commits in head not in base).""" + fmt = _REC_SEP + "%H" + _FIELD_SEP + "%an" + _FIELD_SEP + "%aI" + _FIELD_SEP + "%B" + _FIELD_SEP + args = ["log", f"--format={fmt}", "--numstat", "--date=iso-strict"] + if not include_merges: + args.append("--no-merges") + args.append(f"{base}..{head}") + r = _git(core, *args) + if r.returncode != 0: + raise RuntimeError(f"git log {base}..{head} failed: {r.stderr.strip()}") + + commits: list[dict] = [] + chunks = r.stdout.split(_REC_SEP) + for chunk in chunks: + if not chunk.strip(): + continue + parts = chunk.split(_FIELD_SEP) + if len(parts) < 5: + continue + sha, author, date_iso, body, numstat_block = parts[0], parts[1], parts[2], parts[3], parts[4] + sha = sha.strip() + if not sha: + continue + paths: list[str] = [] + for line in numstat_block.splitlines(): + line = line.strip() + if not line: + continue + cols = line.split("\t") + if len(cols) == 3 and cols[2]: + paths.append(cols[2]) + commit = { + "sha": sha, + "commit": { + "message": body.strip("\n"), + "author": {"name": author, "date": date_iso}, + }, + "html_url": f"https://github.com/{owner_repo}/commit/{sha}", + "files": [{"filename": p} for p in paths], + } + commits.append(commit) + if limit and len(commits) >= limit: + break + return commits diff --git a/scripts/uidrift/config.py b/scripts/uidrift/config.py new file mode 100644 index 0000000000..3d462b0c2e --- /dev/null +++ b/scripts/uidrift/config.py @@ -0,0 +1,115 @@ +"""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, field +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", + local_path_default=".", # native: the detector runs inside wandb/docs + content_exts=(".mdx",), + primary_locale="en", + mirror_locales=("ja", "ko", "fr"), + exclude_dirs=( + ".git", "node_modules", ".claude", "docengine-site", + "scripts", "snippets", "images", "static", + ), + 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. +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 + +# Below this, a finding can never reach the agent lane. +MIN_AGENT_CONFIDENCE = 0.6 diff --git a/scripts/uidrift/extract.py b/scripts/uidrift/extract.py new file mode 100644 index 0000000000..9d5c196ce1 --- /dev/null +++ b/scripts/uidrift/extract.py @@ -0,0 +1,289 @@ +"""Stage 1: a unified diff in, label deltas out. + +Deterministic, no model, no network, no token. This is the module that must +produce identical output for identical input forever -- the ledger's dedupe and +`settled` logic both assume a given commit always yields the same finding id. + +Two rules here were learned the expensive way and are load-bearing: + +1. Normalize whitespace, NEVER case. Lowercasing collapses `MODELS SEAT` into + `Models Seat`, which silently destroys the single richest real finding in the + corpus -- an eight-header title-casing that has been wrong in published docs + for six weeks. + +2. Never filter on conventional-commit type. `refactor(app):` and `chore(ui):` + carried real user-visible label changes in every sampled window. The commit + type is recorded as metadata and used by nothing. +""" + +from __future__ import annotations + +import fnmatch +import re +from dataclasses import dataclass +from typing import Iterator, Sequence + +from . import config +from ._vendor.diff_signals import _iter_file_diffs + +# --- Extractors ----------------------------------------------------------- +# Four shapes, because wandb/core has no i18n catalog. Strings are inline in +# JSX, so there is no single file to watch and no key to diff. + +# Attribute names that carry copy are open-ended -- a component library invents +# `saveLabel`, `cancelLabel`, `emptyText`, `confirmText` as it grows. Enumerating +# them guarantees silent misses: the secret-drawer consolidation moved its copy +# into `saveLabel=` and an enumerated matcher scored zero on it. Match on suffix. +_ATTR_SUFFIX = r"[Ll]abel|[Tt]ext|[Tt]itle|[Hh]eader|[Pp]laceholder|[Tt]ooltip|[Hh]eading" +ATTR = re.compile( + r'\b((?:[a-zA-Z][a-zA-Z0-9]*)?(?:' + _ATTR_SUFFIX + r')|aria-label|name)' + r'\s*=\s*"([^"]{2,})"' +) + +# `name=` is genuinely ambiguous: `` is +# copy, `` is an identifier. Reject all-lowercase slug-shaped +# values for these keys only, so unambiguous keys keep values like +# `placeholder="my-reference-bucket"`. +_AMBIGUOUS_ATTR_KEYS = frozenset({"name", "text"}) +_IDENTIFIER_VALUE = re.compile(r"^[a-z][a-z0-9_-]*$") + +# A label reached through an expression rather than a plain string, e.g. +# saveLabel={drawerMode === 'edit' ? 'Replace secret' : 'Add secret'} +# Common wherever one component serves two modes. Each literal is emitted +# separately; `wrapped` marks them, since replacing one blind would be wrong. +ATTR_EXPR = re.compile( + r'\b((?:[a-zA-Z][a-zA-Z0-9]*)?(?:' + _ATTR_SUFFIX + r'))\s*=\s*\{([^}]{2,200})\}' +) +_EXPR_LITERAL = re.compile(r"['\"]([A-Z][^'\"]{1,80})['\"]") + +_OBJ_KEYS = ( + "name|title|slug|label|description|header|Header|tooltip" + "|placeholder|text|subtitle" +) +# Plain literal value: safe for find-and-replace. +OBJ = re.compile( + r"^\s*(" + _OBJ_KEYS + r")\s*:\s*['\"`]([^'\"`$]{2,})['\"`]\s*,?\s*$" +) +# Template literal carrying interpolation, e.g. `Allow ${AGENT_NAME} to ...`. +# Captured so a new setting is still detected, but marked wrapped=True so it can +# never become an unattended find-and-replace target. +OBJ_INTERP = re.compile( + r"^\s*(" + _OBJ_KEYS + r")\s*:\s*`([^`]*\$\{[^`]*)`\s*,?\s*$" +) + +# Text between tags on one line: List only visible runs +# Requiring a real closing `` would otherwise capture `Promise`). +JSX_INLINE = re.compile(r">\s*([A-Z][A-Za-z0-9 ,'’.\-?!:%()/]{1,80}?)\s* tuple[str, str, str]: + """Strict identity, for detecting reflow within a file. + + Prettier re-indentation preserves the expression form exactly, so kind + and key must participate: only an identical string in an identical + position is reflow. + """ + return (self.kind, self.key, self.norm) + + @property + def moved_ident(self) -> str: + """Loose identity, for detecting a string relocating across the commit. + + Deliberately just the string. A label can move from JSX text into a + prop -- the secret-drawer consolidation moved `Add secret` from + `Add secret` into `saveLabel="Add secret"`. The user still + sees it, so it is a move, not a removal. Keying on (kind, key) here + would report 23 phantom removals for that one commit. + """ + return self.norm + + +def normalize(raw: str) -> str: + """Collapse whitespace. Case is preserved deliberately -- see module docstring.""" + return re.sub(r"\s+", " ", raw).strip() + + +def path_is_ui(path: str, cfg: config.SourceRepo = config.SOURCE) -> bool: + if not any(path.startswith(root) for root in cfg.ui_roots): + return False + if not any(path.endswith(ext) for ext in cfg.ui_exts): + return False + return not any(fnmatch.fnmatch(path, pat) for pat in cfg.exclude_globs) + + +def _reject_ownline(text: str) -> bool: + # ` Avatar,` is an import specifier, not a label. + if text.endswith(","): + return True + # A single word with no space is overwhelmingly an identifier. + return " " not in text + + +def _reject_inline(text: str) -> bool: + if not _PASCAL_IDENT.match(text): + return False + # Keep `Save`, `Cancel`, `Delete`; drop `Promise`, `ReactNode`, `WBTable`. + return not _SIMPLE_WORD.match(text) + + +def _scan_line(line: str, sign: str, path: str, line_no: int, in_import: bool) -> Iterator[LabelDelta]: + body = line[1:] + + for m in ATTR.finditer(body): + key, raw = m.group(1), m.group(2) + if key in _AMBIGUOUS_ATTR_KEYS and _IDENTIFIER_VALUE.match(raw): + continue + yield LabelDelta(sign, path, "attr", key, raw, normalize(raw), line_no, False) + + for m in ATTR_EXPR.finditer(body): + key = m.group(1) + for lit in _EXPR_LITERAL.finditer(m.group(2)): + raw = lit.group(1) + yield LabelDelta(sign, path, "attr", key, raw, normalize(raw), line_no, True) + + m = OBJ.match(body) + if m: + raw = m.group(2) + yield LabelDelta(sign, path, "obj", m.group(1), raw, normalize(raw), line_no, False) + return + + m = OBJ_INTERP.match(body) + if m: + raw = m.group(2) + yield LabelDelta(sign, path, "obj", m.group(1), raw, normalize(raw), line_no, True) + return + + for m in JSX_INLINE.finditer(body): + raw = m.group(1) + if _reject_inline(raw): + continue + yield LabelDelta(sign, path, "jsx", "_", raw, normalize(raw), line_no, False) + + stripped = body.strip() + if not in_import and JSX_OWNLINE.match(stripped) and not _reject_ownline(stripped): + # Prettier reflowed this out of its element. The literal itself is + # complete, so it stays a valid replace target. + yield LabelDelta(sign, path, "jsx", "_", stripped, normalize(stripped), line_no, False) + + +def extract_deltas( + diff: str, cfg: config.SourceRepo = config.SOURCE +) -> list[LabelDelta]: + """Walk a unified diff and return every added/removed user-facing string.""" + out: list[LabelDelta] = [] + if not diff: + return out + + for path, body in _iter_file_diffs(diff): + if not path_is_ui(path, cfg): + continue + + old_ln = new_ln = 0 + in_import = False + + for line in body.splitlines(): + m = _HUNK.match(line) + if m: + old_ln, new_ln = int(m.group(1)), int(m.group(2)) + in_import = False + continue + if line.startswith("+++") or line.startswith("---"): + continue + + payload = line[1:] if line and line[0] in "+- " else line + if re.match(r"^\s*import\s", payload) and " from " not in payload: + in_import = True + elif in_import and ("from " in payload or payload.rstrip().endswith(";")): + in_import = False + + if line.startswith("+"): + out.extend(_scan_line(line, "+", path, new_ln, in_import)) + new_ln += 1 + elif line.startswith("-"): + out.extend(_scan_line(line, "-", path, old_ln, in_import)) + old_ln += 1 + else: + old_ln += 1 + new_ln += 1 + + return out + + +# --- Set arithmetic ------------------------------------------------------- +# Generalized from diff_signals.graphql_contract_change, which uses the same +# trick to decide whether a .graphql change is client-visible. + + +def file_has_net_change(deltas: Sequence[LabelDelta]) -> bool: + """Did this file's copy actually change? + + Prettier reflow shows up as `-aria-label="X"` / `+ aria-label="X"`: the same + string on both sides. Set-equality kills it deterministically, with no model + and no heuristic. This is the dominant false positive. + """ + added = sorted(d.ident for d in deltas if d.sign == "+") + removed = sorted(d.ident for d in deltas if d.sign == "-") + return added != removed + + +def commit_net_change( + deltas: Sequence[LabelDelta], +) -> tuple[list[LabelDelta], list[LabelDelta], list[LabelDelta]]: + """Split commit-wide deltas into (added, removed, moved). + + A string removed from file X and added in file Y cancels globally: the user + still sees it, it just lives somewhere else now. Without this, a drawer + consolidation that relocates 23 strings files 23 false "removed" findings. + """ + added = [d for d in deltas if d.sign == "+"] + removed = [d for d in deltas if d.sign == "-"] + both = {d.moved_ident for d in added} & {d.moved_ident for d in removed} + + moved = [d for d in added if d.moved_ident in both] + return ( + [d for d in added if d.moved_ident not in both], + [d for d in removed if d.moved_ident not in both], + moved, + ) + + +def surviving_deltas(deltas: Sequence[LabelDelta]) -> list[LabelDelta]: + """Drop files whose copy did not net-change, then return what remains.""" + by_path: dict[str, list[LabelDelta]] = {} + for d in deltas: + by_path.setdefault(d.path, []).append(d) + out: list[LabelDelta] = [] + for path_deltas in by_path.values(): + if file_has_net_change(path_deltas): + out.extend(path_deltas) + return out diff --git a/scripts/uidrift/finding.py b/scripts/uidrift/finding.py new file mode 100644 index 0000000000..5ccb4fab4e --- /dev/null +++ b/scripts/uidrift/finding.py @@ -0,0 +1,117 @@ +"""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 + +# 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 + + 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}|{self.surface}" + 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", "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", +) diff --git a/scripts/uidrift/tests/__init__.py b/scripts/uidrift/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/scripts/uidrift/tests/fixtures/9573f30.diff b/scripts/uidrift/tests/fixtures/9573f30.diff new file mode 100644 index 0000000000..9a287ae84c --- /dev/null +++ b/scripts/uidrift/tests/fixtures/9573f30.diff @@ -0,0 +1,2627 @@ +diff --git a/frontends/app/src/agents.md b/frontends/app/src/agents.md +index ecd12d76eda..48af2ffdf3c 100644 +--- a/frontends/app/src/agents.md ++++ b/frontends/app/src/agents.md +@@ -271,9 +271,9 @@ bun run generate:workspace-templates + + - **Redact sensitive data from Meticulous recordings.** We use [Meticulous](https://meticulous.ai) to record user sessions and run visual regression tests. When adding or modifying UI that displays API keys, secrets, tokens, or other sensitive data, you **must** redact it from recordings. There are two layers: + +- **DOM redaction** — Wrap any element that renders sensitive text with the `` component from `src/util/SensitiveInfoWrapper.tsx`. This applies the `meticulous-redact-recording` CSS class with `display: contents` so layout is unaffected: ++ **DOM redaction** — Wrap any element that renders sensitive text with the `` component from `@wandb/weave/components/SensitiveInfoWrapper`. This applies the `meticulous-redact-recording` CSS class with `display: contents` so layout is unaffected: + ```tsx +- import {SensitiveInfoWrapper} from '../util/SensitiveInfoWrapper'; ++ import {SensitiveInfoWrapper} from '@wandb/weave/components/SensitiveInfoWrapper'; + + + +diff --git a/frontends/app/src/components/ApiKeyViewer.tsx b/frontends/app/src/components/ApiKeyViewer.tsx +index 29a691d42c7..9e7ae41b7fb 100644 +--- a/frontends/app/src/components/ApiKeyViewer.tsx ++++ b/frontends/app/src/components/ApiKeyViewer.tsx +@@ -1,4 +1,5 @@ + import {Icon, IconButton} from '@wandb/components'; ++import {SensitiveInfoWrapper} from '@wandb/weave/components/SensitiveInfoWrapper'; + import React, {type FC, useMemo} from 'react'; + import {Button, Divider, Header} from 'semantic-ui-react'; + +@@ -7,7 +8,6 @@ import { + useGenerateApiKeyMutation, + useViewerApiKeysQuery, + } from '../generated/graphql'; +-import {SensitiveInfoWrapper} from '../util/SensitiveInfoWrapper'; + import CopyableText from './CopyableText'; + import {SemanticLoader as Loader} from './utility/InstrumentedLoader'; + +diff --git a/frontends/app/src/components/ApiKeyViewerWithSearch.tsx b/frontends/app/src/components/ApiKeyViewerWithSearch.tsx +index 5f5b7a8ec83..4dc472a89d0 100644 +--- a/frontends/app/src/components/ApiKeyViewerWithSearch.tsx ++++ b/frontends/app/src/components/ApiKeyViewerWithSearch.tsx +@@ -2,6 +2,7 @@ import {Banner, Button, DropdownMenu, IconButton} from '@wandb/components'; + import {fuzzyMatchWithMapping} from '@wandb/weave/common/util/fuzzyMatch'; + import {TextField} from '@wandb/weave/components/Form/TextField'; + import {Icon as WeaveIcon} from '@wandb/weave/components/Icon'; ++import {SensitiveInfoWrapper} from '@wandb/weave/components/SensitiveInfoWrapper'; + import {Tailwind} from '@wandb/weave/components/Tailwind'; + import _ from 'lodash'; + import type {FC} from 'react'; +@@ -16,7 +17,6 @@ import {DeleteApiKeyModal} from '../pages/Billing/AccountSettings/APIKeysTab/Del + import {EditApiKeyDrawer} from '../pages/Billing/AccountSettings/APIKeysTab/EditApiKeyDrawer'; + import {MAX_API_KEYS_PER_SCOPE} from '../util/constants'; + import {DateFormat, format} from '../util/date'; +-import {SensitiveInfoWrapper} from '../util/SensitiveInfoWrapper'; + import {CreateApiKeyDrawer} from './CreateApiKeyDrawer'; + import {SemanticLoader as Loader} from './utility/InstrumentedLoader'; + +diff --git a/frontends/app/src/components/CreateApiKeyDrawer.tsx b/frontends/app/src/components/CreateApiKeyDrawer.tsx +index 6ae908108be..7ea4df507fb 100644 +--- a/frontends/app/src/components/CreateApiKeyDrawer.tsx ++++ b/frontends/app/src/components/CreateApiKeyDrawer.tsx +@@ -2,6 +2,7 @@ import {Drawer} from '@mui/material'; + import {Button} from '@wandb/components'; + import {TextField} from '@wandb/weave/components/Form/TextField'; + import {Icon} from '@wandb/weave/components/Icon'; ++import {SensitiveInfoWrapper} from '@wandb/weave/components/SensitiveInfoWrapper'; + import {Tailwind} from '@wandb/weave/components/Tailwind'; + import type {FC} from 'react'; + import React, {useState} from 'react'; +@@ -16,7 +17,6 @@ import type {GenerateApiKeyMutationData} from '../graphql/users'; + import {useHandleScroll} from '../pages/HomePage/HomePageSidebar/useHandleScroll'; + import {DateFormat, format} from '../util/date'; + import {extractErrorMessageFromApolloError} from '../util/errors'; +-import {SensitiveInfoWrapper} from '../util/SensitiveInfoWrapper'; + import {Banner, BannerFlexWrapper, BannerTextWrapper} from './Banner'; + import {ResizableDrawer} from './common/ResizableDrawer/ResizableDrawer'; + +diff --git a/frontends/app/src/components/Launch/JobDrawer/JobDrawerContent.tsx b/frontends/app/src/components/Launch/JobDrawer/JobDrawerContent.tsx +index 7f7a0a467da..eee64bfb765 100644 +--- a/frontends/app/src/components/Launch/JobDrawer/JobDrawerContent.tsx ++++ b/frontends/app/src/components/Launch/JobDrawer/JobDrawerContent.tsx +@@ -61,7 +61,7 @@ const getViewConfig = (view: JobDrawerView): ViewConfig => { + }; + case JobDrawerView.Secrets: + return { +- title: '', ++ title: 'Add secret', + showBackButton: true, + }; + } +diff --git a/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.test.tsx b/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.test.tsx +new file mode 100644 +index 00000000000..8f656e49a0d +--- /dev/null ++++ b/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.test.tsx +@@ -0,0 +1,113 @@ ++import {render, screen, waitFor} from '@testing-library/react'; ++import userEvent from '@testing-library/user-event'; ++import React from 'react'; ++import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; ++ ++import {type Secret} from '../../../generated/graphql'; ++import { ++ JobDrawerSecretsContext, ++ type JobDrawerSecretsContextType, ++} from './JobDrawerSecretsContext'; ++import {JobDrawerSecretsSubview} from './JobDrawerSecretsSubview'; ++ ++const mockUpsertTeamSecret = vi.hoisted(() => vi.fn()); ++ ++vi.mock('../../../generated/graphql', () => ({ ++ useUpsertTeamSecretMutation: () => [mockUpsertTeamSecret, {loading: false}], ++})); ++ ++const makeSecret = (name: string): Secret => ({name}) as Secret; ++ ++const renderSubview = ({ ++ contextOverrides, ++ onClose = vi.fn(), ++}: { ++ contextOverrides?: Partial; ++ onClose?: () => void; ++} = {}) => { ++ const contextValue: JobDrawerSecretsContextType = { ++ secretsData: [], ++ secretsLoading: false, ++ secretsError: false, ++ refetchSecrets: vi.fn(), ++ ...contextOverrides, ++ }; ++ ++ render( ++ ++ ++ ++ ); ++ ++ return {contextValue, onClose}; ++}; ++ ++const fillValidSecretForm = async () => { ++ const user = userEvent.setup(); ++ ++ await user.type( ++ screen.getByPlaceholderText('Ex. OPENAI_API_KEY'), ++ 'OPENAI_API_KEY' ++ ); ++ await user.type( ++ screen.getByPlaceholderText('Paste your secret value'), ++ 'secret-value' ++ ); ++ ++ return user; ++}; ++ ++describe('JobDrawerSecretsSubview', () => { ++ beforeEach(() => { ++ mockUpsertTeamSecret.mockResolvedValue({ ++ data: {upsertTeamSecret: {secret: {name: 'OPENAI_API_KEY'}}}, ++ }); ++ }); ++ ++ afterEach(() => { ++ vi.clearAllMocks(); ++ }); ++ ++ it('warns on duplicate team secret names case-insensitively and blocks creation', async () => { ++ renderSubview({ ++ contextOverrides: {secretsData: [makeSecret('openai_api_key')]}, ++ }); ++ ++ await fillValidSecretForm(); ++ ++ expect( ++ screen.getByText( ++ 'A secret with this name already exists. Choose a different name or edit the existing secret.' ++ ) ++ ).toBeInTheDocument(); ++ expect(screen.getByRole('button', {name: 'Add secret'})).toBeDisabled(); ++ expect(mockUpsertTeamSecret).not.toHaveBeenCalled(); ++ }); ++ ++ it('creates a team secret when the name is valid and not duplicated', async () => { ++ const refetchSecrets = vi.fn(); ++ const onClose = vi.fn(); ++ renderSubview({ ++ contextOverrides: { ++ secretsData: [makeSecret('OTHER_SECRET')], ++ refetchSecrets, ++ }, ++ onClose, ++ }); ++ ++ const user = await fillValidSecretForm(); ++ await user.click(screen.getByRole('button', {name: 'Add secret'})); ++ ++ await waitFor(() => ++ expect(mockUpsertTeamSecret).toHaveBeenCalledWith({ ++ variables: { ++ entityName: 'test-team', ++ secretName: 'OPENAI_API_KEY', ++ secretValue: 'secret-value', ++ }, ++ }) ++ ); ++ expect(refetchSecrets).toHaveBeenCalled(); ++ expect(onClose).toHaveBeenCalled(); ++ }); ++}); +diff --git a/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.tsx b/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.tsx +index f23a2e79b12..9fd16093c14 100644 +--- a/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.tsx ++++ b/frontends/app/src/components/Launch/JobDrawer/JobDrawerSecretsSubview.tsx +@@ -1,23 +1,12 @@ +-import {Button} from '@wandb/components'; +-import {Checkbox} from '@wandb/weave/components/Checkbox'; +-import {TextArea} from '@wandb/weave/components/Form/TextArea'; +-import {TextField} from '@wandb/weave/components/Form/TextField'; +-import {TailwindContents} from '@wandb/weave/components/Tailwind'; +-import React, {useContext, useState} from 'react'; ++import {Alert} from '@wandb/weave/components/Alert'; ++import {CreateSecretFormContent} from '@wandb/weave/components/Secrets'; ++import React, {useContext} from 'react'; + import {toast} from 'react-toastify'; + +-import {useInsertSecretMutation} from '../../../generated/graphql'; ++import {useUpsertTeamSecretMutation} from '../../../generated/graphql'; + import {extractErrorMessageFromApolloError} from '../../../util/errors'; +-import {SensitiveInfoWrapper} from '../../../util/SensitiveInfoWrapper'; +-import * as TeamSettingsStyles from '../../TeamSettings/TeamSettingsSecrets.styles'; + import {JobDrawerSecretsContext} from './JobDrawerSecretsContext'; + +-const validateSecretName = (name: string) => { +- // Any alphanumeric+underscore string starting with an alphabet character or underscore +- const re = /^[a-zA-Z_][a-zA-Z0-9_]*$/; +- return re.test(String(name)); +-}; +- + export const JobDrawerSecretsSubview = ({ + entityName, + onClose, +@@ -25,115 +14,46 @@ export const JobDrawerSecretsSubview = ({ + entityName: string; + onClose?: () => void; + }) => { +- const [secretName, setSecretName] = useState(''); +- const [secretValue, setSecretValue] = useState(''); +- const [showSecret, setShowSecret] = useState(false); +- +- const {refetchSecrets} = useContext(JobDrawerSecretsContext); ++ const {refetchSecrets, secretsData} = useContext(JobDrawerSecretsContext); + +- // Prevent initial input error when the secret name is empty +- const [isSecretNameModified, setIsSecretNameModified] = useState(false); ++ const [upsertTeamSecret, {loading: isSaving}] = useUpsertTeamSecretMutation(); + +- const [insertSecret] = useInsertSecretMutation(); +- +- const onClickAddSecret = async () => { ++ const handleSave = async (input: { ++ secretName: string; ++ secretValue: string; ++ }) => { + try { +- await insertSecret({ +- variables: {entityName, secretName, secretValue}, ++ await upsertTeamSecret({ ++ variables: { ++ entityName, ++ secretName: input.secretName, ++ secretValue: input.secretValue, ++ }, + }); +- toast.success( +- +- Added secret {secretName} +- +- ); ++ toast.success(`Added secret ${input.secretName}`); + refetchSecrets(); + onClose?.(); + } catch (err) { + const errMsg = extractErrorMessageFromApolloError(err); + console.error(errMsg); +- toast.error(`Problem adding secret ${secretName}: ${errMsg}`); ++ toast.error(`Problem adding secret ${input.secretName}: ${errMsg}`); + } + }; + +- const inputError = isSecretNameModified && !validateSecretName(secretName); +- const canAddSecret = +- !inputError && secretValue.length > 0 && secretName.length > 0; +- + return ( +- +-
+-
+-
+- Add secret +-
+-
+-
+- This secret will be created for {entityName}, +- owner of the destination project for your launch. Team secrets can +- be managed in team settings by team admins. +-
+-
+-
+- Secret name +-
+-
+- { +- setSecretName(e); +- setIsSecretNameModified(true); +- }} +- /> +- {inputError && ( +-
+- Secret names can only contain alphanumeric characters ([a-z], +- [A-Z], [0-9]) or underscores (_) and start with a letter ([a-z], +- [A-Z]) or underscores (_). +-
+- )} +-
+-
Secret
+-
+- +-