Wide events audit 2026-07-28 - #141
Conversation
Automated weekly audit against .claude/skills/wide-events/SKILL.md. Deterministic checks pass; report notes coverage gaps for review-page comment writes, submitReview, and discard/restore, plus two minor diagnostic suggestions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SC2nK64wnrbaBjk2XgUC3W
📝 WalkthroughWalkthroughAdds a Markdown report for the 2026-07-28 wide-events audit, covering deterministic checks, five logging findings, clean areas, and residual risks. ChangesWide Events Audit
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@reports/wide-events/2026-07-28.md`:
- Around line 13-14: Update the audit report’s executed-check table and
clean-area section to distinguish routine deterministic commands from
supplemental or out-of-band verification. Add an owner/check matrix mapping each
of the seven owners and every posture guarantee to a specific listed command or
evidence artifact, including the default-channel verification, so all aggregate
claims are reproducible.
- Around line 34-40: Update discardFileChanges() and restoreDiscardedFile() so
canonical events cover every terminal outcome, including validation or
skipped-case returns before the try block; structure the control flow so those
outcomes emit the appropriate discarded/restored event. In each catch path,
branch GitCommandException-specific stderr and exitCode handling from generic
Throwable handling, sanitize diagnostics with LogSanitizer::summary and include
error_class, while preserving the existing danger toasts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f8948c12-bec1-48a8-8ee9-9ac47ab2e531
📒 Files selected for processing (1)
reports/wide-events/2026-07-28.md
| | `php artisan test --compact tests/Arch/LoggingConventionsTest.php` | pass | 8 tests, 8 assertions, ~59ms | | ||
| | `php artisan test --compact tests/Feature/LogChannelPostureTest.php` | pass | Default channel resolves to `daily` only (verified out of band, not part of the routine's required command) | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Separate executed checks from aggregate audit claims.
The table labels the out-of-band channel verification as a deterministic check, while the clean-area section generalizes to seven owners and multiple posture guarantees without mapping each claim to a listed command or evidence artifact. Mark supplemental checks explicitly and include an owner/check matrix so the report is reproducible.
Also applies to: 60-66
🤖 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 `@reports/wide-events/2026-07-28.md` around lines 13 - 14, Update the audit
report’s executed-check table and clean-area section to distinguish routine
deterministic commands from supplemental or out-of-band verification. Add an
owner/check matrix mapping each of the seven owners and every posture guarantee
to a specific listed command or evidence artifact, including the default-channel
verification, so all aggregate claims are reproducible.
| ### [WARNING] Discard and restore swallow exceptions to a toast with no log | ||
|
|
||
| - **File:** `app/Concerns/ReviewPage/ManagesReviewTrash.php:70` | ||
| - **Rule:** Agentic check A11 (Log::error only for swallowed unexpected failures) and A1 (canonical event on every terminal outcome). | ||
| - **Evidence:** `discardFileChanges()` catches every `Throwable`, extracts a message (`$e instanceof GitCommandException ? $e->stderr : $e->getMessage()`), routes it to a `Flux::toast(variant: 'danger', ...)`, and returns. No `Log::warning`, no `Log::error`, no canonical `Log::info`. `restoreDiscardedFile()` at line 101 is identical. Both are destructive/reversible git-mutating operations. | ||
| - **Impact:** When a user reports "discard failed", the developer has no trace — the raw stderr flashed on screen for a few seconds and vanished. The success paths also emit nothing, so total discard/restore volume is unknown. | ||
| - **Suggested fix:** Add the canonical owner pattern to both methods with a sanitized diagnostic warning on the error path (`LogSanitizer::summary($e->stderr)`, `exit_code`, `error_class`) and a canonical `review.file.discarded` / `review.file.restored` from `finally`. The toast text can stay as-is for the user, but the log payload must go through the sanitizer. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover early returns and exception types in the remediation.
discardFileChanges() returns before its try block for several terminal cases, so a finally around only the action call still misses skipped/rejected outcomes. Also, $e->stderr and exitCode are only available for GitCommandException; generic Throwable failures need a separate sanitized payload.
🤖 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 `@reports/wide-events/2026-07-28.md` around lines 34 - 40, Update
discardFileChanges() and restoreDiscardedFile() so canonical events cover every
terminal outcome, including validation or skipped-case returns before the try
block; structure the control flow so those outcomes emit the appropriate
discarded/restored event. In each catch path, branch
GitCommandException-specific stderr and exitCode handling from generic Throwable
handling, sanitize diagnostics with LogSanitizer::summary and include
error_class, while preserving the existing danger toasts.
![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: cab44e58e3
ℹ️ 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".
| - **Impact:** We can measure how often context comments are written but cannot measure the review comment surface at all. The two are the same shape of operation, and the asymmetry breaks any dashboard that groups them. | ||
| - **Suggested fix:** Introduce `review.comment.written` at the Livewire owner (or move the canonical event into `ReviewCommentWorkflowAction` for both review and context flows so ownership is uniform across surfaces). |
There was a problem hiding this comment.
The comparison treats the context comment surface as measurable, but context.comment.written only wraps createComment() in ⚡context-page.blade.php:241-309. Context-side updateComment, deleteComment, clearAllComments, and restoreComments, plus the four shared handlers in ManagesCommentReplies, perform the same persistent mutations without canonical events. Fixing only the listed review methods would therefore leave equivalent context operations invisible and the warning count understated; include these operations or explicitly narrow this finding to comment creation.
Useful? React with 👍 / 👎.
| - **Evidence:** The `git.exclude.append_failed` payload includes `reason`, `repo`, and `error_class`, but nothing that distinguishes an I/O failure (`File::append` on a read-only fs) from a git failure. The `resolveExcludePath` warning at line 76 has the same shape. Both call paths can throw `GitCommandException`, which exposes `exitCode` and `stderr`. | ||
| - **Impact:** When exclude registration fails, the triage signal is only the class name. Whether it was git or the filesystem is guesswork. | ||
| - **Suggested fix:** When `$e instanceof GitCommandException`, add `exit_code` and `stderr_summary` (via `LogSanitizer::summary`). For the generic `Throwable` branch, the current shape is already fine. |
There was a problem hiding this comment.
A GitCommandException from resolveExcludePath() is caught by that method at EnsureRfaGitExcludeAction.php:75, logged as git.exclude.resolve_failed, and converted to null, after which handle() returns at lines 28-30. It therefore cannot reach the outer git.exclude.append_failed catch, whose remaining operations are filesystem calls. The event names already distinguish git resolution from append failures, so adding a GitCommandException branch to the append warning would be unreachable; apply the extra git fields only to resolve_failed and revise this evidence and impact accordingly.
Useful? React with 👍 / 👎.
| - **Evidence:** Seven `Log::warning` sites in `GitMetadataService` (lines 26, 196, 219, 247, 271, 316, 390) all pass `'repo' => $repoPath`. Same for `GetProjectStatusAction:30`, `ResolveBranchBaseAction:84`, `EnsureRfaGitExcludeAction:54,78`. The absolute path is technically the failed input (exemption applies), but every one of these paths is reached from a caller that has a Project in hand. | ||
| - **Impact:** Absolute paths embed the user's home directory in every git-op warning. Not private per se, but noisier than needed and inconsistent with owner-event practice, which uses `rfa.project_slug`. | ||
| - **Suggested fix:** Have the Action-layer callers put `rfa.project_slug` on `Context` before invoking the service (child warnings will inherit it), and drop the redundant `'repo'` inline field. The service level does not need to know about projects. |
There was a problem hiding this comment.
Adding rfa.project_slug in arbitrary Action-layer callers can itself violate the context-lifecycle rule. For example, the public branch-explorer methods loadSnapshot() and loadMore() call LoadBranchExplorerSnapshotAction, which reaches several of these warning sites, but that boundary neither flushes Context nor emits a canonical event. Adding the slug inside the action would leave it in the long-lived renderer context for a later operation. The remediation should establish and flush a real logging owner before adding the slug, or retain a self-contained warning payload where no owner exists.
Useful? React with 👍 / 👎.
2 participants
Automated weekly wide-events logging audit.
Artifact:
reports/wide-events/2026-07-28.mdThis PR is human-gated. It contains the report only.
Generated by Claude Code
Summary by CodeRabbit