Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Dynamic filter dependencies and LDAP snapshot assumptions can produce unsafe or falsely complete results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (4)
| Severity | Finding |
|---|---|
lib/entitlements.rb — This removes every extension-defined constant under Entitlements::Rule, not only classes loaded… |
|
lib/entitlements/desired_groups.rb — skip_dynamic_groups is not propagated through calculated-group filters. Filters are instantiated… |
|
lib/entitlements/extras/ldap_group/rules/ldap_group.rb — Missing shellentitlements is treated as definite non-membership, although that attribute is… |
|
README.md — Remove the stray -app suffix. This issue also appears on line 21 of the same file. |
What changed in this PR
Adds deterministic, CI-ready entitlement membership comparison using frozen people and time inputs.
Changes:
- Adds desired-membership export and bounded JSON/Markdown smart diff.
- Handles dynamic groups, mirrors, LDAP rules, and frozen expiration evaluation.
- Packages the new executable and adds comprehensive fixtures/specs.
| File | Description |
|---|---|
.gitignore |
Tracks the new executable. |
README.md |
Documents CI smart diff usage. |
Gemfile.lock |
Updates the gem version. |
entitlements-app.gemspec |
Packages the executable. |
bin/entitlements-smart-diff |
Adds the command entry point. |
lib/version.rb |
Bumps version to 1.2.2. |
lib/entitlements.rb |
Adds evaluation time, reset behavior, and requires. |
lib/entitlements/desired_groups.rb |
Implements deterministic membership export. |
lib/entitlements/smart_diff.rb |
Implements comparison and report rendering. |
lib/entitlements/smart_diff/cli.rb |
Implements CLI parsing and output. |
lib/entitlements/data/groups/calculated.rb |
Adds dynamic-group skipping and mirror warnings. |
lib/entitlements/data/groups/calculated/base.rb |
Cleans up failed dynamic dependencies. |
lib/entitlements/data/groups/calculated/ruby.rb |
Detects and reloads Ruby groups. |
lib/entitlements/data/groups/calculated/text.rb |
Uses frozen evaluation time. |
lib/entitlements/data/groups/calculated/modifiers/expiration.rb |
Freezes expiration evaluation. |
lib/entitlements/data/groups/calculated/rules/group.rb |
Resets caches and propagates options. |
lib/entitlements/extras/ldap_group/rules/ldap_group.rb |
Adds snapshot-based offline matching. |
spec/unit/entitlements/desired_groups_spec.rb |
Tests export behavior. |
spec/unit/entitlements/smart_diff_spec.rb |
Tests comparison and rendering. |
spec/unit/entitlements/extras/ldap_group/rules/ldap_group_spec.rb |
Tests offline LDAP matching. |
spec/unit/fixtures/smart-diff/config.yaml |
Configures smart-diff fixtures. |
spec/unit/fixtures/smart-diff/people.yaml |
Supplies frozen people data. |
spec/unit/fixtures/smart-diff/groups/internal/contractors.txt |
Defines contractor membership. |
spec/unit/fixtures/smart-diff/groups/internal/engineers.txt |
Defines engineer membership. |
spec/unit/fixtures/smart-diff/groups/teams/direct.txt |
Defines direct membership. |
spec/unit/fixtures/smart-diff/groups/teams/expiring.yaml |
Exercises expiration behavior. |
spec/unit/fixtures/smart-diff/groups/teams/filtered.txt |
Exercises filtering. |
spec/unit/fixtures/smart-diff/groups/teams/nested.yaml |
Exercises nested groups. |
spec/unit/fixtures/smart-diff/groups/teams/ruby-group.rb |
Exercises Ruby membership. |
spec/unit/fixtures/dynamic-groups/config.yaml |
Configures dynamic-group fixtures. |
spec/unit/fixtures/dynamic-groups/groups/teams/dependent.txt |
Tests dynamic dependencies. |
spec/unit/fixtures/dynamic-groups/groups/teams/dynamic.rb |
Defines unsafe dynamic behavior. |
spec/unit/fixtures/dynamic-groups/groups/teams/static-ruby.rb |
Tests conservative Ruby detection. |
spec/unit/fixtures/dynamic-groups/groups/teams/static.txt |
Provides exportable static membership. |
Suppressed comments (1)
README.md:21
- Remove the stray
-appsuffix.
Ruby entitlement groups are treated as dynamic by smart diff because arbitrary Ruby cannot be proven deterministic from frozen inputs. Groups that use or transitively depend on Ruby definitions are omitted from the membership comparison, and both JSON and Markdown report that the result is incomplete. Normal deployment behavior is unchanged.-app
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Entitlements::Rule.constants(false).each do |constant| | ||
| Entitlements::Rule.send(:remove_const, constant) unless constant == :Base | ||
| end |
| Entitlements::Data::Groups::Calculated.read_all( | ||
| group_name, | ||
| group_config, | ||
| skip_dynamic_groups: allow_incomplete |
| if Entitlements.cache[:desired_groups_export] | ||
| return Set.new(Entitlements.cache[:people_obj].read.values.select do |person| | ||
| begin | ||
| Array(person["shellentitlements"]).map(&:downcase).include?(value.downcase) | ||
| rescue KeyError | ||
| false | ||
| end |
|
|
||
| Entitlements.logger.debug "Calculating members from #{filename}" | ||
| result = members_from_rules(rules) | ||
| @members = result unless result == :calculating |
There was a problem hiding this comment.
This is both a general evaluator bug fix and a requirement for smart diff: :calculating is a transient in-progress sentinel, so caching it here can permanently poison the file object and prevent a later retry from producing the actual member set.
|
|
||
| Entitlements.logger.debug "Calculating members from #{filename}" | ||
| result = members_from_rules(rules) | ||
| @members = result unless result == :calculating |
There was a problem hiding this comment.
This is both a general evaluator bug fix and a requirement for smart diff: :calculating is a transient in-progress sentinel, so caching it here can permanently poison the file object and prevent a later retry from producing the actual member set.
Export deterministic desired memberships and compare PR base/head without provider reconciliation. Report incomplete results when arbitrary Ruby groups prevent a complete offline calculation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b96e14a4-88bb-45b9-9b94-a681a34d05bc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b96e14a4-88bb-45b9-9b94-a681a34d05bc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b96e14a4-88bb-45b9-9b94-a681a34d05bc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b96e14a4-88bb-45b9-9b94-a681a34d05bc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b96e14a4-88bb-45b9-9b94-a681a34d05bc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b96e14a4-88bb-45b9-9b94-a681a34d05bc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b96e14a4-88bb-45b9-9b94-a681a34d05bc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 38d1ec2b-befd-4535-8e8a-e32447bd9f1a
Track and unload only constants introduced by entitlement Ruby files. Propagate dynamic skipping through filters, omit dynamic dependency closures, and render collapsible backend-specific report tables without warning output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ee634509-607d-4e8e-8072-59a390fecedf
Calculate only the affected entitlement closure while allowing Ruby groups and their recursive dependencies to use the normal evaluator. Track per-file filter references and fail when dynamic evaluation cannot complete. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: edf74ff7-9e50-4f79-abf0-582add2d82d4
4e9064a to
97222bf
Compare
| if expiration =~ /\A(\d{4})-(\d{2})-(\d{2})\z/ | ||
| year, month, day = Regexp.last_match(1).to_i, Regexp.last_match(2).to_i, Regexp.last_match(3).to_i | ||
| return Time.utc(year, month, day, 0, 0, 0) <= Time.now.utc | ||
| return Time.utc(year, month, day, 0, 0, 0) <= Entitlements.evaluation_time.utc |
There was a problem hiding this comment.
We can hit bugs by repeatedly calling Time.now throughout long running entitlements calculations, so I am encouraging the use of a time snapshot at the start of the run, rather than evaluating Time.now repeatedly during execution.
Memoize the first evaluation timestamp until Entitlements state is reset so long-running calculations cannot cross an expiration boundary inconsistently. Preserve explicit timestamps supplied by smart diff. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e07e050e-c1bf-4e35-b737-5fd9cf86b63b
| def handle_and(rule) | ||
| ensure_type!("and", rule, Array) | ||
| return result unless rule.any? | ||
| return Set.new unless rule.any? |
There was a problem hiding this comment.
result was previously an uninitialized value. This is a bugfix with an accompanying test.
Evaluate base and head snapshots in separate Ruby processes so arbitrary entitlement Ruby state cannot leak between trees. Remove recursive constant tracking and fail closed when a snapshot worker fails or returns malformed output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e07e050e-c1bf-4e35-b737-5fd9cf86b63b
Start the isolated base and head workers together, wait for both to settle, and propagate worker failures after joining the pair. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e07e050e-c1bf-4e35-b737-5fd9cf86b63b
Pass the parent Ruby load path to isolated workers and allow callers to explicitly require repository plugin bundles in each process. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e07e050e-c1bf-4e35-b737-5fd9cf86b63b
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the unpublished JSON artifact contract with a normalized SQLite database containing snapshot metadata, membership changes, scoped groups, and base/head identity facts for changed users. Generate Markdown from the database so policy consumers and human output share one canonical source. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c95d4e60-1a4e-4eca-96c1-554b08603174
Allow the supported Ruby 3.1 Linux jobs to install SQLite without invoking the source gem's mini_portile configuration path, which is incompatible with that runtime's Psych version. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c95d4e60-1a4e-4eca-96c1-554b08603174
Record SQLite's x86_64 Linux package alongside the source fallback so supported CI runtimes select the precompiled extension. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c95d4e60-1a4e-4eca-96c1-554b08603174
Upgrade to the secure SQLite 2.8 line while retaining Ruby 3.1 support, and use the binding-array API shared by current sqlite3 releases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c95d4e60-1a4e-4eca-96c1-554b08603174
Provide the filename Bundler 2.5 derives for the GNU Linux SQLite package on Ruby 3.1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c95d4e60-1a4e-4eca-96c1-554b08603174
Avoid vulnerable sqlite3-ruby releases while preserving Ruby 3.1 support. The generated database and SQL contract remain standard SQLite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c95d4e60-1a4e-4eca-96c1-554b08603174
Remove approval-policy bookkeeping from the artifact schema so repository-owned SQL queries can classify changes externally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c95d4e60-1a4e-4eca-96c1-554b08603174
Store identity attributes as relational person_facts rows instead of JSON objects so approval queries use ordinary SQL joins. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c95d4e60-1a4e-4eca-96c1-554b08603174



Add a read-only, scoped smart-diff evaluator between two entitlement trees. It evaluates changed groups and reverse dependents, rebuilds canonical base/head identity snapshots with one frozen timestamp, globally validates supported entitlement files, and exports a facts-only SQLite database containing snapshot metadata, affected groups, membership gains/losses, changed users, and normalized
person_factsrows. No identity data or policy evidence is stored as JSON.Markdown is reconstructed from the database. SQLite is provided by
extralite-bundle2.10 so the evaluator remains self-contained on Ruby 3.1+ without relying on the vulnerablesqlite3-rubycallback implementation. All 18 supported CI checks pass at5e8c32856eac1dcf378fe53239ef1e6ca56f205b.