Skip to content

๐Ÿ”’ [๋ณด์•ˆ] CLI ๋ฌด์ œํ•œ ํŒŒ์ผ ์ฝ๊ธฐ ์ทจ์•ฝ์  ์ˆ˜์ • - #811

Draft
seonghobae wants to merge 168 commits into
developfrom
fix-cli-unbounded-read-5165758910965089497
Draft

๐Ÿ”’ [๋ณด์•ˆ] CLI ๋ฌด์ œํ•œ ํŒŒ์ผ ์ฝ๊ธฐ ์ทจ์•ฝ์  ์ˆ˜์ •#811
seonghobae wants to merge 168 commits into
developfrom
fix-cli-unbounded-read-5165758910965089497

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Security and availability boundary

The CLI treats stdin, inline --job, and file-backed --job as bounded, fail-closed input authorities. This branch preserves the existing JSON wire contract while preventing caller-selected network, device, stream, symlink, non-regular-file, and unbounded-read authority.

Current exact lineage

  • Protected base integrated: develop@314ddeae7b775a4957594b599358c8255617eb2e
  • Pre-repair head preserved: 375873551da4e06387aaf486e2dfa040280d19f5
  • RED + protected-base reconciliation: bd32c2135e5dd94822f3df93c31f71e3f0d63854
  • Canonical formatting owner integrated as a parent: #1176 at 8fe6b6d99c009527ef0bcba419e6f6debdb23c23
  • Exact current head: d2b4c0bffd21c86bd278a17f1bd56c93e32a3b9a
  • Current ancestry: 164 commits ahead / 0 behind protected develop; no force push or destructive rebase.

Direct repairs

  • Integrated current consolidated workflow and Tauri lock baseline through ordinary two-parent history.
  • Preserved bounded raw-byte reads, UTF-8 validation, descriptor identity checks, O_NONBLOCK/no-follow/close-on-exec handling, and Win32 UNC/device/alternate-stream classification before filesystem lookup.
  • Replaced ambiguous private CLI identifiers with the job-input ubiquitous language: job_path, job_file_path, path_authority, preflight_status, open_flags, file_descriptor, job_request_payload, and analysis_request.
  • Removed the retired no-op cli.TemporalAnalyzer compatibility hook. Temporal analysis belongs to the orchestration API; CLI tests now observe that owner boundary rather than monkeypatching dead state.
  • Repaired stale operator documentation whose root-level coverage command produced module-not-imported / no-data-collected. AGENTS.md, CLAUDE.md, and the harness guide now run the coverage target from services/analysis-engine.
  • Added and updated ARCHITECTURE.md, CHANGELOG.md, doctoring evidence, and docs/product-technical-gap-baseline.md.

Local exact-tree evidence

  • Naming RED failed both contracts before production repair.
  • CLI/security slice: 113 passed.
  • Full analysis engine: 764 passed, 24 explicit native-parity skips, 100% statements and branches.
  • Exact Node 22.22.2 / npm 10.9.9 harness: ESLint, documentation/security/supply-chain checks, Ruff, Bandit, mypy, 216 desktop tests, 20 shared-contract tests, 764 runnable Python tests, 100% coverage, and production build passed.
  • git diff --check passed.

Compatibility and persistence

Public JSON keys, CLI flags, exit behavior, file-size limit, accepted local-file semantics, analysis result shape, database/persistence schema, migrations, indexes, locks, transactions, and model/provider contracts are unchanged. External JSON names remain at the adapter boundary and translate immediately to semantic internal identifiers.

Merge gate

This PR remains Draft. Fresh checks must complete on exact head d2b4c0bffd21c86bd278a17f1bd56c93e32a3b9a, every valid current-head review finding must be resolved, and a qualifying independent non-author approval must satisfy ordinary branch protection. Queued, skipped, predecessor-head, self/author, model-only, or administrative-bypass evidence is not success.

@google-labs-jules

Copy link
Copy Markdown

๐Ÿ‘‹ Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a ๐Ÿ‘€ emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • โ–ถ๏ธ Resume reviews
  • ๐Ÿ” Trigger review
๐Ÿ“ Walkthrough

Walkthrough

CLI์˜ stdin, ์ธ๋ผ์ธ ์ž‘์—…, ์ž‘์—… ํŒŒ์ผ ์ž…๋ ฅ์— UTF-8 ๋ฐ”์ดํŠธ ๊ธฐ์ค€ 10MB ์ œํ•œ์„ ์ถ”๊ฐ€ํ–ˆ์Šต๋‹ˆ๋‹ค. ์ž˜๋ชป๋œ ์ธ์ž์™€ ๊ถŒํ•œ ์—†๋Š” ์ž‘์—… ํŒŒ์ผ์„ ๊ฑฐ๋ถ€ํ•ฉ๋‹ˆ๋‹ค. ์ง์ ‘์ ์ธ TemporalAnalyzer ์‹คํ–‰ ๋กœ์ง์€ ์ œ๊ฑฐํ•˜๊ณ  ํ˜ธํ™˜์„ฑ ๋ณ„์นญ์€ ์œ ์ง€ํ•ฉ๋‹ˆ๋‹ค.

Changes

CLI ์ž…๋ ฅ ๋ฐ ์ž‘์—… ํŒŒ์ผ ๊ฒ€์ฆ

Layer / File(s) Summary
์ž…๋ ฅ ์ œํ•œ ๋ฐ CLI ์˜ค์ผ€์ŠคํŠธ๋ ˆ์ด์…˜
services/analysis-engine/src/bandscope_analysis/cli.py
stdin, ์ธ๋ผ์ธ --job, ์ž‘์—… ํŒŒ์ผ ์ž…๋ ฅ์— 10MB ๋ฐ”์ดํŠธ ์ œํ•œ์„ ์ ์šฉํ•ฉ๋‹ˆ๋‹ค. UTF-8 ์˜ค๋ฅ˜์™€ ์ž˜๋ชป๋œ ์ธ์ž๋ฅผ ๊ตฌ์กฐํ™”๋œ ์˜ค๋ฅ˜๋กœ ๋ฐ˜ํ™˜ํ•ฉ๋‹ˆ๋‹ค. ์ง์ ‘์ ์ธ TemporalAnalyzer ์‹คํ–‰ ๊ฒฝ๋กœ๋ฅผ ์ œ๊ฑฐํ•˜๊ณ  ๋ณ„์นญ์„ ์œ ์ง€ํ•ฉ๋‹ˆ๋‹ค.
์ž‘์—… ํŒŒ์ผ ๊ถŒํ•œ ๊ฒ€์ฆ
services/analysis-engine/src/bandscope_analysis/cli.py, services/analysis-engine/tests/test_cli_job_*, docs/doctoring/cli-job-file-authority.md, AGENTS.md, ARCHITECTURE.md
UNC, ์žฅ์น˜ ๊ฒฝ๋กœ, ๋“œ๋ผ์ด๋ธŒ ์ƒ๋Œ€ ๊ฒฝ๋กœ, ์˜ˆ์•ฝ ํŒŒ์ผ๋ช…, ์ฝ˜์†” ํ•ธ๋“ค, ๋””๋ ‰ํ„ฐ๋ฆฌ, ์‹ฌ๋ณผ๋ฆญ ๋งํฌ์™€ ๋น„์ •๊ทœ ํŒŒ์ผ์„ ๊ฑฐ๋ถ€ํ•ฉ๋‹ˆ๋‹ค. lstat() ๋ฐ fstat() ๊ฒฐ๊ณผ์™€ inode๋ฅผ ๋น„๊ตํ•œ ๋’ค ์ œํ•œ๋œ ๋ฐ”์ดํŠธ๋ฅผ ์ฝ์Šต๋‹ˆ๋‹ค.
์ž…๋ ฅ ๋ฐ ์ธ์ž ํšŒ๊ท€ ๊ฒ€์ฆ
services/analysis-engine/tests/test_cli_input_bounds.py, services/analysis-engine/tests/test_cli_unknown_arguments.py, services/analysis-engine/tests/test_cli.py
stdin, ์ธ๋ผ์ธ ์ž‘์—…, ์ž‘์—… ํŒŒ์ผ์˜ ํฌ๊ธฐ์™€ UTF-8 ๊ฒฝ๊ณ„๋ฅผ ๊ฒ€์ฆํ•ฉ๋‹ˆ๋‹ค. ๋ช…์‹œ์  ์ธ์ž์™€ ์ž˜๋ชป๋œ ์ธ์ž๊ฐ€ stdin์„ ์†Œ๋น„ํ•˜์ง€ ์•Š๋Š”์ง€ ๊ฒ€์ฆํ•ฉ๋‹ˆ๋‹ค.
๋ณ€๊ฒฝ ๊ธฐ๋ก ๋ฐ ์šด์˜ ๊ทœ์น™
CHANGELOG.md, AGENTS.md, ARCHITECTURE.md, docs/doctoring/cli-job-file-authority.md
CLI ์ž…๋ ฅ ๊ฒ€์ฆ๊ณผ ์ž‘์—… ํŒŒ์ผ ๊ถŒํ•œ ๊ฒฝ๊ณ„์˜ ๋ณ€๊ฒฝ ์‚ฌํ•ญ์„ ๊ธฐ๋กํ•ฉ๋‹ˆ๋‹ค.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ๐ŸŸก Moderate ยท up to 72748

