Wide events audit 2026-07-21 - #137
Conversation
Automated weekly wide-events logging audit artifact. Report only, human-gated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FYCbsumpVgsThCVBHepRC4
📝 WalkthroughWalkthroughAdds a Markdown audit report covering wide-events checks, logging inventories, canonicalization findings, clean areas, and residual risks. ChangesWide-events audit
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 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: 1
🧹 Nitpick comments (1)
reports/wide-events/2026-07-21.md (1)
7-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the inventory reproducible.
The report gives file and call-site counts from a “standard
rgsweep” but omits the exact command, audited revision, and scope/exclusions. Include those details so future weekly reports can verify that the counts came from the same snapshot.🤖 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-21.md` around lines 7 - 16, Update the production log inventory section of the report to include the exact ripgrep command used, the audited revision or commit, and the search scope and exclusions. Ensure these details identify the same snapshot and make the reported file and call-site counts reproducible.
🤖 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-21.md`:
- Around line 34-35: Align the deeplink.opened outcome contract across the
owner-pattern guidance and the approved vocabulary: either explicitly add
rejected to the documented vocabulary and validation checks, or map rejection
cases to an existing completed/error outcome with a stable rejection reason.
Update the relevant outcome references consistently, especially the handler
guidance and the Line 64 contract.
---
Nitpick comments:
In `@reports/wide-events/2026-07-21.md`:
- Around line 7-16: Update the production log inventory section of the report to
include the exact ripgrep command used, the audited revision or commit, and the
search scope and exclusions. Ensure these details identify the same snapshot and
make the reported file and call-site counts reproducible.
🪄 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
Run ID: 365d6342-cb13-4e40-b342-b0e7a1f1e317
📒 Files selected for processing (1)
reports/wide-events/2026-07-21.md
| - **Suggested fix:** Wrap the handler body in the owner pattern (flush context, capture `microtime(true)`, `try`/`finally`, emit `deeplink.opened` with `rfa.outcome` in `completed`/`rejected`/`error`, plus `rfa.mode`, `rfa.route`, `rfa.path_hash`, `rfa.project_slug` where resolvable). | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the proposed outcome vocabulary consistent.
The suggested deep-link event introduces rejected, while Line 64 states that the approved rfa.outcome vocabulary is only completed and error. Either extend the documented contract and its checks to include rejected, or use an existing outcome with a stable rejection reason.
🤖 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-21.md` around lines 34 - 35, Align the
deeplink.opened outcome contract across the owner-pattern guidance and the
approved vocabulary: either explicitly add rejected to the documented vocabulary
and validation checks, or map rejection cases to an existing completed/error
outcome with a stable rejection reason. Update the relevant outcome references
consistently, especially the handler guidance and the Line 64 contract.
![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: 261042d5de
ℹ️ 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".
| - `Log::warning()` calls: 12 | ||
| - `Log::error()` calls: 1 (`updater.failed` diagnostic, paired with the canonical info in `finally`) | ||
| - `Log::critical()` calls: 0 | ||
| - `Log::debug()` calls: 0 | ||
| - `Context::flush()` call sites: 4 (all owners, no child) | ||
| - `Context::add()` call sites: 21 (all keys `rfa.*`) |
There was a problem hiding this comment.
Running the report's stated standard rg sweep against this commit yields 16 Log::warning() calls and 25 Context::add() call sites, not 12 and 21 respectively. Because this report is the audit artifact, these understated totals make its production inventory inaccurate; update both figures to match the reproducible sweep.
Useful? React with 👍 / 👎.
| - **Rule:** Scope + Logging Ownership. The wide-events skill lists NativePHP listeners for deep links, menu events, and updater events as in scope, and says the listener owns the canonical event unless it delegates to a logged action. | ||
| - **Evidence:** `HandleDeepLink::handle()` parses the URL, records two breadcrumbs through `RecordRuntimeDiagnosticAction`, calls `OpenProjectFromPathAction` (which logs only on unexpected failure), then swaps the window URL. No `Log::info()` is emitted on any terminal outcome. `RecordRuntimeDiagnosticAction` writes to a separate JSONL file through `RuntimeDiagnosticsService`, not the Laravel Log channel. | ||
| - **Impact:** Deep-link opens do not appear in the canonical `rfa.outcome` distribution. Invalid scheme rejections, "not a git repo" outcomes, and successful navigations are indistinguishable from a query over the Log channel. The updater listeners in `NativeAppServiceProvider` set the precedent for how a listener-owned unit should emit even on failure. | ||
| - **Suggested fix:** Wrap the handler body in the owner pattern (flush context, capture `microtime(true)`, `try`/`finally`, emit `deeplink.opened` with `rfa.outcome` in `completed`/`rejected`/`error`, plus `rfa.mode`, `rfa.route`, `rfa.path_hash`, `rfa.project_slug` where resolvable). |
There was a problem hiding this comment.
When registration throws, OpenProjectFromPathAction.php:30-42 catches the Throwable and returns null, which is also returned for missing paths and non-git repositories. Wrapping HandleDeepLink::handle() alone therefore cannot distinguish the proposed error outcome from rejected; it would misclassify unexpected database or registration failures. The fix needs the action to return a typed outcome, rethrow, or own the relevant canonical event.
Useful? React with 👍 / 👎.
| - **Impact:** The `rfa.outcome` distribution for user-driven work is dominated by refreshes and updater callbacks, so real usage patterns and failure rates for submit, export, discard, register, and comment writes are invisible when the Log channel is the source. Some of these paths are the first place you would look when a user reports "my review didn't submit." | ||
| - **Suggested fix:** Pick the operations where the answer to "what happened to this unit of work?" is worth having in the Log channel and add the owner pattern (Action or Livewire method) with a chosen event name. `review.submitted`, `review.snapshot.exported`, `review.file.discarded`, `project.registered`, `comment.saved`, `file.reviewed` are candidates. | ||
|
|
||
| ### [INFO] `git.*` and repo-registration warnings expose the absolute repo path |
There was a problem hiding this comment.
The audit routine explicitly requires [WARNING] for avoidable absolute paths. This finding's own impact calls the exposure avoidable and its fix recommends removing or hashing the paths, so classifying it as [INFO] contradicts the rubric and understates the summary; either promote it to a warning and update the totals, or explain why the standard's no-project-context exception applies and remove the avoidability claim.
Useful? React with 👍 / 👎.
…and context comment writes (#139) * feat(logging): canonical events for deep links, menu clicks, and context comment writes Address the warning-level findings from the 2026-07-14 and 2026-07-21 wide-events audits (PRs #136/#137): - HandleDeepLink now owns a canonical deeplink.opened event emitted on every terminal outcome (completed / rejected / error) with rfa.mode, rfa.route, rfa.path_hash, and project context. - HandleMenuItemClicked now owns a canonical menu.item.clicked event with rfa.menu_id, outcome (completed / cancelled / skipped / error), and duration; sub-handlers report their outcome instead of void. - The UpdateAvailable / UpdateDownloaded listeners no longer hardcode rfa.outcome = completed in finally: a catch now flips the outcome to error (with rfa.error_class and a stable reason) before rethrowing. - Context-page comment writes own a canonical context.comment.written event (completed / rejected / skipped / error); the rejected branch keeps its diagnostic warning, whose payload keys move to snake_case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Jts3UrUJYfdJFjvqnJZy * fix(logging): truthful outcomes for swallowed picker, registration, and scan failures Address Codex review on #139: OpenProjectFromPathAction and OpenRepositoryDialogAction collapse distinct failure modes into null, which the new canonical events mislabeled. - OpenProjectFromPathAction's swallowed-Throwable branch now marks the owner Context (rfa.reason = project_registration_failed + rfa.error_class), so HandleDeepLink records error instead of rejected/not_a_project for database or filesystem failures. - OpenRepositoryDialogAction marks not_a_git_repository picks and registration failures via Context; HandleMenuItemClicked maps a null project to cancelled / rejected / error accordingly. - scan-directory now reports partial (with repos_found / registered / already_tracked / failed counts) when any registration failed, and completed only for clean scans. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Jts3UrUJYfdJFjvqnJZy * fix(logging): skipRender on no-op comment writes, cover skipped/error outcomes Address CodeRabbit nitpicks on #139: - createComment now calls skipRender() on the rejected and skipped early returns — neither touches $comments, so the render was pure waste. - Tighten the rejected-branch test to assert the warning event name and payload shape, and add coverage for the skipped (no comment produced) and error (rethrown Throwable) canonical outcomes. The suggestion to hoist the canonical-event try/catch/finally into a shared helper is intentionally skipped: the wide-events skill's owner pattern is written inline at each logging owner by design, matching softRefresh() and the updater listeners. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Jts3UrUJYfdJFjvqnJZy * refactor(logging): exception-derived rejection reasons and clearer null-project mapping Apply the fgilio-review branch audit: - OpenProjectFromPathAction now marks its own null causes via Context (path_not_found, not_a_git_repository), so the deep-link and dialog flows share one reason vocabulary instead of the deep-link relabeling not-a-repo as not_a_project (kept only as the addIf fallback). - Rename outcomeForDismissedPicker to outcomeForNullProject: it maps all three null-project outcomes, not just dismissal. - Hoist the twice-computed mode and path hash in HandleDeepLink into locals shared by Context and the diagnostic breadcrumb. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Jts3UrUJYfdJFjvqnJZy * fix(logging): report used_cached_project only when the cache resolved the project A stale cached id falls back to the picker, but is_int($cachedId) still read true, so both the canonical menu.item.clicked event and the diagnostic breadcrumb attributed picker-based opens to the cache. Track whether cache resolution actually produced the project instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Jts3UrUJYfdJFjvqnJZy --------- Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Automated weekly wide-events logging audit.
Artifact:
reports/wide-events/2026-07-21.mdThis PR is human-gated. It contains the report only.
Generated by Claude Code
Summary by CodeRabbit