feat: add max-header-length for header length check#85
feat: add max-header-length for header length check#85aj3sh wants to merge 1 commit intoopensource-nepal:mainfrom
Conversation
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Supply chain security: please keep SHA pins for write-privileged actions. See inline comments.
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Bug: --max-header-length falsy check allows invalid values. See inline comment with reproduction steps.
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Question on the architectural direction for configurable validator options. See inline comment.
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Additional review comments using conventional comments labels.
aj3sh
force-pushed
the
max-header-length
branch
from
be9b5b3 to
7844137
Compare
- disable default header length check - update dependency actions version - add release details on contributing file
aj3sh
force-pushed
the
max-header-length
branch
from
7844137 to
3c7c482
Compare
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Review: feat: add max-header-length
Good feature addition overall. The AppParams dataclass is well-proportioned, positive_int_type handles edge cases correctly, and the validate_fns rename removes ambiguity with the module name.
Two test bugs need fixing before merge, and one product decision needs resolution. See inline comments.
Findings
| # | Label | File | Finding |
|---|---|---|---|
| 1 | issue (blocking) | test__linter.py:29 |
skip_detail test passes False instead of True (copy-paste bug) |
| 2 | issue (blocking) | test_run_commitlint.py:65 |
All 4 tests fail in isolation due to env var leakage |
| 3 | question (blocking) | validators.py:77 |
Breaking change: header length check is now opt-in (was always 72) |
| 4 | issue | utils.py:67 |
Docstring claims "returns None" but raises ValueError |
| 5 | suggestion | utils.py:76 |
Add positive-int validation to match CLI path |
| 6 | suggestion | cli.py:276 |
DRY: extract AppParams construction with dataclasses.replace |
| 7 | suggestion | test_cli.py:409 |
Missing test coverage for --file/--hash/--from-hash + max-header-length |
| 8 | nitpick | app_params.py:18 |
Typo: "Specially" and "Github" |
| 9 | nitpick | validators.py:36 |
self.params public vs self._commit_message private |
| 10 | suggestion | release-please.yml:22 |
Pin actions to commit hashes in privileged workflow |
| def test__lint_commit_message__skip_detail(fixture_data): | ||
| commit_message, expected_success, _ = fixture_data | ||
| success, _ = lint_commit_message(commit_message, skip_detail=True) | ||
| success, _ = lint_commit_message(commit_message, AppParams(skip_detail=False)) |
There was a problem hiding this comment.
issue (blocking): Copy-paste bug. This passes skip_detail=False, making it identical to test_lint_commit_message above. Should be AppParams(skip_detail=True) to exercise the SimplePatternValidator + fail_fast code path.
| "subprocess.check_output", | ||
| return_value="feat: valid commit message", | ||
| ) | ||
| @patch.dict(os.environ, {**os.environ, "INPUT_MAX_HEADER_LENGTH": "72"}) |
There was a problem hiding this comment.
issue (blocking): All four tests in this file fail when run in isolation:
pytest tests/test_github_actions/test_run/test_run_commitlint.py
# 4 failed — KeyError: 'INPUT_VERBOSE' or 'INPUT_MAX_HEADER_LENGTH'
This test patches INPUT_MAX_HEADER_LENGTH but not INPUT_VERBOSE. The three existing tests above patch INPUT_VERBOSE but not INPUT_MAX_HEADER_LENGTH. Since run_commitlint() now reads both env vars, each test needs both.
They only pass in the full suite because set_github_env_vars() from other test files leaks env vars without cleanup.
Fix: add both env vars to every @patch.dict:
@patch.dict(os.environ, {**os.environ, "INPUT_VERBOSE": "false", "INPUT_MAX_HEADER_LENGTH": ""})| """ | ||
| max_header_length = self.params.max_header_length | ||
|
|
||
| if max_header_length is None: |
There was a problem hiding this comment.
question (blocking): This changes the default behavior from "always check 72 chars" to "never check unless opt-in." Users upgrading will silently lose header length enforcement.
The Conventional Commits spec doesn't mandate a header length, so opt-in is defensible, but this is a breaking change for anyone relying on the implicit 72-char guard.
Two options:
- Keep as opt-in (current), but document as a breaking change and bump the major version.
- Default to 72 (
--max-header-lengthdefaults to 72, add--max-header-length 0to disable).
Which is intended?
| key: Input key. | ||
|
|
||
| Returns: | ||
| The integer value of the input. If not integer, returns None |
There was a problem hiding this comment.
issue (non-blocking): Docstring says "If not integer, returns None" but the function raises ValueError for non-integer, non-empty strings (line 78). Only empty string returns None.
Returns:
The integer value of the input, or None if the value is an empty string.
Raises:
ValueError: If the value is a non-empty, non-integer string.| return None | ||
|
|
||
| try: | ||
| return int(val) |
There was a problem hiding this comment.
suggestion (non-blocking): The CLI validates > 0 via positive_int_type(), but this function accepts negative and zero values. A user setting max_header_length: 0 in their workflow YAML would get a confusing subprocess error instead of a clear message.
Consider adding a positive-integer check:
result = int(val)
if result <= 0:
raise ValueError(f"Input '{key}' must be a positive integer (> 0).")
return result| _handle_commit_message( | ||
| commit_message, skip_detail=args.skip_detail, hide_input=args.hide_input | ||
| commit_message, | ||
| AppParams( |
There was a problem hiding this comment.
suggestion (non-blocking): AppParams is constructed four times in main(), and three are identical (only the --file branch adds strip_comments=True). Adding a new field to AppParams requires four edits.
Consider:
from dataclasses import replace
params = AppParams(
skip_detail=args.skip_detail,
hide_input=args.hide_input,
max_header_length=args.max_header_length,
)
# file branch only:
_handle_commit_message(commit_message, replace(params, strip_comments=True))
# all other branches:
_handle_commit_message(commit_message, params)| main() | ||
| assert config.verbose is True | ||
|
|
||
| # main : max-header-length |
There was a problem hiding this comment.
suggestion (non-blocking): max_header_length is tested only for the direct commit message path. The --file, --hash, and --from-hash branches also construct AppParams with max_header_length but have no corresponding tests. At minimum, a test for the --file path (which uniquely adds strip_comments=True) would catch branch-specific bugs.
| # Skips the detailed error check (fails immediately without detail error message). | ||
| skip_detail: bool = False | ||
|
|
||
| # Hide input from stdout/stderr. Specially used by Github Actions. |
There was a problem hiding this comment.
nitpick (non-blocking): "Specially" → "Specifically" and "Github" → "GitHub" (capital H, matching the project's naming elsewhere).
| def __init__(self, commit_message: str) -> None: | ||
| def __init__(self, commit_message: str, params: AppParams) -> None: | ||
| self._commit_message = commit_message | ||
| self.params = params |
There was a problem hiding this comment.
nitpick (non-blocking): self.params is public while self._commit_message and self._errors use the private convention. Consider self._params for consistency.
| uses: googleapis/release-please-action@16a9c90856f42705d54a6fda1823352bdc62cf38 # v4.4.0 | ||
|
|
||
| - uses: actions/checkout@v4 | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
suggestion (non-blocking): Lines 11-12 say "always pin actions to a specific commit hash," but this line and actions/setup-python@v6 (line 37) use mutable version tags in a workflow with id-token: write, contents: write, and pull-requests: write permissions. Consider pinning to commit hashes, like you did for release-please-action and gh-action-pypi-publish in the same file.
2 participants
Description
Related Issue
Fixes #67
Type of Change
Please mark the appropriate option below to describe the type of change your pull request introduces:
Checklist
Examples:
"fix: Fixed foobar bug","feat(accounts): Added foobar feature".README.md.Additional Notes
[Add any additional notes or context that you think the reviewers should know about.]
By submitting this pull request, I confirm that I have read and complied with the contribution guidelines of this project.