WA-DOC-020: Refresh Rails 7.2 notes with current blockers

[kitcommerce]

Summary

Updates docs/rails7-migration-patterns/rails-7-2-notes.md to reflect the current Rails 7.2 appraisal state and link to canonical active blocker issues.

Client impact

None expected.

Verification Plan

• Read updated doc for clarity.
• Click each referenced issue/guide link to confirm it resolves.

[kitcommerce]

🔒 Wave 1 Security Review — CHANGES_REQUIRED (LOW)

Reviewer: security
Verdict: CHANGES_REQUIRED
Severity: LOW

Finding

File: docs/rails7-migration-patterns/rails-7-2-notes.md (line 18)

A hardcoded local filesystem path /Users/Shared/openclaw/projects/workarea-modernization/repos/workarea discloses internal workspace layout (tooling name, project structure) in a public repository.

Suggested fix: Replace with a generic placeholder:

# Before
cd /Users/Shared/openclaw/projects/workarea-modernization/repos/workarea

# After  
cd /path/to/workarea   # or use: cd $(git rev-parse --show-toplevel)

---

Other wave 1 reviewers: architecture=PASS, simplicity=PASS, rails-conventions=PASS

[kitcommerce]

🔄 Wave 1 Fix — Dispatched

Security (LOW — CHANGES_REQUIRED)

A hardcoded local filesystem path (/Users/Shared/openclaw/projects/workarea-modernization/repos/workarea) is present in the documentation file. This discloses internal workspace layout in a public repository.

All other reviewers (architecture, simplicity, rails-conventions) PASSED.

A fix agent has been dispatched to replace the hardcoded path with a generic placeholder. Issue remains at status:changes-requested.

[kitcommerce]

Rails Security Review

Verdict: PASS_WITH_NOTES

Summary

No security issues introduced. The changes are docs + a CI diagnostic shell script + workflow adjustments.

Findings

Shell script () — PASS

• set -euo pipefail is present ✓
• All variables are quoted throughout; no injection vectors ("$log_file", "$status") ✓
• RUNNER_TEMP is a GitHub Actions-controlled env var used only as a file path argument — not interpolated unsafely ✓
• grep -Eq "(hardcoded pattern)" "$log_file" — pattern is static, file arg is quoted ✓
• The second bundle install runs in an already-checked-out CI workspace; no untrusted external input flows into it ✓

continue-on-error: true — PASS_WITH_NOTES

• Previously: continue-on-error: ${{ matrix.experimental }} — only experimental builds continued on bundle failure.
• Now: continue-on-error: true on bundle install, with a conditional hint step that exits 1 for non-experimental failures.
• Result: job failure semantics are preserved for non-experimental matrix entries (hint exits 1 → job fails). Experimental entries get a warning-only path (intentional).
• Downstream steps after hint (e.g. apt-get install) are skipped when hint exits 1, so no unintended execution occurs.
• Note: slightly more permissive than before (more steps execute before the failure is surfaced), but this is by design and the failure is still correctly propagated.

YAML expression injection — IMPROVED

• Old code: if [[ "${{ matrix.experimental }}" == "true" ]] — a GitHub expression interpolated directly into a shell run: block (injection surface if value were user-controlled).
• New code: condition moved to GitHub Actions if: expression context (if: steps.bundle.outcome != 'success' && !matrix.experimental) — safer by design ✓
• ${{ matrix.ruby-version }} appears in an echo run step, but matrix values are workflow-defined (not external/user input), so not exploitable ✓

No brakeman scope — docs/scripts only, confirmed N/A.

[kitcommerce]

Database Review

Verdict: PASS

Summary

No database changes in this PR. Confirmed: the diff touches only .github/workflows/ci.yml, docs/rails7-migration-patterns/rails-7-2-notes.md, and script/ci/bundler_lockfile_hint. No migrations, schema changes, model changes, or query modifications. No action required.

[kitcommerce]

Test Quality Review

Verdict: PASS_WITH_NOTES

Summary

No Ruby logic was changed, so rspec coverage is N/A. The new bundler_lockfile_hint shell script has no automated tests, which is acceptable for this type of CI diagnostic utility.

Findings

Documentation (rails-7-2-notes.md) — PASS

• No testable logic; doc accuracy reviewed in Wave 1 / by the author. No test gap here.

script/ci/bundler_lockfile_hint — PASS_WITH_NOTES (no blocking concern)

• The script is a 48-line CI diagnostic helper. Its only responsibility is: re-run bundler, grep output for known patterns, emit a GHA annotation, exit 1.
• Shell script testing with a framework like bats-core is not common in Ruby/Rails projects and would add tooling overhead disproportionate to the risk.
• Worst-case failure mode: annotation is missing or misleading — the job still exits 1 and fails correctly. No data integrity or security risk.
• The script is self-contained, short, and easy to verify by inspection.
• Note: If this script grows (more pattern categories, more complex logic), adding bats tests would be worthwhile. For now, a code comment explaining the scope and an integration test via manual CI trigger is sufficient.

