Doc/spec kit and project sync #200
Conversation
WalkthroughLimits CI pytest scope to tests/unit, restructures governance docs and templates, deletes a shared Bash helper, removes a top-level integration script, and adds a guarded integration test under tests/integration. Adjusts unit test imports and adds comprehensive generator tests. Updates CONTRIBUTING and DEVELOPER docs, including branch naming conventions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant Py as pytest (tests/unit)
participant IT as pytest (tests/integration)
participant BC as BulkSubIssueCollector
participant GHAPI as GitHub API
Dev->>GH: Push / PR
GH->>Py: Run unit tests (tests/unit)
alt Integration tests gated
Py-->>GH: Unit results
GH->>IT: Run integration tests (if GITHUB_TOKEN)
IT->>BC: instantiate with CollectorConfig (TLS verify off)
IT->>BC: scan_sub_issues_for_parents(parents)
BC->>GHAPI: fetch sub-issues
GHAPI-->>BC: results
BC-->>IT: parents_sub_issues updated
IT-->>GH: Integration results
else No token
GH-->>Dev: Integration tests skipped
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
.github/workflows/test.yml (2)
106-108: Verify PYTHONPATH value matches project layout.Docs use src layout (“export PYTHONPATH=.../src”), but here PYTHONPATH points to release_notes_generator/release_notes_generator. Please confirm; if using src/ layout, adjust:
- - name: Set PYTHONPATH environment variable - run: echo "PYTHONPATH=${GITHUB_WORKSPACE}/release_notes_generator/release_notes_generator" >> $GITHUB_ENV + - name: Set PYTHONPATH environment variable + run: echo "PYTHONPATH=${GITHUB_WORKSPACE}/src" >> $GITHUB_ENV
110-110: Add a dedicated integration-tests jobThe pytest step only targets
tests/unit, sotests/integration/integration_test.pyis never executed. Add a separate (or conditional) job to run integration tests and prevent silent regressions.CONTRIBUTING.md (1)
28-40: Clarify the rename guidance and add a quick self-check tip.Generalize the rename example and provide a one-liner to verify the current branch.
- Rename if needed before pushing: - ```shell - git branch -m fix/<new-name> - ``` + Rename if needed before pushing: + ```shell + git branch -m <prefix>/<new-name> + ``` + Quick check: + ```shell + git rev-parse --abbrev-ref HEAD | grep -E '^(feature|fix|docs|chore)/' || echo 'Branch naming violation' + ```DEVELOPER.md (2)
128-129: Align wording with unit-only commands; add optional note for integration.Commands target tests/unit; update nearby text accordingly and hint how to run integration tests.
- This will execute all tests located in the tests directory. + This will execute all unit tests located in tests/unit.- This will execute all tests in the tests directory and generate a code coverage report. + This will execute all unit tests (tests/unit) and generate a code coverage report. + To run integration tests locally: + ```shell + pytest -v tests/integration + ```Also applies to: 138-140, 131-131, 142-142
183-203: Branch naming section looks solid; consider DRYing with CONTRIBUTING.To avoid drift, add a link back to CONTRIBUTING’s Branch Naming section or centralize in one doc.
.specify/templates/spec-template.md (1)
8-19: Make alignment items checkable to drive accountability.Switch bullets to markdown checkboxes so authors can tick compliance in specs.
- List how this feature will comply with core principles: - - Test‑First (P1): ... + Confirm compliance with core principles: + - [ ] Test‑First (P1): ... + - [ ] Explicit Configuration Boundaries (P2): ... + - [ ] Deterministic Output (P3): ... + - [ ] Lean Python Design (P8): ... + - [ ] Localized Error Handling (P9): ... + - [ ] Dead Code Prohibition (P10): ... + - [ ] Focused Comments (P11): ... + - [ ] Test Path Mirroring (P12): ... + - [ ] Branch Naming Consistency (P13): ...tests/unit/release_notes_generator/test_action_inputs.py (3)
331-331: Tighten mocks for robustness and lint-friendliness.
- Return an empty list instead of None from get_issues_for_pr to match expected iterable type.
- Honor defaults in get_action_input side_effect; also fixes Ruff ARG005.
- Patch booleans with True/False, not "true"/"false".
- mocker.patch("release_notes_generator.record.factory.default_record_factory.get_issues_for_pr", return_value=None) + mocker.patch("release_notes_generator.record.factory.default_record_factory.get_issues_for_pr", return_value=[])- mocker.patch("release_notes_generator.record.factory.default_record_factory.get_issues_for_pr", return_value=None) + mocker.patch("release_notes_generator.record.factory.default_record_factory.get_issues_for_pr", return_value=[])- mocker.patch("release_notes_generator.record.factory.default_record_factory.get_issues_for_pr", return_value=None) + mocker.patch("release_notes_generator.record.factory.default_record_factory.get_issues_for_pr", return_value=[])- mock_get_action_input.side_effect = lambda first_arg, **kwargs: ( - "{number} _{title}_ in {pull-requests} {unknown} {another-unknown}" if first_arg == ROW_FORMAT_ISSUE else None - ) + def _fake_get_action_input(key, default=None): + if key == ROW_FORMAT_ISSUE: + return "{number} _{title}_ in {pull-requests} {unknown} {another-unknown}" + return default + mock_get_action_input.side_effect = _fake_get_action_input- mocker.patch("release_notes_generator.action_inputs.ActionInputs.get_published_at", return_value="true") + mocker.patch("release_notes_generator.action_inputs.ActionInputs.get_published_at", return_value=True)Also applies to: 375-375, 415-415, 371-374, 395-395
35-35: Prefer boolean over string in success_case.Use True for is_coderabbit_support_active to be type-correct and future-proof.
- "is_coderabbit_support_active": "true", + "is_coderabbit_support_active": True,
285-294: Consider splitting generator tests into their own file.Keeping ActionInputs and Generator tests separate (e.g., tests/unit/release_notes_generator/test_generator.py) improves cohesion and discovery.
.specify/templates/plan-template.md (1)
32-41: Clarify the shell command's purpose in the template.The inline shell command on line 41 appears to be documentation/guidance rather than an executable verification step. Consider clarifying this with a note like "Example verification command:" or moving it to a code block with clear context.
Apply this diff to clarify:
-- Branch Naming Consistency: Confirm current branch uses allowed prefix (feature|fix|docs|chore): - `git rev-parse --abbrev-ref HEAD | grep -E '^(feature|fix|docs|chore)/'`. +- Branch Naming Consistency: Confirm current branch uses allowed prefix (feature|fix|docs|chore). + Example verification command: + ```shell + git rev-parse --abbrev-ref HEAD | grep -E '^(feature|fix|docs|chore)/' + ```tests/integration/integration_test.py (1)
1-2: Add more context to the integration test documentation.The comment mentions "Constitution test directory structure" but doesn't explain what this test validates or when it should run.
Apply this diff to improve documentation:
-# Integration test relocated per Constitution test directory structure. -# Performs a smoke scan of sub-issues when GITHUB_TOKEN is provided; otherwise skipped. +""" +Integration test for BulkSubIssueCollector (relocated per Constitution Principle 12). + +This test validates the sub-issue collection functionality against a live GitHub repository. +It requires a GITHUB_TOKEN environment variable with repository read access. + +Note: This test is skipped in CI unless GITHUB_TOKEN is explicitly provided. +""".specify/memory/constitution.md (1)
1-151: Clarify or consolidate constitution file purposes
The two files differ significantly:.specify/memory/constitution.mdholds only core governance principles, whereas.specify/constitution.mdis the full action specification (architecture, modules, workflows). Rename them for clarity or merge overlapping content to ensure a single source of truth for principles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/test.yml(1 hunks).specify/constitution.md(8 hunks).specify/memory/constitution.md(1 hunks).specify/scripts/bash/common.sh(0 hunks).specify/templates/plan-template.md(3 hunks).specify/templates/spec-template.md(1 hunks).specify/templates/tasks-template.md(2 hunks)CONTRIBUTING.md(2 hunks)DEVELOPER.md(3 hunks)integration_test.py(0 hunks)tests/integration/integration_test.py(1 hunks)tests/unit/release_notes_generator/builder/test_release_notes_builder.py(1 hunks)tests/unit/release_notes_generator/data/test_filter.py(0 hunks)tests/unit/release_notes_generator/data/test_miner.py(1 hunks)tests/unit/release_notes_generator/record/factory/test_default_record_factory.py(1 hunks)tests/unit/release_notes_generator/test_action_inputs.py(2 hunks)tests/unit/release_notes_generator/test_generator.py(3 hunks)tests/utils/__init__.py(0 hunks)
💤 Files with no reviewable changes (4)
- tests/utils/init.py
- integration_test.py
- .specify/scripts/bash/common.sh
- tests/unit/release_notes_generator/data/test_filter.py
🧰 Additional context used
🧬 Code graph analysis (6)
tests/unit/release_notes_generator/record/factory/test_default_record_factory.py (1)
tests/unit/conftest.py (1)
mock_safe_call_decorator(51-56)
tests/unit/release_notes_generator/builder/test_release_notes_builder.py (1)
tests/unit/conftest.py (2)
mock_safe_call_decorator(51-56)MockLabel(44-46)
tests/integration/integration_test.py (1)
release_notes_generator/data/utils/bulk_sub_issue_collector.py (2)
CollectorConfig(21-41)BulkSubIssueCollector(44-234)
tests/unit/release_notes_generator/test_action_inputs.py (4)
release_notes_generator/action_inputs.py (1)
ActionInputs(64-517)release_notes_generator/generator.py (1)
ReleaseNotesGenerator(42-118)tests/unit/conftest.py (7)
mock_repo(103-106)mock_issue_closed(201-223)mock_issue_closed_i1_bug(227-250)mock_pull_closed_with_rls_notes_101(552-576)mock_pull_closed_with_rls_notes_102(580-604)mock_commit(720-726)mock_git_release(111-114)release_notes_generator/data/miner.py (1)
get_latest_release(309-347)
tests/unit/release_notes_generator/data/test_miner.py (1)
tests/unit/conftest.py (1)
FakeRepo(48-49)
tests/unit/release_notes_generator/test_generator.py (1)
tests/unit/conftest.py (7)
mock_repo(103-106)mock_issue_closed(201-223)mock_issue_closed_i1_bug(227-250)mock_pull_closed_with_rls_notes_101(552-576)mock_pull_closed_with_rls_notes_102(580-604)mock_commit(720-726)mock_git_release(111-114)
🪛 LanguageTool
.specify/memory/constitution.md
[grammar] ~15-~15: There might be a mistake here.
Context: ... ### Principle 1: Test‑First Reliability All core logic (mining, filtering, recor...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...unit tests written before implementation and passing after. Refactors MUST preser...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...flags or undeclared env vars prohibited. Add inputs → MINOR version bump; rename/...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...consistent (additions allowed; removals require MAJOR bump). Rationale: Stable diffs & ...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...s allowed; removals require MAJOR bump). Rationale: Stable diffs & reliable downs...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...rinciple 5: Transparency & Observability Structured logging MUST trace lifecycle:...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...& Observability Structured logging MUST trace lifecycle: start → inputs validated → m...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ... build done → finish. Errors logged with context; verbose flag unlocks extra diag...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...a diagnostics without altering behavior. Rationale: Fast debugging in ephemeral C...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...breaking behavior. Provide opt-in flags if impact uncertain. Document additions in...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...behavior. Provide opt-in flags if impact uncertain. Document additions in README ...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...ed. ### Principle 8: Lean Python Design Prefer pure functions; introduce classes...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ...nce; favor composition. Utility modules keep narrow surface. Rationale: Improves rea...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ...on. Utility modules keep narrow surface. Rationale: Improves readability, testabi...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ....g., missing auth token) may exit early at entry point. Rationale: Predictable act...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ...th token) may exit early at entry point. Rationale: Predictable action completion...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ... ### Principle 10: Dead Code Prohibition Unused functions/methods (except propert...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...ired inherited methods) MUST be removed in same PR that obsoletes them. Utility fi...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ... removed in same PR that obsoletes them. Utility files contain ONLY invoked logic...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ...nts, refactors without behavioral change Examples: - `feature/add-hierarchy-suppo...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...e allowed set; otherwise branch renamed before PR. - Descriptor: lowercase kebab-case;...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...set; otherwise branch renamed before PR. - Descriptor: lowercase kebab-case; hyphen...
(QB_NEW_EN)
[grammar] ~93-~93: There might be a mistake here.
Context: ...y; no spaces/underscores/trailing slash. - Optional numeric ID may precede descript...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ..., feature-fix/), uppercase, camelCase. - Scope alignment: PR description MUST ali...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...(e.g., docs-only PR on feature/ branch). Rationale: Enables automated classificat...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...y & Testing - Test Directory Structure: - tests/unit/: All unit tests (test_<modul...
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ... (test_.py) — required location. - tests/integration/: End-to-end tests (in...
(QB_NEW_EN)
[style] ~106-~106: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...res/: Optional static data samples. - tests/helpers/ & tests/utils/: Test-only help...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~122-~122: There might be a mistake here.
Context: ...cking errors) — treat Any proliferation as smell (justify if unavoidable). 4. Test...
(QB_NEW_EN)
[grammar] ~122-~122: There might be a mistake here.
Context: ...ation as smell (justify if unavoidable). 4. Tests: pytest all green. 5. Coverage: ≥8...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ...fy any temporary dip (must be recovered within next PR). 6. Dead Code: grep for unused...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ... dip (must be recovered within next PR). 6. Dead Code: grep for unused utilities; re...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ...ed utilities; remove or reference usage in same PR. 7. Determinism: (Manual) Valid...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ...s; remove or reference usage in same PR. 7. Determinism: (Manual) Validate repeated ...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ... repeated runs produce identical output for sample dataset. 8. Branch Naming: CI/Re...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ...uce identical output for sample dataset. 8. Branch Naming: CI/Review MUST verify all...
(QB_NEW_EN)
[grammar] ~137-~137: There might be a mistake here.
Context: ...compliance or list justified exceptions. - Versioning (this constitution): Semantic...
(QB_NEW_EN)
[grammar] ~139-~139: Use a hyphen to join words.
Context: ... Remove/redefine a principle or backward incompatible process change. - MINOR: ...
(QB_NEW_EN_HYPHEN)
[grammar] ~139-~139: There might be a mistake here.
Context: ...or backward incompatible process change. - MINOR: Add new principle/section (curren...
(QB_NEW_EN)
[grammar] ~144-~144: There might be a mistake here.
Context: ...ith rationale & impact assessment. 2. Update Sync Impact Report header (include affe...
(QB_NEW_EN)
[grammar] ~144-~144: There might be a mistake here.
Context: ...er (include affected templates & TODOs). 3. Bump version according to rule above. ...
(QB_NEW_EN)
[grammar] ~146-~146: There might be a mistake here.
Context: ...mergency fixes allow retroactive review. - Compliance Review: PR template SHOULD re...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ...inciples violated without justification. - Backward Compatibility: Input names & pl...
(QB_NEW_EN)
[grammar] ~148-~148: There might be a mistake here.
Context: ...ty: Input names & placeholder semantics require MAJOR bump if changed. - Enforcement: C...
(QB_NEW_EN)
[grammar] ~148-~148: There might be a mistake here.
Context: ...semantics require MAJOR bump if changed. - Enforcement: CI pipeline SHOULD automate...
(QB_NEW_EN)
[grammar] ~149-~149: There might be a mistake here.
Context: ...in. Branch naming can be auto-validated by simple prefix check script. *Version...
(QB_NEW_EN)
[grammar] ~151-~151: There might be a mistake here.
Context: ...025-10-12 | Last Amended: 2025-10-14
(QB_NEW_EN)
CONTRIBUTING.md
[grammar] ~28-~28: There might be a mistake here.
Context: ...Issue_. ## Branch Naming (Principle 13) Branches MUST start with one of the allo...
(QB_NEW_EN)
.specify/constitution.md
[grammar] ~73-~73: There might be a mistake here.
Context: ...tly embed presentation markup elsewhere. - Error boundary: modules MUST NOT leak ra...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ... 8 & 10). ### Module Boundary Follow-Up A scheduled audit SHALL verify: - `utils...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...ains only actively referenced functions (dead code removal list to be created). -...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...on into a separate validators/ module. - Logging configuration centralization: co...
(QB_NEW_EN)
[grammar] ~136-~136: There might be a mistake here.
Context: ...eration using mocked or controlled data. - Future: contract/snapshot tests MAY be i...
(QB_NEW_EN)
[grammar] ~153-~153: There might be a mistake here.
Context: ...r per-file fixtures; keep scope minimal. - Parametrization: use `@pytest.mark.param...
(QB_NEW_EN)
[grammar] ~154-~154: There might be a mistake here.
Context: ...ization: use @pytest.mark.parametrize for input matrix instead of loops. - Covera...
(QB_NEW_EN)
[grammar] ~154-~154: There might be a mistake here.
Context: ...rize` for input matrix instead of loops. - Coverage: new logic MUST raise overall c...
(QB_NEW_EN)
[grammar] ~155-~155: There might be a mistake here.
Context: ...overage requires explicit justification. - NEW: Unit test file path MUST mirror sou...
(QB_NEW_EN)
[grammar] ~157-~157: There might be a mistake here.
Context: .../ fix / docs / chore PRs MUST originate from correctly prefixed branch (Principle 13...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ...alidate. ### Organization & Integration - Integration tests MUST import public int...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: ...enerator`) not internal private helpers. - Unit tests MUST avoid real network calls...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ... tests into tests/unit/ SHALL be part of first compliance PR post-amendment. ##...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...inciple 10) — introduce usage or delete in same PR. - Backward compatibility: no s...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ... — introduce usage or delete in same PR. - Backward compatibility: no silent change...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ...to input names or placeholder semantics without version bump & documentation update. - ...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ...out version bump & documentation update. - Branch naming compliance (Principle 13) ...
(QB_NEW_EN)
[grammar] ~346-~346: There might be a mistake here.
Context: .... ### Principle 12: Test Path Mirroring Each unit test file MUST reside under `t...
(QB_NEW_EN)
[grammar] ~347-~347: There might be a mistake here.
Context: ...ive_path>/test_<original_file_name>.py`. Mandatory Rules: - One test file per sou...
(QB_NEW_EN)
[grammar] ~351-~351: There might be a mistake here.
Context: ...s require mirrored test path in same PR. Rationale: Ensures predictable test disc...
(QB_NEW_EN)
[grammar] ~360-~360: There might be a mistake here.
Context: ...ency bumps, CI, non-behavioral refactors Examples: `feature/add-hierarchy-support...
(QB_NEW_EN)
[grammar] ~361-~361: There might be a mistake here.
Context: ..., CI, non-behavioral refactors Examples: feature/add-hierarchy-support, `fix/56...
(QB_NEW_EN)
[grammar] ~362-~362: There might be a mistake here.
Context: ...eadme-start, chore/upgrade-semver-lib` Rules: - Prefix REQUIRED and MUST be in ...
(QB_NEW_EN)
[grammar] ~363-~363: There might be a mistake here.
Context: ...tart, chore/upgrade-semver-lib` Rules: - Prefix REQUIRED and MUST be in approved ...
(QB_NEW_EN)
[style] ~364-~364: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...oved set; rename non-compliant branches prior to PR. - Descriptor: lowercase kebab-case;...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~364-~364: There might be a mistake here.
Context: ...name non-compliant branches prior to PR. - Descriptor: lowercase kebab-case; hyphen...
(QB_NEW_EN)
.specify/templates/tasks-template.md
[grammar] ~22-~22: There might be a mistake here.
Context: ...it/release_notes_generator/x/test_y.py`. - Branch naming: Branch MUST start with al...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...py`. - Branch naming: Branch MUST start with allowed prefix (feature|fix|docs|chore)...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...01 Create any new module directories in release_notes_generator/ - [ ] T002 [P] Ensure mirrored test path s...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...e for new/relocated tests (Principle 12) - [ ] T002a Verify branch prefix matches r...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...rinciple 13) or rename before proceeding - [ ] T003 [P] Add initial failing unit te...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...s/unit/` for new logic (Test‑First gate) - [ ] T004 [P] Configure/verify linting an...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...ts/unit/test_action_inputs.py` extended) - [ ] T006 [P] Add utilities (if needed) w...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ...ests (tests/unit/test_utils_<name>.py) - [ ] T007 Setup error handling pattern (l...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ...urn) — no cross-module exception leakage - [ ] T008 Dead code removal (list obsolet...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ... Story 1 - [Title] (Priority: P1) 🎯 MVP Goal: [Brief description] **Independe...
(QB_NEW_EN)
[grammar] ~67-~67: There might be a mistake here.
Context: ...sts/unit/test_.py` (start failing) - [ ] T010 [US1] Update integration test (...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...T011 [P] [US1] Implement function(s) in release_notes_generator/<module>.py - [ ] T012 [US1] Logging additions (INFO l...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...dditions (INFO lifecycle, DEBUG details) - [ ] T013 [US1] Ensure deterministic orde...
(QB_NEW_EN)
[grammar] ~120-~120: There might be a mistake here.
Context: ...] Documentation updates in README.md, docs/ - [ ] TXXX Code cleanup (remove any newly ...
(QB_NEW_EN)
[grammar] ~121-~121: There might be a mistake here.
Context: ...e cleanup (remove any newly unused code) - [ ] TXXX Performance optimization - [ ] ...
(QB_NEW_EN)
[grammar] ~122-~122: There might be a mistake here.
Context: ...ode) - [ ] TXXX Performance optimization - [ ] TXXX [P] Additional unit tests (edge...
(QB_NEW_EN)
[grammar] ~123-~123: There might be a mistake here.
Context: ...] Additional unit tests (edge cases) in tests/unit/ - [ ] TXXX Security/robustness improvement...
(QB_NEW_EN)
[grammar] ~130-~130: There might be a mistake here.
Context: ...parallelize after Foundational) → Polish - Failing unit tests precede implementatio...
(QB_NEW_EN)
[grammar] ~141-~141: There might be a mistake here.
Context: ...ategy ### MVP First (User Story 1 Only) 1. Setup & Foundational 2. User Story 1 tes...
(QB_NEW_EN)
[grammar] ~142-~142: There might be a mistake here.
Context: ...er Story 1 Only) 1. Setup & Foundational 2. User Story 1 tests → implementation → va...
(QB_NEW_EN)
[grammar] ~143-~143: There might be a mistake here.
Context: ...ry 1 tests → implementation → validation 3. Demo/merge if stable ### Incremental De...
(QB_NEW_EN)
[grammar] ~146-~146: There might be a mistake here.
Context: ...erge if stable ### Incremental Delivery Add each story with its own failing test...
(QB_NEW_EN)
.specify/templates/plan-template.md
[grammar] ~3-~3: There might be a mistake here.
Context: ...}) | Date: [DATE] | Spec: [link] Input: Feature specification from `/...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ....g., Python 3.11 or NEEDS CLARIFICATION] Primary Dependencies: [e.g., PyGithub,...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ..., PyYAML, semver or NEEDS CLARIFICATION] Testing: pytest ONLY (per Constitution...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...esting**: pytest ONLY (per Constitution) Target Platform: GitHub Action runners...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ... runners (Ubuntu) or NEEDS CLARIFICATION Performance Goals: [domain-specific, e...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ....g., <5s generation time for 500 issues] Constraints: [e.g., rate limit adheren...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ... adherence, stable deterministic output] Scale/Scope: [e.g., repos up to 10k is...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...gnment items: - Test‑First Reliability: Provide failing unit test list BEFORE implement...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...ng unit test list BEFORE implementation. - Explicit Configuration Boundaries: All n...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...allowed prefix (feature|fix|docs|chore): `git rev-parse --abbrev-ref HEAD | grep ...
(QB_NEW_EN)
[grammar] ~74-~74: There might be a mistake here.
Context: ...ed] ## Complexity Tracking *Fill ONLY if Constitution Check has violations that ...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ...| Simpler Alternative Rejected Because | |-----------|------------|--------------...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...-|-------------------------------------| | [e.g., new class] | [current need] | [...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...on approach insufficient due to state] | | [e.g., added input] | [feature toggle]...
(QB_NEW_EN)
.specify/templates/spec-template.md
[grammar] ~3-~3: There might be a mistake here.
Context: ...<prefix> ∈ {feature, fix, docs, chore} Created: [DATE] Status: Draft ...
(QB_NEW_EN)
[grammar] ~4-~4: There might be a mistake here.
Context: ... fix, docs, chore} Created: [DATE] Status: Draft Input: User descri...
(QB_NEW_EN)
[grammar] ~5-~5: There might be a mistake here.
Context: ... Created: [DATE] Status: Draft Input: User description: "$ARGUMENTS" ...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ..." ## Constitution Alignment (Mandatory) List how this feature will comply with c...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...est_.py` BEFORE implementation. - Explicit Configuration Boundaries (P2): ...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...impacts; MUST remain stable across runs. - Lean Python Design (P8): Prefer function...
(QB_NEW_EN)
DEVELOPER.md
[grammar] ~183-~183: There might be a mistake here.
Context: ... Branch Naming Convention (Principle 13) All work branches MUST use an allowed pr...
(QB_NEW_EN)
[grammar] ~184-~184: There might be a mistake here.
Context: ...b-case descriptor (optional numeric ID): Allowed prefixes: - feature/ : new funct...
(QB_NEW_EN)
[grammar] ~185-~185: There might be a mistake here.
Context: ...(optional numeric ID): Allowed prefixes: - feature/ : new functionality & enhanceme...
(QB_NEW_EN)
[grammar] ~186-~186: There might be a mistake here.
Context: ...ture/ : new functionality & enhancements - fix/ : bug fixes / defect resolution...
(QB_NEW_EN)
[grammar] ~187-~187: There might be a mistake here.
Context: ...ix/ : bug fixes / defect resolutions - docs/ : documentation-only updates - ...
(QB_NEW_EN)
[grammar] ~188-~188: There might be a mistake here.
Context: ... - docs/ : documentation-only updates - chore/ : maintenance, CI, dependency b...
(QB_NEW_EN)
[grammar] ~189-~189: There might be a mistake here.
Context: ...pendency bumps, non-behavioral refactors Examples: - feature/add-hierarchy-suppor...
(QB_NEW_EN)
[grammar] ~190-~190: There might be a mistake here.
Context: ...umps, non-behavioral refactors Examples: - feature/add-hierarchy-support - fix/456-...
(QB_NEW_EN)
[grammar] ~191-~191: There might be a mistake here.
Context: ...xamples: - feature/add-hierarchy-support - fix/456-null-title-parsing - docs/update...
(QB_NEW_EN)
[grammar] ~192-~192: There might be a mistake here.
Context: ...chy-support - fix/456-null-title-parsing - docs/update-readme-quickstart - chore/up...
(QB_NEW_EN)
[grammar] ~193-~193: There might be a mistake here.
Context: ...-parsing - docs/update-readme-quickstart - chore/upgrade-pygithub Rules: - Prefix m...
(QB_NEW_EN)
[grammar] ~194-~194: There might be a mistake here.
Context: ...adme-quickstart - chore/upgrade-pygithub Rules: - Prefix mandatory; rename non-co...
(QB_NEW_EN)
[grammar] ~195-~195: There might be a mistake here.
Context: ...ickstart - chore/upgrade-pygithub Rules: - Prefix mandatory; rename non-compliant b...
(QB_NEW_EN)
[grammar] ~196-~196: There might be a mistake here.
Context: ...git branch -m feature/` etc.). - Descriptor lowercase kebab-case; hyphens...
(QB_NEW_EN)
[grammar] ~198-~198: There might be a mistake here.
Context: ... PR MUST use docs/ prefix, not feature/. Verification Tip: ```shell git rev-parse...
(QB_NEW_EN)
🪛 Ruff (0.14.0)
tests/unit/release_notes_generator/test_action_inputs.py
372-372: Unused lambda argument: kwargs
(ARG005)
🔇 Additional comments (9)
tests/unit/release_notes_generator/builder/test_release_notes_builder.py (1)
23-23: Import path update aligns with new tests/unit layout.Looks correct; conftest exposes MockLabel and mock_safe_call_decorator.
tests/unit/release_notes_generator/record/factory/test_default_record_factory.py (1)
31-31: LGTM on import move to tests/unit.Matches the restructured test tree.
.specify/templates/spec-template.md (1)
3-3: “Work Branch” header change is clear and consistent with naming rules.tests/unit/release_notes_generator/test_action_inputs.py (1)
28-28: Good: icon is a single code point.Using a Unicode escape ensures len==1 for validation.
.specify/templates/tasks-template.md (1)
1-155: LGTM! Well-structured task template.The template clearly emphasizes test-first development and aligns with the updated constitution principles. The phase separation and mandatory test requirements are well-defined.
.specify/constitution.md (1)
1-372: LGTM! Comprehensive governance document.The constitution is well-structured with clear principles, testing guidelines, and governance rules. The expansion from basic principles to 13 detailed principles provides strong guidance for development.
tests/unit/release_notes_generator/data/test_miner.py (1)
33-33: LGTM! Import path aligns with new test structure.The import path update from
tests.conftesttotests.unit.conftestcorrectly reflects the test structure reorganization described in the constitution changes.tests/unit/release_notes_generator/test_generator.py (1)
46-54: LGTM! Improved parameter formatting.The increased indentation for function parameters improves readability without any functional changes.
Also applies to: 87-95, 140-148
tests/integration/integration_test.py (1)
19-19: Security: TLS verification disabled in integration test.Disabling TLS verification (
verify_tls=False) exposes the test to potential MITM attacks. While this may be necessary for certain test environments, it should be documented and ideally configurable via environment variable.Consider making TLS verification configurable:
- cfg = CollectorConfig(verify_tls=False) + # Allow disabling TLS verification for test environments via environment variable + verify_tls = os.getenv("INTEGRATION_TEST_VERIFY_TLS", "true").lower() != "false" + cfg = CollectorConfig(verify_tls=verify_tls)Add a comment explaining when/why TLS verification might need to be disabled:
# Note: Set INTEGRATION_TEST_VERIFY_TLS=false only for testing against # self-signed certificates or development environments with invalid certs.⛔ Skipped due to learnings
Learnt from: miroslavpojer PR: AbsaOSS/generate-release-notes#192 File: release_notes_generator/data/miner.py:110-113 Timestamp: 2025-10-08T10:54:57.790Z Learning: In release_notes_generator/data/miner.py, the verify_tls=False parameter in the CollectorConfig for BulkSubIssueCollector should be kept as per the repository maintainer's explicit requirement, despite security best practices suggesting otherwise.
|
|
||
| from release_notes_generator.data.utils.bulk_sub_issue_collector import CollectorConfig, BulkSubIssueCollector | ||
|
|
||
| urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) |
There was a problem hiding this comment.
Avoid globally disabling urllib3 warnings.
Disabling warnings at the module level can mask legitimate security issues throughout the test execution. Instead, use a context manager to suppress warnings only for specific operations.
Apply this diff to localize warning suppression:
-import urllib3
-
from release_notes_generator.data.utils.bulk_sub_issue_collector import CollectorConfig, BulkSubIssueCollector
-urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)
-Then, wrap the collector usage:
def test_bulk_sub_issue_collector_smoke():
+ import warnings
+ import urllib3
+
token = os.getenv("GITHUB_TOKEN")
assert token is not None # guarded by skip above
cfg = CollectorConfig(verify_tls=False)
collector = BulkSubIssueCollector(token, cfg=cfg)
new_parents = ["absa-group/AUL#2960"]
iterations = 0
- while new_parents and iterations < 2: # limit iterations for test speed
- new_parents = collector.scan_sub_issues_for_parents(new_parents)
- iterations += 1
+ with warnings.catch_warnings():
+ warnings.filterwarnings("ignore", category=urllib3.exceptions.InsecureRequestWarning)
+ while new_parents and iterations < 2: # limit iterations for test speed
+ new_parents = collector.scan_sub_issues_for_parents(new_parents)
+ iterations += 1📝 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.
| urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) | |
| import os | |
| from release_notes_generator.data.utils.bulk_sub_issue_collector import CollectorConfig, BulkSubIssueCollector | |
| def test_bulk_sub_issue_collector_smoke(): | |
| import warnings | |
| import urllib3 | |
| token = os.getenv("GITHUB_TOKEN") | |
| assert token is not None # guarded by skip above | |
| cfg = CollectorConfig(verify_tls=False) | |
| collector = BulkSubIssueCollector(token, cfg=cfg) | |
| new_parents = ["absa-group/AUL#2960"] | |
| iterations = 0 | |
| with warnings.catch_warnings(): | |
| warnings.filterwarnings("ignore", category=urllib3.exceptions.InsecureRequestWarning) | |
| while new_parents and iterations < 2: # limit iterations for test speed | |
| new_parents = collector.scan_sub_issues_for_parents(new_parents) | |
| iterations += 1 |
🤖 Prompt for AI Agents
In tests/integration/integration_test.py around line 9, do not call
urllib3.disable_warnings at module import time; instead remove that global call
and localize suppression by using a context manager (warnings.catch_warnings +
warnings.simplefilter("ignore", urllib3.exceptions.InsecureRequestWarning)) only
around the specific code that needs it (wrap the collector usage/HTTP call),
ensuring other tests keep normal warning behavior.
1 participant
Summary by CodeRabbit