Repository navigation
[Payment due @linhvovan29546] Prevent removing or demoting RuleBot while Agent rules exist - #96981
Conversation
This comment has been minimized.
This comment has been minimized.
|
Fixed the failing Root cause: The new Fix: Added a localized Verified locally: Notes
|
|
|
|
✅ What was wrong: The new Fix: Added a localized ESLint, Storybook, and the builds are also green. The remaining |
|
@linhvovan29546 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6741e16d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
All code checks are now green after my earlier push ( The one remaining red check is PR Reviewer Checklist / Recap of the fixThe real failure was |
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Fixed the failing Root cause: Fix: Removed Verified locally: ESLint passes on the file, and the internal call in Notes
|
When a workspace has Agent rules but no live RuleBot assigned (none assigned, the agent was deleted, or it was removed from the workspace member list), the rules are silently not enforced. Surface this in the Agent rules section header with a red warning so admins know. Adds a hasRules input to useAgentRulesSectionHeader and renders the warning when rules exist but ruleBotAccountID is missing or the assigned bot is no longer an active policy member. Co-authored-by: Nicolás Bonet <NicolasBonet@users.noreply.github.com>
Co-authored-by: Nicolás Bonet <NicolasBonet@users.noreply.github.com>
The server assigns a RuleBot when it processes a rule creation, so the optimistic add briefly shows the warning until the response arrives. Only count server-confirmed rules when deciding enforcement is missing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the not-enforced warning approach: instead of flagging rules without an enforcer after the fact, prevent RuleBot from being removed (member details page and bulk members removal) and its account from being closed while agent rules still exist on the workspace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0e6d520 to
89bdcec
Compare
Co-authored-by: Nicolás Bonet <NicolasBonet@users.noreply.github.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixed! |
…Enforced # Conflicts: # src/pages/workspace/WorkspaceMembersPage.tsx # src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx
|
The only failing check is PR Author Checklist / Why it's failing: the
How to clear it: check off the 10 remaining boxes at the bottom of the PR description (the "I verified that similar component doesn't exist…", props/ This is an author-completion checklist that asserts a human thought about each item, so I'm intentionally not auto-checking the boxes on your behalf. |
|
Status unchanged since my previous comment: the only failing check is still PR Author Checklist / All other checks are green or still running with none failing (ESLint, Oxfmt, Bun tests, Storybook, spellcheck, Knip, React Compiler, and the iOS builds have passed; Jest, typecheck, Android builds, and perf tests are still in progress). To clear the red check, tick the 10 remaining component-review boxes at the bottom of the PR description once you've confirmed each — it re-runs on the next edit/push and will pass. I'm intentionally not auto-checking them, since that checklist asserts a human thought about each item. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApptelegram-cloud-document-5-6289458079998160731.mp4Android: mWeb Chrometelegram-cloud-document-5-6289458079998160730.mp4iOS: HybridAppScreen.Recording.2026-07-28.at.22.04.44.moviOS: mWeb SafariScreen.Recording.2026-07-28.at.21.58.58.movMacOS: Chrome / SafariScreen.Recording.2026-07-28.at.21.55.11.mov |
|
✅ Fixed — the The only failing check was PR Author Checklist / I re-triggered the checklist workflow against the now-complete description. The new run (30371153759) completed successfully, so the check is cleared. No code changes were needed. |
|
All checks are now passing ✅ — there's nothing left to fix. The |
|
Bug: @NicolasBonet If I clear the cache, then open the Maybe the backend should include the Screen.Recording.2026-07-28.at.22.08.58.mov |
|
That is a NAB indeed, let's approve without that @linhvovan29546 , I will add it to the backend in a follow up |
|
🎯 @linhvovan29546, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 yuwenmemon has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/yuwenmemon in version: 9.4.46-0 🚀
|
|
📝 Help site changes required — draft PR created. This PR introduces new user-facing behavior: RuleBot can't be removed, demoted, or deleted while a workspace still has Agent rules, and closing the RuleBot account is blocked in that state. The RuleBot help article previously only said RuleBot is "managed by Expensify" and didn't explain this guard or how to resolve it, so it needed an update. Draft PR: #97390 What it changes —
I reviewed and applied @NicolasBonet, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.46-10 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Payment issue created: #97537 |
Explanation of Change
When a workspace has Agent rules, they are enforced by a RuleBot agent that is added as a workspace admin. If that RuleBot is removed from the workspace (or its account is closed, the agent is deleted, or it's demoted to a role that can't act on other members' reports), the rules silently stop being enforced.
Instead of flagging that broken state after the fact, this PR prevents it from happening:
hasAgentRules/isRuleBotEnforcingRules/isRuleBotEnforcingRulesOnAnyPolicyhelpers toAgentRulesUtils. Rules pending delete don't count, so removal is allowed once the last rule is deleted (even while the delete is still optimistic).Once all Agent rules are deleted from the workspace, RuleBot can be removed, demoted, or deleted normally.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/663163
PROPOSAL: https://github.com/Expensify/Expensify/issues/663163#issuecomment-5060447097
Tests
settings/security/closeAccount, fill in the form, and submit. Verify the "Unable to close account" modal appears and the account is not closed.Offline tests
Same as Tests — the guard is derived from Onyx policy data, so the blocking modals appear identically while offline.
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Screenshots/Videos
MacOS: Chrome / Safari