Skip to content

Commit 6763134

Browse files
committed
[feat] Added BATS test suite for manage_section
[fix] Improved error messages [chore] Updated manage_section documentation
1 parent 9a9a2cc commit 6763134

File tree

5 files changed

+285
-13
lines changed

5 files changed

+285
-13
lines changed

docs/todos.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,33 @@
11
# TODOs
22

3+
## Git Command Robustness:
4+
The use of `git diff` and `git rev-list` assumes the repository is in a clean state. If the user runs giv inside a shallow clone or with unusual ref names, some commands could fail.
5+
6+
The script does check `git rev-parse --is-inside-work-tree` early to ensure you’re in a git repo.
7+
8+
One scenario: very large diffs. Using `--compact-summary` and unified=3 limits output size, but if a commit touched 500 files, the diffstat could still be long.
9+
10+
It might be worth limiting or truncating extremely large diffs before feeding to the LLM summary (perhaps summarizing the diffstat itself). This is more a potential enhancement than a bug; currently, a huge commit might risk hitting token limits for summarization. The code doesn’t explicitly guard against that beyond the diff settings.
11+
12+
In practice, this might rarely be an issue, but it’s good to document.
13+
14+
## Environment Variable Confusion:
15+
There are both config file `.env` loading and environment variables like `GIV_MODEL`, `GIV_API_URL`, etc. The code merges these by preferring CLI args, then env vars, then defaults.
16+
17+
One thing to double-check: if the user sets `GIV_MODEL_MODE=none` (or uses `--model-mode none`), the script sets dry_run=true for generation but still goes through summarization unless explicitly handled.
18+
19+
Actually, in generate_response, if mode is "none", it doesn’t have a case and will default to local which tries Ollama (not desired). There is a check setting model_mode="none" will skip model usage and treat it effectively as dry-run.
20+
21+
This area could be refactored to make the logic clearer: e.g. a single flag that indicates “no model calls” which both summarization and final generation honor. As is, it does seem to work: model_mode none triggers warnings and then they set dry_run=true which causes generate_from_prompt to just print the prompt.
22+
23+
It might be cleaner if summarize_target also respected dry-run and skipped calling the model (currently if you do `--model-mode none`, it might still run summarize_commit which calls generate_response; though since they set model_mode="none" and pass that in, generate_response would return empty immediately or error).
24+
25+
This flow is a bit convoluted. A refactor could short-circuit summarization when no model is to be used – instead, just concatenate raw commit messages or something.
26+
27+
This ties into perhaps providing a mode where the tool generates a draft prompt for manual use (which it does via `--dry-run` or `--prompt-file` in document). Not a critical bug, but worth reviewing.
28+
29+
30+
331
## Pending
432

533
- CHORE: write doc on using external APIs

src/giv.sh

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,10 @@ parse_args() {
342342
api_url=$2
343343
shift 2
344344
;;
345+
--api-key)
346+
api_key=$2
347+
shift 2
348+
;;
345349
--version-file)
346350
version_file=$2
347351
shift 2
@@ -392,12 +396,12 @@ parse_args() {
392396
print_debug "ollama not found, forcing remote mode (local model unavailable)"
393397
model_mode="remote"
394398
if [ -z "${api_key}" ]; then
395-
print_warn "ollama not found, so remote mode is required, but GIV_API_KEY is not set"
399+
print_warn "No local model and no API configured; outputting prompt template only."
396400
model_mode="none"
397401
dry_run=true
398402
fi
399403
if [ -z "${api_url}" ]; then
400-
print_warn "ollama not found, so remote mode is required, but no API URL provided (use --api-url or GIV_API_URL)"
404+
print_warn "No local model and no API configured; outputting prompt template only."
401405
model_mode="none"
402406
dry_run=true
403407
fi
@@ -415,7 +419,7 @@ parse_args() {
415419
fi
416420
fi
417421

418-
[ "${model_mode}" = "none" ] && print_warn "Model mode set to \"none\", no model will be used."
422+
[ "${model_mode}" = "none" ] && print_warn "Model mode set to \"none\", only prompt templates will be returned."
419423

420424
print_debug "Environment variables:"
421425
print_debug " GIV_TMPDIR: ${GIV_TMPDIR:-}"
@@ -558,7 +562,8 @@ cmd_message() {
558562
build_history "${hist}" "${commit_id}" "${todo_pattern}" "${PATHSPEC}"
559563
print_debug "Generated history file ${hist}"
560564
pr=$(portable_mktemp "commit_message_prompt_XXXXXX.md")
561-
build_prompt --template "${TEMPLATE_DIR}/message_prompt.md" --summary "${hist}" >"${pr}"
565+
build_prompt --template "${TEMPLATE_DIR}/message_prompt.md" \
566+
--summary "${hist}" >"${pr}"
562567
print_debug "Generated prompt file ${pr}"
563568
res=$(generate_response "${pr}" "${model_mode}" "0.9" "32768")
564569
if [ $? -ne 0 ]; then
@@ -632,7 +637,7 @@ cmd_changelog() {
632637
# 4) Build the AI prompt
633638
prompt_template="${TEMPLATE_DIR}/changelog_prompt.md"
634639
print_debug "Building prompt from template: $prompt_template"
635-
tmp_prompt_file=$(portable_mktemp "prompt.XXXXXXX.md") || {
640+
tmp_prompt_file=$(portable_mktemp "changelog_prompt.XXXXXXX.md") || {
636641
printf 'Error: cannot create temp file for prompt\n' >&2
637642
rm -f "$summaries_file"
638643
exit 1
@@ -644,7 +649,7 @@ cmd_changelog() {
644649
fi
645650

646651
# 5) Generate AI response
647-
response_file=$(portable_mktemp "response.XXXXXXX.md") || {
652+
response_file=$(portable_mktemp "changelog_response.XXXXXXX.md") || {
648653
printf 'Error: cannot create temp file for AI response\n' >&2
649654
rm -f "$summaries_file" "$tmp_prompt_file"
650655
exit 1
@@ -657,7 +662,7 @@ cmd_changelog() {
657662
fi
658663

659664
# 6) Prepare a working copy of the changelog
660-
tmp_out=$(portable_mktemp "changelog.XXXXXXX.md") || {
665+
tmp_out=$(portable_mktemp "changelog_output.XXXXXXX.md") || {
661666
printf 'Error: cannot create temp file for changelog update\n' >&2
662667
exit 1
663668
}
@@ -744,7 +749,7 @@ if [ "${_is_sourced}" -eq 0 ]; then
744749
"${REVISION}" \
745750
"${output_file:-$announce_file}" \
746751
"${model_mode}" \
747-
"0.6" \
752+
"0.5" \
748753
"65536" ;;
749754
changelog) cmd_changelog ;;
750755
help)

src/helpers.sh

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ run_local() {
252252
# - Any other value: Calls the `run_local` function with the input file path as an argument, along with temperature and context window size if provided.
253253
generate_response() {
254254
gen_mode="${2:-$model_mode:-auto}"
255-
temp="${3:-0.9}" # Default to a neutral temperature of 1.0
255+
temp="${3:-0.5}" # Default to a neutral temperature of 0.5
256256
ctx_window="${4:-32768}" # Default context window size
257257

258258
print_debug "Generating response using $gen_mode mode with temperature=$temp and context window size=$ctx_window"
@@ -936,7 +936,7 @@ summarize_commit() {
936936
printf '\n\n' >>"$res_file"
937937
echo "${res}" >>"${res_file}"
938938

939-
printf '%s\n' "${res}"
939+
cat "${res_file}"
940940
}
941941

942942
# -------------------------------------------------------------------

src/markdown.sh

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,9 +84,21 @@ extract_section() {
8484
sed -n "${start},${end}p" "$file"
8585
}
8686

87-
# manage_section <title> <file> <new_content_file> <mode> <section_id> [<header_id>]
88-
# mode: append|prepend|update
89-
# header_id: literal "#" string (e.g. "##" or "###"), defaults to "##"
87+
# manage_section - Manages sections within a markdown file.
88+
#
89+
# This function allows for appending, prepending, or updating a section within a markdown file.
90+
# It takes care of handling the header and content insertion while preserving the original structure.
91+
#
92+
# Parameters:
93+
# title (string) : The title to be used in the markdown file.
94+
# file (string) : Path to the existing markdown file.
95+
# newf (string) : Path to the new content file to be inserted.
96+
# mode (string) : Mode of operation: 'append', 'prepend', or 'update'.
97+
# section (string) : The section header to manage within the markdown file.
98+
# header (string, opt) : The header level for the section. Defaults to "##".
99+
#
100+
# Returns:
101+
# Path to a temporary file containing the modified markdown content.
90102
manage_section() {
91103
title=$1
92104
file=$2

tests/manage_sections.bats

Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
#!/usr/bin/env bats
2+
3+
# Path to the script under test; adjust as needed
4+
SCRIPT="${BATS_TEST_DIRNAME}/../src/markdown.sh"
5+
HELPERS="${BATS_TEST_DIRNAME}/../src/helpers.sh"
6+
7+
load 'test_helper/bats-support/load'
8+
load 'test_helper/bats-assert/load'
9+
load "$HELPERS"
10+
load "$SCRIPT"
11+
12+
# Stub dependencies and source function
13+
setup() {
14+
# Stub portable_mktemp and normalize_blank_lines
15+
portable_mktemp() { mktemp; }
16+
normalize_blank_lines() { cat "$1"; }
17+
18+
# # Source the script containing manage_section
19+
# [ -f "$SCRIPT" ] || BATS_SKIP="Script under test not found: $SCRIPT"
20+
# source "$SCRIPT"
21+
}
22+
23+
teardown() {
24+
# Clean up any temporary files
25+
rm -f "$tmp_orig" "$tmp_new" "$tmpfile"
26+
}
27+
28+
# Helper to extract managed output file path
29+
get_tmpfile() {
30+
# $output from prior run: last line is tmpfile path
31+
echo "$output" | tail -n1
32+
}
33+
34+
@test "append mode: appends new section to existing file" {
35+
tmp_orig=$(mktemp)
36+
cat <<EOF >"$tmp_orig"
37+
Line1
38+
Line2
39+
EOF
40+
tmp_new=$(mktemp)
41+
echo "Appended content" >"$tmp_new"
42+
43+
run manage_section "# Changelog" "$tmp_orig" "$tmp_new" append "NewSection"
44+
[ "$status" -eq 0 ]
45+
tmpfile=$(get_tmpfile)
46+
47+
run cat "$tmpfile"
48+
[ "$status" -eq 0 ]
49+
# original content preserved
50+
[[ "$output" == *"Line1"* ]]
51+
[[ "$output" == *"Line2"* ]]
52+
# new header and content at end
53+
[[ "$output" == *"## NewSection"* ]]
54+
[[ "$output" == *"Appended content"* ]]
55+
}
56+
57+
@test "append mode: missing original file behaves like empty" {
58+
tmp_new=$(mktemp)
59+
echo "Only content" >"$tmp_new"
60+
61+
run manage_section "# Changelog" "/nonexistent/file" "$tmp_new" append "OnlySection"
62+
[ "$status" -eq 0 ]
63+
tmpfile=$(get_tmpfile)
64+
65+
run cat "$tmpfile"
66+
[ "$status" -eq 0 ]
67+
# should only contain new section
68+
[[ "$output" == *"## OnlySection"* ]]
69+
[[ "$output" == *"Only content"* ]]
70+
}
71+
72+
@test "prepend mode: empty original file inserts title and section" {
73+
tmp_new=$(mktemp)
74+
echo "NewLine1" >"$tmp_new"
75+
76+
run manage_section "# Changelog" "/nonexistent" "$tmp_new" prepend "Intro"
77+
[ "$status" -eq 0 ]
78+
tmpfile=$(get_tmpfile)
79+
80+
run cat "$tmpfile"
81+
# should start with title
82+
[[ "$output" =~ "# Changelog" ]]
83+
# then new section header
84+
[[ "$output" =~ "## Intro" ]]
85+
# then content
86+
[[ "$output" =~ "NewLine1" ]]
87+
}
88+
89+
@test "prepend mode: existing title, inserts before first same-level header" {
90+
tmp_orig=$(mktemp)
91+
cat <<EOF >"$tmp_orig"
92+
# Changelog
93+
94+
## OldSec
95+
Old content
96+
EOF
97+
tmp_new=$(mktemp)
98+
echo "Fresh content" >"$tmp_new"
99+
100+
run manage_section "# Changelog" "$tmp_orig" "$tmp_new" prepend "FreshSec"
101+
[ "$status" -eq 0 ]
102+
tmpfile=$(get_tmpfile)
103+
104+
run cat "$tmpfile"
105+
# # Changelog remains at top
106+
assert_output --regexp "^# Changelog"
107+
# FreshSec comes before OldSec
108+
assert_output --regexp "## FreshSec"
109+
assert_output --regexp "Fresh content"
110+
# OldSec still present after
111+
assert_output --regexp "## OldSec"
112+
}
113+
114+
@test "update mode: replaces existing section content" {
115+
tmp_orig=$(mktemp)
116+
cat <<EOF >"$tmp_orig"
117+
# Changelog
118+
119+
## SectionA
120+
Old line1
121+
Old line2
122+
123+
## SectionB
124+
B content
125+
EOF
126+
tmp_new=$(mktemp)
127+
echo "NewA1" >"$tmp_new"
128+
echo "NewA2" >>"$tmp_new"
129+
130+
run manage_section "# Changelog" "$tmp_orig" "$tmp_new" update "SectionA"
131+
[ "$status" -eq 0 ]
132+
tmpfile=$(get_tmpfile)
133+
134+
run cat "$tmpfile"
135+
# SectionA content should be replaced
136+
[[ "$output" == *"## SectionA"* ]]
137+
[[ "$output" == *"NewA1"* ]]
138+
[[ "$output" == *"NewA2"* ]]
139+
# SectionB remains
140+
[[ "$output" == *"## SectionB"* ]]
141+
[[ "$output" == *"B content"* ]]
142+
}
143+
144+
@test "update mode: fallback to prepend when section missing" {
145+
tmp_orig=$(mktemp)
146+
echo "# Changelog" >"$tmp_orig"
147+
tmp_new=$(mktemp)
148+
echo "Fallback content" >"$tmp_new"
149+
150+
run manage_section "# Changelog" "$tmp_orig" "$tmp_new" update "MissingSec"
151+
[ "$status" -eq 0 ]
152+
tmpfile=$(get_tmpfile)
153+
154+
run cat "$tmpfile"
155+
# MissingSec should be prepended
156+
[[ "$output" =~ "## MissingSec" ]]
157+
[[ "$output" =~ "Fallback content" ]]
158+
}
159+
160+
@test "invalid mode returns error" {
161+
tmp_orig=$(mktemp)
162+
tmp_new=$(mktemp)
163+
echo "X" >"$tmp_new"
164+
165+
run manage_section "Title" "$tmp_orig" "$tmp_new" invalid "Sec"
166+
[ "$status" -ne 0 ]
167+
[[ "$output" =~ "Invalid mode provided" ]]
168+
}
169+
170+
@test "custom header insertion" {
171+
tmp_orig=$(mktemp)
172+
echo "# Changelog" >"$tmp_orig"
173+
tmp_new=$(mktemp)
174+
echo "Custom content" >"$tmp_new"
175+
176+
# Using single '#' header instead of default '##'
177+
run manage_section "# Changelog" "$tmp_orig" "$tmp_new" prepend "CustomHdr" "##"
178+
[ "$status" -eq 0 ]
179+
tmpfile=$(get_tmpfile)
180+
181+
run cat "$tmpfile"
182+
assert_output --regexp "^# Changelog"
183+
# Should use '# CustomHdr'
184+
assert_output --regexp "## CustomHdr"
185+
assert_output --partial "Custom content"
186+
}
187+
188+
@test "long new content handled correctly" {
189+
tmp_orig=$(mktemp)
190+
echo "# Changelog" >"$tmp_orig"
191+
tmp_new=$(mktemp)
192+
# generate 1000 lines
193+
# shellcheck disable=SC2034
194+
for i in $(seq 1 1000); do printf "Line %s\n" "$i" >>"$tmp_new"; done
195+
196+
run manage_section "# Changelog" "$tmp_orig" "$tmp_new" prepend "Bulk"
197+
[ "$status" -eq 0 ]
198+
tmpfile=$(get_tmpfile)
199+
200+
run wc -l <"$tmpfile"
201+
cat "$tmpfile" | head
202+
# Should be original lines + 1 title + blanks + 1 header + blank + 1000 content lines
203+
expected=$((1 + 1 + 1 + 1 + 1000))
204+
assert_output "$expected"
205+
}
206+
207+
@test "unexpected formatting: missing blank lines around headers" {
208+
tmp_orig=$(mktemp)
209+
# Title and section without blank lines
210+
cat <<EOF >"$tmp_orig"
211+
# Changelog
212+
## OldSec
213+
Old
214+
EOF
215+
tmp_new=$(mktemp)
216+
echo "NewFmt" >"$tmp_new"
217+
218+
run manage_section "# Changelog" "$tmp_orig" "$tmp_new" prepend "FmtTest"
219+
[ "$status" -eq 0 ]
220+
tmpfile=$(get_tmpfile)
221+
222+
run sed -n '1,5p' "$tmpfile"
223+
# Check that new section inserted with blank lines
224+
[[ "$output" =~ "# Changelog" ]]
225+
[[ "$output" =~ "## FmtTest" ]]
226+
[[ "$output" =~ "NewFmt" ]]
227+
}

0 commit comments

Comments
 (0)