feat: change detection and re-review for approved MCP servers - #5198
Open
daviddanialy wants to merge 21 commits into
Open
feat: change detection and re-review for approved MCP servers#5198daviddanialy wants to merge 21 commits into
daviddanialy wants to merge 21 commits into
Conversation
A daily Temporal sweep re-gathers evidence for approved approval requests and compares the permission-relevant slice — OAuth scopes, authority mode, demanded credentials, published advisories — against the snapshot the approval rested on. Drift sets a changed-since-approval flag (cleared only by recording a new decision), announces once per distinct change through the audit-log webhook channel, and surfaces as a diff banner on the review page and a badge on the inventory. Tool declarations, versions, and traffic are deliberately excluded from the fingerprint: they move constantly without widening what the server may do. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
🦋 Changeset detectedLatest commit: 1bdb18f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Warning-token wash, thick left accent edge, triangle-alert icon, and an uppercase warning-colored title — the drift banner is the page's reason to exist when it fires, and the hairline border read as decoration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
cubic analysis
All reported issues were addressed
Linked issue analysis
Linked issue: AIS-477: feat: change detection and re-review
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Compute a permission-relevant fingerprint/diff (OAuth scopes, authority mode, demanded credentials, published advisories) and ignore non-permission churn | New evidencediff package implements narrow comparison logic and ignores churn; SDK/model additions expose EvidenceDiff. |
| ✅ | Daily scheduled re-check that re-gathers evidence and compares snapshots (Temporal sweep, skip-overlap) | A scheduled workflow and worker wiring were added for a daily sweep that pages approved requests and runs the recheck. |
| ✅ | Set 'changed-since-approval' flag when drift detected and clear it only when recording a new decision | Database field, repo methods and clear-on-decision helper were added, plus a test asserting record-decision clears the flag. |
| ✅ | Announce drift via audit action that flows to org webhooks (no new parallel delivery path) | New audit action and log event were added and the client/frontend action list updated; activities emit audit entries on detection. |
| ✅ | Surface changed state in the UI: banner with diff on the review page and badge in inventory | Frontend components added the evidence-diff banner on the review page and the inventory badge, and API now exposes evidenceDiff/evidenceChangedAt to the UI. |
| ✅ | Compute the detail diff on read from the decision snapshot and current gather | The server computes EvidenceDiff on read (so the page shows the diff even before sweep flags it). |
| ✅ | Provide test coverage for drift detection semantics (unit/integration checks: drift detection, non-permission churn ignored, advisory rotation, repo/flag lifecycle) | Unit tests for evidencediff and evidence-flag lifecycle were added; CI-level suite claim is noted in changeset and tests are present in repo. |
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
- Derive section gathered-ness from the document's Gaps rather than from the section being nil. A server that publishes no OAuth metadata gathers a nil authority section with no gap, so nil-means-unknown made dropping OAuth entirely — the largest widening of a standing approval there is — the one change the diff could not see. - Load both comparison sides when a recheck runs instead of at scan time. The scan now returns identity only, so a page of evidence documents no longer crosses the Temporal boundary twice (a 500-tool server's snapshot measures ~147KB, and 50 of them exceed the payload limit), and a decision recorded mid-sweep is seen rather than compared against. - Make the flag write the sole arbiter of whether a drift is news: it declines a matching fingerprint, an unapproved request, or a request decided since the compared snapshot, and the announcement follows only a written row. That makes an activity retry a no-op instead of a second webhook, and stops a stale sweep permanently resurrecting a flag an admin just cleared. - Key the announce-once fingerprint on the diff rather than the gather, so a source flapping between reachable and gapped cannot re-announce byte-identical news. - Refuse to store a gather that failed on every remote source over a document that did better, matching the manual refresh path. - Carry the audit actor on the event struct instead of hardcoding it in the logger, and reuse the package's UTC time helper on both inventory paths. - Move the drift banner into its own component with a shared chip, and cover the sweep workflow and the flag write with tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
- Require a present advisories section as well as no lookup gap before comparing: a vulnerability database that does not cover a registry leaves the section absent with no gap, and reading that as a clean zero would announce a phantom drop in advisory count. Authority keeps the opposite rule, since there a clean probe legitimately returns nothing. - Stop flagging when a concurrent refresh wins the evidence CAS. The discarded gather is not what the request holds, so announcing on it would describe a drift the page cannot show and stamp the winner's evidence with someone else's fingerprint. - Continue as new before the run timeout rather than dying mid-page: the next scheduled run restarts at the beginning of the id space, so a sweep that never finishes would re-check the same prefix forever and never reach the tail. - Render authority modes in the banner with the words the evidence panel uses, so a server that publishes nothing reads as Undeclared instead of an empty chip, and announce the banner to assistive technology. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two decisions sharing a decided_at would otherwise resolve arbitrarily, so a recheck could compare against the older frozen snapshot on one sweep and the newer one on the next. Applies to both pickers — the recheck's load and the decision history whose head the read-path diff uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The repo's UI anti-pattern check rejects a thick one-sided accent edge. The banner keeps its urgency from the warning-toned border, the tinted wash, the alert icon, and the uppercase title. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The recheck picks its comparison decision by (decided_at, id) but the flag write asked only whether a decision existed with a later decided_at. In a decided_at tie the decision that wins by id was invisible to the guard, so a sweep could re-stamp evidence_changed_at against the losing decision's snapshot and resurrect a flag the winner had just cleared. Only a new decision clears the flag, so that leaves a permanent false re-review banner. Return the decision id alongside its timestamp and make the guard a row comparison over the same pair the read side orders by. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The review ran to two and a half screens for an ordinary dossier, most of it chrome: a labelled card for a status the badge already stated, a stack of bordered cards per requester, and six full-width question groups that each used half their width. The status moves onto the section heading, requesters and prior decisions become framed lists sharing a row, and the evidence questions lay out in two columns. Fact lists switch from viewport breakpoints to container queries so a group picks its column count from the width it got — which is also why the group with the most facts keeps the full width, where its list is half the rows. Observed traffic moves in from its own trailing section as "Who is currently using it?", beside the tools the server declares. It renders even when the review itself fails to load, since what the server is doing now is exactly what an admin still wants then. The tool list takes the height that group settles on and scrolls past it; a fixed preview count would be a guess about the neighbour. Measured on a fully gathered remote server: 2420px to 1731px. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
A declared tool list is whatever an untrusted MCP server answered tools/list with, so its length is not ours to bound. Rendering every row in flow makes first paint proportional to that length: measured in Chromium, 5000 rows cost 79ms of layout, against 3ms for a realistic 200. content-visibility: auto makes the cost proportional to what is on screen instead — the same 5000 rows drop to 13ms — without hiding anything from the person reviewing the server. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The block link filed the request as the page loaded, and the note that reached the review was the policy's block reason — the same sentence for everyone that policy stopped. The approval page's "who asked, and why" therefore showed the same boilerplate quoted once per requester, which is the opposite of what it exists for. The request page now redeems into a short form: what do you need this for, sent with the token. createPolicyBypassRequest takes an optional note and prefers it over the block reason, on both the approval intake and the legacy bypass row. A client that sends none keeps the old behaviour rather than recording silence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The field had no accessible name, and nothing stopped a note longer than the 4,000 characters the endpoint accepts — the server refused it with a generic failure after the requester had already written it. A visible label now names the field, and the length is checked in runes, the way Goa counts a string's MaxLength, so an emoji costs the same on both sides. Over the limit the page says by how much and holds the send rather than truncating: the note is the one thing here the requester cannot get back from the link. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
Screen.Recording.2026-08-12.at.1.53.00.PM.mov |
Contributor
Author
|
Contributor
Author
|
daviddanialy
force-pushed
the
daviddanialy/ais-477-feat-change-detection
branch
from
August 12, 2026 21:14
ce9acb5 to
7f4e10f
Compare
…daviddanialy/ais-477-feat-change-detection
…daviddanialy/ais-477-feat-change-detection
…daviddanialy/ais-477-feat-change-detection
…daviddanialy/ais-477-feat-change-detection # Conflicts: # server/internal/mcpapproval/repo/queries.sql.go
daviddanialy
force-pushed
the
daviddanialy/ais-477-feat-change-detection
branch
from
August 12, 2026 21:48
263bbe0 to
683a35a
Compare
…daviddanialy/ais-477-feat-change-detection
…daviddanialy/ais-477-feat-change-detection
…daviddanialy/ais-477-feat-change-detection
…daviddanialy/ais-477-feat-change-detection
…daviddanialy/ais-477-feat-change-detection
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements AIS-477: an approval is a point-in-time judgement about a moving target; this notices when the approved thing has changed and puts it back in front of the admin.
Stacked on #5196 (mig) → #5158 → #5147. Merges last.
What counts as "changed" — permission surface only
The fingerprint/diff (
mcpapproval/evidencediff) compares the permission-relevant slice of two evidence documents: OAuth scopes, authority mode, dynamic client registration, demanded credentials, and published advisories (additions only — the stored advisory sample is a most-recent window, so an id leaving it is rotation, not withdrawal). Deliberately excluded: tool declarations, package versions, repository stats, traffic, maintainer counts — they churn constantly without widening what the server may do, and every false flag trains admins to ignore the banner. A section is compared only when both gathers consulted its source, so a flaky registry or failed OAuth probe reads as not-consulted rather than as every-scope-removed.Schedule
Daily Temporal sweep (
McpApprovalRecheckWorkflow, interval schedule, skip-overlap): pages approved requests with their latest decision snapshot, re-gathers through the same evidence assembler the intake path uses, stores via the existing CAS write (a concurrent manual refresh wins), compares, and on new drift sets the flag + emits the audit event in one transaction. Rechecks run sequentially — kinder to the third-party sources than a parallel burst, with hours of headroom at daily cadence. Feature-gated per org at recheck time. A failed recheck logs and moves on; it runs again tomorrow.Flag lifecycle
evidence_changed_atset on first detection, cleared only by recording a new decision (which freezes a fresh snapshot — the natural acknowledgment). A reverted gather or quieter sweep never un-flags what an admin hasn't looked at.notified_change_fingerprintmakes announcement once-per-distinct-drift: the same drift is never re-announced daily, a materially new drift is.Notification
New audit action
mcp_approval_request:evidence_changed(system actor, compact diff summary as event metadata) — flows to org webhooks through the existing audit→Svix relay, zero new delivery code. Added to the frontend audit-actions parity list.Surfacing
Verification
evidencediffunit tests: drift detection, non-permission churn ignored, ungathered sections skipped, advisory sample rotation.🤖 Generated with Claude Code
Summary by cubic
Detects permission‑surface drift for approved MCP servers and triggers re‑review (AIS‑477). Previously approvals stayed valid as servers changed; now a daily Temporal sweep re‑gathers evidence, compares it to the last approved snapshot, flags “Changed since approval,” emits an audit event, and surfaces a diff banner and an inventory badge.
evidence_diff; summaries and inventory addevidence_changed_at; new audit actionmcp_approval_request:evidence_changed. The diff is computed on read. The review shows an accessible warning banner and a denser two‑column layout; observed usage appears beside declared tools.risk.createPolicyBypassRequestaccepts optionalnote(up to 4,000 runes). The request page prompts for justification and validates length; the server prefers this note over the policy’s block reason. Older clients still fall back.Rollout
--github-evidence-token(GRAM_GITHUB_EVIDENCE_TOKEN) for evidence repository lookups.noteviarisk.createPolicyBypassRequest.Written for commit 1bdb18f. Summary will update on new commits.