Skip to content

test(console): pin the read-only draft to callers who can change neither grant - #3616

Merged
freeqaz merged 1 commit into
mainfrom
ms1025e-config
Oct 25, 2026
Merged

freeqaz merged 1 commit into
mainfrom
ms1025e-config

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Problem

A mutation sweep of the read-only draft from #3600 (getAgentCallerPermissions configurationUpdate, and the console's configurationReadOnly / draftReadOnly gating in detail.mts and credentials.mts) left four changes that no test caught:

  • draftReadOnly without its Agent-update half: an Agent editor without Configuration update gets the reader's draft notice.
  • draftReadOnly with || in place of &&: a Configuration editor without Agent update gets the reader's draft notice.
  • A caller who can deploy but change nothing loses the draft checklist. Only a caller who also cannot deploy should lose it.
  • Deploy's status drops the "Your access does not include changing this version draft." suffix for that caller.

The rest was killed. That includes the server mutants: configurationUpdate probing the Agent's update, Configuration read, the Namespace-wide Configuration, or OR-ing in Agent update.

Change

Test only, in tests/browser/console-agent-detail.test.mjs:

  • The Agent-only editor test and the permission-reread test (before the revocation) check that the draft notice still addresses an editor.
  • New test: a member with Agent read and deploy, but no update grants, opens a draft whose Slack tokens are unbound. The page shows the read-only draft notice and keeps the "Prepare this version" checklist. Deploy's status ends with the suffix.

Verification

  • Each new assertion fails against its mutant and passes on main. The survivors were rechecked against eight related console browser files, and none of those files caught them.
  • The touched tests pass 20/20 at 2 CPUs on a remote test VM in a private network namespace. prettier and eslint pass.

Not pursued: a redraw that only matters when a permission reread lands between the rebuilt tab's draw and the end of the refresh, and a native editor replaced while open, which needs a revocation during editing followed by a save refusal. Neither can be reached without an ordering hook.

Risks

Test only. It adds one browser test (about 3 s). #3611 also edits this file. Its hunks are in other lines, so the two PRs should merge cleanly in either order; whichever lands second should rerun this file.

…her grant

The draft reads as a reader's only when the caller has neither Agent update nor
Configuration update. Dropping either half survived, and so did hiding a deployer's
checklist or dropping Deploy's "cannot change this version draft" suffix: no test
covered an editor with one grant's notice or a deployer without update.
@freeqaz
freeqaz requested a review from a team as a code owner October 25, 2026 10:49
@clawsweeper

clawsweeper Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 25, 2026
@clawsweeper

clawsweeper Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR adds browser assertions for draft notices, preparation checklists, and deployment guidance under separate Agent, Configuration, and deploy grants.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Focused, correct test additions with passing browser CI; evidence consists of application-harness coverage and reported mutation checks.
Proof confidence 🦐 gold shrimp (3/6) This maintainer-authored, test-only PR changes no shipped behavior; inspected linked screenshots demonstrate earlier Channel Secrets changes rather than the new draft assertions.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: Future permission-gating regressions could describe editors as readers or remove useful preparation guidance from deployers.
Reason: The maintainer-authored change protects explicit existing behavior across three meaningful permission combinations.

Merge readiness

✅ Ready for maintainer review

Keep open: this PR adds useful, distinct browser coverage for existing permission behavior, with no actionable defect found.

Priority: P3
Reviewed head: 767ec6fbeda34afcee2db6b8c66cd4fbc8a01d84

Before merge

None.

Findings

None.

Agent review details

How this fits together

The Console reads advisory permission answers from the controller’s IAM-backed endpoint and uses them to render draft controls; these tests exercise that path through Chromium and the real Fastify application with fixture storage.

flowchart LR
  Test[Browser test] --> Browser[Chromium Console]
  Browser --> API[Fastify permission endpoint]
  API --> IAM[Native IAM Driver]
  IAM --> Grants[Fixture role grants]
  API --> Draft[Draft notices and checklist]
  Draft --> Assertions[Visible behavior assertions]
Loading

Technical review

Best possible solution:

Retain the focused browser coverage alongside the existing independent-grant implementation.

Do we have a high-confidence way to reproduce the issue?

This test-only PR reports mutation survivors rather than a current product failure; source supports the scenarios, while the claimed mutant failures were not independently executed.

Is this the best way to solve the issue?

Extending two existing browser cases and adding the missing deploy-only combination provides targeted coverage without duplicating application logic or introducing test-only production hooks.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 097d75696e99.

Provenance checked

  • Independent-grant draft presentation keeps the original intent (aaabca1: Distinguish readers from callers who can edit either resource, while preserving preparation guidance for callers who can deploy.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Review metrics

Metric Value Why it matters
Change scope Production +0; browser tests +67 One new permission combination and two extensions cover distinct reported mutation survivors without adding production seams.

Labels

Label changes:

  • add P3: This is preventive test coverage for behavior already implemented on main.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P3: This is preventive test coverage for behavior already implemented on main.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 25, 2026, 7:03 AM ET / 11:03 UTC.

@freeqaz
freeqaz merged commit 47a8c6e into main Oct 25, 2026
56 checks passed
@freeqaz
freeqaz deleted the ms1025e-config branch October 25, 2026 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant