Skip to content

security: fail generic cleanup closed without object-bound recycle - #174

Merged
seonghobae merged 21 commits into
mainfrom
security/generic-cleanup-identity-bound-v1
Aug 26, 2026
Merged

security: fail generic cleanup closed without object-bound recycle#174
seonghobae merged 21 commits into
mainfrom
security/generic-cleanup-identity-bound-v1

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Purpose

Bounded security repair for #170. Generic desktop cleanup previously registered commands::clean_paths, whose core ultimately passed a pathname to safety::trash_delete. Path revalidation alone cannot preserve filesystem-object identity through a pathname-consuming OS recycle primitive.

Exact current state

  • Exact source head: 8d88051b55845418e3d5c67288766903f770610c.
  • Independently resolved live protected main: be418bac9bfbdc8aa41b1a698713833e3dbccfdc.
  • Fresh main -> head comparison is ahead, ahead_by = 16, behind_by = 0, with merge base exactly equal to live protected main.
  • The semantic delta remains exactly src-tauri/src/generic_cleanup.rs, src-tauri/src/lib.rs, and src-tauri/tests/generic_cleanup_mutation_authority.rs.
  • Native exact-head workflows are green: Test 31585043488, Release 31585043524, Security Scan 31585043494, and SAST Semgrep 31585043691 all completed successfully.
  • The exact-head check set contains no observed failed check. Required central OpenCode/Noema/Strix/queue/merge-scheduler/CodeQL evidence on this head completed without a source-backed current-head finding.
  • Required-workflow success is not a formal approval. The only formal review still recorded is an OpenCode CHANGES_REQUESTED review anchored to predecessor head d29f3fc37fe3ebcf11f90dd2e66ec929e87a847c; its stated blocker was failed coverage evidence rather than a source defect on this head, and predecessor evidence does not transfer.
  • Current review threads are empty. Fresher repository governance evidence from current DiskSage security/release lanes requires one qualifying independent approval, approval after the latest push, review-thread resolution, and the configured central workflows. Older zero-approval snapshots are not treated as current authority.
  • Protected-main Test push run 31584223427 and dynamic Code Quality run 31584222543 succeeded on current main, proving the latest integrated Homebrew-cleanup base is operationally green before this delta.
  • The PR remains Draft because repository-wide exact 100% owned-production coverage is still materially unsatisfied on canonical coverage owner ci: require exact-head production coverage evidence #156. Green native/central workflow status does not waive that product-quality requirement.

Test-first repair

  • RED contract 79c6e4e04198e76cf7e69c78163d4b46b9282d67 required the registered Tauri clean_paths route to leave the pathname-consuming legacy command and move behind a dedicated fail-closed authority.
  • src-tauri/src/generic_cleanup.rs returns stable generic-cleanup-identity-bound-recycle-unavailable refusals and performs no filesystem mutation.
  • Unit regressions prove stable refusal, empty-request no-op behavior, and byte-identical preservation of a real temporary filesystem fixture.
  • The first implementation exposed a compile-time Tauri macro collision because new and legacy Rust functions shared the name clean_paths; the Rust handler is now fail_closed_clean_paths while the external IPC name remains clean_paths through #[tauri::command(rename = "clean_paths")].
  • A later RED proved the generic repair had accidentally unregistered the separately object-bound clean_dev_artifacts IPC route. The narrow fix restored only commands::clean_dev_artifacts; its production path continues to use object-identity-bound staging/recycle semantics while generic clean_paths stays fail closed.
  • src-tauri/src/lib.rs therefore registers generic_cleanup::fail_closed_clean_paths and commands::clean_dev_artifacts, and never registers commands::clean_paths.

Scope / non-duplication

Cache cleanup and Maven apply-mode already fail closed on protected main. Protected-main cloud-source eviction also fails closed before its historical pathname-consuming path. #171 is protected-main history for developer-artifact object-identity staging and Git-worktree mutation. #156 separately owns repository-wide exact-production-coverage evidence and is not imported here. #176 and #185 are protected-main history and are deliberately preserved by the current-base convergence.

Required before merge

Keep Draft until the unchanged exact head satisfies every live required organization workflow, repository-wide exact 100% owned-production coverage without exclusions or threshold weakening, zero valid current-head findings, the qualifying independent approval/latest-push requirement actually imposed by live governance, and a fresh live-base/ancestry check. Pending, queued, skipped-required, stale-head, predecessor, synthetic, diagnostic-only, status-only, model-only, author-only, or rate-limited evidence is not passing.

Summary by CodeRabbit

  • 버그 수정

    • 파일 경로 기반 정리 요청이 항상 안전하게 거부되도록 변경되었습니다.
    • 거부된 요청에는 일관된 오류 식별자가 표시됩니다.
    • 정리 요청 처리 중 파일 시스템이 변경되지 않습니다.
    • 빈 정리 요청도 예외 없이 일관된 결과를 반환합니다.
    • 기존 개발 아티팩트 정리 기능은 계속 사용할 수 있습니다.
  • 테스트

    • 정리 요청의 결과 일관성과 파일 미변경 동작을 검증하는 테스트가 추가되었습니다.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3b46c82a-e5d8-44d8-b4ea-30dd9830bfe7

📥 Commits

Reviewing files that changed from the base of the PR and between 198fea8 and 3864a8d.

📒 Files selected for processing (3)
  • src-tauri/src/generic_cleanup.rs
  • src-tauri/src/lib.rs
  • src-tauri/tests/generic_cleanup_mutation_authority.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Generic cleanup IPC 경로를 fail-closed 구현으로 전환했습니다. 모든 경로는 파일시스템 변경 없이 동일한 오류를 반환합니다. Tauri 명령 연결과 기존 개발 artifact 정리 경로를 검증하는 테스트를 추가했습니다.

