Add react_on_rails:install_rsc_agent_guardrails: ship RSC agent-safety skill + hook into apps#4606
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an idempotent RSC agent-guardrails installer with a Claude skill, advisory hook, settings registration, Rake task, and RSC generator integration. Tests cover hook analysis, settings migration, reruns, error handling, and generator behavior. ChangesRSC Agent Guardrails
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RSCGenerator as RSC generator
participant Installer as AgentGuardrails::Installer
participant App as Rails application
participant Claude as Claude Code hook
RSCGenerator->>Installer: install(destination_root, skip_existing)
Installer->>App: write skill and executable hook
Installer->>App: merge PostToolUse settings
Claude->>App: inspect changed routes or controllers
App-->>Claude: emit advisory warning JSON when authentication is missing
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: de1202cb53
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Greptile SummaryThis PR adds installable RSC agent guardrails for React on Rails apps. The main changes are:
Confidence Score: 4/5The guardrail install path and hook checks need fixes before merging.
agent_guardrails.rb and rsc_app_safety_check.sh
|
| Filename | Overview |
|---|---|
| react_on_rails/lib/generators/react_on_rails/rsc_generator.rb | Loads the new installer and runs it during RSC generation. |
| react_on_rails/lib/react_on_rails/agent_guardrails.rb | Adds the installer and settings merge logic, but incompatible valid settings JSON can crash the install path. |
| react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.sh | Adds the advisory hook, but it can silently miss checks when jq is unavailable or controllers are nested. |
| react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_skill.md | Adds RSC safety guidance for app-level usage. |
| react_on_rails/lib/tasks/agent_guardrails.rake | Adds a rake task that invokes the installer and reports actions. |
| react_on_rails/spec/react_on_rails/agent_guardrails_spec.rb | Adds installer coverage for fresh installs, idempotency, happy-path merges, and malformed JSON. |
| react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb | Checks that the RSC generator installs the guardrail files and settings entry. |
Reviews (1): Last reviewed commit: "Add CHANGELOG entry for RSC agent guardr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review summaryReviewed the diff ( Findings (posted inline)
Other observations (not inline)
What looks good
|
ReviewReviewed the diff via OverviewClean, well-scoped addition: an idempotent Findings (posted inline)
Other observations (non-blocking)
Overall: solid, low-risk addition. The two inline findings are worth fixing before merge, particularly #1 since it undermines confidence that the hook actually fires as documented. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@react_on_rails/lib/generators/react_on_rails/rsc_generator.rb`:
- Around line 125-132: Update install_agent_guardrails to honor the generator’s
options before invoking ReactOnRails::AgentGuardrails.install: skip installation
when options[:skip] or options[:pretend] is set, while preserving the existing
success and error messaging for actual installations.
In `@react_on_rails/lib/react_on_rails/agent_guardrails.rb`:
- Around line 53-65: Update copy_file so executable permissions are re-applied
before returning the "unchanged" result when dest_path ends with ".sh". Ensure
existing unchanged content still reports "unchanged" while restoring mode 0755,
and preserve the current write and status behavior for changed or newly created
files.
- Around line 79-95: Update read_settings so the parsed JSON must be a Hash
before being returned; treat valid non-object values such as arrays or strings
like invalid settings by raising the same friendly Error and leaving the file
untouched. Preserve the existing JSON::ParserError message and ensure
hook_registered? and add_hook only receive validated object settings.
In
`@react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.sh`:
- Around line 38-50: Update the FILE case patterns in the routes and controller
checks to match both bare relative paths such as config/routes.rb and
app/controllers/example.rb and paths containing preceding directories, while
preserving the existing warning conditions and messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec5f977b-5389-4f04-ae50-475ff466e180
📒 Files selected for processing (8)
CHANGELOG.mdreact_on_rails/lib/generators/react_on_rails/rsc_generator.rbreact_on_rails/lib/react_on_rails/agent_guardrails.rbreact_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.shreact_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_skill.mdreact_on_rails/lib/tasks/agent_guardrails.rakereact_on_rails/spec/react_on_rails/agent_guardrails_spec.rbreact_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3288c60e31
ℹ️ 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.sh`:
- Around line 46-47: Update the preprocessing used by the safety checks around
lines and renderer detection so Ruby line comments and =begin/=end block
comments are removed consistently before authentication and renderer evidence
checks. Ensure commented callbacks cannot suppress unsecured-renderer warnings
and commented renderer tokens cannot trigger false warnings, and add regression
coverage for both cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a30085d7-7d1e-43e5-9e43-b90f377a7b7d
📒 Files selected for processing (7)
react_on_rails/lib/generators/react_on_rails/rsc_generator.rbreact_on_rails/lib/react_on_rails/agent_guardrails.rbreact_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.shreact_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_skill.mdreact_on_rails/lib/tasks/agent_guardrails.rakereact_on_rails/spec/react_on_rails/agent_guardrails_spec.rbreact_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
🚧 Files skipped from review as they are similar to previous changes (5)
- react_on_rails/lib/tasks/agent_guardrails.rake
- react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_skill.md
- react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
- react_on_rails/lib/generators/react_on_rails/rsc_generator.rb
- react_on_rails/lib/react_on_rails/agent_guardrails.rb
Review summaryWhat this does: Adds Code quality
Security
Test coverageStrong — Overall this is solid, low-risk, well-tested tooling. The two inline notes are both minor (messaging accuracy and false-positive noise) rather than functional bugs. |
|
Review Summary What this does: Ships an idempotent installer ( Strengths
Issues raised inline
Minor / nit
Security |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CHANGELOG.md (1)
40-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMark this Pro-only feature with the inline Pro tag.
The installer targets React on Rails Pro RSC applications, but the changelog entry currently presents it as a general feature.
Proposed fix
-- **RSC agent guardrails installer**: New `rake react_on_rails:install_rsc_agent_guardrails` task +- **[Pro]** **RSC agent guardrails installer**: New `rake react_on_rails:install_rsc_agent_guardrails` taskAs per coding guidelines, “Pro-only changelog entries must use an inline
**[Pro]**tag within standard category sections.”🤖 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 `@CHANGELOG.md` around lines 40 - 47, Add the inline **[Pro]** tag to the RSC agent guardrails installer changelog entry, keeping it within the existing standard category section and leaving the rest of the entry unchanged.Source: Coding guidelines
react_on_rails/lib/react_on_rails/agent_guardrails.rb (1)
132-137: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTolerate string-based hooks to avoid aborting installation on valid settings.
Claude Code natively supports specifying hooks as basic strings (e.g.,
"hooks": ["npm run lint"]). Currently,valid_hook_group?strictly enforceshooks.all?(Hash). If the user'ssettings.jsoncontains any string hooks, the installer treats the valid configuration as malformed, leaves the file untouched, and aborts with a manual fallback message.Relax the validation to permit strings, and update the predicates to safely type-check the hook before accessing its keys.
🔧 Proposed fix
def valid_hook_group?(entry) return false unless entry.is_a?(Hash) hooks = entry["hooks"] - hooks.nil? || (hooks.is_a?(Array) && hooks.all?(Hash)) + hooks.nil? || hooks.is_a?(Array) endAnd update the predicates:
def registered_hook?(hook) - hook["type"] == "command" && hook["command"] == HOOK_COMMAND && hook["args"] == HOOK_ARGS + hook.is_a?(Hash) && hook["type"] == "command" && hook["command"] == HOOK_COMMAND && hook["args"] == HOOK_ARGS end def managed_hook?(hook) - registered_hook?(hook) || hook["command"] == LEGACY_HOOK_COMMAND + registered_hook?(hook) || (hook.is_a?(Hash) && hook["command"] == LEGACY_HOOK_COMMAND) || hook == LEGACY_HOOK_COMMAND endAlso applies to: 168-174
🤖 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 `@react_on_rails/lib/react_on_rails/agent_guardrails.rb` around lines 132 - 137, Update valid_hook_group? and the related hook predicates to accept hook entries represented as either Hash or String, while retaining support for nil and arrays. Ensure any hook key access is guarded by a type check so string hooks cannot cause errors, allowing valid string-based settings to pass validation.
🧹 Nitpick comments (1)
react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.rb (1)
55-69: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider supporting
prepend_before_actionandappend_before_action.The current implementation only looks for the standard
before_actionandskip_before_action. If an application usesprepend_before_action :authenticate_user!to guarantee ordering, this advisory hook will miss the evidence and emit a false-positive warning.Since the parser relies on a flexible regex, you can safely expand the
callback_nameto include these variants.♻️ Proposed refactor
def authenticated_callback_names(lines) - logical_statements(lines, "before_action").filter_map do |statement| - parsed = parse_callback(statement, "before_action") + logical_statements(lines, "(?:prepend_|append_)?before_action").filter_map do |statement| + parsed = parse_callback(statement, "(?:prepend_|append_)?before_action") parsed&.first if parsed && applies_to_payload?(parsed.last, unknown: false) end end def remove_skipped_callbacks(lines, authenticated_callbacks) - logical_statements(lines, "skip_before_action").each do |statement| - parsed = parse_callback(statement, "skip_before_action") + logical_statements(lines, "skip_(?:prepend_|append_)?before_action").each do |statement| + parsed = parse_callback(statement, "skip_(?:prepend_|append_)?before_action") next unless parsed && authenticated_callbacks.include?(parsed.first) authenticated_callbacks.delete(parsed.first) if applies_to_payload?(parsed.last, unknown: true) end end🤖 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 `@react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.rb` around lines 55 - 69, Extend authenticated_callback_names and remove_skipped_callbacks to recognize prepend_before_action and append_before_action alongside the existing before_action and skip_before_action callbacks. Update the callback-name matching passed to logical_statements and parse_callback so these variants are parsed and evaluated with the same authentication and skip semantics, preventing false-positive warnings.
🤖 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.
Inline comments:
In `@CHANGELOG.md`:
- Line 140: Merge the changelog entries currently under the duplicate “Added”
heading into the existing “Added” section near the top of CHANGELOG.md, then
remove the later heading while preserving all entry content and ordering.
In `@react_on_rails/spec/react_on_rails/agent_guardrails_spec.rb`:
- Around line 162-188: The file-access guardrail specs should use deterministic
fixtures: update the unrelated file in “does not read unrelated edited files” to
contain an RSC trigger while preserving the expectation that it is ignored, and
replace chmod-based failure setup in “remains non-blocking when a matching file
cannot be read” with an injected reader or invalid file type that reliably
causes reading to fail, including under privileged users.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 40-47: Add the inline **[Pro]** tag to the RSC agent guardrails
installer changelog entry, keeping it within the existing standard category
section and leaving the rest of the entry unchanged.
In `@react_on_rails/lib/react_on_rails/agent_guardrails.rb`:
- Around line 132-137: Update valid_hook_group? and the related hook predicates
to accept hook entries represented as either Hash or String, while retaining
support for nil and arrays. Ensure any hook key access is guarded by a type
check so string hooks cannot cause errors, allowing valid string-based settings
to pass validation.
---
Nitpick comments:
In
`@react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.rb`:
- Around line 55-69: Extend authenticated_callback_names and
remove_skipped_callbacks to recognize prepend_before_action and
append_before_action alongside the existing before_action and skip_before_action
callbacks. Update the callback-name matching passed to logical_statements and
parse_callback so these variants are parsed and evaluated with the same
authentication and skip semantics, preventing false-positive warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c86568e-da58-4e0b-9d1d-55e9b25954ce
📒 Files selected for processing (6)
CHANGELOG.mdreact_on_rails/lib/generators/react_on_rails/rsc_generator.rbreact_on_rails/lib/react_on_rails/agent_guardrails.rbreact_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.rbreact_on_rails/spec/react_on_rails/agent_guardrails_spec.rbreact_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
- react_on_rails/lib/generators/react_on_rails/rsc_generator.rb
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CHANGELOG.md (1)
40-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMark this Pro-only feature with the inline Pro tag.
The installer targets React on Rails Pro RSC applications, but the changelog entry currently presents it as a general feature.
Proposed fix
-- **RSC agent guardrails installer**: New `rake react_on_rails:install_rsc_agent_guardrails` task +- **[Pro]** **RSC agent guardrails installer**: New `rake react_on_rails:install_rsc_agent_guardrails` taskAs per coding guidelines, “Pro-only changelog entries must use an inline
**[Pro]**tag within standard category sections.”🤖 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 `@CHANGELOG.md` around lines 40 - 47, Add the inline **[Pro]** tag to the RSC agent guardrails installer changelog entry, keeping it within the existing standard category section and leaving the rest of the entry unchanged.Source: Coding guidelines
react_on_rails/lib/react_on_rails/agent_guardrails.rb (1)
132-137: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTolerate string-based hooks to avoid aborting installation on valid settings.
Claude Code natively supports specifying hooks as basic strings (e.g.,
"hooks": ["npm run lint"]). Currently,valid_hook_group?strictly enforceshooks.all?(Hash). If the user'ssettings.jsoncontains any string hooks, the installer treats the valid configuration as malformed, leaves the file untouched, and aborts with a manual fallback message.Relax the validation to permit strings, and update the predicates to safely type-check the hook before accessing its keys.
🔧 Proposed fix
def valid_hook_group?(entry) return false unless entry.is_a?(Hash) hooks = entry["hooks"] - hooks.nil? || (hooks.is_a?(Array) && hooks.all?(Hash)) + hooks.nil? || hooks.is_a?(Array) endAnd update the predicates:
def registered_hook?(hook) - hook["type"] == "command" && hook["command"] == HOOK_COMMAND && hook["args"] == HOOK_ARGS + hook.is_a?(Hash) && hook["type"] == "command" && hook["command"] == HOOK_COMMAND && hook["args"] == HOOK_ARGS end def managed_hook?(hook) - registered_hook?(hook) || hook["command"] == LEGACY_HOOK_COMMAND + registered_hook?(hook) || (hook.is_a?(Hash) && hook["command"] == LEGACY_HOOK_COMMAND) || hook == LEGACY_HOOK_COMMAND endAlso applies to: 168-174
🤖 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 `@react_on_rails/lib/react_on_rails/agent_guardrails.rb` around lines 132 - 137, Update valid_hook_group? and the related hook predicates to accept hook entries represented as either Hash or String, while retaining support for nil and arrays. Ensure any hook key access is guarded by a type check so string hooks cannot cause errors, allowing valid string-based settings to pass validation.
🧹 Nitpick comments (1)
react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.rb (1)
55-69: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider supporting
prepend_before_actionandappend_before_action.The current implementation only looks for the standard
before_actionandskip_before_action. If an application usesprepend_before_action :authenticate_user!to guarantee ordering, this advisory hook will miss the evidence and emit a false-positive warning.Since the parser relies on a flexible regex, you can safely expand the
callback_nameto include these variants.♻️ Proposed refactor
def authenticated_callback_names(lines) - logical_statements(lines, "before_action").filter_map do |statement| - parsed = parse_callback(statement, "before_action") + logical_statements(lines, "(?:prepend_|append_)?before_action").filter_map do |statement| + parsed = parse_callback(statement, "(?:prepend_|append_)?before_action") parsed&.first if parsed && applies_to_payload?(parsed.last, unknown: false) end end def remove_skipped_callbacks(lines, authenticated_callbacks) - logical_statements(lines, "skip_before_action").each do |statement| - parsed = parse_callback(statement, "skip_before_action") + logical_statements(lines, "skip_(?:prepend_|append_)?before_action").each do |statement| + parsed = parse_callback(statement, "skip_(?:prepend_|append_)?before_action") next unless parsed && authenticated_callbacks.include?(parsed.first) authenticated_callbacks.delete(parsed.first) if applies_to_payload?(parsed.last, unknown: true) end end🤖 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 `@react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.rb` around lines 55 - 69, Extend authenticated_callback_names and remove_skipped_callbacks to recognize prepend_before_action and append_before_action alongside the existing before_action and skip_before_action callbacks. Update the callback-name matching passed to logical_statements and parse_callback so these variants are parsed and evaluated with the same authentication and skip semantics, preventing false-positive warnings.
🤖 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.
Inline comments:
In `@CHANGELOG.md`:
- Line 140: Merge the changelog entries currently under the duplicate “Added”
heading into the existing “Added” section near the top of CHANGELOG.md, then
remove the later heading while preserving all entry content and ordering.
In `@react_on_rails/spec/react_on_rails/agent_guardrails_spec.rb`:
- Around line 162-188: The file-access guardrail specs should use deterministic
fixtures: update the unrelated file in “does not read unrelated edited files” to
contain an RSC trigger while preserving the expectation that it is ignored, and
replace chmod-based failure setup in “remains non-blocking when a matching file
cannot be read” with an injected reader or invalid file type that reliably
causes reading to fail, including under privileged users.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 40-47: Add the inline **[Pro]** tag to the RSC agent guardrails
installer changelog entry, keeping it within the existing standard category
section and leaving the rest of the entry unchanged.
In `@react_on_rails/lib/react_on_rails/agent_guardrails.rb`:
- Around line 132-137: Update valid_hook_group? and the related hook predicates
to accept hook entries represented as either Hash or String, while retaining
support for nil and arrays. Ensure any hook key access is guarded by a type
check so string hooks cannot cause errors, allowing valid string-based settings
to pass validation.
---
Nitpick comments:
In
`@react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.rb`:
- Around line 55-69: Extend authenticated_callback_names and
remove_skipped_callbacks to recognize prepend_before_action and
append_before_action alongside the existing before_action and skip_before_action
callbacks. Update the callback-name matching passed to logical_statements and
parse_callback so these variants are parsed and evaluated with the same
authentication and skip semantics, preventing false-positive warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c86568e-da58-4e0b-9d1d-55e9b25954ce
📒 Files selected for processing (6)
CHANGELOG.mdreact_on_rails/lib/generators/react_on_rails/rsc_generator.rbreact_on_rails/lib/react_on_rails/agent_guardrails.rbreact_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.rbreact_on_rails/spec/react_on_rails/agent_guardrails_spec.rbreact_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
- react_on_rails/lib/generators/react_on_rails/rsc_generator.rb
🛑 Comments failed to post (2)
CHANGELOG.md (1)
140-140: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Merge this duplicate
Addedsection into the existing one.Move Lines 142-181 beneath the
Addedheading at Line 38 and remove this heading; MD024 currently reports it as a duplicate.🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 140-140: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 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 `@CHANGELOG.md` at line 140, Merge the changelog entries currently under the duplicate “Added” heading into the existing “Added” section near the top of CHANGELOG.md, then remove the later heading while preserving all entry content and ordering.Source: Linters/SAST tools
react_on_rails/spec/react_on_rails/agent_guardrails_spec.rb (1)
162-188: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the file-access fixtures deterministic.
The unrelated file contains nothing that would expose an accidental analysis, while
chmod(0o000)is ineffective under privileged users. Write an RSC trigger into the unrelated file, and induce the matching-file failure without relying on permission bits (for example, through an injected reader or deterministic invalid file type).🤖 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 `@react_on_rails/spec/react_on_rails/agent_guardrails_spec.rb` around lines 162 - 188, The file-access guardrail specs should use deterministic fixtures: update the unrelated file in “does not read unrelated edited files” to contain an RSC trigger while preserving the expectation that it is ignored, and replace chmod-based failure setup in “remains non-blocking when a matching file cannot be read” with an injected reader or invalid file type that reliably causes reading to fail, including under privileged users.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b5b496d14
ℹ️ 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".
Review: RSC agent-safety guardrails installerOverviewThis ships an installer ( Strengths
Issues raised inline
Other observations (not blocking)
Nice test discipline overall — my main ask is confirming point 1 actually fires end-to-end in a real Claude Code session before merging, since that's the entire value proposition of the PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react_on_rails/spec/react_on_rails/agent_guardrails_spec.rb (1)
98-482: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared fixture-writing helpers to cut repeated boilerplate.
Nearly every example repeats
FileUtils.mkdir_p(File.dirname(path)); File.write(path, content)for eitherconfig/routes.rbor a controller file. Extracting small helpers would remove this duplication across ~30 examples and make each test's intent (the file content) the only thing that stands out.♻️ Example helper extraction
+ def write_routes(content) + routes_path = File.join(`@app_root`, "config/routes.rb") + FileUtils.mkdir_p(File.dirname(routes_path)) + File.write(routes_path, content) + routes_path + end + + def write_controller(content, name: "rsc_payload_controller.rb") + controller_path = File.join(`@app_root`, "app/controllers/#{name}") + FileUtils.mkdir_p(File.dirname(controller_path)) + File.write(controller_path, content) + controller_path + end + it "parses Claude hook input without requiring jq" do described_class.install(`@app_root`) - routes_path = File.join(`@app_root`, "config/routes.rb") - FileUtils.mkdir_p(File.dirname(routes_path)) - File.write(routes_path, "rsc_payload_route\n") + routes_path = write_routes("rsc_payload_route\n")🤖 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 `@react_on_rails/spec/react_on_rails/agent_guardrails_spec.rb` around lines 98 - 482, Extract shared fixture-writing helpers in the spec, such as helpers for writing routes and controller files, encapsulating directory creation plus file writing. Replace the repeated FileUtils.mkdir_p/File.write setup across the examples while preserving each fixture path and content; keep test behavior unchanged and make the example bodies focus on their fixture content.
🤖 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 `@react_on_rails/spec/react_on_rails/agent_guardrails_spec.rb`:
- Around line 98-482: Extract shared fixture-writing helpers in the spec, such
as helpers for writing routes and controller files, encapsulating directory
creation plus file writing. Replace the repeated FileUtils.mkdir_p/File.write
setup across the examples while preserving each fixture path and content; keep
test behavior unchanged and make the example bodies focus on their fixture
content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26594e4c-2ed0-40a4-8572-141d1932e86a
📒 Files selected for processing (7)
CHANGELOG.mdreact_on_rails/lib/generators/react_on_rails/rsc_generator.rbreact_on_rails/lib/react_on_rails/agent_guardrails.rbreact_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.rbreact_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_skill.mdreact_on_rails/spec/react_on_rails/agent_guardrails_spec.rbreact_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_skill.md
- CHANGELOG.md
- react_on_rails/lib/react_on_rails/agent_guardrails/templates/rsc_app_safety_check.rb
- react_on_rails/lib/react_on_rails/agent_guardrails.rb
… skill Ship an installable RSC "agent safety" guardrail into apps that use React on Rails Pro React Server Components, so AI coding agents avoid the RSC API footguns found in the security audit (#4595 / #4596 / #4597). - ReactOnRails::AgentGuardrails installer: idempotently copies an rsc-app-safety Claude Code skill + an advisory PostToolUse hook into the app's .claude/, and safe-merges the hook into .claude/settings.json (refuses to touch malformed JSON). - rake react_on_rails:install_rsc_agent_guardrails: installs/updates the assets into an existing app; safe to re-run after upgrades. - rsc-app-safety skill + app hook templates: warn about mounting rsc_payload_route without auth, trusting server-component props, exposing the node renderer, and leaking secrets/PII. - The RSC generator now runs the installer so new RSC apps get the guardrails. Specs: agent_guardrails_spec (install / idempotency / safe-merge / malformed-JSON) plus an rsc_generator_spec assertion; rubocop and shellcheck clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Scan only executable Ruby in the RSC safety hook, write settings.json atomically, and resolve the Rails app root for the rake task. - The advisory hook reconstructed each line from Ripper tokens but blanked only comment tokens, so string and heredoc bodies (:on_tstring_content) were scanned as if they were code. A `skip_before_action` mentioned inside a heredoc cancelled a real `before_action`, producing a false "no authentication" warning on a properly authenticated controller. String and heredoc bodies are now blanked alongside comments. Blanking is line-start-aware so mid-line content such as `only: "rsc_payload"` still parses as a callback scope, and heredoc bodies are tracked explicitly because Ripper reports their indentation as a separate token. The routes check now uses the same tokenized lines instead of a raw `#` strip. - settings.json is a user-owned file that may hold unrelated configuration, so it is now replaced by writing a sibling temp file and renaming over the target. An interrupted or failed write can no longer truncate or half-write it. Existing file permissions are preserved. - The rake task resolved its destination from Dir.pwd, so invoking it from a subdirectory installed guardrails into the wrong place. It now prefers the Rails application root via AgentGuardrails.default_destination_root, still honouring DESTINATION and falling back to Dir.pwd outside a Rails app. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
201e9ee to
6b8f044
Compare
|
+ci-force-full Requesting the full hosted suite for head 6b8f044. This PR rebased across #4632's rsc_generator rework, and during that work real cross-file RSpec pollution was found and fixed (a |
Force-Full Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
github-actions
Bot
added
force-full-hosted-ci
Review: RSC agent-safety guardrails installerOverviewThis PR ships Code quality
Security considerations
Other notes
No blocking issues found. This is a well-scoped, defensively-implemented feature with strong test coverage. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b8f044e1e
ℹ️ 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".
- The malformed-settings recovery message pointed users at .claude/hooks/rsc-app-safety-check.rb, but settings are validated before anything is copied, so that file does not exist when the error is raised. Following the advice registered a hook script that was never installed, leaving the RSC advisory guardrail silently disabled. The message now states that nothing was installed and directs users to fix settings.json and re-run the task. Kept the "write nothing on malformed settings" invariant rather than copying the hook before raising: it is explicitly specced, and writing guardrail files into an app whose settings could not be parsed is the more surprising behaviour. - AUTH_NAME required an exact match against a short allowlist, so real authentication callbacks were missed and properly authenticated controllers drew a false "shows no before_action/authentication locally" warning. Devise generates per-scope bang callbacks (authenticate_admin_user!, authenticate_api_user!) and app-defined callbacks follow the same convention, so any authenticate*/authorize* name ending in a bang now counts, as do require_authentication/require_authorization and verify_authenticated. Names without a bang still need the narrow allowlist, which keeps authenticate_analytics_session and authorize_for_metrics from reading as authentication. Recognizing a callback only silences the advisory, so over-broadening would trade a false positive for a false negative; verify_authenticity_token (CSRF, not authentication) is specced as a non-match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review: RSC agent-safety guardrails installerOverviewThis ships a new Overall this is a mature, carefully defensive piece of code — the settings.json merge logic validates before writing anything (so a malformed file is never touched), uses an atomic tempfile+rename write, preserves file permissions, and the Ruby-based heuristic hook (replacing an earlier bash version) has extensive edge-case coverage for comments/heredocs/string literals so it doesn't false-negative on real callbacks. Nice attention to the "silence must not be mistaken for safety" principle throughout (e.g. Most important thing to verifyLeft as an inline comment on Other findings (inline)
Minor / non-blocking
Test coverageExcellent — idempotency, skip-existing, malformed JSON, permission preservation, legacy hook migration/dedup, and dozens of callback-heuristic edge cases (heredocs, comments, scoped SecurityThe design is appropriately conservative for a security-guardrail generator: never touches a malformed |
…t-policy * origin/main: Agents: automate public demo fleet standing health (#4751) Tolerate lone UTF-16 surrogates in renderer JSON (#4710) (#4745) Agents: add fleet validation lifecycle closeout (#4747) CI: split generator specs into setup-safe subshards (#4746) Add react_on_rails:install_rsc_agent_guardrails: ship RSC agent-safety skill + hook into apps (#4606) Revert "Fix SSR crash on unpaired UTF-16 surrogates from JS JSON output (#4710) (#4726)"
1 participant
What
Ships an installable RSC "agent safety" guardrail for apps that use React on Rails Pro React Server Components. A new rake task (and the RSC generator) drops a Claude Code skill + an advisory hook into the host app's
.claude/, so AI coding agents working in that app are steered away from the RSC API footguns surfaced in the recent RSC security audit.This is the downstream half of a two-PR effort. The framework-internals half (a repo-local
rsc-guardrailsskill + payload-escaping regression test) is #4599.Why
RSC spans a trusted-server → untrusted-client boundary. The audit (issues #4595, #4596, #4597) found the framework solid but identified usage footguns an app can hit: mounting
rsc_payload_routewithout authentication, trusting server-component props, exposing the Node renderer, or leaking secrets/PII. Those live in app code, not the gem — so the guardrail has to ship into the app.Changes
react_on_rails:install_rsc_agent_guardrailsrake task — idempotently installs/updates the guardrail assets into the current app's.claude/. Safe to re-run after upgrades.ReactOnRails::AgentGuardrailsinstaller — copies the skill + hook and registers the hook in.claude/settings.jsonvia a safe JSON merge (preserves existing hooks; refuses to touch malformed JSON and tells you to add the one line manually).rsc-app-safetyskill template — the RSC usage footguns + a per-change checklist, installed as.claude/skills/rsc-app-safety/SKILL.md.PostToolUsewarning whenconfig/routes.rbmountsrsc_payload_route, or an RSC controller lacks a visiblebefore_action. Installed as.claude/hooks/rsc-app-safety-check.sh.rails g react_on_rails:rscnow runs the installer, so new RSC apps get the guardrails automatically.Verification
agent_guardrails_spec.rb: 4/4 pass — fresh install, idempotency, safe merge into an existingsettings.json, and raise-not-clobber on malformed JSON.rsc_generator_spec.rb: passes with the generator wiring.rsc_payload_route/ RSC controller; silent otherwise.shellcheckclean; matching anchored on the absolute path (robust to symlinked//varcheckouts).rubocopclean;prettier --checkclean.Deliberately deferred (fast-follow)
A dedicated docs page — the
rsc-app-safetyskill and the rake taskdescare self-documenting for now; a docs/ page would pull in the llms-full regeneration gate. Happy to add it in a follow-up if you want it in the docs site.🤖 Generated with Claude Code
Summary by CodeRabbit
react_on_rails:install_rsc_agent_guardrailsto install/update the guardrails into the app’s.claude/directory.1.