test: enforce 100% coverage and docstring quality gates (v3) - #41
test: enforce 100% coverage and docstring quality gates (v3)#41seonghobae wants to merge 11 commits into
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Cache: Disabled due to Reviews > Disable Cache setting Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughCI 워크플로우에 AST 기반 docstring 커버리지(100%) 검사와 의존성 스냅샷 제출/검토 단계, PR 코멘트 기반 의존성 경고 게이트 및 관련 문서·테스트·스크립트·일부 모듈의 docstring/표기 보강을 추가했습니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as "GitHub Actions (workflow)"
participant Runner as "Runner (checkout + deps)"
participant Tests as "pytest (coverage enforced)"
participant DocCheck as "DocstringCheck (scripts/docstring_coverage.py)"
participant DepSubmit as "DependencySubmission Action"
GH->>Runner: checkout & setup
Runner->>Tests: run pytest (includes --cov-fail-under=100)
Tests-->>GH: exit code / results
Runner->>DocCheck: run python scripts/docstring_coverage.py --min-percent 100
DocCheck-->>GH: report + exit code (may fail job)
GH->>DepSubmit: conditional submit (push or PR non-fork)
DepSubmit->>GH: submit dependency snapshot
GH-->>GH: downstream build/publish gated on results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
|
PR continuity: related/duplicate PRs detected and handled. Duplicates (same repo): |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Added commit 4b0c4a1 to create a post-merge validation harness for issues #38 and #40. The new script scripts/review_checks/dependency_review_warning_gate.py parses dependency-review bot comments and enforces snapshot/unknown-license warning policy thresholds, with TDD coverage in tests/test_dependency_review_warning_gate.py and docs linkage in harness engineering docs. @coderabbitai review |
🧩 Analysis chainLet me inspect the new files from the commit before reviewing. 🏁 Script executed: Length of output: 10168 Good — files read. Let me also check the docs linkage and any existing coverage configuration. 🏁 Script executed: Length of output: 2412
|
| Untested path | Risk |
|---|---|
parse_dependency_review_comment with clean body (no warnings) |
has_snapshot_warning=False, unknown_license_count=0 |
evaluate_warning_policy pass case (ok=True) |
Only failure paths are tested |
find_latest_dependency_review_comment_body returning None |
No test for the "no matching comment" branch |
_decode_comment_entry directly |
The double-decode logic is tricky; isolated unit test would be valuable |
main() exit codes 0, 1, 2 |
Entire CLI entrypoint untested |
fetch_issue_comments / _run_gh |
Require subprocess mocking (unittest.mock.patch) |
Since --cov=vector_topic_modeling is scoped to src/vector_topic_modeling only, these gaps won't trip the CI 100% gate — but they leave real functional risk uncovered, especially the missing-comment path (exit code 2) and the main() pass/fail paths.
5. MODULE = _load_module() at module scope
Loading and executing the module at import time (module scope) means any import-time side effect in the script would affect all test collection. This is fine given the current script, but it's fragile. A session-scoped pytest fixture would be more conventional and explicit.
Docs
6. harness-engineering.md linkage ✅
The new entry correctly documents the required and optional args and is consistent with the actual parse_args() implementation.
Summary
The core parsing and policy logic is well-structured. The main actionable items are:
- Add a test for the clean-pass path of
evaluate_warning_policy - Add a test for
find_latest_dependency_review_comment_bodyreturningNone - Add a test for
parse_dependency_review_commentwith a body that has no warnings - Add tests for
_decode_command_entry(direct unit test) andmain()(usingunittest.mock.patchforsubprocess.run) - Clarify or remove the
body.replace("\\n", "\n")line with a comment - Consider moving the
max(…, 0)clamp intoevaluate_warning_policy
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
|
@coderabbitai pause |
|
@coderabbitai resume |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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/docstring_coverage.py`:
- Around line 110-111: The comparison uses an unnecessary float() cast; change
the check in the block using report.percent and args.min_percent to compare
directly (i.e., replace "if report.percent < float(args.min_percent):" with a
direct comparison using args.min_percent) so the code relies on argparse's
type=float and avoids the redundant conversion while preserving the existing
return behavior.
- Around line 27-38: _iter_symbol_docstrings currently calls ast.parse without
handling SyntaxError; wrap the ast.parse(...) call in a try/except that catches
SyntaxError (and optionally UnicodeDecodeError), and on exception log or warn
including the offending file_path and the exception text, then return an empty
list (i.e. skip that file) so a single bad file doesn't abort the whole run;
keep the rest of the function (building symbols, using ast.get_docstring)
unchanged so callers still get an empty result for skippable files.
- Around line 11-17: The DocstringCoverageReport dataclass currently uses a
mutable list for missing_symbols while being frozen; change the field type to an
immutable tuple (e.g., tuple[str, ...]) in the DocstringCoverageReport
definition and update any producer code (notably
build_docstring_coverage_report) to construct and return a tuple for
missing_symbols instead of a list (convert with tuple(missing_symbols) or build
as tuple) so the report is truly immutable and safe from in-place mutation.
In `@scripts/review_checks/dependency_review_warning_gate.py`:
- Around line 84-103: fetch_issue_comments calls _run_gh which uses check=True
and can raise subprocess.CalledProcessError; modify main() to wrap calls that
invoke fetch_issue_comments/_run_gh in a try/except that specifically catches
subprocess.CalledProcessError, import subprocess if missing, log or print a
concise, user-friendly error message including e.returncode and e.cmd (or
str(e)) and then exit with a non-zero status so the process fails cleanly
instead of propagating a full stack trace.
- Around line 65-75: The _decode_comment_entry function currently calls
json.loads(candidate) (and a second json.loads(item) when item is a string)
without handling json.JSONDecodeError; wrap each json.loads call in try/except
json.JSONDecodeError and on exception return None (or otherwise skip the invalid
line) so a malformed JSON line does not raise and stop the script; ensure you
reference the json.loads calls inside _decode_comment_entry and handle both the
initial parse and the secondary parse when isinstance(item, str).
- Around line 169-171: The call is doing redundant type conversions: remove the
unnecessary int(...) and bool(...) wrappers and pass args.max_unknown_licenses
and args.allow_snapshot_warning directly to the constructor/function (the
parameters named max_unknown_licenses and allow_snapshot_warning). Ensure
argparse is configured to provide an int for args.max_unknown_licenses and a
boolean (store_true) for args.allow_snapshot_warning so the direct values have
the correct types.
- Around line 78-81: The subprocess call in _run_gh triggers static-analysis
S603 because owner/repo/pull_number come from CLI; either explicitly mark this
call as an accepted risk with a trailing "# noqa: S603" comment on the _run_gh
function or add input validation earlier (e.g., in parse_args) to sanitize owner
and repo (validate with a safe regex like ^[\w.-]+$ and ensure pull_number is an
integer) before they are used to build the gh API path; update the repository to
use one of these approaches and ensure the change references _run_gh (and
parse_args if adding validation).
In `@tests/test_clustering.py`:
- Around line 225-228: RescueDisplayDominanceResult's field names don't match
the test keys (tests expect "display_top_share_before"/"display_top_share_after"
but the TypedDict in RescueDisplayDominanceResult defines
"top_cluster_share_before"/"top_cluster_share_after"); update the TypedDict in
src/vector_topic_modeling/clustering.py (RescueDisplayDominanceResult) to use
the display_* key names or change the code that constructs the result to emit
the display_top_share_before/after keys so that the produced dict keys match the
test's res["display_top_share_before"] and res["display_top_share_after"]
lookups.
In `@tests/test_dependency_review_warning_gate.py`:
- Around line 14-22: In _load_module(), split the combined assertion `assert
spec is not None and spec.loader is not None` into two separate checks so
failures indicate which is None; e.g., first `assert spec is not None` (or raise
with message referencing `spec`) and then `assert spec.loader is not None` (or
raise with message referencing `spec.loader`) before calling
`spec.loader.exec_module(module)`, so debugging of the _load_module function
clearly shows whether spec or spec.loader was the cause.
In `@tests/test_docstring_coverage.py`:
- Around line 15-23: The compound assertion in _load_docstring_coverage_module()
makes debugging harder; split "assert spec is not None and spec.loader is not
None" into two separate assertions that check spec and spec.loader individually
(e.g., assert spec is not None with a message, then assert spec.loader is not
None with a message) so failures identify whether spec creation or loader
resolution failed when executing spec.loader.exec_module(module).
- Around line 89-93: The lambda used in monkeypatch.setattr for
build_docstring_coverage_report declares repo_root and src_root but doesn’t use
them; to silence Ruff ARG005, rename those parameters to use an underscore
prefix (e.g., _repo_root and _src_root) in the lambda signature so the call
still matches build_docstring_coverage_report while marking the args as
intentionally unused; update the lambda passed to monkeypatch.setattr in the
test to accept the underscored names.
In `@tests/test_pipeline.py`:
- Around line 150-152: Add the missing type annotation to the pytest monkeypatch
fixture for the test function
test_fit_predict_allows_sessions_without_selected_representative: change the
parameter signature to annotate monkeypatch with pytest.MonkeyPatch so the
function reads monkeypatch: pytest.MonkeyPatch, and ensure pytest is imported if
not already referenced in the test module.
🪄 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: 3159aa7d-4a27-41e7-99c4-74c3d0e76a23
📒 Files selected for processing (34)
.github/workflows/ci.yml.github/workflows/dependency-review.yml.github/workflows/dependency-submission.yml.github/workflows/publish.yml.github/workflows/release.ymlARCHITECTURE.mddocs/engineering/acceptance-criteria.mddocs/engineering/harness-engineering.mddocs/maintainers/releasing.mddocs/operations/deploy-runbook.mddocs/security/api-security-checklist.mddocs/workflow/one-day-delivery-plan.mddocs/workflow/pr-continuity.mdpyproject.tomlscripts/docstring_coverage.pyscripts/review_checks/dependency_review_warning_gate.pysrc/vector_topic_modeling/_sanitize.pysrc/vector_topic_modeling/clustering.pysrc/vector_topic_modeling/ingestion.pysrc/vector_topic_modeling/pipeline.pysrc/vector_topic_modeling/providers/base.pysrc/vector_topic_modeling/providers/openai_compat.pysrc/vector_topic_modeling/service.pysrc/vector_topic_modeling/sessioning.pysrc/vector_topic_modeling/text.pytests/test_clustering.pytests/test_dependency_review_warning_gate.pytests/test_docs_ci_policy_alignment.pytests/test_docs_release_gate_alignment.pytests/test_docstring_coverage.pytests/test_ingestion.pytests/test_pipeline.pytests/test_pyproject_license_metadata.pytests/test_sessioning.py
✅ Actions performedReviews paused. |
✅ Actions performedReviews resumed. |
|
Follow-up commit 0f70ee7 addresses the incremental review gaps in the warning-gate harness. Added tests for clean parse path, allow/pass policy path, negative threshold normalization, none-path for comment discovery, double-encoded comment decoding, and main() exit codes 0/1/2. Also normalized max_unknown_licenses inside evaluate_warning_policy and simplified comment normalization handling. @coderabbitai review |
🧩 Analysis chainLet me read the updated files from the latest commit to verify the fixes. 🏁 Script executed: Length of output: 13387
One trivial nit: The harness is solid. No further concerns from the previous review thread. ✅ Actions performedReview triggered.
|
|
@coderabbitai resume |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resolve |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Pull request was closed
🧩 Analysis chain🏁 Script executed: Length of output: 13387
One trivial nit: The harness is solid. No further concerns from the previous review thread. ✅ Actions performedReview triggered.
|
✅ Actions performedReview triggered.
|
✅ Actions performedReviews resumed. |
✅ Actions performedFull review triggered. |
✅ Actions performedComments resolved and changes approved. |
✅ Actions performedReviews resumed. |
✅ Actions performedComments resolved and changes approved. |
1 participant
Summary
Verification
Supersedes #39 as canonical merge path.
📝 Walkthrough
Walkthrough
이 변경은 100% 문서화 문자열 적용 강제, 의존성 검토 워크플로우 구성 업데이트, 라이선스 메타데이터 검증을 추가하고 CI/CD 및 지원 문서 전반에 걸쳐 이러한 정책을 문서화하는 포괄적인 품질 및 보안 개선 사항입니다.
Changes
.github/workflows/ci.yml,.github/workflows/dependency-review.yml,.github/workflows/dependency-submission.yml,.github/workflows/publish.yml,.github/workflows/release.ymlpyproject.tomllicense-files추가; pytest 옵션 확장하여 100% 라인 및 분기 커버리지 강제 적용.scripts/docstring_coverage.py,scripts/review_checks/dependency_review_warning_gate.pysrc/vector_topic_modeling/_sanitize.py,src/vector_topic_modeling/clustering.py,src/vector_topic_modeling/ingestion.py,src/vector_topic_modeling/pipeline.py,src/vector_topic_modeling/providers/...,src/vector_topic_modeling/service.py,src/vector_topic_modeling/sessioning.py,src/vector_topic_modeling/text.pyARCHITECTURE.md,docs/engineering/acceptance-criteria.md,docs/engineering/harness-engineering.md,docs/maintainers/releasing.md,docs/operations/deploy-runbook.md,docs/security/api-security-checklist.md,docs/workflow/one-day-delivery-plan.md,docs/workflow/pr-continuity.mdtests/test_clustering.py,tests/test_dependency_review_warning_gate.py,tests/test_docs_ci_policy_alignment.py,tests/test_docs_release_gate_alignment.py,tests/test_docstring_coverage.py,tests/test_ingestion.py,tests/test_pipeline.py,tests/test_pyproject_license_metadata.py,tests/test_sessioning.pyEstimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
#22: 의존성 검토 GitHub Actions 워크플로우 수정—제출 워크플로우 추가 및 런타임 동작 조정(스냅샷 경고 재시도 및 PR 댓글 정책).#15: 동일한 섭취 서브시스템(TopicDocumentIngestionConfig,load_ingestion_config,load_jsonl_topic_documents) 수정 및 CLI 변경.#31:redact_pii_and_secrets문서화 및 퍼징 하니스 추가로 동일 코드 영역 수정.Poem