Changes

Generic cleanup fail-closed 경로

Layer / File(s) Summary
Fail-closed 구현 및 명령 연결
src-tauri/src/generic_cleanup.rs, src-tauri/src/lib.rs
fail_closed_clean_paths는 각 경로에 ok: falsegeneric-cleanup-identity-bound-recycle-unavailable 오류를 반환합니다. Tauri IPC 이름은 clean_paths로 유지합니다.
동작 및 명령 권한 검증
src-tauri/src/generic_cleanup.rs, src-tauri/tests/generic_cleanup_mutation_authority.rs
경로별 결과 일관성, 파일 미변경, 빈 요청 처리를 검증합니다. fail-closed 명령 등록과 clean_dev_artifacts 경로 유지를 확인합니다.

Estimated code review effort: 3 (보통) | ~20분

Merge Risk: ⚪ Minimal · up to 3864a

The localized fail-closed cleanup change has no actionable merge-blocking risk in the supplied evidence. An outdated build-version statement should be removed, but it does not affect runtime behavior.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 generic cleanup을 보안 목적으로 fail-closed 처리하고 object-bound recycle을 사용하지 않는 핵심 변경을 직접 설명합니다. 표현은 다소 어색하지만 변경 내용과 명확히 관련됩니다.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/generic-cleanup-identity-bound-v1

Warning

Some tools did not complete. Review the errors below.

🔧 Clippy (1.97.1)

Clippy execution failed


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae marked this pull request as ready for review August 11, 2026 23:05

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head d29f3fc37fe3ebcf11f90dd2e66ec929e87a847c.

  • Head SHA: d29f3fc37fe3ebcf11f90dd2e66ec929e87a847c

  • Workflow run: 31545716676

  • Workflow attempt: 1

Coverage evidence

Coverage Decision

  • Result: FAIL
  • Test evidence: not proven passing
  • Docstring evidence: not proven passing when configured
  • Failure count: 1

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["Changed file (3 files)"]
  S1 --> I1["repository behavior"]
  I1 --> R1["Review risk: Changed file (3 files)"]
  R1 --> V1["required checks"]
Loading

@opencode-agent

opencode-agent Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

OpenCode Review Overview

  • Head SHA: 8d88051b55845418e3d5c67288766903f770610c
  • Workflow run: 31876345461
  • Workflow attempt: 1
  • Gate result: REQUEST_CHANGES (approval step)

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head 8d88051b55845418e3d5c67288766903f770610c.

  • Head SHA: 8d88051b55845418e3d5c67288766903f770610c

  • Workflow run: 31876345461

  • Workflow attempt: 1

Coverage evidence

Coverage Decision

  • Result: FAIL
  • Test evidence: not proven passing
  • Docstring evidence: not proven passing when configured
  • Failure count: 1

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["Changed file (3 files)"]
  S1 --> I1["repository behavior"]
  I1 --> R1["Review risk: Changed file (3 files)"]
  R1 --> V1["required checks"]
Loading

@seonghobae
seonghobae marked this pull request as draft August 12, 2026 00:01
@seonghobae
seonghobae dismissed opencode-agent[bot]’s stale review August 13, 2026 04:00

This review applies only to predecessor SHA d29f3fc. The current head 8d88051 has replacement same-head coverage-evidence and OpenCode checks. Dismissing the stale review does not approve the PR; all current merge gates remain required.

@seonghobae
seonghobae marked this pull request as ready for review August 15, 2026 09:06

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head 8d88051b55845418e3d5c67288766903f770610c.

  • Head SHA: 8d88051b55845418e3d5c67288766903f770610c

  • Workflow run: 31876345461

  • Workflow attempt: 1

Coverage evidence

Coverage Decision

  • Result: FAIL
  • Test evidence: not proven passing
  • Docstring evidence: not proven passing when configured
  • Failure count: 1

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["Changed file (3 files)"]
  S1 --> I1["repository behavior"]
  I1 --> R1["Review risk: Changed file (3 files)"]
  R1 --> V1["required checks"]
Loading

@seonghobae
seonghobae marked this pull request as draft August 17, 2026 09:05
@seonghobae
seonghobae dismissed opencode-agent[bot]’s stale review August 22, 2026 07:35

Dismissing: this REQUEST_CHANGES referenced workflow run 31876345461 where coverage-evidence failed. At the same head SHA, coverage-evidence has since been re-run and now reports pass (verified via gh pr checks). No unresolved review threads remain. Re-request review to re-validate.

@opencode-agent opencode-agent Bot added area: security Security boundary, hardening, or vulnerability prevention priority: high High-priority or P1 work status: draft Draft pull request type: security Security vulnerability or security-specific remediation labels Aug 22, 2026
@seonghobae
seonghobae marked this pull request as ready for review August 25, 2026 22:35

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Comment thread src-tauri/src/generic_cleanup.rs
Comment thread src-tauri/src/lib.rs
@seonghobae
seonghobae enabled auto-merge August 25, 2026 23:41
@seonghobae
seonghobae merged commit 79067c1 into main Aug 26, 2026
37 checks passed
@seonghobae
seonghobae deleted the security/generic-cleanup-identity-bound-v1 branch August 26, 2026 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: security Security boundary, hardening, or vulnerability prevention priority: high High-priority or P1 work status: draft Draft pull request type: security Security vulnerability or security-specific remediation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant