Skip to content

fix(console): skip the native admin probe for a caller without agent administer - #3622

Merged
freeqaz merged 8 commits into
mainfrom
fix-native-admin-probe
Oct 25, 2026
Merged

freeqaz merged 8 commits into
mainfrom
fix-native-admin-probe

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The Agent page asks GET /namespaces/{namespaceId}/agents/{agentId}/native-admin for every caller. The endpoint requires Agent administer. So every viewer without it gets a browser console error and an audited openclaw.agents.native_admin.read denial for each Agent, once per tab. This was nit 6 in the internal review of #3600, and it stayed open.

Change

  • GET .../agents/{agentId}/permissions also answers administer. This is the Agent administer decision that the native admin endpoints make on the exact Agent (getAdministerableAgent). Like the other bits, it uses the same callerMay path, writes no audit row and has no side effect. It is additive. Contracts, OpenAPI and docs/reference/api.md are regenerated.
  • Disclosure: the answer needs Agent read (route authorization). Any caller with Agent read can already learn the same decision from a GET to /native-admin, which costs them a 403 and costs the Installation an audit row. So the bit reveals nothing new. Native admin UI also needs a signed-in console session. That condition is listed in the field description, not folded into the bit, because the bit reports only the IAM action, like the others.
  • The native admin card waits for the caller's latest permission answer before it probes. If administer is false, the card does not ask and stays hidden, as it does after a denial today. If the read fails or the bit is missing, the card asks as before and still remembers the denial once per tab.
  • The exact-shape fixtures and assertions (Storybook, browser and integration tests) now include the field, including the credentialsSourcesOperateDenied scenario that fix(console): disable agent-side saves when an agent update would be refused for a bound source #3618 added.
  • docs/flows/agent-native-admin.md: the flow and its console step now show that the console reads native admin status only when the administer answer allows it or is unknown. To stay under the 2,500-word limit, two repeated sentences and one Debugging bullet that restated step 7 were shortened.