The CLI still misclassifies CONOUT$: as an alternate stream before applying console-handle validation, causing the required regression test to fail at the current head. Merge should wait for the classification-order fix and passing regression; documentation and test-coverage follow-up also remain.

Sequence Diagram(s)

sequenceDiagram
  participant InputSource
  participant cli_main
  participant JobFile
  participant JSONParser
  InputSource->>cli_main: stdin, inline --job, or job file argument
  cli_main->>cli_main: validate explicit arguments
  cli_main->>JobFile: bounded regular-file read with identity checks
  JobFile-->>cli_main: bounded UTF-8 bytes
  cli_main->>JSONParser: decode and parse JSON
  JSONParser-->>cli_main: parsed job or structured error
Loading

Possibly related issues

๐Ÿšฅ Pre-merge checks | โœ… 5
โœ… Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage โœ… Passed Docstring coverage is 96.00% which is sufficient. The required threshold is 80.00%.
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.
Description Check โœ… Passed Check skipped - CodeRabbitโ€™s high-level summary is enabled.
Title check โœ… Passed PR ์ œ๋ชฉ์€ CLI์˜ ๋ฌด์ œํ•œ ํŒŒ์ผ ์ฝ๊ธฐ ์ทจ์•ฝ์  ์ˆ˜์ •์ด๋ผ๋Š” ํ•ต์‹ฌ ๋ณ€๊ฒฝ์„ ์ •ํ™•ํ•˜๊ณ  ๊ฐ„๊ฒฐํ•˜๊ฒŒ ์„ค๋ช…ํ•ฉ๋‹ˆ๋‹ค.
โœจ Finishing Touches ๐Ÿ’ก 1
๐Ÿ› ๏ธ Fix failing CI checks ๐Ÿ’ก
  • Create stacked PR
  • Commit on current branch
๐Ÿ“ Generate docstrings
  • Create stacked PR
  • Commit on current branch
๐Ÿงช Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-cli-unbounded-read-5165758910965089497

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.

@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 ec183bf199e0e82b6c29d6c2a01b795e31746e97.

  • Head SHA: ec183bf199e0e82b6c29d6c2a01b795e31746e97

  • Workflow run: 31338219474

  • Workflow attempt: 1

Coverage evidence

Coverage evidence job did not run or did not publish coverage evidence.

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"]
  Evidence --> S2["Test (2 files)"]
  S2 --> I2["regression suite"]
  I2 --> R2["Review risk: Test (2 files)"]
  R2 --> V2["targeted test run"]
Loading

@opencode-agent

opencode-agent Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

OpenCode Review Overview

Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment.

@seonghobae
seonghobae marked this pull request as draft August 14, 2026 11:15

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Please rerun the coverage/review contract on exact current head e156cf00cee63068e098137c6d2cf2d1c2a55aea. The branch now differs from protected develop in exactly the intended three CLI bounded-read files; unrelated .trivyignore, pdfjs-dist, and root lock drift was restored from the base. Treat predecessor-head ec183bf199e0e82b6c29d6c2a01b795e31746e97 review evidence as historical and publish current-head coverage/docstring evidence before changing review state.

@seonghobae
seonghobae force-pushed the fix-cli-unbounded-read-5165758910965089497 branch from 8bab6d6 to 071ba74 Compare August 14, 2026 20:04
@seonghobae seonghobae added bug Something isn't working type: bug Defect or incorrect behavior labels Sep 7, 2026 — with ChatGPT Codex Connector
@seonghobae
seonghobae marked this pull request as draft September 7, 2026 02:30
seonghobae and others added 6 commits September 7, 2026 11:49
โ€ฆtifiers

Preserve the CLI security delta while integrating the protected workflow and dependency baseline through ordinary two-parent history. The new naming and retired-hook contracts are intentionally RED until the production repair follows.
Replace ambiguous private CLI input/file-authority names, remove the retired TemporalAnalyzer test seam, and make tests observe the orchestration API boundary. Integrate #1176's exact formatting delta as a parent instead of duplicating its change. Document the coverage working-directory RCA and product-technical gap baseline.
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 bug Something isn't working priority: high High-priority or P1 work type: bug Defect or incorrect behavior type: security Security vulnerability or security-specific remediation

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants