fix(scripts): improve git-ai-commit diff handling#682
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR reworks Changesgit-ai-commit diff handling and docs
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant main as main()
participant skip as _skip_binary_and_generated
participant build as _build_messages
participant trunc as _truncate_diff
main->>skip: staged diff
skip-->>main: cleaned diff
main->>main: compute raw_len from pre-truncation prompt size
alt escalation needed
main->>main: set model and max_diff_chars = escalated cap
else
main->>main: keep default cap
end
main->>build: diff, amend_no_diff, max_diff_chars
build->>trunc: truncate diff
trunc-->>build: truncated diff + omission marker
build-->>main: prompt messages
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates git-ai-commit to better control prompt content and enforce Conventional Commits formatting, while also reducing prompt bloat via diff filtering/truncation and a smaller commit-history window.
Changes:
- Update the system prompt to require Conventional Commits v1.0.0 (type/scope, breaking changes, body/footer rules).
- Add staged-diff context optimizations: skip binary/generated-file sections and apply hunk-aware truncation with a model-dependent diff-size cap.
- Reduce recent history depth from 10 to 5, and add an
--amend“no staged changes” mode that rewrites the prior message to conform to Conventional Commits.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/git-ai-commit | Implements Conventional Commits prompt rules, diff skipping/truncation, escalation ordering changes, and amend-with-no-diff rewrite behavior. |
| scripts/test/test_git_ai_commit.py | Adds unit tests for truncation, diff skipping, and max-diff-chars behavior. |
| scripts/man/man1/git-ai-commit.1 | Updates user-facing documentation for new diff handling and history depth. |
| docs/dev/git-ai-commit-context/git-ai-commit-context-optimization.md | Adds a design/plan record describing the context-optimization approach and decisions. |
| docs/dev/git-ai-commit-context/git-ai-commit-context-optimization.json | Adds a structured plan/steps artifact for the optimization work. |
| docs/dev/git-ai-commit-context/git_ai_commit_context_guidelines.md | Adds the guideline doc plus an “Implementation status” section reflecting what was implemented/rejected. |
| # Split diff into sections starting with "diff --git". | ||
| sections = re.split(r"(?=^diff --git )", diff, flags=re.MULTILINE) | ||
| kept_sections: list[str] = [] | ||
| for sec in sections: | ||
| if not sec.strip(): | ||
| continue | ||
| # Binary detection. | ||
| if "Binary files" in sec and "differ" in sec: | ||
| # Extract path from the line, e.g., "Binary files a/foo.bin and b/foo.bin differ" | ||
| m = re.search(r"Binary files ([^ \n]+) and ([^ \n]+) differ", sec) | ||
| path = m.group(2) if m else "<unknown>" | ||
| kept_sections.append(f"[skipped binary/generated file: {path}]\n") | ||
| continue | ||
| # Generated file detection via header line. | ||
| header_match = re.search(r"^diff --git a/([^ \n]+) b/([^ \n]+)", sec, flags=re.MULTILINE) | ||
| if header_match: | ||
| path = header_match.group(2) | ||
| if any(fnmatch.fnmatch(path, pat) for pat in _GENERATED_FILE_GLOBS): | ||
| kept_sections.append(f"[skipped binary/generated file: {path}]\n") | ||
| continue | ||
| # Keep original section. | ||
| kept_sections.append(sec) | ||
| return "".join(kept_sections) |
| omitted = total_files - len(kept_sections) | ||
| final = stat_block + "".join(kept_sections) | ||
| if omitted or line_boundary_cut: | ||
| final += ( | ||
| f"\n\n[diff truncated: {omitted} of {total_files} files omitted to fit context budget]" | ||
| ) | ||
| return final |
| # Determine max diff chars based on model and escalation. | ||
| if backend == "kilo" and (model == _ESCALATION_MODEL_KILO or model_explicit): | ||
| max_diff_chars = _MAX_DIFF_CHARS_ESCALATED | ||
| else: | ||
| max_diff_chars = _MAX_DIFF_CHARS_DEFAULT |
| # Build a large file section with many lines | ||
| lines = [f"+line {i}\n" for i in range(100)] | ||
| file_section = "diff --git a/large.py b/large.py\nnew file mode 100644\n".join(lines) | ||
| diff = stat_block + file_section | ||
|
|
| # Build a diff larger than 60 000 chars | ||
| lines = [f"+line {i} with some padding to make it longer\n" for i in range(1500)] | ||
| file_section = "diff --git a/large.py b/large.py\nnew file mode 100644\n".join(lines) | ||
| diff = stat_block + file_section | ||
| assert len(diff) > 60_000 |
| # Build a diff larger than 60 000 but smaller than 400 000 | ||
| lines = [f"+line {i} with some padding to make it longer\n" for i in range(1500)] | ||
| file_section = "diff --git a/large.py b/large.py\nnew file mode 100644\n".join(lines) | ||
| diff = stat_block + file_section | ||
| assert 60_000 < len(diff) < 400_000 |
| _MAX_DIFF_CHARS_DEFAULT = 60_000 | ||
| _MAX_DIFF_CHARS_ESCALATED = 400_000 # Large ceiling for escalated path | ||
|
|
||
| # When the kilo model was not explicitly chosen and the prompt exceeds this | ||
| # character count, escalate to a free high-context model rather than risk |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
21 fixed, 0 new since branch point (486ea00) ✅ 21 CodeQL alerts resolved since the previous PR commit
✅ 21 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/git-ai-commit`:
- Around line 241-247: The truncation summary in the diff builder is inaccurate
when a file is only partially included via the line-boundary cut path. Update
the logic around the `kept_sections` / `omitted` calculation in
`scripts/git-ai-commit` so a partial section is counted as truncated rather than
fully kept, and make the final marker message reflect that one or more files
were cut mid-file. Keep the fix localized to the truncation branch that sets
`final` and appends the `[diff truncated: ...]` note.
In `@scripts/test/test_git_ai_commit.py`:
- Around line 827-868: The large-diff tests in
test_default_cap_truncates_large_diff and test_escalated_cap_keeps_large_diff
duplicate the same fragile diff-building logic using the `.join()` separator
trick. Refactor the repeated diff construction into a small helper such as a
local `_make_large_diff(...)` fixture/helper in test_git_ai_commit.py, and reuse
it in both tests so the threshold assertions remain the same while removing
copy/paste drift.
- Around line 716-743: The test named test_single_oversized_file_line_boundary
is building many small diff sections instead of one oversized file section, so
it never hits the single-section line-boundary fallback in _truncate_diff.
Update the fixture in test_single_oversized_file_line_boundary to construct one
real diff --git section with a large body that exceeds the budget on its own, so
_truncate_diff exercises the kept_sections == false fallback path. Keep the
existing assertions, but make sure the test data targets the fallback branch in
_truncate_diff rather than the normal multi-file truncation path.
🪄 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 Plus
Run ID: e59fc2b4-408f-4cee-9ac3-80dcccb80a69
📒 Files selected for processing (6)
docs/dev/git-ai-commit-context/git-ai-commit-context-optimization.jsondocs/dev/git-ai-commit-context/git-ai-commit-context-optimization.mddocs/dev/git-ai-commit-context/git_ai_commit_context_guidelines.mdscripts/git-ai-commitscripts/man/man1/git-ai-commit.1scripts/test/test_git_ai_commit.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Analyze python with CodeQL
- GitHub Check: copilot-pull-request-reviewer
⚠️ CI failures not shown inline (6)
GitHub Actions: greenc-FNAL checking Markdown format / markdown-check: greenc-FNAL checking Markdown format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = "success" ]; then�[0m
�[36;1m echo "✅ Markdown formatting check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Markdown formatting check failed."�[0m
GitHub Actions: greenc-FNAL checking Markdown format / 0_markdown-check.txt: greenc-FNAL checking Markdown format
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = "success" ]; then�[0m
�[36;1m echo "✅ Markdown formatting check passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Markdown formatting check failed."�[0m
GitHub Actions: greenc-FNAL checking Python code / python-check: greenc-FNAL checking Python code
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ] && [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ Python checks passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Python checks failed. Comment '@${REPO_NAME}bot python-fix' on the PR to attempt auto-fix."�[0m
GitHub Actions: greenc-FNAL checking Python code / 0_scripts-test.txt: greenc-FNAL checking Python code
Conclusion: failure
_2 PASSED [ 25%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_missing_github_repository_exits_2 PASSED [ 26%]
scripts/test/test_check_codeql_alerts.py::TestMainApiMode::test_api_mode_skipped_when_sarif_has_baseline PASSED [ 26%]
scripts/test/test_check_codeql_alerts.py::TestMainApiModeWithPrRef::test_api_mode_pr_ref_produces_filtered_url PASSED [ 26%]
scripts/test/test_check_codeql_alerts.py::TestMainEntrypoint::test_entrypoint_no_alerts_exits_zero PASSED [ 26%]
scripts/test/test_clang_tidy_check_summary.py::TestLoadDiagnostics::test_reads_from_file PASSED [ 26%]
scripts/test/test_clang_tidy_check_summary.py::TestLoadDiagnostics::test_reads_from_stdin PASSED [ 27%]
scripts/test/test_clang_tidy_check_summary.py::TestLoadDiagnostics::test_invalid_yaml_returns_empty PASSED [ 27%]
scripts/test/test_clang_tidy_check_summary.py::TestLoadDiagnostics::test_non_dict_yaml_returns_empty PASSED [ 27%]
scripts/test/test_clang_tidy_check_summary.py::TestLoadDiagnostics::test_missing_diagnostics_key_returns_empty PASSED [ 27%]
scripts/test/test_clang_tidy_check_summary.py::TestLoadDiagnostics::test_non_list_diagnostics_value_returns_empty PASSED [ 27%]
scripts/test/test_clang_tidy_check_summary.py::TestLoadDiagnostics::test_empty_diagnostics_returns_empty_list PASSED [ 28%]
scripts/test/test_clang_tidy_check_summary.py::TestCountUniqueDiagnostics::test_single_entry_counted PASSED [ 28%]
scripts/test/test_clang_tidy_check_summary.py::TestCountUniqueDiagnostics::test_identical_triplet_counted_once PASSED [ 28%]
scripts/test/test_clang_tidy_check_summary.py::TestCountUniqueDiagnostics::test_different_offsets_counted_separately PASSED [ 28%]
scripts/test/test_clang_tidy_check_summary.py::TestCountUniqueDiagnostics::test_multiple_checks_counted_independently PASSED [ 29%]
scripts/test/test_clang_tidy_check_summary.py::TestCountUniqueDiagnostics::test_empty_list_returns_empty_dict PASSED [ 29%]
scripts/test/test_clang_tidy_check_summary.py...
GitHub Actions: greenc-FNAL checking Python code / 1_python-check.txt: greenc-FNAL checking Python code
Conclusion: failure
##[group]Run REPO_NAME="${REPO##*/}"
�[36;1mREPO_NAME="${REPO##*/}"�[0m
�[36;1mif [ "success" = 'success' ] && [ "success" = 'success' ]; then�[0m
�[36;1m echo "✅ Python checks passed."�[0m
�[36;1melse�[0m
�[36;1m echo "::error::Python checks failed. Comment '@${REPO_NAME}bot python-fix' on the PR to attempt auto-fix."�[0m
GitHub Actions: greenc-FNAL checking Python code / scripts-test: greenc-FNAL checking Python code
Conclusion: failure
##[group]Run codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f
with:
files: coverage-scripts.xml
flags: scripts
name: phlex-scripts-coverage
fail_ci_if_error: false
verbose: true
root_dir: phlex-src
***REDACTED***
disable_file_fixes: false
disable_search: false
disable_safe_directory: false
disable_telem: false
dry_run: false
git_service: github
gcov_executable: gcov
handle_no_reports_found: false
recurse_submodules: false
run_command: upload-coverage
skip_validation: false
use_legacy_upload_endpoint: false
use_oidc: false
use_pypi: false
version: latest
env:
CODECOV_***REDACTED***
UV_PYTHON_INSTALL_DIR: /home/runner/work/_temp/uv-python-dir
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
##[group]Run missing_deps=""
�[36;1mmissing_deps=""�[0m
�[36;1m�[0m
�[36;1m# Check for always-required commands�[0m
�[36;1mfor cmd in bash git curl; do�[0m
�[36;1m if ! command -v "$cmd" >/dev/null 2>&1; then�[0m
�[36;1m missing_deps="$missing_deps $cmd"�[0m
�[36;1m fi�[0m
�[36;1mdone�[0m
�[36;1m�[0m
�[36;1m# Check for gpg only if validation is not being skipped�[0m
�[36;1mif [ "$INPUT_SKIP_VALIDATION" != "true" ]; then�[0m
�[36;1m if ! command -v gpg >/dev/null 2>&1; then�[0m
�[36;1m missing_deps="$missing_deps gpg"�[0m
�[36;1m fi�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1m# Report missing required dependencies�[0m
�[36;1mif [ -n "$missing_deps" ]; then�[0m
�[36;1m echo "Error: The following required dependencies are missing:$missing_deps"�[0m
�[36;1m echo "Please install these dependencies before using this action."�[0m
�[36;1m exit 1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mecho "All required system dependencies are available."�[0m
shell: /usr/bin/sh -e {0}
env:
CODECOV_***REDACTED***
UV_PYTHON_INSTALL_DIR: /home/runner/work/_temp/uv-python-dir
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
INPUT_SKIP_VALIDATION: false
...
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.md: All Markdown files must follow markdownlint rule MD012: no multiple consecutive blank lines (never more than one blank line in a row)
All Markdown files must follow markdownlint rule MD022: headings must be surrounded by exactly one blank line before and after
All Markdown files must follow markdownlint rule MD031: fenced code blocks must be surrounded by exactly one blank line before and after
All Markdown files must follow markdownlint rule MD032: lists must be surrounded by exactly one blank line before and after (including after headings and code blocks)
All Markdown files must follow markdownlint rule MD034: no bare URLs (use markdown link syntax like[text](destination)instead of plain URLs)
All Markdown files must follow markdownlint rule MD036: use # headings for titles, not Bold:
All Markdown files must follow markdownlint rule MD040: always specify code block language (for example, use 'bash', 'python', '```text', etc.)
**/*.md: Do not use multiple consecutive blank lines in Markdown (MD012)
Surround Markdown headings with exactly one blank line (MD022)
Surround Markdown fenced code blocks with exactly one blank line (MD031)
Surround Markdown lists with exactly one blank line (MD032)
Do not use bare URLs in Markdown; use[text](url)syntax instead (MD034)
Use#headings in Markdown, not**Bold**for section titles (MD036)
Always specify language on fenced code blocks in Markdown (MD040)
Files:
docs/dev/git-ai-commit-context/git_ai_commit_context_guidelines.mddocs/dev/git-ai-commit-context/git-ai-commit-context-optimization.md
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Use ruff for Python formatting and linting (configured inpyproject.toml); follow Google-style docstring conventions; use line length of 99 characters; use double quotes for strings
Usefrom __future__ import annotationsto enable deferred evaluation of type annotations (avoids forward-reference issues; Python >=3.12)
Type hints are recommended; mypy is configured inpyproject.toml
Avoid naming Python test scriptstypes.pyor other names that shadow standard library modules, as this causes obscure import errors
Python test scripts should follow the test structure pattern: C++ driver provides data streams, Jsonnet config wires the graph, and Python script implements algorithms
**/*.py: Enforce 99-character line limit and double quotes in Python via ruff configured inpyproject.toml
Use Google-style docstrings in Python code
Use type hints in Python code and configure mypy for type checking
Usefrom __future__ import annotationsin Python to enable deferred evaluation of type annotations
Use PEP 8 naming in Python:CapWordsfor classes,snake_casefor everything else
When using thereadtool for Python files, always use integer values foroffsetandlimitparameters, never float/double values
Files:
scripts/test/test_git_ai_commit.py
🪛 LanguageTool
docs/dev/git-ai-commit-context/git_ai_commit_context_guidelines.md
[style] ~111-~111: Consider an alternative for the overused word “exactly”.
Context: ...conventional‑commit* header, which is exactly what the spec requires. - Therefore the...
(EXACTLY_PRECISELY)
docs/dev/git-ai-commit-context/git-ai-commit-context-optimization.md
[uncategorized] ~20-~20: The official name of this software platform is spelled with a capital “H”.
Context: ... via SourceFileLoader. Run in CI by .github/workflows/python-check.yaml:162 and ...
(GITHUB)
[uncategorized] ~20-~20: The official name of this software platform is spelled with a capital “H”.
Context: ...ub/workflows/python-check.yaml:162and .github/workflows/coverage.yaml:272 (pytest s...
(GITHUB)
[grammar] ~124-~124: Use a hyphen to join words.
Context: ... - New tests for _truncate_diff: under budget returns unchanged; over budget ...
(QB_NEW_EN_HYPHEN)
[grammar] ~124-~124: Use a hyphen to join words.
Context: ...f`: under budget returns unchanged; over budget cuts on file/hunk boundary a...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (18)
scripts/test/test_git_ai_commit.py (4)
679-694: LGTM!
695-715: LGTM!
744-759: LGTM!
766-817: LGTM!scripts/man/man1/git-ai-commit.1 (2)
37-38: 📐 Maintainability & Code Quality | ⚡ Quick winClarify the truncation granularity.
This isn’t really “hunk-aware” in the
@@sense; the helper preserves wholediff --gitsections and falls back to a line-boundary cut for one oversized section. Also, skipped binary/generated files are replaced with a placeholder note, not silently dropped.Suggested wording
-Output of git diff --cached --stat -p, with hunk-aware truncation applied to whole file sections and binary/generated files skipped. +Output of git diff --cached --stat -p, with section-aware truncation that preserves whole file sections and replaces skipped binary/generated files with a placeholder note.
91-92: LGTM!docs/dev/git-ai-commit-context/git-ai-commit-context-optimization.json (1)
84-86: 📐 Maintainability & Code Quality | ⚡ Quick winFix the history-depth wording.
This step still references
git log --oneline -10, which conflicts with the stated reduction to-5. As written, the instruction is self-contradictory and can send the implementation back to the old depth.docs/dev/git-ai-commit-context/git-ai-commit-context-optimization.md (1)
1-133: LGTM!docs/dev/git-ai-commit-context/git_ai_commit_context_guidelines.md (1)
1-179: LGTM!scripts/git-ai-commit (9)
9-9: LGTM!Also applies to: 34-38
134-135: LGTM!
145-192: LGTM!
250-270: LGTM!
334-337: LGTM!
771-809: LGTM!
991-992: LGTM!Also applies to: 1002-1017
1030-1034: 🎯 Functional CorrectnessWorth a sanity check: the diff-cap tiers for non-kilo backends and the default-vs-threshold gap.
Two things jump out here that might be intentional but smell slightly off:
_MAX_DIFF_CHARS_DEFAULT(60k) is larger than_ESCALATION_THRESHOLD_CHARS(30k). On an auto-selected kilo model, any diff big enough to approach the 60k cap will have already tripped escalation (raw_len > 30k) and jumped to the 400k cap first. Net effect: the 60k default cap only ever actually truncates for env-pinned models. If that's the intent, cool; if you expected the default model to sometimes send up to ~60k, it won't.- For non-kilo backends there's no escalation branch, so this line pins them to 60k regardless of an explicitly chosen high-context model. Large commits on
github-models/copilotget quietly truncated with no way to opt into the bigger cap.Can you confirm both are the desired behavior? Happy to be wrong here — just want to make sure the tiers line up with intent.
1035-1071: LGTM!
greenc-FNAL
force-pushed
the
maintenance/conventional-ai-commit
branch
from
a1e91de to
622530e
Compare
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@CodeRabbit rate limit |
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 18 minutes. |
Extract the large diff construction logic into a private `_make_large_diff` helper method to avoid duplication between test cases and improve test readability. - Addresses #682 (comment)
greenc-FNAL
force-pushed
the
maintenance/conventional-ai-commit
branch
from
0114133 to
9e792c6
Compare
greenc-FNAL
changed the title
|
@coderabbitai full review |
✅ Action performedFull review finished. |
coderabbitai
Bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/man/man1/git-ai-commit.1 (1)
140-140: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUpdate stale
fnal-litellmreference in BACKENDS token resolution order.Line 140 still reads
~/.local/share/kilo/auth.json (fnal-litellm key), but the ENVIRONMENT section (lines 201–204) and FILES section (lines 253–254) now document the new fallback order:fnal-azure, thenfnal-ow, then retiredfnal-litellm. The code (_KILO_PROVIDER_FALLBACKS = ["fnal-azure", "fnal-ow", "fnal-litellm"]) confirmsfnal-azureis the primary key. This internal inconsistency will confuse users reading the BACKENDS section.📝 Proposed fix
-\fB~/.local/share/kilo/auth.json\fR (\fIfnal\-litellm\fR key) +\fB~/.local/share/kilo/auth.json\fR (\fIfnal\-azure\fR / \fIfnal\-ow\fR / \fIfnal\-litellm\fR key; see \fBGIT_AI_COMMIT_KILO_PROVIDER\fR)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/man/man1/git-ai-commit.1` at line 140, Update the BACKENDS token resolution order text to match the current fallback sequence. In the git-ai-commit man page, revise the `~/.local/share/kilo/auth.json` entry so it references the active keys in order—`fnal-azure`, then `fnal-ow`, then `fnal-litellm`—and keep the wording consistent with the `_KILO_PROVIDER_FALLBACKS` definition and the ENVIRONMENT/FILES sections.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/git-ai-commit`:
- Around line 745-746: The token ceiling looks like an accidental typo: update
the hardcoded max_tokens value in both the prompt-budget ladder and
_max_tokens_for_prompt to use the intended 8192 instead of 8912. Keep the change
consistent in the conditional block that checks model_lower and in the shared
helper so both paths match the same budget.
- Around line 188-192: The Binary files skip logic in the section-processing
loop is too broad and can falsely match added text inside a normal hunk. Update
the check in the `kept_sections`/`sec` handling so it only triggers when the
section starts with the actual git-generated “Binary files … differ” line, and
keep the existing path extraction regex for that anchored case. This will
prevent valid text diffs from being replaced by the placeholder while still
skipping real binary/generated file sections.
---
Outside diff comments:
In `@scripts/man/man1/git-ai-commit.1`:
- Line 140: Update the BACKENDS token resolution order text to match the current
fallback sequence. In the git-ai-commit man page, revise the
`~/.local/share/kilo/auth.json` entry so it references the active keys in
order—`fnal-azure`, then `fnal-ow`, then `fnal-litellm`—and keep the wording
consistent with the `_KILO_PROVIDER_FALLBACKS` definition and the
ENVIRONMENT/FILES sections.
🪄 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 Plus
Run ID: 193be46f-e5dc-45fc-afb9-d3e0756d0e58
📒 Files selected for processing (6)
docs/dev/git-ai-commit-context/git-ai-commit-context-optimization.jsondocs/dev/git-ai-commit-context/git-ai-commit-context-optimization.mddocs/dev/git-ai-commit-context/git_ai_commit_context_guidelines.mdscripts/git-ai-commitscripts/man/man1/git-ai-commit.1scripts/test/test_git_ai_commit.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.md: All Markdown files must follow markdownlint rule MD012: no multiple consecutive blank lines (never more than one blank line in a row)
All Markdown files must follow markdownlint rule MD022: headings must be surrounded by exactly one blank line before and after
All Markdown files must follow markdownlint rule MD031: fenced code blocks must be surrounded by exactly one blank line before and after
All Markdown files must follow markdownlint rule MD032: lists must be surrounded by exactly one blank line before and after (including after headings and code blocks)
All Markdown files must follow markdownlint rule MD034: no bare URLs (use markdown link syntax like[text](destination)instead of plain URLs)
All Markdown files must follow markdownlint rule MD036: use # headings for titles, not Bold:
All Markdown files must follow markdownlint rule MD040: always specify code block language (for example, use 'bash', 'python', '```text', etc.)
**/*.md: Do not use multiple consecutive blank lines in Markdown (MD012)
Surround Markdown headings with exactly one blank line (MD022)
Surround Markdown fenced code blocks with exactly one blank line (MD031)
Surround Markdown lists with exactly one blank line (MD032)
Do not use bare URLs in Markdown; use[text](url)syntax instead (MD034)
Use#headings in Markdown, not**Bold**for section titles (MD036)
Always specify language on fenced code blocks in Markdown (MD040)
Files:
docs/dev/git-ai-commit-context/git_ai_commit_context_guidelines.mddocs/dev/git-ai-commit-context/git-ai-commit-context-optimization.md
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Use ruff for Python formatting and linting (configured inpyproject.toml); follow Google-style docstring conventions; use line length of 99 characters; use double quotes for strings
Usefrom __future__ import annotationsto enable deferred evaluation of type annotations (avoids forward-reference issues; Python >=3.12)
Type hints are recommended; mypy is configured inpyproject.toml
Avoid naming Python test scriptstypes.pyor other names that shadow standard library modules, as this causes obscure import errors
Python test scripts should follow the test structure pattern: C++ driver provides data streams, Jsonnet config wires the graph, and Python script implements algorithms
**/*.py: Enforce 99-character line limit and double quotes in Python via ruff configured inpyproject.toml
Use Google-style docstrings in Python code
Use type hints in Python code and configure mypy for type checking
Usefrom __future__ import annotationsin Python to enable deferred evaluation of type annotations
Use PEP 8 naming in Python:CapWordsfor classes,snake_casefor everything else
When using thereadtool for Python files, always use integer values foroffsetandlimitparameters, never float/double values
Files:
scripts/test/test_git_ai_commit.py
🪛 ast-grep (0.44.1)
scripts/test/test_git_ai_commit.py
[info] 73-73: use jsonify instead of json.dumps for JSON output
Context: json.dumps(obj)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 LanguageTool
docs/dev/git-ai-commit-context/git_ai_commit_context_guidelines.md
[style] ~111-~111: Consider an alternative for the overused word “exactly”.
Context: ...conventional‑commit* header, which is exactly what the spec requires. - Therefore the...
(EXACTLY_PRECISELY)
docs/dev/git-ai-commit-context/git-ai-commit-context-optimization.md
[uncategorized] ~20-~20: The official name of this software platform is spelled with a capital “H”.
Context: ... via SourceFileLoader. Run in CI by .github/workflows/python-check.yaml:162 and ...
(GITHUB)
[uncategorized] ~20-~20: The official name of this software platform is spelled with a capital “H”.
Context: ...ub/workflows/python-check.yaml:162and .github/workflows/coverage.yaml:272 (pytest s...
(GITHUB)
[grammar] ~124-~124: Use a hyphen to join words.
Context: ... - New tests for _truncate_diff: under budget returns unchanged; over budget ...
(QB_NEW_EN_HYPHEN)
[grammar] ~124-~124: Use a hyphen to join words.
Context: ...f`: under budget returns unchanged; over budget cuts on file/hunk boundary a...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (17)
scripts/test/test_git_ai_commit.py (6)
754-780: This.join()-as-separator pattern intest_single_oversized_file_line_boundarywas previously flagged and marked as addressed. The test does exercise the line-boundary fallback path (the_truncate_diff-computed stat_block absorbs+line 0\n, making the first file section exceed the budget), but the construction is non-obvious. No new action needed.
70-80: LGTM!
178-213: LGTM!
782-797: LGTM!
799-855: LGTM!
857-905: LGTM!docs/dev/git-ai-commit-context/git-ai-commit-context-optimization.json (1)
1-149: LGTM!docs/dev/git-ai-commit-context/git-ai-commit-context-optimization.md (1)
1-179: LGTM!docs/dev/git-ai-commit-context/git_ai_commit_context_guidelines.md (1)
1-133: LGTM!scripts/man/man1/git-ai-commit.1 (4)
37-44: LGTM!
91-92: LGTM!
201-204: LGTM!
253-254: LGTM!scripts/git-ai-commit (4)
224-226: 📐 Maintainability & Code Quality | 💤 Low valueRedundant
reimport — already available at module scope.
reis imported at module level (and you use it bare at line 182), soimport re as _rehere is dead weight. Drop the alias and callre.split(...)directly. This is the same one CodeQL already dinged.🔧 Drop the local re-import
- # Split the diff into stat block + individual file sections. - # Use a zero-width lookahead so each file section retains its "diff --git " prefix. - import re as _re - - raw_sections = _re.split(r"(?=^diff --git )", diff, flags=_re.MULTILINE) + # Split the diff into stat block + individual file sections. + # Use a zero-width lookahead so each file section retains its "diff --git " prefix. + raw_sections = re.split(r"(?=^diff --git )", diff, flags=re.MULTILINE)Source: Linters/SAST tools
255-261: 🎯 Functional Correctness | ⚡ Quick winThe truncation note still fibs on a line-boundary cut.
This looks like the same issue flagged previously (and marked addressed), but the merged version still emits the single message. When the first file alone blows the budget you stash a partial slice in
kept_sections, soomitted = total_files - len(kept_sections)counts a chopped file as fully kept. A single oversized file (total_files == 1) prints[diff truncated: 0 of 1 files omitted…]even though it very much got cut — telling the model "nothing omitted" while it stares at half a hunk. The multi-file case undercounts too. Split the partial branch out so the count stays honest.🔧 Proposed marker fix
omitted = total_files - len(kept_sections) final = stat_block + "".join(kept_sections) - if omitted or line_boundary_cut: - final += ( - f"\n\n[diff truncated: {omitted} of {total_files} files omitted to fit context budget]" - ) + if line_boundary_cut: + final += ( + f"\n\n[diff truncated: first file shown partially; " + f"{total_files - 1} of {total_files} files omitted to fit context budget]" + ) + elif omitted: + final += ( + f"\n\n[diff truncated: {omitted} of {total_files} files omitted to fit context budget]" + )
133-147: LGTM!Also applies to: 521-528, 840-893, 1070-1150
531-562: 📐 Maintainability & Code QualityCheck whether
_chatstill duplicates this max_tokens ladder.If it does, call
_max_tokens_for_prompt()there so the budget stays in one place; if nothing uses this helper, drop it.
| if "Binary files" in sec and "differ" in sec: | ||
| # Extract path from the line, e.g., "Binary files a/foo.bin and b/foo.bin differ" | ||
| m = re.search(r"Binary files ([^ \n]+) and ([^ \n]+) differ", sec) | ||
| path = m.group(2) if m else "<unknown>" | ||
| kept_sections.append(f"[skipped binary/generated file: {path}]\n") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Anchor the "Binary files … differ" check to line start.
Right now the guard is a loose substring scan over the whole section. If a staged text change happens to add a line like +Binary files a/x and b/x differ (docs, tests, a git tutorial, whatever), the entire real hunk gets nuked and replaced with the placeholder — the model then writes a commit message for a diff it never actually saw. Git always emits this as its own line, so anchor it.
🔧 Proposed anchoring
- # Binary detection.
- if "Binary files" in sec and "differ" in sec:
- # Extract path from the line, e.g., "Binary files a/foo.bin and b/foo.bin differ"
- m = re.search(r"Binary files ([^ \n]+) and ([^ \n]+) differ", sec)
+ # Binary detection (git emits this on its own line).
+ m = re.search(r"^Binary files ([^ \n]+) and ([^ \n]+) differ", sec, flags=re.MULTILINE)
+ if m:
path = m.group(2) if m else "<unknown>"
kept_sections.append(f"[skipped binary/generated file: {path}]\n")
continue📝 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.
| if "Binary files" in sec and "differ" in sec: | |
| # Extract path from the line, e.g., "Binary files a/foo.bin and b/foo.bin differ" | |
| m = re.search(r"Binary files ([^ \n]+) and ([^ \n]+) differ", sec) | |
| path = m.group(2) if m else "<unknown>" | |
| kept_sections.append(f"[skipped binary/generated file: {path}]\n") | |
| # Binary detection (git emits this on its own line). | |
| m = re.search(r"^Binary files ([^ \n]+) and ([^ \n]+) differ", sec, flags=re.MULTILINE) | |
| if m: | |
| path = m.group(2) if m else "<unknown>" | |
| kept_sections.append(f"[skipped binary/generated file: {path}]\n") | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/git-ai-commit` around lines 188 - 192, The Binary files skip logic in
the section-processing loop is too broad and can falsely match added text inside
a normal hunk. Update the check in the `kept_sections`/`sec` handling so it only
triggers when the section starts with the actual git-generated “Binary files …
differ” line, and keep the existing path extraction regex for that anchored
case. This will prevent valid text diffs from being replaced by the placeholder
while still skipping real binary/generated file sections.
| if any(kw in model_lower for kw in ("reasoning", "4-5", "qwen3-coder-next", "gemma4-31b")): | ||
| max_tokens = 8912 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
8912 smells like a fat-fingered 8192.
Every other budget in this ladder is a clean power-of-two-ish value (2048/3072/4096), and 8912 isn't a value anyone reaches for on purpose — looks like a digit transposition of 8192. Same literal shows up in _max_tokens_for_prompt (line 554), so fix both. Not fatal (it's a slightly bigger ceiling), but worth correcting the intent.
- max_tokens = 8912
+ max_tokens = 8192📝 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.
| if any(kw in model_lower for kw in ("reasoning", "4-5", "qwen3-coder-next", "gemma4-31b")): | |
| max_tokens = 8912 | |
| if any(kw in model_lower for kw in ("reasoning", "4-5", "qwen3-coder-next", "gemma4-31b")): | |
| max_tokens = 8192 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/git-ai-commit` around lines 745 - 746, The token ceiling looks like
an accidental typo: update the hardcoded max_tokens value in both the
prompt-budget ladder and _max_tokens_for_prompt to use the intended 8192 instead
of 8912. Keep the change consistent in the conditional block that checks
model_lower and in the shared helper so both paths match the same budget.
- Add hunk-aware diff truncation with whole-file-section preservation - Implement two-tier context caps (60k/400k chars) for token budgeting - Replace post-truncation raw_len calculation with untruncated input length - Skip binary and generated files (minimal glob list, extendable) - Reduce commit history depth from 10 to 5 - Trim system prompt filler while preserving Conventional Commits rules - Update man page to reflect truncation, history depth, and provider fallback - Add context optimization documentation and JSON metadata - Expand test coverage for new context handling logic
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ore than once' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Extract the large diff construction logic into a private `_make_large_diff` helper method to avoid duplication between test cases and improve test readability. - Addresses #682 (comment)
Update the documentation to reflect that ~/.local/share/kilo/auth.json now supports multiple keys: fnal-azure, fnal-ow, and fnal-litellm.
Replace hardcoded azure/claude-haiku-4-5 default with qwen/qwen3-coder-next, routing open-weight models (qwen/*, google/*, nvidia/*, BAAI/*) to fnal-ow provider to avoid HTTP 400 budget exceeded errors. Add _determine_kilo_provider() to select fnal-ow for open-weight models and fnal-azure otherwise, while maintaining fallback to fnal-ow/fnal-azure for backward compatibility. Update documentation and tests accordingly.
greenc-FNAL
force-pushed
the
maintenance/conventional-ai-commit
branch
from
5f75be2 to
33df23e
Compare
Code
scripts/git-ai-commitprompt assembly and staged-diff handling.Tests
Docs
git-ai-commit.