Workflow (.github/workflows/ci.yml) — PASS

• Workflow correctness is validated by CI runs themselves. The refactoring (expression injection fix, cleaner step structure) improves maintainability without introducing logic that needs unit tests.

[kitcommerce]

Accessibility Review

Verdict: PASS

Summary

No UI or accessibility-relevant changes in this PR. The diff is limited to:

• docs/rails7-migration-patterns/rails-7-2-notes.md (documentation only)
• script/ci/bundler_lockfile_hint (CI shell script only)
• .github/workflows/ci.yml (CI configuration only)

No HTML, views, ARIA attributes, color/contrast, focus management, or interactive UI changes. Accessibility review is N/A — returning PASS.

[kitcommerce]

Performance Review

Verdict: PASS_WITH_NOTES

Summary

The CI changes introduce no meaningful performance overhead on the happy path. On the failure path, one additional bundle install run occurs (inside bundler_lockfile_hint), which is acceptable given it only runs when the build is already broken.

Findings

1. continue-on-error: true allows a few steps to run after bundle failure

• The old code used continue-on-error: ${{ matrix.experimental }}, halting non-experimental builds immediately on bundle failure.
• The new code uses continue-on-error: true so the hint step can surface a diagnostic annotation before failing.
• As a side effect, the "Install system deps" step (apt-get) runs after a bundle failure before the hint step terminates the job. This is a minor and acceptable waste on an already-failed build.

2. Double bundle install on failure (diagnostic only)

• The hint script re-runs bundle install to capture output for pattern matching. This is scoped entirely to the failure path. On successful builds, zero additional overhead is introduced.
• For a failure path, re-running bundle for diagnostic signal is a reasonable trade-off.

3. Experimental build path preserved

• Experimental bundle failures produce a ::warning:: annotation (no retry, no hint) — this is efficient and correct.

Recommendation

No blocking issues. The minor overhead of running apt-get before the hint step terminates could be addressed by reordering, but it's inconsequential. Approving as PASS_WITH_NOTES.

[kitcommerce]

Frontend Review

Verdict: PASS

Summary

No frontend changes in this PR. The diff is limited to:

• docs/rails7-migration-patterns/rails-7-2-notes.md (documentation only)
• script/ci/bundler_lockfile_hint (CI shell script only)
• .github/workflows/ci.yml (CI configuration only)

No JavaScript, CSS, view templates, or asset pipeline changes. Frontend review is N/A — returning PASS.

[kitcommerce]

Documentation Review

Verdict: PASS

Summary

The updated rails-7-2-notes.md is clear, accurate, and well-structured. The bundler_lockfile_hint script is adequately commented.

Findings

rails-7-2-notes.md

• ✅ Title updated from "forward-compatibility notes" to "appraisal notes" — accurately reflects current state (this is a pre-bundling diagnostic, not a live compatibility matrix).
• ✅ Tracking issue links at top are correct and canonical (umbrella Rails 7.2: add appraisal + run test suite under rails_7_2.gemfile #768, blocker WA-FWD: Upgrade Mongoid to support Rails 7.1/7.2 appraisals #841, repro Rails 7.2 appraisal can't bundle: mongoid requires activemodel < 7.1 (blocks bundler-audit) #839, security snapshot Rails 7.2 appraisal: update Mongoid/ODM deps to support ActiveModel 7.2 #840).
• ✅ Blocker cause is clearly stated: Mongoid 7.x constrains activemodel < 7.1, making Rails 7.2 unsatisfiable.
• ✅ Repro instructions are specific and reproducible (includes rbenv shell setup, correct env var usage).
• ✅ "Known follow-up failures" section links to concrete issues (Rails 7.2: Fix Rack::Cache uninitialized constant in cache_varies_integration_test #787, Rails 7.2 / Mongoid 8: slug_caching test failure in Catalog::ProductTest #788, Rails 7.2 / Mongoid 8: password reuse validation test failure in UserTest #789) — useful for follow-up reviewers.
• ✅ Old content that was speculative or superseded (Rails version constraint as primary blocker, alias_attribute scan, force_ssl grep) is cleanly removed.
• ⚠️ Minor: The repro block says "Ensure you're using the repo Ruby (example uses rbenv)" but doesn't mention rvm/asdf alternatives. Not blocking — rbenv is a reasonable default example.

script/ci/bundler_lockfile_hint

• ✅ Script header explains why it exists, what it does, and the relevant context (committed .bundle/config with BUNDLE_FROZEN).
• ✅ Inline comments explain the edge cases (unexpected success, non-lockfile failures).
• ✅ set -euo pipefail for safety; set +e / set -e pair around the diagnostic run is correctly handled.
• ✅ Output messages are intentionally terse/high-signal — this is noted in a comment.
• ⚠️ Minor: The heredoc MSG variable would be cleaner as two separate echo statements, but this is a style nit and doesn't affect correctness.

No blocking documentation concerns. Returning PASS.

[kitcommerce]

✅ All Review Waves Passed. Labeled merge:ready.