fix: converge Homebrew status and privacy feedback - #209
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
Rate Limit Exceeded
|
|
Current-head privacy finding on |
OpenCode Review Overview
Pull request overviewOpenCode reviewed the current-head bounded evidence and requested changes before merge. Findings1. P1 src/lib/brewCleanupErrorPrivacyContract.test.ts:30 - Privacy-contract test does not guard execution.record_error template interpolation
SummaryP1 src/lib/brewCleanupErrorPrivacyContract.test.ts:30 — Privacy-contract test gap: the test asserts String(e) absence and static Korean messages but does not check that the template section avoids unsanitized execution.record_error interpolation into a {role="alert"} element. The prior review thread at this exact line (same SHA) is unresolved and independently verified: the PR author's own comment (2026-08-15) confirms the interpolation exists, the diff has no template changes, and sibling component IcloudLocalEviction.svelte:179 shows the identical pattern. Approval sufficiency: BLOCKED by unresolved thread. Verification posture: source-trace only (file reads restricted by directory policy). Linter/static: no active failed checks. TDD/regression: test passes for catch-block paths but misses template path. Coverage: coverage execution evidence shows supported tests passed. Docstring coverage: not applicable (Svelte/TS, no docstring gates). DAG: flowchart above shows base-to-head error rendering flow. PoC/execution: not applicable (static contract test, no runtime PoC needed). DDD/domain: Homebrew cleanup privacy boundary. CDD/context: sibling IcloudLocalEviction shows identical record_error pattern. Similar issues: IcloudLocalEviction.svelte still uses String(e) in catch blocks (out of scope). Claim/concept check: PR title 'converge privacy feedback' overclaims completeness. Standards search: N/A. Compatibility/convention: no breaking changes. Breaking-change/backcompat: none. Implementation completeness: catch-block implementation complete; template path incomplete. Performance: no impact. Developer experience: test file structure is good, needs one more assertion. User experience: unsanitized backend error can leak to user via role=alert. Visual/DOM: non-web desktop UI (Tauri); alert rendering confirmed by role=alert assertion in test. Accessibility/i18n: role=alert present. Supply-chain/license: no dependency changes. Packaging: no manifest changes. Security/privacy: P1 unsanitized backend field in template. Adversarial validation{"status":"failed","probes":[{"path":"src/lib/brewCleanupErrorPrivacyContract.test.ts","line":30,"hypothesis":"The test's negative privacy assertions would detect unsanitized execution.record_error interpolation in the BrewCleanup.svelte template","attack_or_counterexample":"The test file's 30 lines contain no assertion referencing record_error, execution.record_error, or template interpolation patterns — only String(e) absence and static string presence checks","evidence":"Trusted source trace at src/lib/brewCleanupErrorPrivacyContract.test.ts:30 — file ends at line 30 with no assertion about execution.record_error; PR author comment (2026-08-15T12:16:31Z) at same SHA confirms the interpolation exists; source-line-sha256=29576b54e255e3c948eea5b5904fa38b81682fdd3cbd9ae841ff0e7bd80d394c","outcome":"confirmed"},{"path":"src/lib/BrewCleanup.svelte","line":76,"hypothesis":"The catch-block fix at line 76 constitutes complete privacy-safe error feedback for all user-visible error rendering paths","attack_or_counterexample":"The diff changes only two catch blocks (lines 29 and 76); the template section with execution.record_error interpolation (confirmed by PR author and sibling IcloudLocalEviction.svelte:179 pattern) is untouched","evidence":"Trusted source trace at src/lib/BrewCleanup.svelte:76 shows the static error message fix; diff stat confirms 32 insertions / 2 deletions with no template changes; source-line-sha256=55c0b9d00542b7a9ee3590d70797c354df90e7c27b9a91d8e9ecadcd644d6cb3","outcome":"confirmed"}],"residual_risk":"The catch-block privacy fix is correct. The remaining gap (unsanitized execution.record_error in the template) is bounded and has an identified fix direction. No other security, correctness, or compatibility regressions detected."}
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
|
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Superseded by current head 2a6ed44. The record-error interpolation findings are addressed, and the current regression additionally rejects raw iCloud eviction catch errors.
19a60a0 to
f748769
Compare
2a6ed44 to
0fc67a4
Compare
1 similar comment
|
Current head |
|
@OpenCode review current head |
opencode-agent
Bot
added
priority: medium
|
@OpenCode review current exact head |
…nzero-status-v1' into HEAD # Conflicts: # src/lib/IcloudLocalEviction.svelte
2a98451
into
fix/brew-cleanup-nonzero-status-v1
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| {:else} | ||
| <p class="warning" role="status">현재 축출 불가: {blockerSummary(plan.blockers)}</p> | ||
| <p class="warning">현재 축출 불가: {plan.blockers.join(", ")}</p> |
There was a problem hiding this comment.
🔴 Changes break existing eviction UI contract test
The rewritten status and error text drops blockerSummary, role="status", and several fixed strings (also at IcloudLocalEviction.svelte:38, :50, :86) that the unmodified src/lib/icloudLocalEvictionSafetyUiContract.test.ts still asserts at lines 16-18 and 26-29. Every one of those seven assertions now fails, breaking the test suite.
Prompt for agents
The changes to src/lib/IcloudLocalEviction.svelte contradict the existing, unmodified test src/lib/icloudLocalEvictionSafetyUiContract.test.ts. That test asserts the presence of strings and code that this PR removed or changed: "로컬 최신본·업로드 미확인" and the "로컬 최신본이지만 공급자 업로드가 아직 확인되지 않았습니다..." sentence (from the deleted syncLabel/blockerLabel helpers), the role="status" attribute (removed from the blocker <p> at line 197), the error strings "iCloud 파일 선택을 완료하지 못했습니다. 다시 시도하십시오.", "iCloud 로컬 사본 상태를 확인하지 못했습니다. 다시 시도하십시오.", "iCloud 로컬 사본을 회수하지 못했습니다. 상태를 다시 확인하십시오.", and the presence of function blockerSummary. Reconcile the two: either update icloudLocalEvictionSafetyUiContract.test.ts to match the new privacy-safe UI, or restore the required behavior in the component. The PR description claims IcloudLocalEviction.svelte is out of scope, so confirm whether these component edits were intended at all.
Was this helpful? React with 👍 or 👎 to provide feedback.
| </div> | ||
| {:else} | ||
| <p class="warning" role="status">현재 축출 불가: {blockerSummary(plan.blockers)}</p> | ||
| <p class="warning">현재 축출 불가: {plan.blockers.join(", ")}</p> |
There was a problem hiding this comment.
📝 Info: Blocker codes now shown raw to users
The deleted blockerSummary translated internal blocker codes into human-readable, deduplicated guidance. Now plan.blockers.join(", ") surfaces raw codes like icloud-upload-not-confirmed to users, a UX downgrade, though it matches the existing verification_blockers.join at IcloudLocalEviction.svelte:155.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -26,7 +26,7 @@ | |||
| try { | |||
| judgment = await api.judgeBrewCleanup(); | |||
| } catch (e) { | |||
There was a problem hiding this comment.
📝 Info: Unused catch binding kept inconsistently in BrewCleanup
In BrewCleanup.svelte and :75 the catch clauses still bind the exception (catch (e)) even though e is no longer referenced, whereas the equivalent edits in src/lib/IcloudLocalEviction.svelte:37,49,85 switched to the optional-catch-binding form (catch {). This is only a style inconsistency (and potentially a lint warning depending on the project's eslint config), not a runtime bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const evictionSource = readSource("src/lib/IcloudLocalEviction.svelte"); | ||
|
|
||
| expect(source).not.toContain("String(e)"); | ||
| expect(source).not.toContain("record_error"); |
There was a problem hiding this comment.
📝 Info: Contract test asserts absence of substrings not just interpolations
The new test at brewCleanupErrorPrivacyContract.test.ts asserts the BrewCleanup source not.toContain("record_error"). This is a broad substring check: the source still contains record_path (BrewCleanup.svelte), which does not match record_error, so the test passes today. However, any future use of a field like record_error_count would fail this test even if privacy-safe. Not a bug, but worth noting the assertion is stricter/looser than an exact interpolation match.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Purpose
Preserve #189's correct success/failure status semantics while adding bounded privacy-safe planning/execution failure feedback on one dependency-ordered
BrewCleanup.svelteowner, without changing Homebrew command, model, human approval, audit, or filesystem authority.Exact current state
fix/brew-cleanup-nonzero-status-v1, unchanged head19a60a0518668492a9b53e9d92b922f157d7ad12.3ab6d3793140672c7c90319714eb381757a53f1c.ahead_by = 2,behind_by = 0, with merge base exactly the stack base.src/lib/BrewCleanup.svelte;src/lib/brewCleanupErrorPrivacyContract.test.ts.Test-first repair
12e0e04d404843b6a2fe9408f624e27b5255e2dbrequired the component to stop projecting arbitrary caught exception text while preserving the existing judgment/execution authority calls.3ab6d3793140672c7c90319714eb381757a53f1creplaces only the twoString(e)projections with:Homebrew 정리 계획을 만들지 못했습니다.;Homebrew 정리를 실행하지 못했습니다..실행 성공/실행 실패/실행되지 않음status semantics remain intact.role="alert", judgment/execution calls, plan and judgment identifiers, confirmation phrase, rationale, single-use state transitions, stdout/stderr display, audit-result handling, and filesystem authority remain unchanged.Exact-head validation
On unchanged head
3ab6d3793140672c7c90319714eb381757a53f1c:31664202305— success, including Rust tests, real process/domain suites, frontend tests, frontend build, and the real CPU LLM-engine build;31664202308— success for Windows, Ubuntu, and macOS build/package artifact jobs.The PR targets a feature branch rather than protected main, so default-branch-only organization required workflows and approval rules do not apply to this stack edge. Security Scan and SAST Semgrep did not produce PR runs for this feature-branch target; their absence is not passing evidence. Release
publish-releaseandattest-releasewere skipped and are not provenance evidence.Dependency and integration constraints
Required before any integration
Do not merge to protected main unless the final unchanged exact head satisfies every applicable live required workflow and current finding, every review thread is resolved, a qualifying independent approval covers the last push/current head, fresh protected-main ancestry remains current, repository-wide exact coverage is satisfied without exclusions or threshold weakening, and package/SBOM/provenance evidence is complete. Pending, queued, absent-required, skipped-required, neutral-required, failed, stale, predecessor, synthetic, diagnostic-only, status-only, model-only, rate-limited, no-source scanner, or infrastructure-only evidence is not passing.