Fix disposable review app shared grant mapping - #199
Conversation
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
WalkthroughAdds an RSpec test that loads Control Plane configuration and template YAML, identifies the disposable review app’s shared secret and database credential mapping, then validates the secret reference before and after placeholder substitution. ChangesDatabase secret mapping
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Address-review summaryScan scope: full PR history; no earlier address-review checkpoint existed. Mattered
Skipped
Verification checkpoint: base-fail/head-pass reproduced independently; full repository validation passed; CI readiness is Next default scan starts after this comment. Say |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/controlplane/template_mapping_spec.rb (1)
41-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUtilize direct RSpec matchers for better failure messages.
Evaluating a boolean expression and then expecting
trueusingbe(true)obscures RSpec's built-in diffing capabilities on failure (e.g., outputtingexpected true, got falseinstead of providing a detailed string or number comparison diff). Use direct matchers likeeqandbe_nilto improve test failure observability.
spec/controlplane/template_mapping_spec.rb#L41-L44: Replace the boolean assignment andexpect(mapping_matches_grant).to be(true)withexpect(review_database_reference).to eq(shared_grant_target).spec/controlplane/template_mapping_spec.rb#L15-L15: Replaceexpect(matches.length == 1).to be(true)withexpect(matches.length).to eq(1).spec/controlplane/template_mapping_spec.rb#L22-L22: Replaceexpect(grants.length == 1).to be(true)withexpect(grants.length).to eq(1).spec/controlplane/template_mapping_spec.rb#L33-L33: Replaceexpect(mappings.length == 1).to be(true)withexpect(mappings.length).to eq(1).spec/controlplane/template_mapping_spec.rb#L36-L36: Replaceexpect(!match.nil?).to be(true)withexpect(match).not_to be_nil.♻️ Proposed RSpec refactors
- expect(matches.length == 1).to be(true), "expected exactly one disposable review app configuration" + expect(matches.length).to eq(1), "expected exactly one disposable review app configuration"- expect(grants.length == 1).to be(true), "expected exactly one disposable review app shared grant" + expect(grants.length).to eq(1), "expected exactly one disposable review app shared grant"- expect(mappings.length == 1).to be(true), "expected exactly one disposable database credential mapping" + expect(mappings.length).to eq(1), "expected exactly one disposable database credential mapping"- expect(!match.nil?).to be(true), "expected a repository-managed credential reference" + expect(match).not_to be_nil, "expected a repository-managed credential reference"- mapping_matches_grant = review_database_reference == shared_grant_target - - expect(mapping_matches_grant).to be(true), - "disposable database credential must match its declared shared grant target" + expect(review_database_reference).to eq(shared_grant_target), + "disposable database credential must match its declared shared grant target"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/controlplane/template_mapping_spec.rb` around lines 41 - 44, Replace indirect boolean assertions with direct RSpec matchers in spec/controlplane/template_mapping_spec.rb:15, 22, 33, and 36, asserting each collection length with eq(1) and match presence with not_to be_nil; at spec/controlplane/template_mapping_spec.rb:41-44, remove mapping_matches_grant and directly compare review_database_reference to shared_grant_target with eq.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@spec/controlplane/template_mapping_spec.rb`:
- Around line 41-44: Replace indirect boolean assertions with direct RSpec
matchers in spec/controlplane/template_mapping_spec.rb:15, 22, 33, and 36,
asserting each collection length with eq(1) and match presence with not_to
be_nil; at spec/controlplane/template_mapping_spec.rb:41-44, remove
mapping_matches_grant and directly compare review_database_reference to
shared_grant_target with eq.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46824044-ee8a-4a1b-863b-720c1a23a386
📒 Files selected for processing (2)
.controlplane/templates/app-review.ymlspec/controlplane/template_mapping_spec.rb
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 560e851872
ℹ️ 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".
Address-review summaryScan scope: since previous summary at 2026-07-18T14:17:28Z. Mattered
Skipped
Deferred-work tracking: none. Next default scan starts after this comment. Say |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94db28a8dc
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/controlplane/template_mapping_spec.rb (1)
58-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse idiomatic RSpec matchers instead of evaluating booleans.
Evaluating expressions to a boolean before passing them to
expect(...).to be(true)obscures test failures, as RSpec will only outputexpected true, got falseinstead of the actual and expected values. Use idiomatic RSpec matchers (e.g.,eq,be_nil) to retain meaningful diffs in failure messages.
spec/controlplane/template_mapping_spec.rb#L58-L64: comparerendered_referencevalues directly against the expected values usingeqinstead of creating intermediate booleans.spec/controlplane/template_mapping_spec.rb#L22-L22: change toexpect(grants.length).to eq(1), ...spec/controlplane/template_mapping_spec.rb#L37-L37: change toexpect(mappings.length).to eq(1), ...spec/controlplane/template_mapping_spec.rb#L43-L43: change toexpect(match).not_to be_nil, ...♻️ Proposed refactor for all sites
Lines 22:
- expect(grants.length == 1).to be(true), "expected exactly one disposable review app shared grant" + expect(grants.length).to eq(1), "expected exactly one disposable review app shared grant"Lines 37:
- expect(mappings.length == 1).to be(true), "expected exactly one disposable database credential mapping" + expect(mappings.length).to eq(1), "expected exactly one disposable database credential mapping"Lines 43:
- expect(!match.nil?).to be(true), "expected a repository-managed dictionary-field reference" + expect(match).not_to be_nil, "expected a repository-managed dictionary-field reference"Lines 58-64:
- mapping_matches_grant = rendered_reference.fetch(:target) == grant.fetch("secret_name") - mapping_matches_field = rendered_reference.fetch(:field) == mapping.fetch("name").split("_").last.downcase - - expect(mapping_matches_grant).to be(true), - "renderer substitution must target the declared shared grant" - expect(mapping_matches_field).to be(true), - "renderer substitution must retain the declared dictionary field" + expect(rendered_reference.fetch(:target)).to eq(grant.fetch("secret_name")), + "renderer substitution must target the declared shared grant" + expect(rendered_reference.fetch(:field)).to eq(mapping.fetch("name").split("_").last.downcase), + "renderer substitution must retain the declared dictionary field"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/controlplane/template_mapping_spec.rb` around lines 58 - 64, Replace boolean-based assertions in spec/controlplane/template_mapping_spec.rb:58-64 with direct eq expectations comparing rendered_reference[:target] and rendered_reference[:field] to their expected values. Also update spec/controlplane/template_mapping_spec.rb:22 to use eq(1) for grants.length, line 37 to use eq(1) for mappings.length, and line 43 to use not_to be_nil for match, preserving each existing failure message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@spec/controlplane/template_mapping_spec.rb`:
- Around line 58-64: Replace boolean-based assertions in
spec/controlplane/template_mapping_spec.rb:58-64 with direct eq expectations
comparing rendered_reference[:target] and rendered_reference[:field] to their
expected values. Also update spec/controlplane/template_mapping_spec.rb:22 to
use eq(1) for grants.length, line 37 to use eq(1) for mappings.length, and line
43 to use not_to be_nil for match, preserving each existing failure message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3af6738-cdc0-407b-a575-bc5c3204290c
📒 Files selected for processing (1)
spec/controlplane/template_mapping_spec.rb
Address-review summaryScan scope: since previous summary at 2026-07-18T14:52:40Z. Binding: maintainer-selected PORTABLE Mattered
Skipped
Validation
Review-fix inventory is closed. Merge/deploy remains maintainer-gated for the coordinator's audit and deployed verification; no merge, deployment, or verification PR was attempted here. Next default scan starts after this comment. Say |
✅ Review App DeletedReview app for PR #199 is deleted |
1 participant
Why
Disposable review apps currently reference a different repository-managed database credential than the target declared by their shared grant. The diagnostic replay confirmed this is repository-owned mapping drift: the intended shared object and grant subject exist, and shared routing aligns.
This replaces #197, whose proposed mapping followed the persistent-app template instead of the disposable review app's declared shared-grant target.
Related to #194. Diagnostic evidence: #194 (comment)
Implementation
TDD evidence
bundle exec rspec spec/controlplane/template_mapping_spec.rb— 1 example, 1 intended sanitized mapping failure.Validation
.agents/bin/validate— all 12 CI-core stages passed.git diff --check.Scope and churn
Two files only: one scalar template correction plus one focused regression spec. No workload, policy, workflow, persistent-template, external-state, or upstream React on Rails changes.
Summary by CodeRabbit