Verification

  • tests/integration/console-api.test.mjs, new test: a member with every other Agent action (read, update, deploy, operate, delete) gets administer: false, and the answer writes no audit row. The real /native-admin GET is then refused with 403 and audited. After an administer grant, the answer is true and the GET returns 200. Mutants killed: each other action in place of administer (read, update, deploy, operate, delete), and a Namespace or Configuration target.
  • tests/browser/console-agent-detail-2.test.mjs, new test: a viewer gets administer: false. Across two views of the page the card makes no /native-admin request and no denial is audited. With the console change reverted (the server bit kept), the test fails: 1 request. The existing once-per-tab test now runs with the permission answer withheld (a failed read), so the fallback path keeps its coverage. Both tests passed 20/20 at 2 CPUs on a remote test VM.
  • console-agent-detail.test.mjs, console-agent-detail-2.test.mjs and console-agent-overview.test.mjs all pass (191 tests). tsc --build, eslint and prettier pass on the touched files.
  • After merging main (fix(console): disable agent-side saves when an agent update would be refused for a bound source #3618 and fix(console): disable create secret for a member the api would refuse #3620 landed), the generated docs/reference/api.md and occ-api.openapi.json were regenerated with node scripts/generate-occ-openapi.mjs; --check is clean. On the merged head, tests/integration/console-api.test.mjs passes locally (13 tests), and console-agent-detail.test.mjs, console-agent-detail-2.test.mjs and console-agent-logs.test.ts pass on a remote test VM (177 tests). tsc passes.
  • No visible UI change. The card was already hidden for these callers. What changes is the missing console error and the missing audit row, so there is no before/after media.

Risks

  • For callers with administer, the native admin status read now starts after the permission answer instead of in parallel with it. The first reveal waits for both, so the page can show one round trip later. It still has the existing 2 s reveal cap.
  • A grant that arrives while the page is open is applied on the next page load, as the remembered 403 does today.

…administer

The Agent page asked GET .../native-admin for every caller. A caller
without Agent administer got a browser console error and an audited
denial for each Agent, once per tab (review of #3600, nit 6).

GET agents/{id}/permissions now also answers administer: the Agent
administer decision the native admin endpoint makes on the exact
Agent, with no audit row or side effect. The native admin card waits
for that answer and does not ask when it is false. A failed read keeps
today's probe and its once-per-tab denial memory.
@freeqaz
freeqaz requested a review from a team as a code owner October 25, 2026 11:16
@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-repro Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed clawsweeper:intake:bug 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 changes before merge.

What this changes

This PR adds an advisory Agent administer permission and skips the native-admin probe when that permission is denied.

Example: A Namespace member can read an Agent but cannot administer it.

  • Before: Opening the Agent page requests native-admin status, receives 403, and records an authorization denial.
  • After: The permission response reports administer:false, so the card stays hidden without requesting native-admin status.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The implementation is focused and has meaningful integration coverage, but applicable Console proof is absent and the owning runtime flow is stale.
Proof confidence 🧂 unranked krab (1/6) Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🐚 platinum hermit (4/6) 1 actionable review finding remain.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Agent readers generate an avoidable browser error and authorization-denial audit event while opening a page they may read.
Reason: The maintainer-authored direction removes unnecessary failed requests through the established advisory permission API.

Merge readiness

⛔ Needs changes before merge - 1 item remains

Keep open: this maintainer-authored PR addresses unnecessary authorization denials with a focused change; its runtime flow documentation needs updating.

Priority: P2
Reviewed head: 253d731a444736a0fb219f677b8af4d657281b1f

Before merge

  • Update the native-admin flow for permission-gated probing (P3) - When administer:false is returned, this branch now skips the status request entirely, but docs/flows/agent-native-admin.md still shows Agent detail immediately issuing that GET and describes the unconditional read in its execution trace. AGENTS.md and local-dev require the owning flow to change with consequential asynchronous handling. Update the existing diagram and trace to describe waiting for the advisory answer, skipping false, and probing when the answer is unknown; retain the independent server authorization boundary.

Findings

  • [P3] Update the native-admin flow for permission-gated probing — apps/controller/src/console/agents/native-admin.mts:133-135

Tests

  • Missing end-to-end proof: Repository-required Console captures are missing for the final native-admin flow; the body reports a failing-base/passing-head regression test, which was not independently executed during this read-only review.
Agent review details

How this fits together

The Console reads advisory permissions from the controller's selected IAM Driver before requesting native-admin availability; actual native-admin requests retain independent server authorization.

flowchart TD
  Console[Agent detail Console] --> Permissions[Caller permissions endpoint]
  Permissions --> IAM[Selected IAM Driver]
  IAM --> Hint[Advisory administer answer]
  Hint --> Gate[Native admin card]
  Gate --> Native[Authorized native-admin endpoint]
Loading

Technical review

Best possible solution:

Keep the existing advisory permission owner and server authorization, and document the new permission-gated probe in the native-admin flow.

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

Current main unconditionally probes native-admin status, whose real server path requires exact-Agent administer; a reader without that permission therefore reaches the reported 403 path.

Is this the best way to solve the issue?

The patch uses the existing permission-summary owner and preserves unknown-answer fallback, remembered denials, refresh coalescing, and server authorization; no introduced runtime defect was established.

Full review comments:

  • [P3] Update the native-admin flow for permission-gated probing — apps/controller/src/console/agents/native-admin.mts:133-135
    When administer:false is returned, this branch now skips the status request entirely, but docs/flows/agent-native-admin.md still shows Agent detail immediately issuing that GET and describes the unconditional read in its execution trace. AGENTS.md and local-dev require the owning flow to change with consequential asynchronous handling. Update the existing diagram and trace to describe waiting for the advisory answer, skipping false, and probing when the answer is unknown; retain the independent server authorization boundary.
    Confidence: 0.98

Overall correctness: patch is correct
Overall confidence: 0.92

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 9892abfb1471.

Provenance checked

  • Unconditional native-admin probing changes intended behavior with a stated reason (feat: add trusted-operator Agent native admin UI pilot #256: The pilot obtains availability through a protected exact-Agent administer endpoint; the PR explains replacing avoidable denied probes with an equivalent advisory decision.)
  • Permission-summary semantics keeps the original intent (be5deb2: Permission summaries inform Console presentation without authorizing subsequent operations, and unavailable hints preserve the normal request path.)
  • Native-admin refresh and denial memory keeps the original intent (fix(console): refresh native admin access when the active version changes #938: Availability refresh follows selected-version or runtime changes, coalesces concurrent refreshes, and retains remembered-denial suppression.)
  • Retained member Logs view keeps the original intent (c21f047: Expected permission refusals must not force a member's retained Logs view to rebuild; changing the expected native-admin read count preserves that behavior.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

Acceptance criteria:

  • python3 .agents/skills/local-dev/scripts/validate_flow_doc.py --kind flow-doc --doc docs/flows/agent-native-admin.md
  • pnpm docs:check
  • pnpm docs:check-length
  • pnpm format:check
  • git diff --check

What I checked:

  • Pinned introduced changes: The introduced diff adds the permission field, gates the existing card, updates generated contracts and fixtures, and adds API and browser coverage. (253d731a4447)
  • Exact-resource advisory decision: The existing callerMay owner now evaluates administer on the exact Agent alongside the other advisory actions; it neither grants authority nor bypasses the native-admin endpoint. (apps/controller/src/index.ts:2410, 253d731a4447)
  • Current main still probes unconditionally: Fetched main calls renderNativeAdminAccess without a permission hint, and its load function requests status after checking remembered denials. (apps/controller/src/console/agents/native-admin.mts:130, 9892abfb1471)
  • Meaningful regression coverage: The API test distinguishes administer from every other Agent action and checks audited refusal followed by an authorized response; browser coverage checks zero probes and retains the unknown-permission fallback. (tests/integration/console-api.test.mjs:831, 253d731a4447)
  • Stale runtime flow: The flow diagram still connects opening Agent detail directly to the native-admin GET, and its execution trace describes an unconditional read rather than the new advisory gate. (docs/flows/agent-native-admin.md:27, 253d731a4447)
  • Accepted latency tradeoff: The maintainer-authored PR explicitly accepts starting the status read after the permission response and retaining the existing reveal cap; this needs no additional product approval. (253d731a4447)

Likely related people:

  • Free Wortley: Raw commit 1065fce adds apps/controller/src/index.ts:2409 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 1065fce57eab; files: apps/controller/src/index.ts)
  • Anmol: Raw commit 14d9489 adds apps/controller/src/console/agents/native-admin.mts:96 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 14d94894b42b; files: apps/controller/src/console/agents/native-admin.mts)

Labels

Label changes:

  • add P2: The repair removes recurring browser errors and audit noise for ordinary Agent readers without changing actual authorization.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Label justifications:

  • P2: The repair removes recurring browser errors and audit noise for ordinary Agent readers without changing actual authorization.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

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:36 AM ET / 11:36 UTC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant