Skip to content

fix(abac): resolve merge with durable-ids config reload - #1015

Closed
nickmisasi wants to merge 2 commits into
abac/pep-and-apifrom
cursor/resolve-durable-ids-merge-7994
Closed

fix(abac): resolve merge with durable-ids config reload#1015
nickmisasi wants to merge 2 commits into
abac/pep-and-apifrom
cursor/resolve-durable-ids-merge-7994

Conversation

@nickmisasi

@nickmisasi nickmisasi commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Summary

Resolves the merge conflict between abac/pep-and-api (PR 971) and its base abac/durable-ids.

durable-ids moved ABAC ID migration before the in-memory config load (ef5936f5) so a cluster lock-loser still sees remapped service IDs. This branch had a second runABACIDMigrations call after load (and still passed the old *config.Container argument). Keep the durable-ids ordering and drop the duplicate call. PEP wiring (accessChecker into bots/MCP/API) is unchanged.

After this lands on abac/pep-and-api, PR 971 should be mergeable into abac/durable-ids.

Ticket Link

Stacked on #971

Screenshots

N/A (activation-path merge resolution, no UI).

Release Note

NONE
Open in Web Open in Cursor 

Summary by CodeRabbit

  • Bug Fixes
    • Improved configuration migration sequencing to ensure access-control updates are applied before configuration is loaded.
    • Prevented stale in-memory configuration from being used after migrations.
    • Ensured migrated configuration is read from persistent storage at the appropriate stage.

nickmisasi and others added 2 commits August 31, 2026 12:59
A follower that waits on the migration lock sees Migrated=false and
must still read the winner's remapped service/MCP IDs before EnsureBots.

Co-authored-by: Cursor <cursoragent@cursor.com>
Keep ID migration before the in-memory config load so lock-losers
see remapped service IDs. Drop the duplicate post-load call from
this branch.

Co-authored-by: Nick Misasi <nick13misasi@gmail.com>
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 6a23c901-4ddd-4447-9dae-c7cdda422947

📥 Commits

Reviewing files that changed from the base of the PR and between d41487d and 96f687d.

📒 Files selected for processing (2)
  • server/abac_id_migrations.go
  • server/main.go

Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour.


📝 Walkthrough

Walkthrough

ABAC ID migrations now run against the persisted store before configuration loads into memory. The migration function no longer reloads or persists the configuration container. Startup returns migration failures as activation errors.

Changes

ABAC migration ordering

Layer / File(s) Summary
Run ABAC migrations before configuration load
server/abac_id_migrations.go, server/main.go
runABACIDMigrations no longer accepts the in-memory configuration. Startup runs the migration before loading configuration and removes the previous post-load invocation and update publications.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 96f68

This localized change preserves durable-ID migration ordering and removes a duplicate migration call; no actionable merge-blocking risk remains after normal checks and review.

Suggested labels: Setup Cloud Test Server

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the ABAC durable-IDs configuration reload fix and matches the main change: running ID migration before loading in-memory configuration.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cursor/resolve-durable-ids-merge-7994

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

🤖 LLM Evaluation Results

OpenAI

⚠️ Overall: 18/19 tests passed (94.7%)

Provider Total Passed Failed Pass Rate
⚠️ OPENAI 19 18 1 94.7%

❌ Failed Evaluations

Show 1 failures

OPENAI

1. TestReactEval/[openai]_react_cat_message

  • Score: 0.00
  • Rubric: The word/emoji is a cat emoji or a heart/love emoji
  • Reason: The output is the text string "heart_eyes_cat", not an actual cat emoji (e.g., 😺) or heart/love emoji (e.g., ❤️).

Anthropic

⚠️ Overall: 18/19 tests passed (94.7%)

Provider Total Passed Failed Pass Rate
⚠️ ANTHROPIC 19 18 1 94.7%

❌ Failed Evaluations

Show 1 failures

ANTHROPIC

1. TestReactEval/[anthropic]_react_cat_message

  • Score: 0.00
  • Rubric: The word/emoji is a cat emoji or a heart/love emoji
  • Reason: The output is the text string "heart_eyes_cat", not an actual cat emoji (e.g., 😺/🐱) or a heart/love emoji (e.g., ❤️/😍).

This comment was automatically generated by the eval CI pipeline.

@nickmisasi nickmisasi closed this Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants