Skip to content

Wide events audit 2026-07-14 #136

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-14 #136

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
70 changes: 70 additions & 0 deletions reports/wide-events/2026-07-14.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# Wide Events Audit: 2026-07-14

## Summary

- Deterministic checks: fail (environment could not install composer dependencies)

@coderabbitai coderabbitai Bot Jul 14, 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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE='reports/wide-events/2026-07-14.md'

echo '--- line count ---'
wc -l "$FILE"

echo '--- around lines 1-90 ---'
sed -n '1,90p' "$FILE" | cat -n

echo '--- grep for deterministic/assertions/warning payload/storage ---'
rg -n 'deterministic|assertions|warning payload|storage|retention|local' "$FILE"

Repository: fgilio/rfa

Length of output: 12359

🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE='reports/wide-events/2026-07-14.md'
sed -n '1,90p' "$FILE" | nl -ba

Repository: fgilio/rfa

Length of output: 188

Scope the clean findings more narrowly. The summary already says the deterministic checks were blocked by Composer auth, so “Every warning payload…” and “Storage stays local and bounded” should read as inventory-based observations, not runtime guarantees, until the missing test and dynamic-path coverage runs.

🤖 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-14.md` at line 5, Update the report’s “Every
warning payload…” and “Storage stays local and bounded” findings to explicitly
describe inventory-based observations rather than runtime guarantees, while
preserving the existing note that deterministic checks were blocked by Composer
authentication.

- Findings: 1 critical, 2 warning, 2 info
- Files reviewed: 10 production PHP/Blade files carrying `Log::` or `Context::` calls

## Deterministic Checks

| Command | Result | Notes |
|---|---|---|
| `php artisan test --compact tests/Arch/LoggingConventionsTest.php` | fail | `[CRITICAL]` Composer install could not authenticate against github.com in this session, so `vendor/laravel/framework/` (and several other packages) resolved to empty directories. `php artisan` bails with "Class Illuminate\\Foundation\\Application not found" before Pest runs. See finding CR-1. |

## Findings

### [CRITICAL] Deterministic checks did not run

- **File:** `tests/Arch/LoggingConventionsTest.php`
- **Rule:** Skill flow step 3, "Run deterministic checks"
- **Evidence:** `composer install` failed with "Could not authenticate against github.com" while syncing multiple packages. `composer dump-autoload --no-scripts` regenerated a 177-class map, but every downstream `artisan` invocation still fatals on the missing framework classes.
- **Impact:** The C1 through C9 rules covered by `LoggingConventionsTest.php` (and C8 in `LogChannelPostureTest.php`) were not exercised this run, so mechanically-checkable regressions in outcome vocabulary, static context keys, reason payloads, or raw exception text would not have been caught. The agentic pass below is a partial substitute.
- **Suggested fix:** Rerun this routine in an environment where `composer install` can reach GitHub, or seed a warm `vendor/` before invoking. No production code change is implied.

### [WARNING] Updater success listeners hardcode `rfa.outcome = 'completed'` in `finally`

- **File:** `app/Providers/NativeAppServiceProvider.php:113`, `app/Providers/NativeAppServiceProvider.php:154`
- **Rule:** Outcome Semantics (agentic check) and A5 in the skill's Agent Review Checklist
- **Evidence:** `Event::listen(UpdateAvailable::class, ...)` and `Event::listen(UpdateDownloaded::class, ...)` open a `try { ... } finally { ... }` with no `catch`. The `finally` block writes `Context::add('rfa.outcome', 'completed')` unconditionally, then emits the canonical `Log::info('updater.available')` / `Log::info('updater.downloaded')`. If the `try` body throws (for example, `Cache::put` or `Notification::new()->show()` fails), the canonical event records `completed` while the exception propagates to the framework reporter.
- **Impact:** The success-path outcome distribution stops being trustworthy for these two listener-owned units. Compare with `resources/views/pages/⚡review-page.blade.php:531`, which tracks a `$outcome` variable, flips it to `'error'` in the `catch`, and reads it back in the `finally` — the correct shape per the skill's canonical example.
- **Suggested fix:** Adopt the review-page pattern in both success listeners: initialize `$outcome = 'completed'`, add a `catch (Throwable $e)` that sets `$outcome = 'error'` and records `rfa.error_class` / `rfa.reason` before rethrowing, and read `$outcome` in the `finally`. That keeps the failure path on the same canonical event name (rule A13) but with truthful outcome semantics.

### [WARNING] `context.comment.rejected` has no companion owner canonical event

- **File:** `resources/views/pages/⚡context-page.blade.php:256`
- **Rule:** Logging Ownership (agentic check) plus A1/A5 in the Agent Review Checklist and the skill's guidance that "Do not warn for every normal rejected input. Use the owner canonical event with `rfa.outcome = rejected`."
- **Evidence:** `createComment()` catches `ContextCommentRejectedException` and emits `Log::warning('context.comment.rejected', ['reason' => ..., 'fileId' => ..., 'side' => ..., 'startLine' => ..., 'endLine' => ...])`, then early-returns. The happy path and other-error paths emit no canonical `Log::info()`. The Livewire component is the boundary for this operation (no downstream action owns it), so per the ownership table it is the intended owner.
- **Impact:** The comment-write unit-of-work has no queryable outcome distribution. A rejected write shows up as a warning; a completed write shows up as nothing. Warnings are also expected to represent degraded behavior, but here they carry both "renderer-state drift" (unexpected) and normal-rejection semantics — the two are indistinguishable without reading `$e->reason->value`.
- **Suggested fix:** Wrap the whole `createComment()` body in the canonical pattern (`Context::flush()` → `try / catch / finally` → `Log::info('context.comment.written')` or similar), setting `rfa.outcome` to `rejected` on the caught path, `completed` on the happy path, and `error` on any other Throwable. Keep the diagnostic warning inside the rejected branch only if the underlying `$e->reason` is genuinely unexpected (e.g. `stale_snapshot`) rather than a normal user retry.

### [INFO] Many git-facing warnings pass absolute `repo` paths

- **File:** `app/Services/GitMetadataService.php:29`, `app/Services/GitMetadataService.php:199`, `app/Services/GitMetadataService.php:221`, `app/Services/GitMetadataService.php:249`, `app/Services/GitMetadataService.php:273`, `app/Services/GitMetadataService.php:318`, `app/Services/GitMetadataService.php:392`, `app/Actions/EnsureRfaGitExcludeAction.php:52`, `app/Actions/EnsureRfaGitExcludeAction.php:76`, `app/Actions/GetProjectStatusAction.php:28`, `app/Actions/ResolveBranchBaseAction.php:82`
- **Rule:** Privacy (agentic check). The skill allows absolute paths in warnings "only when no project context can resolve a relative path or when the path itself is the failed input being diagnosed."
- **Evidence:** Every listed warning carries `'repo' => $repoPath` where `$repoPath` is a raw absolute filesystem path passed in from the caller.
- **Impact:** These are the failed input for the git commands, so the current form is defensible. It still spreads user-home-anchored paths across every warning and duplicates information the caller could have summarized (project slug plus a stable reason code carry the same triage signal without leaking `/Users/<name>/...`). It is not a privacy violation today, but the surface area is wide.
- **Suggested fix:** Where the caller has a `Project` in scope (all three actions do, and `EnsureRfaGitExcludeAction` is called by three export actions that hold one), consider funneling the slug in as an extra warning field or via owner Context. Leave the absolute path in place when the callers really are path-only entry points (git plumbing invoked directly from a listener).

### [INFO] `updater.available` / `updater.downloaded` context could carry more debug fields

- **File:** `app/Providers/NativeAppServiceProvider.php:113`, `app/Providers/NativeAppServiceProvider.php:154`
- **Rule:** Canonical Event Fields (agentic check) — required fields (`rfa.outcome`, `rfa.duration_ms`) are present; extra context would materially improve debugging.
- **Evidence:** The two listeners record `rfa.update_version`, `rfa.outcome`, and `rfa.duration_ms`. They do not record whether release notes were normalized as array or string, the cache TTL bucket (`downloading` vs `ready`), or whether the notification actually surfaced.
- **Impact:** When a user reports "the update banner never appeared", the current event tells us the callback ran and finished, but not which arm of `normalizeReleaseNotes` was hit or whether the notification pipeline swallowed the payload. This is a soft observability gap, not a correctness bug.
- **Suggested fix:** Add `rfa.release_notes_format` ("array" | "string" | "none"), `rfa.notification_sent` (bool), and `rfa.cache_state` ("downloading" | "ready" | "error") to the finally block, alongside the existing fields.

## Clean Areas

- `resources/views/pages/⚡review-page.blade.php:526` — `softRefresh()` follows the canonical pattern exactly: `Context::flush()` up front, `$outcome` tracked through `try / catch / finally`, generous IDs and counts (project id/slug, target key, `is_since_base_view`, before/after file counts, `changed_file_ids` truncated with a boolean flag) and `rfa.duration_ms` on every terminal outcome.
- `app/Providers/NativeAppServiceProvider.php:161` — the `UpdateError` listener emits both a diagnostic `Log::error('updater.failed', ...)` with sanitized `message` and `stack` fields, and a canonical `Log::info('updater.failed')` from `finally` with `rfa.outcome = 'error'`. This is the exact pattern required by A13.
- All warning and error payloads across the audited files route raw exception text through `LogSanitizer::summary()` (e.g. `GetProjectStatusAction.php:32`, `LoadFileDiffAction.php:56`) or restrict themselves to safe fields (`exit_code`, `error_class`, `path`). No bare `$e->getMessage()`, `$e->getTraceAsString()`, or `$e->stderr` reached a payload value in the reviewed files.
- Every warning payload includes a stable `reason` code (`diff_process_failed`, `commit_range_list_failed`, `exclude_append_failed`, and so on) — C5 in the checklist.
- `config/logging.php:25` resolves the default channel to `daily`, whose `driver` is `daily` with `days` bounded by `LOG_DAILY_DAYS` (7). The remote channels (`slack`, `papertrail`, `syslog`, `errorlog`, `stderr`) exist as Laravel scaffolding but are never referenced by the default channel or the default stack expansion. Storage stays local and bounded.

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 14, 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 Audit env overrides before clearing storage

This clean-area conclusion only checks the fallback in config/logging.php, but normal setup copies .env.example into .env (composer.json post-root and README), and .env.example:18-21 sets LOG_CHANNEL=stack, LOG_STACK=single, and LOG_LEVEL=debug. In that environment config('logging.default') resolves to stack/single, not bounded daily, so the audit masks the wide-events Storage issue it is supposed to surface.

Useful? React with 👍 / 👎.


## Residual Risks

- The deterministic CI test could not run in this session (see the [CRITICAL] finding). Anything the file scanners in `LoggingConventionsTest.php` and the resolved-config test in `LogChannelPostureTest.php` would have caught is not verified this run. Reruns should confirm nothing regressed silently.
- The agentic review reads the audit surface via `rg` inventory and file-by-file reads. Call chains that only meet at runtime (dynamic dispatch, indirect `app()->call` targets, deferred listeners) are not fully traced. If a caller emits a canonical event we did not read, some of the ownership commentary above may be over-broad.
- `RecordRuntimeDiagnosticAction` writes to a separate JSONL breadcrumb file rather than the Laravel `Log` facade, so it is intentionally out of scope for the wide-events rules. That leaves high-frequency flows (diff load, browser samples) with observability that does not surface on the same axis as the canonical events. Any future finding about "the canonical stream is missing X" should first confirm whether X is already covered by the breadcrumb stream.
- The privacy [INFO] on absolute repo paths is a preference, not a violation of the current skill. Acting on it needs a call-site pass, not a mechanical rewrite.