Skip to content

Wide events audit 2026-07-28 #141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fgilio wants to merge 1 commit into from
Closed

Wide events audit 2026-07-28 #141

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions reports/wide-events/2026-07-28.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# Wide Events Audit: 2026-07-28

## Summary

- Deterministic checks: pass
- Findings: 0 critical, 3 warning, 2 info
- Files reviewed: 15

## Deterministic Checks

| Command | Result | Notes |
|---|---|---|
| `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) |
Comment on lines +13 to +14

@coderabbitai coderabbitai Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.


## Findings

### [WARNING] Review-page comment writes have no canonical event

- **File:** `resources/views/pages/⚡review-page.blade.php:656`
- **Rule:** Agentic check A1 (one logging owner per externally meaningful operation), and consistency with `context.comment.written` on the context-page.
- **Evidence:** `createComment()` delegates to `ReviewCommentWorkflowAction` and applies the returned mutation via `applyCommentMutation()`. No `Log::info()` and no `Context::add()` happen in the review-page or in the workflow action. Same pattern for `updateComment` (line 677), `deleteComment` (685), `clearAllComments` (740), and `restoreComments` (752). The parallel operation on `⚡context-page.blade.php:308` does emit `context.comment.written`.
- **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).
Comment on lines +23 to +24

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include all context-side comment mutations in the audit

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 👍 / 👎.


### [WARNING] submitReview has no canonical event

- **File:** `app/Concerns/ReviewPage/ExportsReview.php:31`
- **Rule:** Agentic check A1. Submitting a review is the terminal user operation of the app.
- **Evidence:** `submitReview()` calls `ExportReviewAction::handle()`, then updates state, toasts, dispatches events, and drops the submitted comment ids. Neither the trait nor `ExportReviewAction` emits a `Log::info()`. `exportSnapshot()` on line 77 is in the same shape.
- **Impact:** The most externally meaningful user operation ("I finished a review") is unqueryable — no outcome distribution, no duration, no comment/file counts, no `excludedComments` visibility.
- **Suggested fix:** Wrap `submitReview` in the canonical owner pattern (flush, try/catch/finally, `Log::info('review.submitted')` from `finally`) with `rfa.submitted_comment_count`, `rfa.excluded_comment_count`, `rfa.file_count`, `rfa.duration_ms`, `rfa.outcome`.

### [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.
Comment on lines +34 to +40

@coderabbitai coderabbitai Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.


### [INFO] Git-exclude warnings could carry `exit_code` when the exception is a GitCommandException

- **File:** `app/Actions/EnsureRfaGitExcludeAction.php:52`
- **Rule:** Agentic check A12 (diagnostic fields should actually aid triage).
- **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.
Comment on lines +46 to +48

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Limit GitCommandException advice to resolve failures

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 👍 / 👎.


### [INFO] Service-layer warnings pass absolute `$repoPath` where a project slug could suffice

- **File:** `app/Services/GitMetadataService.php:26`
- **Rule:** Agentic check A9 (avoidable absolute paths). Falls under the standard's "prefer project slugs" guidance.
- **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.
Comment on lines +54 to +56

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep project context setup in a canonical logging owner

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 👍 / 👎.


## Clean Areas

- All seven canonical owners flush Context first and emit the `Log::info()` from `finally`, matching the pattern in the standard.
- `rfa.outcome` values seen in code (`completed`, `error`, `skipped`, `cancelled`, `rejected`, `partial`) match the vocabulary. `HandleMenuItemClicked::handleScanDirectory` correctly maps to `partial` when some repos in the scan failed.
- `rfa.duration_ms` present on every canonical event, including the updater listener where a reliable start time is not available (documented in `NativeAppServiceProvider::elapsedMs`).
- The updater failure path is the reference implementation of the listener-owned canonical-on-every-outcome rule: diagnostic `Log::error('updater.failed', [...])` plus canonical `Log::info('updater.failed')` from `finally`.
- `LogSanitizer::summary` is the only wrapper carrying externally-sourced text into a payload (updater `message` / `stack`). The C9 arch test confirms nothing else leaks raw exception text.
- Child services emit warnings without flushing Context, so child logs correlate with the owner event.
- `config/logging.php` default resolves to `daily` with 7-day retention, and the `LogChannelPostureTest` recursively expands the default stack and asserts nothing remote is active.

## Residual Risks

- This routine reviews the code as text. It does not exercise the app end-to-end or read a running log file, so any dynamic string used as an event name or reason (there are none observed) would slip past the arch scanners and this review alike.
- Dynamic Context values (`$outcome`, `$e::class`, `Context::get('rfa.reason')`) are trusted to hold sensible values. Wrong assignments inside branches are not statically enforced.
- The "missing canonical event" findings are coverage judgments. If comment/discard/submit are intentionally out of scope for wide events, the standard should say so explicitly.
- Bounded local retention (`LOG_DAILY_DAYS=7`) is a default, not a hard cap. A user overriding the env var to a large value would extend retention without changing this repo.