Skip to content

ENG-1038: warn on the analyzer rule form when an Access Request rule covers a role - #2122

Open
rogefm wants to merge 3 commits into
mainfrom
claude/kind-babbage-3xds0m
Open

rogefm wants to merge 3 commits into
mainfrom
claude/kind-babbage-3xds0m

Conversation

@rogefm

@rogefm rogefm commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Opened by the support agent (AI) from ENG-1038.

📝 Description

An Access Request rule of type command or jit_command runs before the AI Session Analyzer, so a role in both waits for approval whatever the risk level (PicPay). The gateway analyzer rule form now lists each selected role that such a rule covers, says how (the role is listed, or a shared attribute), and links to the rule. Save is not blocked.

📣 User-facing impact

The AI Session Analyzer rule form warns when an Access Request rule also covers a selected resource role, and names that rule.

🔗 Related Issue

ENG-1038

🚀 Type of Change

  • ✨ New feature (non-breaking change which adds functionality)

📋 Changes Made

  • AiSessionAnalyzer/helpers.js: accessRequestOverlap, the same match as services.GetRuleForConnection (names or attributes; command, jit_command), with the reason for each match.
  • AiSessionAnalyzer/store.js: loads attributes. Each list is cleared when its fetch starts and guarded by a request id, and is reset on logout.
  • Create/sections/AccessRequestOverlap.jsx + GatewayAiAnalyzerForm.jsx: the warning under "Roles configuration" waits until both lists have loaded. A load failure says the check could not run. Rule links open in a new tab.

🧪 Testing

Result: when an admin selects a resource role that a command Access Request rule also covers, the form says so, names the rule and says how it reaches the role. Independent QA: PASS (round 2, 1028929). ce6f903 adds only the logout request-id bump (unit tests, lint and build pass).

1-analyzer-form-overlap

Same seed on both sides: ar-cmd (command) lists pg-cmd; ar-attr (command) covers attribute production of pg-attr; ar-jit (jit only) lists pg-jit; pg-free has no rule. Main shows nothing. The branch lists pg-cmd (ar-cmd, lists the role) and pg-attr (ar-attr, attribute production).

2-lists-fail-to-load

What it solves: admins setting up the AI Session Analyzer see why a medium-risk command still waits for approval, and which Access Request rule to change.

  • Warning under "Roles configuration" on create and edit for a role covered by a command/jit_command rule.
  • "Covered" matches the gateway: by role name or by a shared attribute.
  • Names each role and rule, says the rule runs first, and links to the rule's edit page.
  • jit-only rule: no warning. Uncovered role: no warning.
  • Follows the selection as roles are added and removed.
  • Lists fail to load: "Could not check whether an Access Request rule also covers these resource roles."
  • Save is not blocked.
  • Unit tests: webapp_v2/test/aiAnalyzerAccessRequestOverlap.test.js.

Re-run:

cd webapp_v2 && npx vitest run test/aiAnalyzerAccessRequestOverlap.test.js && npm run lint && npm run build
# by hand: scripts/dev/run-sandbox.sh start --ui; create roles pg-cmd, pg-attr (attribute production),
# command rules ar-cmd (pg-cmd) and ar-attr (attribute production); open
# /features/ai-session-analyzer/rules/new?traffic=agent and select the roles

📄 Additional Notes

  • Risk: low. UI only; no API, agent, wire, flag, migration, env var or analytics change.
  • Not exercised end to end: one rule that matches by name and by attribute together (the gateway refuses that seed); a unit test covers it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KYT56i5cyocHaNYzijzevr

…covers a role

An Access Request rule of type command runs before the analyzer, so the
form lists each selected role that such a rule also covers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KYT56i5cyocHaNYzijzevr
@rogefm rogefm added minor Bumps the minor version on release (new features) from-agent-support labels Nov 5, 2026 — with Claude
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Warn when Access Request rules cover analyzer roles

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Warn when selected analyzer roles have command approval rules that run first.
• Match roles by name or shared attribute, with links to covering rules.
• Show a check-failure notice and test overlap detection.
Diagram

graph TD
  Form["Analyzer form"] --> Store["Analyzer store"] --> Rules["Access rules"] --> Match{"Command overlap?"} --> Warning["Role warning"] --> Editor["Rule editor"]
  Store --> Attr["Role attributes"] --> Match
  Form --> Match
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Server-side overlap endpoint
  • ➕ Could use gateway matching logic directly and avoid duplicating it in the client.
  • ➖ Requires a new API and backend work for an advisory form warning.

Recommendation: Keep the client-side check for this UI-only warning: it reuses existing list services without changing the API. A server-side check would be preferable if overlap detection later needs to be authoritative.

Files changed (5) +212 / -5

Enhancement (4) +149 / -5
GatewayAiAnalyzerForm.jsxShow overlap feedback beneath selected roles +26/-5

Show overlap feedback beneath selected roles

• Fetches attributes alongside Access Request rules and passes both to the new warning in the Roles configuration section. A failure in either fetch is reported to the warning component.

webapp_v2/src/pages/Features/AiSessionAnalyzer/Create/GatewayAiAnalyzerForm.jsx

AccessRequestOverlap.jsxDisplay covered roles and links to their rules +68/-0

Display covered roles and links to their rules

• Shows an advisory warning for selected roles covered by command-capable Access Request rules, with links to each rule. Shows a separate notice when the overlap check could not run.

webapp_v2/src/pages/Features/AiSessionAnalyzer/Create/sections/AccessRequestOverlap.jsx

helpers.jsDetect command-rule coverage by role or attribute +39/-0

Detect command-rule coverage by role or attribute

• Adds a helper that considers command and jit_command rules, matches selected roles by name or shared attribute, and returns covering rules sorted by name.

webapp_v2/src/pages/Features/AiSessionAnalyzer/helpers.js

store.jsLoad attributes for overlap checks +16/-0

Load attributes for overlap checks

• Adds attribute data and fetch status to the analyzer store, using the existing service to load all attribute pages.

webapp_v2/src/pages/Features/AiSessionAnalyzer/store.js

Tests (1) +63 / -0
aiAnalyzerAccessRequestOverlap.test.jsTest Access Request overlap matching +63/-0

Test Access Request overlap matching

• Covers direct and attribute-based matches, command-capable versus excluded rule types, sorting, deduplication, and empty inputs.

webapp_v2/test/aiAnalyzerAccessRequestOverlap.test.js

@qodo-code-review

qodo-code-review Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. A logout can still let an in-flight account's rules land in the store ✓ Resolved

Resolving commit: ce6f903

Resolution reason
The logout subscriber now reads the current accessRequestRulesRequestId and attributesRequestId and
increments both in the same setState that clears the data and statuses. This is the suggested fix:
in-flight fetches captured before logout fail the requestId !== captured guard and cannot write
stale data back.
Description
The logout subscription in store.js clears accessRequestRules and attributes and sets their
statuses to idle without advancing accessRequestRulesRequestId or attributesRequestId, so
fetchAccessRequestRules and fetchAttributes calls still in flight pass their `requestId !==
captured guard after logout and write the previous account's data back with status success`. The
window is between logout and the next form load, and a multi-page listAll walk widens it, since
the subscriber acts as a second writer that does not take part in the generation check.

2. The warning can show another account's rules ✓ Resolved

Resolving commit: 1028929

Resolution reason
Overlap warning is gated on both fetches succeeding for the current fetch cycle (ready prop), store
arrays are cleared when a fetch starts, stale responses are discarded via request IDs, and
account-scoped data is cleared on logout.
Description
RuleFormFields passes retained rule and attribute arrays to AccessRequestOverlap while both
fetches are loading, because failed checks only for an error. After a same-tab local login to
another account, the form can briefly display the previous account’s role and rule names, or omit a
current overlap until the new requests finish.

3. Attribute matches lead to the wrong fix ✓ Resolved

Resolving commit: 1028929

Resolution reason
The overlap result now records byName and the shared attributes per rule (helpers.js). matchReason()
in AccessRequestOverlap.jsx annotates each linked rule with 'lists the role' and/or the matching
attribute names, and the warning text now says to remove the role, or the shared attribute, from the
Access Request rule. This distinguishes attribute matches and tells the admin what to remove, though
it does not explicitly mention changing the role's attribute association, so it is a different
implementation from the suggested fix.
Description
AccessRequestOverlap advises removing the role from the linked Access Request rule even when
accessRequestOverlap matched that rule only through a shared attribute. In that case the role is
absent from the rule’s resource-role selection, so following the link requires removing the shared
attribute from the rule or changing the role’s attribute association instead.

Grey Divider

Context sources
✅ Compliance rules (platform): 126 rules
✅ Skills: build-ticket
✅ Cross-repo context — repo relationships
Review mode: Auto: ⚖️ Balanced: Concurrency-guard and auth-logout logic; small but risky.
✅ Fix-loop round: fix loop

Grey Divider

Tip of the day
💡 Did you know, you can turn these tips off under Display > Comment parts

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit ce6f903 ⚖️ Balanced

Results up to commit 4fb4aeb ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. The warning can show another account's rules ✓ Resolved

Resolving commit: 1028929

Resolution reason
Overlap warning is gated on both fetches succeeding for the current fetch cycle (ready prop), store
arrays are cleared when a fetch starts, stale responses are discarded via request IDs, and
account-scoped data is cleared on logout.
Description
RuleFormFields passes retained rule and attribute arrays to AccessRequestOverlap while both
fetches are loading, because failed checks only for an error. After a same-tab local login to
another account, the form can briefly display the previous account’s role and rule names, or omit a
current overlap until the new requests finish.

2. Attribute matches lead to the wrong fix ✓ Resolved

Resolving commit: 1028929

Resolution reason
The overlap result now records byName and the shared attributes per rule (helpers.js). matchReason()
in AccessRequestOverlap.jsx annotates each linked rule with 'lists the role' and/or the matching
attribute names, and the warning text now says to remove the role, or the shared attribute, from the
Access Request rule. This distinguishes attribute matches and tells the admin what to remove, though
it does not explicitly mention changing the role's attribute association, so it is a different
implementation from the suggested fix.
Description
AccessRequestOverlap advises removing the role from the linked Access Request rule even when
accessRequestOverlap matched that rule only through a shared attribute. In that case the role is
absent from the rule’s resource-role selection, so following the link requires removing the shared
attribute from the rule or changing the role’s attribute association instead.

Results up to commit 1028929 🚀 Fast


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. A logout can still let an in-flight account's rules land in the store ✓ Resolved

Resolving commit: ce6f903

Resolution reason
The logout subscriber now reads the current accessRequestRulesRequestId and attributesRequestId and
increments both in the same setState that clears the data and statuses. This is the suggested fix:
in-flight fetches captured before logout fail the requestId !== captured guard and cannot write
stale data back.
Description
The logout subscription in store.js clears accessRequestRules and attributes and sets their
statuses to idle without advancing accessRequestRulesRequestId or attributesRequestId, so
fetchAccessRequestRules and fetchAttributes calls still in flight pass their `requestId !==
captured guard after logout and write the previous account's data back with status success`. The
window is between logout and the next form load, and a multi-page listAll walk widens it, since
the subscriber acts as a second writer that does not take part in the generation check.

Grey Divider

Qodo Logo

The warning names the shared attribute, links open in a new tab, and it
reads the lists only after both loaded for this page. Logout drops them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KYT56i5cyocHaNYzijzevr
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 1028929

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit ce6f903

@sandromello

Copy link
Copy Markdown
Contributor

✅ Build Completed with Success, Version=2122.0.0-gce6f903

This branch has not been deployed

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

Labels

from-agent-support minor Bumps the minor version on release (new features)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants