Repository navigation
Conversation
|
@sfl review |
![hemsoft-sfl-reviewer[bot]](https://avatars.githubusercontent.com/in/5406096?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Verdict: NEEDS WORK • Round 1
Confidence: 4/10 · Project risk: 4/10 (hs-buddy)
Review done by claude-opus-5-5 (adversarial to codex)
The PR makes explicit native PR-list opens send one best-effort buddyStats:batchIncrement with Electron's counter mapping, and adds unit, headless and installed-fixture checks. Not ready: the counting open_pr_list call now sits in the sidebar Destination.restore, and the shell keeps that object as a workspace tab. Selecting the retained tab later can therefore record another open, which breaks the PR's own 'retained-tab restoration does not write' guarantee.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 4m 4s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 1 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 1 | 0 |
Key Findings
- 🟡 MEDIUM — Accuracy: Sidebar destinations kept as workspace tabs record activity when the tab is restored.
rust/crates/buddy-app/src/github_view.rs:1130: Observed in the source:sidebar()now setsd.restoreto callview.open_pr_list(mode, cx), and the shell sidebar click inapp.rsrunsthis.tabs.open(d.clone())and thenrestore_tab. That stores this countingDestinationintabs.itemsunder the samegithub:{account}:{route}key thatdestination()uses. Only the GitHub observer swaps it for the non-countingdestination()version (whose restore usesnavigate), and only after a latercx.notify()while the GitHub section is active.navigatereturns early when the route is unchanged, andloadnotifies only when its async load finishes. Trigger (traced statically): with My PRs loaded, click My PRs in the sidebar again. That counts once, as intended, and no notify follows. Then select Dashboard or Ctrl+Tab away, and select the GitHub My PRs tab again.restore_tabcalls the stored(destination.restore), which runsopen_pr_list, which spawns anothertabsOpened+prsViewedincrement. The same applies to any mode tab whose load was aborted because the user moved on before it finished, and to Back/Forward and tab-close restores. Impact: Convex lifetime counters are overcounted, permanently and with no correction path. This contradicts the acceptance criterion and parity doc ('retained-tab restoration do not count as opens') and theshell/navigation.rsinvariant 'Destinations restore selection, never perform writes'. The new headless test calls onlydestination().restore, never a sidebar destination's restore, and the installed fixture's retained-tab step selects the startupdestination()tab, so neither covers this path. A fix: record in the click listener beforetabs.open, or keeprestoreread-only and have the click path call a separate counting entry point.
Coverage
- Files reviewed:
CHANGELOG.mddocs/GPUI-DASHBOARD-PARITY.mdpackage.jsonrust/crates/buddy-app/src/github_view.rsrust/crates/buddy-app/src/github_view/tests.rsrust/crates/buddy-core/src/stats.rsrust/crates/buddy-core/src/stats/pr_list_activity.rsscripts/native-smoke/github-desktop.pyscripts/native-smoke/test_github_desktop.py
- Context read:
rust/crates/buddy-app/src/shell/navigation.rsrust/crates/buddy-app/src/app.rsrust/crates/buddy-app/src/shell/tab_bar.rsrust/crates/buddy-core/src/terminal_backend.rsconvex/buddyStats.tsshared/utils/statsMutationUtils.tssrc/hooks/useAppSessionStats.ts
- Verified:
gh api userreturnedHemSoftunder the per-process personal profile- Compare
base_commit.shaisc0b1ecdf471f8c56cb88ad367b6816ff9ef43131and the last commit is2c28e2ad4d6d17a4db1490a96f499f8412109a57(ahead 2, behind 0) - Counter mapping matches Electron
trackViewOpen(pr-my-prs→prsViewed,pr-needs-review→prsReviewed,pr-recently-merged→prsMergedWatched,tabsOpenedalways) - All sent fields are in
COUNTER_FIELDSandbatchIncrementtakes{fields}as one mutation TerminalBackend::call(..., true)sends a mutation, maps timeouts toUnknownWriteand never retries;record_withcalls the writer once- Sidebar click path:
render_navigationrunstabs.open(d.clone()), thenrestore_tabruns(destination.restore) - GitHub observer replaces the tab only on
cx.notify()whileactive_section == GitHub;navigatereturns early on the same route andloadnotifies only asynchronously - Drive-1 fixture sequence (Needs Review, Recently Merged, Needs a nudge, My PRs ×2) matches the expected deltas 5/2/1/1, and drive 2 adds +1 tab and +1 reviewed
- Headless test:
PrMode::ALLchained withNeedANudgegives the 5 recorded opens it asserts
- Not verified:
- Runtime and installed-fixture behavior: execution is forbidden
- That plain JSON numbers decode to Convex
Float64rather thanInt64: inferred from the existing$integerround-trip test, not read from theconvexcrate source - Exact gpui observer and effect-flush ordering: inferred from the observer code, not from gpui-kit source
- CI status for this head and the PR's pending rendered evidence
docs/GPUI-REWRITE-PLAN.mdslice wording, which was not read
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head 2c28e2ad4d6d17a4db1490a96f499f8412109a57 • SFL v0
|
R1's medium finding is fixed in 0d5dca4. The shell now invokes a separate A regression retains the actual sidebar destination without observer replacement and exercises repeated clicks, tab selection, Back/Forward and tab-close restoration. Restoring the old counting callback makes that test fail; the fixed callback passes. All 323 app tests, 14 integration tests, warnings-denied Clippy, focused Python tests and normal commit hooks pass. Installed qualification now repeats a loaded mode and restores it after switching away; its exact-delta and rendered evidence gates remain pending on this new head. |
|
@sfl review |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 2
Confidence: 8/10 · Project risk: 4/10 (hs-buddy)
Since round 1
New: 0; still open (repeated): 0; resolved: 1.
Resolved means no matching finding in this review, not independent proof of a fix.
New (0)
- None.
Still open (repeated) (0)
- None.
Resolved (1)
- MEDIUM Sidebar destinations kept as workspace tabs record activity when the tab is restored at
rust/crates/buddy-app/src/github_view.rs:1130
Review done by claude-opus-5-5 (adversarial to codex)
Explicit native PR-list opens (sidebar clicks, in-view mode buttons when shell navigation is off, and the status-bar PRs click) now send one best-effort buddyStats:batchIncrement that matches Electron's trackViewOpen mapping. The round-1 Medium is fixed: the counting effect now lives in SidebarLink.on_open, which the shell runs once per click, and only the read-only Destination (whose restore calls navigate) is stored in tabs or history. I found no Critical, High, Medium or Low defect in the reviewed code.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 4m 35s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
CHANGELOG.mddocs/GPUI-DASHBOARD-PARITY.mdpackage.jsonrust/crates/buddy-app/src/app.rsrust/crates/buddy-app/src/github_view.rsrust/crates/buddy-app/src/github_view/tests.rsrust/crates/buddy-app/src/shell/navigation.rsrust/crates/buddy-core/src/stats.rsrust/crates/buddy-core/src/stats/pr_list_activity.rsscripts/native-smoke/github-desktop.pyscripts/native-smoke/test_github_desktop.py
- Context read:
rust/crates/buddy-core/src/terminal_backend.rsrust/crates/buddy-core/src/convex_data.rsrust/crates/buddy-app/src/runtime.rsrust/crates/buddy-app/src/shell/status_bar.rsrust/crates/buddy-app/src/dashboard/active_prs.rssrc/hooks/useAppSessionStats.tssrc/hooks/useAppTabs.tssrc/App.tsxconvex/buddyStats.tsscripts/native-smoke/settings-desktop.pyPR #938 round-1 review
- Verified:
- Compare
base_commit.shaisc0b1ecdf471f8c56cb88ad367b6816ff9ef43131, the last commit is0d5dca4446c1e19a03fcb89dddce36b8bb3eb107, status ahead 3 / behind 0, and the PR head and base match - The GitHub CLI profile resolves to HemSoft before any private read
- Round-1 finding fixed:
sidebar()d.restorecallsnavigate, not the counting path, andSidebarLink::openreturns only a clonedDestinationfortabs.open - The
observe_navigationGitHub observer replaces tabs with the non-countingdestination(), so no stored tab can record an open - Electron
useAppTabs.openTabcallsonViewOpenon every open, which matches native's repeated-open counting; theprStatMapfields matcharguments()and Needs a Nudge adds onlytabsOpened buddyStats:batchIncrementtakes{fields}, and every sent field is inCOUNTER_FIELDScall(..., true)maps a timeout or ack error toUnknownWrite;record_withcalls once with no retryrefresh/load,reconcile(which sets the route directly) andrestoreclosures never reachrecord_pr_list_open- The status-bar click test runs in a child process with
BUDDY_CONVEX_URL=fixture://..., whichTerminalBackend::newrejects, so the test makes no network write - The GitHub view test
setup()installs noRuntimeglobal, so the test comment about a tracked restore panicking holds - The smoke script's expected deltas (5/2/1/1, then 7/2/3/1) match the listed clicks, and
Driver.drivesupports theworkspace,wait_contentandrecord=options Mockis imported intest_github_desktop.py
- Compare
- Not verified:
- Compilation, test results and native package smoke: Rust/native CI jobs (ubuntu, windows, macOS, Package smoke) were still in progress at review time
- Whether the restarted smoke process restores a
My PRsworkspace tab for theworkspace My PRsstep depends on runtime tab-label matching that I did not trace - Whether Electron's status-bar PRs action opens
pr-my-prsthroughopenTab; I did not trace it - Whether the generated smoke config always contains
ui.dashboardCards; I did not check the config schema defaults
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head 0d5dca4446c1e19a03fcb89dddce36b8bb3eb107 • SFL v0
|
Ubuntu installed qualification failed on the second explicit My PRs click because the selected sidebar control and content heading both matched. Captured OCR from the actual failed job reproduces the same Smoke-only fix c3a232e gives PR-mode clicks an explicit sidebar scope, left of the measured workspace Back control. The original content ambiguity check remains strict. Regression coverage rejects missing/duplicate sidebar controls and boundaries, exercises platform/scale geometry, checks Windows action encoding and preserves Mac's two stable measurements. All 36 focused tests and the 561-test smoke suite with eight platform skips pass, as do normal commit hooks. No production Rust changed after the Round 2 PASS. Hosted installed evidence must be regenerated on this head before merge. A fresh review is requested for the exact head, including the smoke-helper changes. |
|
@sfl review |
|
The late Windows failure belongs to superseded head The failed Windows job observed four opens instead of five. Its I replayed the current |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 3
Confidence: 8/10 · Project risk: 4/10 (hs-buddy)
Since round 2
New: 0; still open (repeated): 0; resolved: 0.
Resolved means no matching finding in this review, not independent proof of a fix.
New (0)
- None.
Still open (repeated) (0)
- None.
Resolved (0)
- None.
Review done by claude-opus-5-5 (adversarial to codex)
The PR records explicit native PR-list opens (sidebar, in-view mode buttons, status bar) as one best-effort buddyStats:batchIncrement mutation using Electron's counter mapping. A new SidebarLink wrapper keeps the click effect out of the retained tab and history Destinations. The installed smoke now uses a sidebar-scoped OCR target. Static reading at the exact head found no Critical, High or Medium defect, and the counter mapping, write path and restore paths match what the docs claim.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 2m 15s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
CHANGELOG.mddocs/GPUI-DASHBOARD-PARITY.mdpackage.jsonrust/crates/buddy-app/src/app.rsrust/crates/buddy-app/src/github_view.rsrust/crates/buddy-app/src/github_view/tests.rsrust/crates/buddy-app/src/shell/navigation.rsrust/crates/buddy-core/src/stats.rsrust/crates/buddy-core/src/stats/pr_list_activity.rsscripts/native-smoke/github-desktop.pyscripts/native-smoke/settings-desktop.pyscripts/native-smoke/settings-ocr.pyscripts/native-smoke/test_github_desktop.pyscripts/native-smoke/windows-actions.ps1
- Context read:
rust/crates/buddy-core/src/terminal_backend.rsconvex/buddyStats.tssrc/hooks/useAppSessionStats.tsrust/crates/buddy-app/src/shell/status_bar.rs
- Verified:
- Compare
base_commit.shaisc0b1ecdf471f8c56cb88ad367b6816ff9ef43131, the last commit isc3a232e6be077d9032d395939f1f29dc1de95e83, ahead 4 and behind 0 gh api userreturned HemSoft under the per-processGH_CONFIG_DIRprofile- The counter mapping matches Electron
trackViewOpen:tabsOpenedalways, plusprsViewed,prsReviewedorprsMergedWatchedfor the three modes, and none for Needs a nudge - Every field sent is in Convex
COUNTER_FIELDS, andbatchIncrementtakes{fields}and applies it in one mutation TerminalBackend::callwithwrite=truesends a single mutation under a timeout and maps the timeout toUnknownWritewith no retry, so 'never replayed' holdsrecord_pr_list_openhas exactly three callers:show_status_prs(status-bar click), the in-view mode button andSidebarLink::on_open. Refresh, reconcile andDestination::restorecall onlynavigateSidebarLink::openchecksavailablebefore runningon_open, and the app's sidebar click puts only theDestinationclone intotabs- Automation sidebar links wrap through
From<Destination>with a no-opon_open - The smoke's expected deltas (5/2/1/1, then 7/2/3/1) agree with the scripted sidebar clicks and the restore-only workspace-tab actions
ui.dashboardCardsexists in the base fixture config (settings-desktop.py reads and writes it), so settingworkspace-pulseon it is not a new KeyError- The
sidebarkind reaches the Windowssidebar64→--sidebarpath, the Unix label receipt andlocateoptions, and the macOS repeat-measurement loop
- Compare
- Not verified:
- Runtime behavior, the compiled result and CI status, because execution is forbidden
- Installed-fixture evidence: Workspace Pulse rendering and exact persisted deltas (the PR marks these pending)
- Whether a restored PR-detail page shows a second
Backlabel that would make the sidebar boundary ambiguous at the start ofactivity-restored-dark. That case would fail closed, but the fixture state was not traced - That convex-rs converts the JSON integer
1toFloat64; this rests on the existing write paths using the same conversion - The 958-line OCR fixture JSON was not read word by word
- PR-body claims about the Ubuntu OCR replay and local test counts
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head c3a232e6be077d9032d395939f1f29dc1de95e83 • SFL v0
|
The current-head Ubuntu failure occurred before desktop qualification. Its package container could not resolve I attempted to rerun only failed job 130549177380 in run 43027883073. GitHub refused with HTTP 403: The same worker retains the claim and PR/CI ownership. Next action is to re-read the completed run and retry the failed Ubuntu portion at unchanged head |
|
The current-head Windows capture exposed a different locator failure: native OCR omits the disabled Back control on the initial My PRs page. That made the sidebar boundary unavailable before the first mode click. Smoke-only correction 727aa4c anchors the sidebar to the enabled Repositories control instead. The captured 72-word Windows OCR case fails before the change and passes after it. All five earlier Windows mode-click images replay to the correct sidebar controls. Missing or competing Repositories boundaries still fail closed. All 37 focused tests, the 562-test smoke suite with eight platform skips and normal commit hooks pass. Production Rust is unchanged. Fresh CI on version 0.1.1688 now owns qualification. It supersedes the deferred rerun of the older Ubuntu DNS failure; no old-head rerun was started. Current-head installed screenshots/recording and clean review remain required before merge. |
|
@sfl review |
|
The Windows failure on A separate current-head Electron E2E job failed during |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 4
Confidence: 8/10 · Project risk: 4/10 (hs-buddy)
Since round 3
New: 0; still open (repeated): 0; resolved: 0.
Resolved means no matching finding in this review, not independent proof of a fix.
New (0)
- None.
Still open (repeated) (0)
- None.
Resolved (0)
- None.
Review done by claude-opus-5-5 (adversarial to codex)
The PR records explicit native PR-list opens (shell sidebar, the in-content mode row and the status-bar PR shortcut) as one best-effort buddyStats:batchIncrement with Electron's counter mapping. Tab and history restoration stay read-only through a new SidebarLink wrapper, and the installed smoke qualification gains a sidebar-scoped OCR locator and exact-delta activity checks. Static review found no evidenced Critical, High or Medium defect. Confidence is held below 10 because the PR body still lists current-head hosted CI and installed evidence as pending, and none of it was run here.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 3m 0s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
CHANGELOG.mddocs/GPUI-DASHBOARD-PARITY.mdpackage.jsonrust/crates/buddy-app/src/app.rsrust/crates/buddy-app/src/github_view.rsrust/crates/buddy-app/src/github_view/tests.rsrust/crates/buddy-app/src/shell/navigation.rsrust/crates/buddy-core/src/stats.rsrust/crates/buddy-core/src/stats/pr_list_activity.rsscripts/native-smoke/github-desktop.pyscripts/native-smoke/settings-desktop.pyscripts/native-smoke/settings-ocr.pyscripts/native-smoke/test_github_desktop.pyscripts/native-smoke/windows-actions.ps1
- Context read:
rust/crates/buddy-core/src/terminal_backend.rsrust/crates/buddy-core/src/convex_data.rsconvex/buddyStats.tssrc/hooks/useAppSessionStats.tssrc/App.tsx
- Verified:
gh api userunder the per-process personal profile returnedHemSoft- Compare
base_commit.shaisc0b1ecdf47…, the last commit is727aa4cf33…, status ahead 5 / behind 0, and the merge base equals the base - Counter mapping in
pr_list_activity::argumentsmatches ElectrontrackViewOpen(tabsOpenedplusprsViewed/prsReviewed/prsMergedWatched, none for Needs a nudge) - Every field sent is in Convex
COUNTER_FIELDS, andbatchIncrementtakes{fields} TerminalBackend::call(..., true)usesmutationwith a timeout: a timeout or transport failure maps toUnknownWrite, a server error toRejected, andrecord_withcalls the write exactly oncecallispub(super)in the crate-root moduleterminal_backend, sostats::pr_list_activitycan reach it- Shell sidebar click path:
SidebarLink::openchecksavailable, runson_openonce, and only the clonedDestination(restore callsnavigate, which never records) entersTabs - In-content mode buttons are filtered out when
shell_navigationis on, so one click cannot record through both paths - App launch builds tabs from
destination()(read-only), so initial restoration records nothing - The automation sidebar is wrapped with a no-op
on_openthroughFrom<Destination> - The
sidebaraction kind is wired through the Windows encoder, the PowerShellsidebar64mode and--sidebarflag, the Unix receipt list andinteract, and the Maclocateoptions - Smoke expected deltas match the scripted clicks: 5 opens (Needs Review, Recently Merged, Needs a nudge, My PRs x2), then +2 Needs Review for a total of 7 tabs and 3 reviews
- Parity doc claims checked against the implementation above
- Not verified:
- Runtime behavior, compile, clippy and test results; execution is forbidden
- Current-head hosted CI and installed Linux, Windows and macOS qualification outcomes, which the PR body marks pending
- Whether the restarted process actually shows a
My PRsworkspace tab for theactivity-restored-darkstep; that depends on GitHub route restore at launch, which was not traced - OCR fixture geometry in
github-pr-mode-sidebar.jsonandgithub-windows-disabled-back.jsonwas not inspected word by word status_bar.rscaller ofshow_status_prswas located by code search but not read- Convex
validateStatFields/buildIncrementPatchinternals were not read beyond the field allowlist
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head 727aa4cf33ef63e4ac65aa0fdea579f81edc32e4 • SFL v0
|
Native Ubuntu job 130588311432 on The native matrix still has active/queued Windows and macOS jobs. I retain CI ownership and will retry the failed Ubuntu infrastructure portion after that workflow completes. SFL Round 4 PASS covers this exact head; green native checks and installed evidence remain outstanding before merge. No source, timeout or assertion change is made for this setup failure. |
|
The current-head macOS Intel package job failed while building the DMG: The workflow is still queued for its Apple silicon package job. Once it completes, I will retry the diagnosed failed jobs on unchanged head |
|
Windows installed qualification on the prior head passed both exact activity-delta checks, then failed because the new Needs Review screenshot assertion expected Fixed in 674face. A captured nine-layout OCR regression exercises the actual qualification caller: the old expectation fails, the corrected row passes, and removing the row or route label still fails. Real OCR replay passes all three captured activity screenshots. All 38 focused tests, the full 563-test smoke suite (eight platform skips), and normal commit hooks including 7,640 coverage tests pass. Production source and counter assertions are unchanged. Fresh CI on version 0.1.1689 supersedes the old-head package-setup/DMG retries; none was replayed. Current-head green qualification, installed evidence and clean review are still required before merge. |
|
@sfl review |
|
The current-head Electron E2E job again failed before tests: Ubuntu's Azure mirror stalled and both existing 60-second The parent workflow still has active/queued jobs. I retain ownership and will retry its diagnosed infrastructure failures once the workflow completes, at unchanged head |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 5
Confidence: 8/10 · Project risk: 4/10 (hs-buddy)
Since round 4
New: 0; still open (repeated): 0; resolved: 0.
Resolved means no matching finding in this review, not independent proof of a fix.
New (0)
- None.
Still open (repeated) (0)
- None.
Resolved (0)
- None.
Review done by claude-opus-5-5 (adversarial to codex)
Native PR-list opens now send one best-effort buddyStats:batchIncrement with Electron's trackViewOpen mapping. The opens counted are the shell sidebar, the in-content mode row and the status-bar PR shortcut. A SidebarLink wrapper keeps the counting effect out of retained tabs and history. The installed smoke qualification gains sidebar-scoped OCR and exact-delta activity checks, and round 5 retargets the restored Needs Review check to the unapproved #8 row. Static reading of the exact head found no Critical, High, Medium or Low defect.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 3m 42s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
CHANGELOG.mddocs/GPUI-DASHBOARD-PARITY.mdpackage.jsonrust/crates/buddy-app/src/app.rsrust/crates/buddy-app/src/github_view.rsrust/crates/buddy-app/src/github_view/tests.rsrust/crates/buddy-app/src/shell/navigation.rsrust/crates/buddy-core/src/stats.rsrust/crates/buddy-core/src/stats/pr_list_activity.rsscripts/native-smoke/fixtures/github-restored-needs-review.jsonscripts/native-smoke/github-desktop.pyscripts/native-smoke/settings-desktop.pyscripts/native-smoke/settings-ocr.pyscripts/native-smoke/test_github_desktop.pyscripts/native-smoke/windows-actions.ps1
- Context read:
rust/crates/buddy-core/src/terminal_backend.rsrust/crates/buddy-core/src/convex_data.rsrust/crates/buddy-core/src/github/models.rsrust/crates/buddy-app/src/shell/status_bar.rsrust/crates/buddy-app/src/github_view/controls.rsrust/crates/buddy-app/src/github_view/data.rsrust/crates/buddy-app/src/dashboard/active_prs.rsconvex/buddyStats.tssrc/hooks/useAppSessionStats.tssrc/hooks/useAppTabs.tssrc/App.tsxsrc/components/StatusBar.tsxscripts/native-smoke/github-backend.pyPR #938 round 2-4 reviews
- Verified:
gh api userreturnedHemSoftunder the per-processGH_CONFIG_DIRprofile- Compare
base_commit.shaisc0b1ecdf47…and the last commit is674face2d7…: ahead 6, behind 0, and the merge base equals the base - Round-5 commit
674face2d7(parent727aa4cf33) changes only the restored-list check, a new OCR fixture, a test, the version and the changelog - Counter mapping in
pr_list_activity::argumentsmatches ElectrontrackViewOpen(tabsOpenedplusprsViewed/prsReviewed/prsMergedWatched; none for nudge), and every field is in ConvexCOUNTER_FIELDS batchIncrementtakesfieldsand applies one patch, which backs the doc's 'one atomic mutation' claimrecord_withcalls the write once with no retry, andTerminalBackend::callmaps a timeout toUnknownWritewithout replaying- Electron
openTabcallsonViewOpenunconditionally, and its status bar openspr-my-prs, so counting repeated opens and the status-bar shortcut is parity SidebarLink::openchecksavailable, runson_openonce and returns only theDestination, whoserestorecallsnavigateand records nothingdestination(),reconcile/change_account, the breadcrumb routes incontrols.rsand history do not callopen_pr_list- Needs Review searches include
-author:{user}on the assignee source, which triggers the fixture's paginated#8; only thereviewed-by:source marks#7approved, which matches the new code comment and the CI capture text - The
#8 Paginated fixture follow-upmarker is present in the committed CI OCR layouts forrestored-sidebar-list-dark.png sidebar64is wired throughsettings-desktop.py,windows-actions.ps1(--sidebar) and thesettings-ocr.pyargument parser
- Not verified:
- Runtime and OCR behavior of the installed qualification at this head: no hosted CI or installed run was observed here
- Whether the Convex Rust client resends an in-flight mutation on reconnect before the timeout; this is SDK internals that were not read
- How Needs Review merges the same PR number across sources and how
i_approvedis derived: inferred from the CI capture showing only#8, not read in code Runtime::globalpanicking without a global, as a test comment assumes;runtime.rswas not read at this head
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head 674face2d7c2020eb328cd60054d43ecb7facbb4 • SFL v0
|
The completed CI workflow has only the two diagnosed Ubuntu package-setup failures plus their downstream Requested one failed-jobs rerun of workflow 43045374273 at unchanged head |
|
Current-head native Ubuntu job 130610771312 stopped during host package setup. Both existing 300-second The native Windows/macOS jobs are still active. The failed Ubuntu job is recorded for one retry after this native workflow completes. This is separate from the already-requested standard CI retry. SFL Round 5 PASS remains current; merge waits for green qualification and installed evidence. |
|
CI attempt 2 failed in the same setup step: Electron E2E and Linux package smoke each exhausted both 60-second Inspection of The native matrix is still active; its first targeted Ubuntu retry remains pending workflow completion. A successful same-head Ubuntu qualification could provide that recovery evidence. Exact-head SFL Round 5 PASS remains valid. The claim and PR/CI work remain with this worker; merge remains blocked. |
|
Both hosted Mac jobs reached GitHub qualification but failed on the first sidebar action: sparse Tesseract read the custom menu's Fixed in eafa0e1. The sidebar reuses the existing workspace locator's validated painted-band measurement when menu OCR is missing. It retains one target engine, unique Repositories/control matching and two fresh stable Mac measurements. Ambiguous chrome, duplicate/missing controls or boundaries, and invalid bands still fail. A regression replays the actual Intel capture's OCR and measured pixel strip through the real driver. It fails before the fix and passes after it. Offline replay selects all four modes correctly from both Intel and Apple silicon captures. All 39 focused tests, the full 564-test smoke suite (eight platform skips) and normal hooks including 7,640 coverage tests pass. Production source and counter assertions are unchanged. Fresh CI on version 0.1.1690 supersedes the pending old-head native retry. The previous standard-CI retry remains recorded as failed during package setup; no further old-head rerun is requested. Current-head green qualification, installed evidence and clean review remain required before merge. |
|
@sfl review |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 6
Confidence: 8/10 · Project risk: 4/10 (hs-buddy)
Since round 5
New: 0; still open (repeated): 0; resolved: 0.
Resolved means no matching finding in this review, not independent proof of a fix.
New (0)
- None.
Still open (repeated) (0)
- None.
Resolved (0)
- None.
Review done by claude-opus-5-5 (adversarial to codex)
Native PR-list opens now record Electron's tabsOpened and per-mode PR counters through one best-effort buddyStats:batchIncrement write. The sidebar-click effect stays out of retained tabs and history. Round 6 adds only a smoke-test fallback to measured painted chrome bands when Mac OCR misreads the menu. The diff shows no security issue or breaking change, and the production Rust matches the previously reviewed slice.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 1m 23s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
rust/crates/buddy-app/src/app.rsrust/crates/buddy-app/src/github_view.rsrust/crates/buddy-app/src/github_view/tests.rsrust/crates/buddy-app/src/shell/navigation.rsrust/crates/buddy-core/src/stats.rsrust/crates/buddy-core/src/stats/pr_list_activity.rsscripts/native-smoke/github-desktop.pyscripts/native-smoke/settings-desktop.pyscripts/native-smoke/settings-ocr.pyscripts/native-smoke/test_github_desktop.pyscripts/native-smoke/windows-actions.ps1docs/GPUI-DASHBOARD-PARITY.mdCHANGELOG.mdpackage.json
- Context read:
convex/buddyStats.tssrc/hooks/useAppSessionStats.tsrust/crates/buddy-core/src/terminal_backend.rs
- Verified:
- Compare
base_commit.shaisc0b1ecdf471f8c56cb88ad367b6816ff9ef43131, the last commit iseafa0e15a406417e5210d1512eb16a2345909023, ahead 7, behind 0 - The last commit (
674face2d7...eafa0e15) touches only the changelog,package.json, a Mac fixture,settings-ocr.pyand its tests; no production Rust - Counter mapping matches Electron
trackViewOpen(prsViewed,prsReviewed,prsMergedWatched; Needs a Nudge adds onlytabsOpened) - Every field sent is in Convex
COUNTER_FIELDSforbatchIncrement SidebarLink::openchecksavailablebefore callingon_open; only the read-onlyDestinationreachesTabs, and therestoreclosure callsnavigate, notopen_pr_listrecord_withsends one write and does not retry onRejected/UnknownWrite/UnavailableTerminalBackend::callispub(super)on a crate-root module, sostats::pr_list_activitycan call itsidebar_targetre-raisesAmbiguousLabelfromchrome_rowsand falls back only toworkspace_geometry, which fails closed on invalid or misaligned bands- Tesseract TSV boxes and the ffmpeg band rows are both in capture-pixel space, so the band bottom and the word tops are comparable
- Windows
sidebar64action maps to--sidebarinwindows-actions.ps1andsettings-ocr.pymain
- Compare
- Not verified:
- Runtime behavior, installed qualification and CI results on this head (the PR body says they are pending)
- Hosted Mac OCR/band behavior beyond the committed fixtures
- Convex
buildIncrementPatch/validateStatFieldsinternals beyond the field allowlist - Large added OCR fixture JSON files were not read word by word
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head eafa0e15a406417e5210d1512eb16a2345909023 • SFL v0
|
Current-head Electron E2E and Linux package smoke both stopped before qualification: Ubuntu mirror requests stalled and both existing 60-second The same infrastructure symptom already reproduced on the prior-head retry. Further retry therefore waits for completed-workflow state plus new runner/network recovery evidence, such as successful Ubuntu qualification on this head. The remaining jobs continue. Source, package limits and merge gates are unchanged; this worker retains PR/CI ownership. |
|
Current-head native Ubuntu job 130658422354 also failed before qualification. Its setup log now explicitly reports This is evidence of the continuing runner DNS/mirror problem, not recovery evidence for retrying the two standard-CI setup failures. The remaining native Windows/macOS jobs continue. I retain the claim and PR/CI ownership, require new network-recovery evidence before another unchanged-head infrastructure rerun, and keep merge blocked. Exact-head SFL Round 6 PASS remains valid. |
|
The latest Apple silicon capture reached the first My PRs click after opening all four mode tabs. Those retained tabs extended into the chrome sampler's original strip, hiding the uniform workspace band. The capture reproduces the rejection; sampling the clear farther-right strip measures the expected (34, 68) band. Fixed in 2f43852. Only the sidebar fallback selects that farther-right strip; workspace callers retain their default region. All existing band-layout, target-uniqueness, single-engine and two-fresh-measurement guards remain. A new captured-pixel/OCR regression exercises the actual driver: the previous sampler fails and the correction passes; the occupied strip still fails the unchanged geometry guards. All four modes replay correctly across the new Apple silicon capture and earlier Intel/Apple silicon captures. All 40 focused GitHub tests, 11 browser tests, the full 565-test smoke suite (eight platform skips), and normal hooks including 7,640 coverage tests pass. Production source and counter assertions are unchanged. Native Windows passed the prior head, but fresh current-head installed qualification is still required. Fresh CI on version 0.1.1691 supersedes old-head retries. The recorded Ubuntu DNS/setup failures remain infrastructure evidence; this change does not claim to repair network service. Current-head green qualification, installed evidence and clean review remain required before merge. |
|
@sfl review |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 7
Confidence: 9/10 · Project risk: 4/10 (hs-buddy)
Since round 6
New: 0; still open (repeated): 0; resolved: 0.
Resolved means no matching finding in this review, not independent proof of a fix.
New (0)
- None.
Still open (repeated) (0)
- None.
Resolved (0)
- None.
Review done by claude-opus-5-5 (adversarial to codex)
Native PR-list opens (sidebar, in-view mode switch and status-bar entry) now record tabsOpened plus the per-mode counter through a single best-effort buddyStats:batchIncrement write. SidebarLink keeps the recording callback out of retained tabs and history. Native-smoke OCR gains a sidebar locator, and the changelog, version and parity docs are updated. I found no evidenced security issue or breaking change, and the counter mapping matches Electron's trackViewOpen.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 1m 1s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
CHANGELOG.mddocs/GPUI-DASHBOARD-PARITY.mdpackage.jsonrust/crates/buddy-app/src/app.rsrust/crates/buddy-app/src/github_view.rsrust/crates/buddy-app/src/github_view/tests.rsrust/crates/buddy-app/src/shell/navigation.rsrust/crates/buddy-core/src/stats.rsrust/crates/buddy-core/src/stats/pr_list_activity.rsscripts/native-smoke/github-desktop.pyscripts/native-smoke/settings-desktop.pyscripts/native-smoke/settings-ocr.pyscripts/native-smoke/test_github_desktop.pyscripts/native-smoke/windows-actions.ps1
- Context read:
rust/crates/buddy-core/src/terminal_backend.rsconvex/buddyStats.tssrc/hooks/useAppSessionStats.tsrust/crates/buddy-app/src/github_view.rs (sidebar, destination, existing convex_url callers at head)rust/crates/buddy-app/src/shell/navigation.rs (Destination, Tabs at head)
- Verified:
- Compare
base_commit.shaequalsc0b1ecdf; last commit equals2f438521; status ahead 8, behind 0 - Counter mapping in
arguments()matches ElectronprStatMap(prsViewed/prsReviewed/prsMergedWatched, none for Needs a Nudge) plustabsOpened batchIncrementtakesfieldsand validates every name againstCOUNTER_FIELDS, which includes all four fields sentTerminalBackend::call(.., true)uses a mutation with a timeout that maps toUnknownWrite.record_withsends once and does not retrySidebarLink::openchecksavailablebefore runningon_openand returns only theDestinationclone, soTabsnever holds the recording closure- Sidebar
restoreanddestination().restorestill callnavigate, notopen_pr_list, so restoring a tab writes nothing - Smoke deltas are consistent: the first drive has 5 sidebar opens (2 My PRs, 1 for each other mode), so
tabsOpened=5, prsViewed=2; the second drive adds 2 Needs Review sidebar opens, giving totals of 7 and 3 - The
sidebar/sidebar64action kind is wired through settings-desktop.py, settings-ocr.py (--sidebar) and windows-actions.ps1 - Fixture JSON files are OCR or pixel test data and carry no credentials (checked by file list and diff size)
- Compare
- Not verified:
- Runtime behavior of the GPUI click path and native-smoke qualification on Windows and macOS (execution forbidden)
- Fixture JSON contents were not read word by word; only their use in tests was checked
- Whether each click opening a fresh Convex connection through
settings_backend::connectcosts measurable performance (existing pattern, not measured) - CI status on the head commit
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head 2f438521000a39044378fad898c108c3ceeda065 • SFL v0
Summary
Explicit native PR-list opens record the existing Convex activity totals using Electron's mapping. Each open sends one best-effort atomic mutation. Sidebar click effects stay outside the read-only destinations retained by tabs and history.
Installed qualification checks exact counter deltas across mode clicks, repeated opens, refresh, retained tabs and restart. Sidebar OCR uses measured workspace and Repositories boundaries, including validated painted chrome bands when menu text is unreadable.
Acceptance Criteria
tabsOpened; My PRs, Needs Review and Recently Merged add the same PR counter as Electron.Definition of Done
2f438521000a39044378fad898c108c3ceeda065, with zero findings and zero unresolved review threads.Validation
0d5dca4446c1e19a03fcb89dddce36b8bb3eb107. Formatting, workspace/all-target warnings-denied Clippy, 323 app tests and 14 app integration tests passed after that production fix; the earlier full workspace run passed.Hosted Windows Server 2025, app capture 1044×788, version 0.1.1691, source head
2f438521000a39044378fad898c108c3ceeda065. The clean build manifest matches theinstalled executable's SHA-256; the downloaded package digest also matches.
Passing installed qualification
uses synthetic accounts, fake credentials and an isolated Convex backend.
Repeatable hosted command after the workflow's native package build:
The fixture queries exact deltas across two installed processes, repeated explicit
opens, refresh and retained-tab restoration:
tabsOpenedprsViewedprsReviewedprsMergedWatchedappLaunchestotalUptimeMsNeeds Review after repeated explicit opens, retained-tab selection and refresh:
Selecting retained My PRs restores its list without another activity write:
Workspace Pulse renders 6 aggregate PR views: 2 viewed + 3 reviewed + 1 merged-watched.
Restarted-process interaction recording: repeated Needs Review opens, retained tabs,
refresh and Workspace Pulse (13.75 seconds).
interaction.mp4
Risks and deferred work
Medium risk for sidebar click routing and durable shared counters; #937 records the acknowledgment and review targets. Other native activity counters, session/uptime ownership, live credentials, broader migration/rollback and all remaining #823 criteria stay open. Electron remains primary, gpui-kit stays at 0.8.1, and no backend deployment or production profile access is needed.
Closes #937. Advances #823 without closing it.