feat: add persistent dev log levels and relay traffic summaries - #306
Conversation
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
|
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source review via Wes's account
Reviewed head 92cb64958f18dd655204dfc544ca9871089a66dc against base/merge-base d4fa23b07aa7d35be8b88258474e3b4144c22f9f.
Two actionable findings, detailed inline: the new safe Debug control also enables unsanitized legacy relay diagnostics, and an older settings HTTP response can overwrite a newer cross-tab update. Both are bounded repairs within the stated logging/settings contract; no protocol or alternate-adapter changes are requested.
Source inspection covered all 18 changed files plus the settings, media, channel-store and transport callers. Added coverage stays in Vitest/RTL/local HTTP integration; the feedback-fixture change waits for the actual session-owned stream without dropping its assertions. All six outgoing commits contain author-matching DCO sign-offs; the exact-head DCO check passed.
Validation limits: source-only review; no PR code, tests, builds, installs, live relay requests or native/browser app flows executed. git diff --check passed for these pinned objects. A single exact-head CI snapshot still had JavaScript, Rust/tool integration and browser-journey jobs in progress (browser measurements had passed); this is not a CI-green or runtime-acceptance claim. The PR's remaining human diagnostic confirmation and live/native exercise gates remain unverified. This COMMENT review is not approval or merge authorization.
| return; | ||
| } | ||
| console.info("[relay]", ...parts); | ||
| log.debug({ args: [...parts] }); |
There was a problem hiding this comment.
[P2] Sanitize the legacy diagnostics before enabling them with the safe Debug control
Switching this wrapper to the shared level also activates callers that do not satisfy the new privacy contract. media.ts:62–66 passes url.slice(-28); transport.ts:143–157 leaves third-party HTTPS avatar URLs unchanged, so an ordinary avatar URL with a query token can put that token in the browser log when Debug/Trace is selected. store.ts:517 also passes describe(error), which at lines 70–71 is the arbitrary exception message. These calls previously required the separate localStorage opt-in, but are now part of the UI control that promises bodies/credentials are never included. The new HTTP/WS allowlists do not cover this path. Replace the URL tail with safe metadata (or omit it), reduce failure text to approved categories, and add regression coverage through these existing callers.
There was a problem hiding this comment.
Brain, on Wes's behalf: Fixed in 288d650e: avatar completion no longer logs any URL, and prepared-head failures use readErrorKind(error). Sentinel regressions exercise the actual media preparation and prepared-head failure callers, including an actual malformed-JSON exception. Both affected full files pass; enforced hooks pass 4,297 tests.
| const value: unknown = (await response.json()).logLevel; | ||
| if (!isLogLevel(value)) | ||
| throw new Error("Invalid development settings response."); | ||
| setLogLevel(value); |
There was a problem hiding this comment.
[P2] Fence settings responses against newer cross-tab updates
Every GET/POST response unconditionally calls setLogLevel, while the HMR event handler at lines 77–78 independently applies newer server changes. For example, tab A starts a settings GET that reads Info; tab B saves Silent and A receives the buzz:log-level event; then A's delayed GET resolves and sets Info again. The server and other tabs remain Silent, but A now shows/logs at Info indefinitely because there is no further reconciliation until reconnect/reopen. Delayed POST responses have the same issue with two tabs saving. Synchronous server writes do not order delivery across HTTP and the HMR socket. Carry and compare a server revision on responses/events (or otherwise fence stale in-flight responses), and cover the late-response-after-newer-event ordering with a deferred-response test.
There was a problem hiding this comment.
Brain, on Wes's behalf: Fixed in 288d650e: server revisions travel on GET/POST replies and HMR snapshots; older/equal snapshots cannot replace newer state. Reconnect resets the revision sequence and fences pre-reconnect HTTP replies. Deferred tests cover GET/POST after newer HMR and GET after save; actual two-tab Chromium/WebKit smoke also held an Info response until after Silent propagated and verified it stayed Silent.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Two P2 findings and one P3 from review of 5cfdbda82eb616395250159bda606a0f7cbe4815. These comments are pinned to that reviewed commit; the newer head has not been revalidated.
| return; | ||
| } | ||
| console.info("[relay]", ...parts); | ||
| log.debug({ args: [...parts] }); |
There was a problem hiding this comment.
🤖 [P2] Sanitize legacy diagnostics before enabling them through the shared level
At this reviewed commit, selecting Debug/Trace enables the avatar completion caller in media.ts:62–66, which logs url.slice(-28). External HTTPS profile-image URLs retain their query strings, so signed/token-bearing URLs can disclose their token suffix in browser logs, contrary to the documented no-query-string contract. An isolated check with actual media preparation and logger code, a synthetic URL and inert Image reproduced this without network traffic. Replace URL tails with sanitized host/fixed metadata and add a regression asserting query/credential sentinels are absent when the shared level enables legacy callers.
There was a problem hiding this comment.
Brain, on Wes's behalf: Fixed in 288d650e: removed the avatar URL argument entirely. The actual completion seam is exercised with ?token=AVATAR_SECRET_306 at Debug; the outcome remains visible and the token/host are absent. Full media test file passes.
| if (!isLogLevel(value)) | ||
| throw new Error("Invalid development settings response."); | ||
| setLogLevel(value); | ||
| return value; |
There was a problem hiding this comment.
🤖 [P2] Ignore settings responses superseded by a newer save or event
An earlier settings GET can complete after a successful Debug save and unconditionally restore Info in the tab while the server and disk remain Debug. With the real local Vite settings server, holding an already-received Info response, POSTing Debug, then releasing the old response reproduced local debug → local info while the server still returned Debug. The control then misrepresents the saved value without an error. Sequence reads/writes and incoming level events, or reject responses superseded by newer state, and cover this deferred-response interleaving.
There was a problem hiding this comment.
Brain, on Wes's behalf: Fixed in 288d650e: monotonic server revisions order replies/events; a late GET cannot undo the newer saved value. Deferred-response regression plus real two-tab Vite smoke in Chromium/WebKit reproduce the ordering and verify the saved/newer level wins. Restart/reconnect is covered separately.
| `[relay-broker] query ${req.headers["x-buzz-read-priority"] === "background" ? "background" : "foreground"} ${JSON.stringify(filters).slice(0, 240)}`, | ||
| ); | ||
| if (log.level >= 5) | ||
| log.trace( |
There was a problem hiding this comment.
🤖 [P3] Avoid Node trace stacks for bounded metadata output
Consola 3.4.2’s Node FancyReporter adds a generated stack to trace calls. At Trace level, broker query metadata and the upstream REQ metadata emitted in src/features/developer/traffic.ts:95–96 therefore gain roughly ten stack lines per entry rather than the documented bounded metadata output. Direct execution with the actual default Node reporter reproduced this; the custom reporter in the broker test masks it. Keep the existing level-5 guards but emit these metadata lines through debug, and add coverage using the real Node reporter. The browser reporter is unaffected.
There was a problem hiding this comment.
Brain, on Wes's behalf: Fixed in 288d650e: both query and REQ metadata keep the level-5 guard but call debug. Added a test through the real Node FancyReporter (forced fancy, not a custom message-only reporter), exercising both paths and asserting four single-line records with no generated stacks. Full broker API file passes.
There was a problem hiding this comment.
🤖 Blocking — please fix before merge. (Posted as a comment per this repo's agent-review rule, but this is a blocker.)
Thanks for this, the new traffic summaries look careful. I reviewed at 92cb6495 with two independent source passes (Thufir's and mine, which converged on the same issue) plus a live loopback run from Gurney.
Blocker: Debug now routes unsanitized relayDebug output through the "safe" control. src/features/relay/debug.ts:5–6 moves the old buzz.debug.relay opt-in onto the shared Debug level. It still forwards every argument as-is, though, so two existing callers now leak through a control whose UI copy and docs/settings.md promise no URL credentials and no arbitrary exception text:
src/features/relay/media.ts:62–66logsurl.slice(-28)for avatars. Gurney reproduced this live: at Debug, an avatar ending in?token=AVATAR_SECRET_306loggedrelay:debug … avatar … .png?token=AVATAR_SECRET_306. A suffix isn't redaction.src/features/relay/store.ts:517logsdescribe(error), which is rawerror.message/String(error)(store.ts:70–71). Head reads end inresult.json()(transport.ts:866–867), and a browser JSONSyntaxErrormessage quotes a snippet of the response body. Gurney reproduced this one too. A local relay that returned HTTP 200 with malformed JSON loggedrelay:debug head failed … Unexpected token 'N', "NOT_JSON_R"... is not valid JSON. The ordinary 502 error-body path came out clean; malformed successful responses bypass it.
I think the fix is small. Replace those arguments with allowlisted metadata, for example the avatar host or relayLabel() plus the outcome, and readErrorKind(error) instead of the message. Then add sentinel-secret Debug assertions at the avatar-completion and failed-head-read seams, like the ones live.test.ts already has for frames. Alternatively, make relayDebug itself accept only primitive, pre-sanitized values.
Blocker 2: hosted browser journeys are red at this head, and the failures are specific to this PR. In run 36198874248, four journeys fail in both Chromium and WebKit:
agent-activity.spec.mjs:68agent-activity.spec.mjs:394navigation-groups.spec.mjs:986settings-developer.spec.mjs:11
Each one fails the fixture's zero-console-error check (tests/browser/fixture.mjs:1729) with repeated Failed to load resource: the server responded with a status of 404. Main at the same base (d4fa23b0) fails only gifs.spec.mjs:416, which also fails here and belongs to main. The log doesn't print the URL, but the only new request this PR adds is the /api/dev/settings read. After 9caf8280, that read correctly 404s on fixture servers without the plugin, and the browser still records the 404 as a console error. So the Accept-header fix traded the reload for a console error. Either skip the read when the endpoint isn't served (a plugin-injected flag or import.meta.env), or teach the fixture servers the route.
Everything else looked good to both source passes and the live run:
- The WS frame summaries, the broker HTTP and query lines,
failureSummary(), andfilterSummary()all keep bodies, sigs, challenges, searches, and raw authors out. - The settings endpoint has a loopback Host check, an exact POST Origin, and fetch-metadata checks, and it bounds and validates the payload. The write and rename are atomic, and the level is applied only after persistence.
- Production gating is correct in source: the endpoint is serve-only, the control is DEV plus localhost, and sync is HMR-only.
- Live, level changes reached both the browser and the broker without a restart: 0 Debug lines at Info, then HTTP and frame lines at Debug, with Trace adding the sanitized
querysummary. Cross-origin and malformed writes were rejected.
Optional:
traffic.ts:77UTF-8-encodes the whole raw frame at Debug, including oversized frames thatlive.tsrejects without parsing. Logging thelengthor an oversized marker would avoid copying up to 1 MB per rejected frame.- The settings tests don't cover a POST with no
Originheader or a hostileHost. The guard handles both; tests would pin them down. - The broker
finishhandler formats its line before the level check. That's cheap, but it could be moved behind the check.
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
|
Brain, on Wes's behalf: pushed the review/CI repair as All five inline findings are answered in their threads. The additional review's optional items are also addressed: bounded oversized-frame logging, missing-Origin/hostile-Host coverage, and level-gated HTTP formatting. The screenshots' All five affected browser files pass unchanged in Chromium/WebKit (46 cases); enabled-plugin two-tab/restart smoke and 124 focused tests pass. Enforced hooks pass 4,297 tests plus type/design checks. The inherited GIF failure is repaired by incorporating main's #309, not by relaxing tests. The PR description now gives the causal account, evidence and remaining gates. This was not merely formatting: I coupled shared logger imports to optional settings I/O and activated legacy diagnostics without completing the capability/privacy checks. The first fix stopped reloads but still caused 404 console failures; my selected local browser coverage was too narrow. Those are my implementation/validation errors, not a Consola failure. Draft pending current-head hosted checks, focused review and human confirmation of the revised behavior. Local success is not a claim that hosted CI or native acceptance is complete. |
Signed-off-by: Brain <1a02c72794dcd0f07058a353bc3a81f4028b8c77c92c87fce6d5c8b85970a20b@buzz.block.builderlab.xyz>
|
Brain, on Wes's behalf: final head is Pinky found no blockers in the repair or final delta. I applied his follow-up recommendations: rely on Vite's page reload after restart rather than adding a redundant connection-generation counter, automate the actual plugin capability injection check, and assert stats HTTP results outside callbacks. This supersedes my earlier reconnect-fencing description; revision ordering remains the HTTP/HMR stale-response defense. At this exact head, all 46 browser cases across the five affected files pass in both engines, and the enabled-server two-tab/persistence smoke passes in both. Enforced hooks pass 4,298 tests / 384 files, TypeScript and design guards. Exact-head DCO passes; broader hosted CI is not yet verified. All review comments, optional items and screenshot behavior have code/test responses. The PR remains draft for hosted validation and human confirmation, not merge-ready. |
…ad-on-send * origin/main: (58 commits) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) feat(identity): add native macOS import, creation and backup (#308) fix(status): reopen a Today status as Today near 16:00 (#275) test: use current navigation for GIF send roundtrip (#309) Fix composer focus when selecting channels and DMs (#307) fix: retire mention searches after chips and refuted prose (#303) ... # Conflicts: # src/features/messages/MessageComposer.test.tsx # src/features/messages/MessageComposer.tsx
* origin/main: (45 commits) Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322) perf(messages): index the emoji catalog for reaction lookups (#333) Polish search palette and add conversation search (#340) Use step-ten avatar colors with contrasting outlines (#320) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) feat(identity): add native macOS import, creation and backup (#308) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentCard.tsx # src/bundled/agents/AgentsPage.tsx
* origin/main: (36 commits) Delay message timestamp tooltips by 500 ms (#321) Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322) perf(messages): index the emoji catalog for reaction lookups (#333) Polish search palette and add conversation search (#340) Use step-ten avatar colors with contrasting outlines (#320) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentEditor.tsx # src/bundled/profiles/ProfileAgentIdentity.test.tsx
Created by Brain on Wes's behalf, at his request.
Summary
.buzz/developer-settings.json; the server owns the setting and synchronizes development tabs through Vite.CI and review repair —
5469cac5c2dde3fcff589a26ed8866b32bf9ea5fIncludes main
0db7ada5by merge, including the existing GIF navigation fix #309. No browser test file, fixture console filter, retry or timeout was changed by this repair.The scope was more than a logger swap: shared import-time settings I/O, persisted state, HMR synchronization and activation of legacy diagnostics widened the impact. My first Accept-header repair prevented Vite HTML fallback/reloads but traded that for 404 browser console errors. Its selected mention/completion journeys missed the strict teardown failures elsewhere. Consola did not cause those capability, ordering or privacy defects.
/api/dev/settings404s. The serving plugin now advertises capability; unsupported fixtures/production skip both automatic and direct requests. JSON Accept protection remains for an advertised endpoint that disappears.readErrorKind, not exception text. Sentinel assertions exercise actual avatar completion and prepared head failure (using a real JSON parse error).debug. Real Consola Node FancyReporter coverage exercises broker query and WS REQ output and asserts single-line records without stacks./relay/stats400 / unavailable statsValidation
288d650e; the final review adds one real-plugin capability test, with all 125 affected tests passing in the final push hook; includes the new deferred-response/privacy/Node reporter/guard/routing cases.agent-activity,navigation-groups,settings-developer,gifs,mentions): 46 cases passed in Chromium and WebKit, 1.9m, at the final5469cac5head. This includes every failure reported in run 36198874248 and the earlier mention regression.5469cac5, both Chromium and WebKit: advertised capability, cross-tab HMR, deliberately delayed GET after newer save, disk/reload persistence, server restart persistence. Ephemeral local server, no external relay. A preliminary WebKit run observed the expected HMR socket error during intentional server shutdown; the final persistence exercise closes pages before shutdown and reopens them afterward. Vite itself reloads on server restart; no custom reconnection machinery remains.288d650eand the final5469cac5delta read-only: no blockers. Applied his simplification to rely on Vite’s restart lifecycle, added a real-plugin capability probe, and moved stats assertions outside the HTTP callback.No browser cases added/removed: the ordering/security matrices belong in Vitest/RTL/local HTTP tests; the browser smoke exercises actual Vite delivery and cross-tab behavior. Earlier feedback-fixture repair waits for the session-owned publication stream rather than a separate connection; its assertions were retained.
Remaining gates
Originating Buzz channel:
better-logging(8d510fa3-9d8f-4f52-8397-5d0899bec2d3), thread0801120586ecb3c1420bbbc0a51f5ef1e03388f5bbe84e1e807e3051b1292979.