chore: enforce python docstring coverage#42
Conversation
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit릴리스 노트
Walkthrough저장소 전반에 Python 도큐스트링(ruff D100–D107) 100% 요구를 문서화하고 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ARCHITECTURE.md`:
- Line 86: Update the docstring gate description in ARCHITECTURE.md so it
matches the actual check implemented by the repository's
"check:python-docstrings" script (which enforces Ruff D100–D107): state that
100% docstring coverage is required for tracked packages, modules, classes,
nested classes, functions, methods (including magic methods like __init__),
tests under services/analysis-engine, and repo-owned Python scripts; reference
the Ruff rules D100–D107 and the package.json script name to make the policy
unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5c7b84b-cf75-4eef-8d5e-caae4dcdd282
📒 Files selected for processing (1)
ARCHITECTURE.md
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/release/package_desktop_artifact.py`:
- Around line 61-76: expected_binary_path() currently uses
BANDSCOPE_TARGET_TRIPLE to pick the binary name, but artifact_identity() still
uses the host platform to form the archive name, causing mismatched archive
names during cross-builds; update artifact_identity() to derive
BANDSCOPE_ARTIFACT_OS and BANDSCOPE_ARTIFACT_ARCH from BANDSCOPE_TARGET_TRIPLE
when that env var is present (use the same mapping logic used in
expected_binary_path() to map "windows"/"apple-darwin"/others to artifact OS and
arch), and if BANDSCOPE_TARGET_TRIPLE is not set and BANDSCOPE_ARTIFACT_OS/ARCH
are missing, raise an explicit error so packaging fails fast rather than
producing a misleading archive name; reference expected_binary_path,
artifact_identity, BANDSCOPE_TARGET_TRIPLE, BANDSCOPE_ARTIFACT_OS, and
BANDSCOPE_ARTIFACT_ARCH when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d03208e-3e6f-44b8-9854-be7babf3437d
📒 Files selected for processing (14)
ARCHITECTURE.mdpackage.jsonscripts/checks/security_gates.pyscripts/checks/verify_docs.pyscripts/checks/verify_github_bootstrap_policy.pyscripts/checks/verify_security_notes.pyscripts/checks/verify_supply_chain.pyscripts/release/package_desktop_artifact.pyservices/analysis-engine/src/bandscope_analysis/health.pyservices/analysis-engine/tests/conftest.pyservices/analysis-engine/tests/test_api.pyservices/analysis-engine/tests/test_health.pyservices/analysis-engine/tests/test_release_packaging.pyservices/analysis-engine/tests/test_supply_chain_policy.py
| def expected_binary_path(repo_root: Path) -> Path: | ||
| system = normalized_platform() | ||
| """Return the expected desktop binary path for the selected target triple.""" | ||
| target_triple = os.environ.get("BANDSCOPE_TARGET_TRIPLE") | ||
| if target_triple and "windows" in target_triple: | ||
| system = "windows" | ||
| elif target_triple and "apple-darwin" in target_triple: | ||
| system = "macos" | ||
| else: | ||
| system = normalized_platform() | ||
| binary_name = ( | ||
| "bandscope-desktop.exe" if system == "windows" else "bandscope-desktop" | ||
| ) | ||
| target_root = repo_root / "apps" / "desktop" / "src-tauri" / "target" | ||
| if target_triple := os.environ.get("BANDSCOPE_TARGET_TRIPLE"): | ||
| if target_triple: | ||
| target_root = target_root / target_triple | ||
| return target_root / "release" / binary_name |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether package_desktop_artifact.py is ever invoked with
# BANDSCOPE_TARGET_TRIPLE but without BANDSCOPE_ARTIFACT_OS / BANDSCOPE_ARTIFACT_ARCH.
rg -n -C3 'package_desktop_artifact\.py|BANDSCOPE_TARGET_TRIPLE|BANDSCOPE_ARTIFACT_OS|BANDSCOPE_ARTIFACT_ARCH'Repository: seonghobae/bandscope
Length of output: 9738
🏁 Script executed:
#!/bin/bash
# Read the test that checks Windows target on non-Windows host (lines 56-76)
sed -n '56,76p' services/analysis-engine/tests/test_release_packaging.pyRepository: seonghobae/bandscope
Length of output: 784
🏁 Script executed:
#!/bin/bash
# Read the full main() function to see how binary path and artifact identity are used together
sed -n '107,150p' scripts/release/package_desktop_artifact.pyRepository: seonghobae/bandscope
Length of output: 1415
🏁 Script executed:
#!/bin/bash
# Check if there's any validation logic around BANDSCOPE_TARGET_TRIPLE and artifact overrides
grep -n "raise\|ValueError\|AssertionError\|if.*BANDSCOPE_TARGET_TRIPLE" scripts/release/package_desktop_artifact.pyRepository: seonghobae/bandscope
Length of output: 346
타깃 트리플과 아티팩트 식별자가 분리된 상태에서 교차 빌드 시 아카이브 이름 불일치 위험
expected_binary_path()는 BANDSCOPE_TARGET_TRIPLE로 올바른 바이너리 이름(.exe 등)을 선택하지만, artifact_identity()는 계속 호스트 플랫폼 값으로 archive_name을 생성합니다. BANDSCOPE_ARTIFACT_OS와 BANDSCOPE_ARTIFACT_ARCH를 설정하지 않은 채로 Windows 타깃 트리플로 macOS 호스트에서 패키징하면, 아카이브 파일명은 "macos"를 포함하지만 내용은 Windows 바이너리가 될 수 있습니다. 타깃 트리플에서 식별자를 함께 유도하거나, 교차 빌드 시 두 환경변수 누락을 명시적으로 실패 처리해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/release/package_desktop_artifact.py` around lines 61 - 76,
expected_binary_path() currently uses BANDSCOPE_TARGET_TRIPLE to pick the binary
name, but artifact_identity() still uses the host platform to form the archive
name, causing mismatched archive names during cross-builds; update
artifact_identity() to derive BANDSCOPE_ARTIFACT_OS and BANDSCOPE_ARTIFACT_ARCH
from BANDSCOPE_TARGET_TRIPLE when that env var is present (use the same mapping
logic used in expected_binary_path() to map "windows"/"apple-darwin"/others to
artifact OS and arch), and if BANDSCOPE_TARGET_TRIPLE is not set and
BANDSCOPE_ARTIFACT_OS/ARCH are missing, raise an explicit error so packaging
fails fast rather than producing a misleading archive name; reference
expected_binary_path, artifact_identity, BANDSCOPE_TARGET_TRIPLE,
BANDSCOPE_ARTIFACT_OS, and BANDSCOPE_ARTIFACT_ARCH when making the change.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/release/package_desktop_artifact.py (1)
47-58:⚠️ Potential issue | 🔴 Critical교차 빌드 시 아카이브/매니페스트 식별자가 여전히 호스트 기준입니다.
expected_binary_path()는BANDSCOPE_TARGET_TRIPLE을 반영하지만artifact_identity()는 아직 호스트의normalized_platform()/normalized_architecture()를 사용합니다. 그래서 예를 들어 macOS 호스트에서 Windows triple로 패키징하면 zip/manifest 이름은macos-*로 남고 내용물만 Windows 바이너리가 되어 릴리스 추적성이 깨집니다.artifact_identity()도 target triple에서 platform/arch를 유도하거나, 교차 빌드에서BANDSCOPE_ARTIFACT_OS와BANDSCOPE_ARTIFACT_ARCH가 없으면 즉시 실패시켜야 합니다.수정 방향 예시
def artifact_identity() -> dict[str, str]: """Build the archive and manifest names for the current artifact target.""" git_sha = os.environ.get("GITHUB_SHA", "local")[:12] - target_platform = normalized_platform() - target_arch = normalized_architecture() + target_platform, target_arch = resolved_artifact_target() suffix = f"bandscope-{target_platform}-{target_arch}-{git_sha}"def resolved_artifact_target() -> tuple[str, str]: target_triple = os.environ.get("BANDSCOPE_TARGET_TRIPLE") if target_triple: arch = target_triple.split("-", 1)[0] platform_name = ( "windows" if "windows" in target_triple else "macos" if "apple-darwin" in target_triple else "linux" ) normalized_arch = ( "arm64" if arch in {"aarch64", "arm64"} else "amd64" if arch in {"x86_64", "amd64"} else arch ) return platform_name, normalized_arch if ( "BANDSCOPE_ARTIFACT_OS" not in os.environ or "BANDSCOPE_ARTIFACT_ARCH" not in os.environ ): raise ValueError( "Set BANDSCOPE_TARGET_TRIPLE or both " "BANDSCOPE_ARTIFACT_OS and BANDSCOPE_ARTIFACT_ARCH" ) return normalized_platform(), normalized_architecture()Also applies to: 61-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/package_desktop_artifact.py` around lines 47 - 58, artifact_identity() currently uses host normalized_platform()/normalized_architecture(), causing archive/manifest names to mismatch cross-builds; update artifact_identity() to derive platform/arch from the target triple or explicit env vars (like BANDSCOPE_TARGET_TRIPLE or BANDSCOPE_ARTIFACT_OS/BANDSCOPE_ARTIFACT_ARCH) similar to expected_binary_path() — implement a helper (e.g., resolved_artifact_target()) that parses BANDSCOPE_TARGET_TRIPLE into a normalized platform ("windows"/"macos"/"linux") and arch ("arm64"/"amd64"/etc.), have artifact_identity() call that helper to build suffix/archive_name/manifest_name, and if neither the triple nor both artifact env vars are present, raise an error immediately to fail fast during cross-builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/checks/security_gates.py`:
- Line 1: The module docstring claims it scans "tracked" files but the
implementation uses Path(".").rglob("*") (see Path(".").rglob) which iterates
the whole working tree; update the code to make behavior and docs consistent:
either change the top-level docstring to state it scans the entire workspace, or
change the file-collection logic to only include tracked files (e.g., replace
Path(".").rglob("*") usage with a git-tracked file list via subprocess/gitpython
or filter by tracked paths returned from `git ls-files`) and ensure functions
that rely on that list (the file iteration loop) use the new tracked-only list.
In `@scripts/checks/verify_security_notes.py`:
- Around line 39-40: Update the main() docstring to accurately reflect that the
function returns a failing exit code not only when the "Security Notes" section
itself is missing, but also when required subsections inside that section are
absent (i.e., the presence and completeness of mandatory sub-sections checked
later in the function such as those validated in the block around lines checking
for required sub-sections). Mention the specific behavior: it fails for a
missing Security Notes section or for missing required subsections, so readers
of the code understand the full validation performed by main().
---
Duplicate comments:
In `@scripts/release/package_desktop_artifact.py`:
- Around line 47-58: artifact_identity() currently uses host
normalized_platform()/normalized_architecture(), causing archive/manifest names
to mismatch cross-builds; update artifact_identity() to derive platform/arch
from the target triple or explicit env vars (like BANDSCOPE_TARGET_TRIPLE or
BANDSCOPE_ARTIFACT_OS/BANDSCOPE_ARTIFACT_ARCH) similar to expected_binary_path()
— implement a helper (e.g., resolved_artifact_target()) that parses
BANDSCOPE_TARGET_TRIPLE into a normalized platform ("windows"/"macos"/"linux")
and arch ("arm64"/"amd64"/etc.), have artifact_identity() call that helper to
build suffix/archive_name/manifest_name, and if neither the triple nor both
artifact env vars are present, raise an error immediately to fail fast during
cross-builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4aeac504-aa62-493e-887c-86d1d4376646
📒 Files selected for processing (14)
ARCHITECTURE.mdpackage.jsonscripts/checks/security_gates.pyscripts/checks/verify_docs.pyscripts/checks/verify_github_bootstrap_policy.pyscripts/checks/verify_security_notes.pyscripts/checks/verify_supply_chain.pyscripts/release/package_desktop_artifact.pyservices/analysis-engine/src/bandscope_analysis/health.pyservices/analysis-engine/tests/conftest.pyservices/analysis-engine/tests/test_api.pyservices/analysis-engine/tests/test_health.pyservices/analysis-engine/tests/test_release_packaging.pyservices/analysis-engine/tests/test_supply_chain_policy.py
| @@ -1,3 +1,5 @@ | |||
| """Scan tracked source files for disallowed security patterns.""" | |||
There was a problem hiding this comment.
tracked라는 표현이 현재 동작과 맞지 않습니다.
이 스크립트는 Line 47의 Path(".").rglob("*")로 작업 트리 전체를 훑기 때문에, 현재 docstring처럼 추적된 파일만 검사하는 동작은 아닙니다. 로컬 quickcheck가 임시/미추적 파일에 의해 흔들릴 수 있으니, 설명을 실제 동작에 맞추거나 파일 수집 로직을 추적 파일 기준으로 맞춰 주세요.
예시 수정안
-"""Scan tracked source files for disallowed security patterns."""
+"""Scan repository files for disallowed security patterns."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Scan tracked source files for disallowed security patterns.""" | |
| """Scan repository files for disallowed security patterns.""" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/checks/security_gates.py` at line 1, The module docstring claims it
scans "tracked" files but the implementation uses Path(".").rglob("*") (see
Path(".").rglob) which iterates the whole working tree; update the code to make
behavior and docs consistent: either change the top-level docstring to state it
scans the entire workspace, or change the file-collection logic to only include
tracked files (e.g., replace Path(".").rglob("*") usage with a git-tracked file
list via subprocess/gitpython or filter by tracked paths returned from `git
ls-files`) and ensure functions that rely on that list (the file iteration loop)
use the new tracked-only list.
| def main() -> int: | ||
| """Return a failing exit code when plan files are missing security notes.""" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
main() docstring이 실제 실패 조건을 덜 설명합니다.
이 함수는 Security Notes 섹션 자체가 없을 때만 아니라, Line 48-50처럼 필수 하위 섹션이 빠져도 실패를 반환합니다. 현재 문구면 체크 범위를 오해하기 쉬우니 설명을 넓혀 두는 편이 좋겠습니다.
예시 수정안
def main() -> int:
- """Return a failing exit code when plan files are missing security notes."""
+ """Return a failing exit code when plan files are missing Security Notes or required subsections."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def main() -> int: | |
| """Return a failing exit code when plan files are missing security notes.""" | |
| def main() -> int: | |
| """Return a failing exit code when plan files are missing Security Notes or required subsections.""" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/checks/verify_security_notes.py` around lines 39 - 40, Update the
main() docstring to accurately reflect that the function returns a failing exit
code not only when the "Security Notes" section itself is missing, but also when
required subsections inside that section are absent (i.e., the presence and
completeness of mandatory sub-sections checked later in the function such as
those validated in the block around lines checking for required sub-sections).
Mention the specific behavior: it fails for a missing Security Notes section or
for missing required subsections, so readers of the code understand the full
validation performed by main().
|
Superseded by #43 to clear repeated stale CodeRabbit review state after fresh fixes and green verification. |
Pull request was closed
1 participant
Summary
Verification
npm run check:python-docstrings./scripts/harness/quickcheck.sh📝 Walkthrough
Walkthrough
services/analysis-engine과 저장소 파이썬 검사 파이프라인에 100% 도큐스트링 요구를 추가하고, 린트 스크립트와 테스트 기대값을 업데이트했습니다. 기능적 동작 변화는 없고 문서화와 검증 확장이 주된 내용입니다.Changes
ARCHITECTURE.mdservices/analysis-engine, 해당 테스트 및 저장소 소유 Python 스크립트에 대해 100% 도큐스트링 요구 추가.package.jsoncheck:python-docstrings스크립트 추가(ruff D100–D107) 및lint체인에 통합.scripts/checks/security_gates.py,scripts/checks/verify_docs.py,scripts/checks/verify_github_bootstrap_policy.py,scripts/checks/verify_security_notes.py,scripts/checks/verify_supply_chain.pyscripts/release/package_desktop_artifact.pyexpected_binary_path에BANDSCOPE_TARGET_TRIPLE기반 플랫폼/타겟 처리 언급(기능 보존).services/analysis-engine/src/bandscope_analysis/health.pyHealthReportTypedDict에 도큐스트링 추가(문서화).services/analysis-engine/tests/...services/analysis-engine/tests/conftest.py,services/analysis-engine/tests/test_api.py,services/analysis-engine/tests/test_health.py,services/analysis-engine/tests/test_supply_chain_policy.pypipeline_stages키 검증 항목 확장.services/analysis-engine/tests/test_release_packaging.pyEstimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
scripts/release/package_desktop_artifact.py및scripts/checks/verify_supply_chain.py관련 변경과 테스트 수정이 겹침.Poem