Wide events audit 2026-08-18

reports/wide-events/2026-08-18.md

@@ -0,0 +1,58 @@
+# Wide Events Audit: 2026-08-18
+
+## Summary
+
+- Deterministic checks: fail
+- Findings: 1 critical, 0 warning, 2 info
+- Files reviewed: 15
+
+## Deterministic Checks
+
+| Command | Result | Notes |
+|---|---|---|
+| `php artisan test --compact tests/Arch/LoggingConventionsTest.php` | fail | [CRITICAL] Could not run. `vendor/autoload.php` was absent and `composer install` was blocked by the session proxy on GitHub API authentication (see Findings). |
+
+## Findings
+
+### [CRITICAL] Deterministic arch test could not run in this session
+
+- **File:** `tests/Arch/LoggingConventionsTest.php:1`
+- **Rule:** Wide Events Audit routine → "Run deterministic checks".
+- **Evidence:** `/home/user/rfa/vendor/autoload.php` was absent and `composer install --no-interaction --prefer-dist` returned "Could not authenticate against github.com" from `AuthHelper.php:132`. The stored composer auth token in `/root/.config/composer/auth.json` was the sentinel string `proxy-injected`, which composer treats as a literal token; removing it did not help because the underlying downloads still hit `api.github.com` for dist archives. Package sync via git URLs completed for most requirements but the install step aborted before `vendor/autoload.php` and the Pest binary were written.
+- **Impact:** C1–C7 and C9–C10 were not re-verified this run. All checkable rules still passed under the same source of truth in the previous audit `reports/wide-events/2026-08-11.md`, and no logging file changed between then and this run outside the report artifacts. The routine still produced findings, but the mechanical safety net was skipped.
+- **Suggested fix:** Provide the session with a working composer auth path (either a real GitHub token in `~/.config/composer/auth.json` or a routine setup step that primes `vendor/` before the audit script runs). No production code change is required.
+
+### [INFO] Warning on `project.registration.failed` embeds the raw deep-link path
+
+- **File:** `app/Actions/OpenProjectFromPathAction.php:48`
+- **Rule:** Agent Review Checklist A9, Privacy → "Prefer project slugs, relative file paths, hashes, counts, booleans, and stable reason codes."
+- **Evidence:** The warning payload passes the incoming `$path` verbatim to `Log::warning('project.registration.failed', ['reason' => 'project_registration_failed', 'path' => $path, 'error_class' => $e::class])`. The action is called by `HandleDeepLink` and by `NativeAppServiceProvider::processInbox`, both of which supply an absolute filesystem path. The Standard permits absolute paths in warning payloads when the path itself is the failed input, which is the case here, so this is not a rule violation.
+- **Impact:** Low. `HandleDeepLink` already writes `rfa.path_hash = hash('xxh128', $path)` into the owner's `Context` before invoking the action, so triage can already correlate the warning to the owner event without keeping the raw path in the warning body. Using the same hash or `LogSanitizer::summary($path)` in the warning would trim one more absolute path from the local log without losing debug value.
+- **Suggested fix:** Replace `'path' => $path` with `'path_hash' => hash('xxh128', $path)` (or `LogSanitizer::summary($path)` if the raw shape needs to remain readable for the inbox path). The inbox path from `NativeAppServiceProvider::processInbox` is not currently hashed on any owner event, so if hashing is preferred, hashing consistently in both callers keeps the correlation key stable.
+
+### [INFO] Unexpected registration failure in `OpenRepositoryDialogAction` never emits a diagnostic warning
+
+- **File:** `app/Actions/OpenRepositoryDialogAction.php:44`
+- **Rule:** Agent Review Checklist A11, Errors And Criticals → "Use `Log::error()` only when the exception is handled or converted into a non-throwing path" and A12 (diagnostic detail).
+- **Evidence:** The `Throwable` branch adds `rfa.reason = project_registration_failed` and `rfa.error_class = $e::class` to Context, then returns null after showing a UI alert. The owner (`HandleMenuItemClicked::outcomeForNullProject`) maps that reason to `rfa.outcome = error` on `menu.item.clicked`, so the failure is queryable through the owner event. The sibling action `OpenProjectFromPathAction` also emits a diagnostic `Log::warning('project.registration.failed', ...)` on the same class of failure. This action does not.
+- **Impact:** Low. The owner canonical event carries `rfa.outcome`, `rfa.reason`, and `rfa.error_class`, which meets the SKILL's minimum for a swallowed unexpected failure. Adding a matching diagnostic warning here would only give parity with `OpenProjectFromPathAction` and slightly richer triage detail; skipping it stays inside the standard.
+- **Suggested fix:** Optional. If parity with `OpenProjectFromPathAction` is preferred, emit a `Log::warning('project.registration.failed', ['reason' => 'project_registration_failed', 'error_class' => $e::class])` inside the `Throwable` branch. Do not include the picked path — the dialog result is a private local filesystem path with no project context to relativize.
+
+## Clean Areas
+
+- Every canonical `Log::info()` has one clear owner and lives in the correct layer: `HandleMenuItemClicked` for `menu.item.clicked`, `HandleDeepLink` for `deeplink.opened`, four terminal updater closures in `NativeAppServiceProvider` for `updater.available` / `updater.current` / `updater.downloaded` / `updater.failed`, and Livewire pages for `context.comment.written` and `review.refreshed`. No child action or service duplicates a parent canonical info event.
+- `Context::flush()` runs first in every owner. Child services and actions (`OpenProjectFromPathAction`, `OpenRepositoryDialogAction`, `GitMetadataService`, `SyntaxHighlightService`, `EnsureRfaGitExcludeAction`) add `rfa.reason` / `rfa.error_class` without flushing, preserving owner correlation.
+- Every canonical event carries `rfa.outcome` and `rfa.duration_ms`. Context is rich enough to reconstruct each operation without reading code: `rfa.repos_found` / `rfa.repos_registered` / `rfa.repos_already_tracked` / `rfa.repos_failed` on scan, `rfa.file_count_before` / `_after` and `rfa.changed_count` / `rfa.changed_file_ids` on review refresh, `rfa.project_slug` / `rfa.file_id` / `rfa.is_draft` on comment writes.
+- `rfa.outcome` values follow the vocabulary and the decision tree. `deeplink.opened` uses `rejected` for `unsupported_url` and `missing_path`, `error` only for a swallowed `project_registration_failed`, and `completed` on the success path. `handleScanDirectory` correctly distinguishes `cancelled` (dialog dismissed), `partial` (some repos failed), and `completed`.
+- `updater.failed` correctly emits a sanitized diagnostic `Log::error` followed by the canonical `Log::info` from `finally`, matching the listener-owned unit-of-work pattern. `updater.current` covers the no-update success path and its own error path.
+- Warning payloads consistently pair `reason` with safe diagnostic fields (`exit_code`, `error_class`, `LogSanitizer::summary($e->stderr)`, relative `path`). No payload passes `$e->getMessage()`, `$e->getTraceAsString()`, or raw `$e->stderr`.
+- No banned absolute-path key appears in any static `Context::add()` call. Warning-payload path values are relative (`LoadFileDiffAction`, `GitMetadataService::getFileContent`) or the failed raw input (`OpenProjectFromPathAction`, allowed by the standard).
+- Storage stays local. `config/logging.php` resolves `default` to `daily` with `LOG_DAILY_DAYS` retention and `info` level. The `slack`, `papertrail`, `syslog`, `errorlog`, and Monolog `stderr` channel definitions remain inert unless env overrides pull them in. `tests/Feature/LogChannelPostureTest.php` guards this by walking the resolved default channel and any stack members.
+
+## Residual Risks
+
+- Deterministic checks were not executable this run. The rules protected by C1–C10 rely on the same file inputs the semantic review just walked, and no logging file changed since the 2026-08-11 audit's green run, but the CI safety net was not re-exercised in this session.
+- Agentic review reads static source, not runtime traffic. Dynamic `Context::add()` values (e.g. `rfa.reason` from an exception's `reason->value` enum) are only spot-checked against the vocabulary. A future reason enum entry could drift from the approved outcome set without arch tests catching it.
+- The routine does not run the app or replay JSONL diagnostics, so runtime-only regressions (e.g. a listener that never reaches `finally` because Electron kills the PHP process mid-callback) are outside its detection surface.
+- `OpenRepositoryDialogAction::handle` still passes `$e->getMessage()` into `Alert::show()` for the user-facing error dialog. This is a UI surface, not a `Log::` or `Context::` payload, so it is outside the SKILL's raw-exception ban, but the exception text still leaves the app boundary through the dialog.
+- `LoadFileDiffAction` runs per-file on lazy render and does not emit a canonical info event. Prior audits classified this as a scope judgment (per-file diff render is not an externally meaningful user operation). Left unchanged, consistent with the 2026-08-11 disposition.