From a7717868d5b7d0c3d1ed1aba9012aff0a6798ebb Mon Sep 17 00:00:00 2001
From: Amar K P <amarkerpala59@gmail.com>
Date: Wed, 7 Oct 2026 13:27:04 +0530
Subject: [PATCH] fix: address review findings on billing, markup, and prompt
 fidelity

- free-tier: pass free_tier to commit telemetry so spend is not repriced at paid rates (bug 1)
- ui: render preview, git output, and menus as literal text so [brackets] survive (bug 2)
- prompt: label size-truncated and binary files by path in the prompt (bugs 3, 4)
- config: honour ignore_globs in pr-summary and changelog extractors (bug 5)
- cache: don't bill chunk summaries served from cache; track last_hit on CachedProvider (bug 6)
- budget: check the monthly cap once per command via check_budget flag (bug 7)
- fallback: say 'Token budgeting failed' instead of 'API unavailable' (bug 8)
---
 src/gitmate/changelog.py       |  3 +-
 src/gitmate/chunking.py        | 13 +++--
 src/gitmate/committer.py       | 20 +++++---
 src/gitmate/fallback.py        |  2 +
 src/gitmate/orchestrator.py    | 43 +++++++++++-----
 src/gitmate/pr_summary.py      |  7 ++-
 src/gitmate/providers/cache.py |  3 ++
 src/gitmate/token_budget.py    |  5 +-
 tests/test_committer.py        | 47 +++++++++++++++++
 tests/test_orchestrator.py     | 93 +++++++++++++++++++++++++++++++---
 tests/test_pr_summary.py       | 17 +++++++
 11 files changed, 219 insertions(+), 34 deletions(-)

diff --git a/src/gitmate/changelog.py b/src/gitmate/changelog.py
index b721b6d..3b69663 100644
--- a/src/gitmate/changelog.py
+++ b/src/gitmate/changelog.py
@@ -221,7 +221,7 @@ def generate_changelog(
     extractor = (
         diff_extractor
         if diff_extractor is not None
-        else DiffExtractor(repo=repo_path, runner=runner)
+        else DiffExtractor(repo=repo_path, runner=runner, extra_ignores=active_cfg.ignore_globs)
     )
     diffs = []
     if include_diff:
@@ -253,6 +253,7 @@ def generate_changelog(
         console=con,
         secret_store=secret_store,
         bypass_cache=bypass_cache,
+        check_budget=False,  # already checked above
     )
 
     if output_file is not None:
diff --git a/src/gitmate/chunking.py b/src/gitmate/chunking.py
index 2d41589..427fccc 100644
--- a/src/gitmate/chunking.py
+++ b/src/gitmate/chunking.py
@@ -42,7 +42,8 @@ def summarize_omitted_diffs(
     """Summarize omitted diffs per file/chunk and return merged notes and token usage.
 
     Consumes BudgetDecision.omitted_diffs (which preserves untruncated patches),
-    prompts the provider for concise per-file summaries, and aggregates token usage.
+    prompts the provider for concise per-file summaries, and aggregates billed token
+    usage (responses served from a CachedProvider are excluded).
     """
     summaries: list[str] = []
     total_in = 0
@@ -63,10 +64,12 @@ def summarize_omitted_diffs(
                 f"{chunk}"
             )
             resp = provider.generate(prompt=prompt, model=model)
-            if resp.input_tokens is not None:
-                total_in += resp.input_tokens
-            if resp.output_tokens is not None:
-                total_out += resp.output_tokens
+            # Cache hits consumed no API tokens this run; don't bill them.
+            if not getattr(provider, "last_hit", False):
+                if resp.input_tokens is not None:
+                    total_in += resp.input_tokens
+                if resp.output_tokens is not None:
+                    total_out += resp.output_tokens
             text = resp.text.strip()
             if text:
                 file_part_summaries.append(text)
diff --git a/src/gitmate/committer.py b/src/gitmate/committer.py
index 8445b64..a34ff60 100644
--- a/src/gitmate/committer.py
+++ b/src/gitmate/committer.py
@@ -13,7 +13,9 @@
 from pathlib import Path
 
 from rich.console import Console
+from rich.markup import escape
 from rich.panel import Panel
+from rich.text import Text
 
 from gitmate import config as config_mod
 from gitmate.config import GitmateConfig
@@ -56,7 +58,7 @@ def _render_panel(console: Console, message: str, model: str, is_fallback: bool)
         else f"[green bold]{model}[/green bold]"
     )
     panel = Panel(
-        message,
+        Text(message),
         title=f"Proposed Commit Message [dim]({status_badge})[/dim]",
         border_style="yellow" if is_fallback else "green",
         padding=(1, 2),
@@ -88,12 +90,12 @@ def _execute_commit(message: str, console: Console | None = None, cwd: Path | No
         )
         if proc.returncode == 0:
             if console is not None and proc.stdout.strip():
-                console.print(proc.stdout.strip())
+                console.print(Text(proc.stdout.strip()))
         else:
             if console is not None:
                 err_msg = proc.stderr.strip() or proc.stdout.strip()
                 if err_msg:
-                    console.print(f"[red]{err_msg}[/red]")
+                    console.print(Text(err_msg, style="red"))
         return proc.returncode
     except FileNotFoundError:
         if console is not None:
@@ -236,6 +238,7 @@ def commit_flow(
         console=con,
         secret_store=secret_store,
         bypass_cache=False,
+        check_budget=False,
     )
     latency_ms = int((time.perf_counter() - t0) * 1000)
 
@@ -248,6 +251,7 @@ def commit_flow(
         latency_ms=latency_ms,
         fallback_used=gen_result.is_fallback,
         estimated_cost_usd=gen_result.estimated_cost_usd,
+        free_tier=active_cfg.free_tier,
     )
 
     current_message = gen_result.text
@@ -265,7 +269,7 @@ def commit_flow(
     while True:
         try:
             raw_input = con.input(
-                "[bold cyan]?[/bold cyan] [bold][a]ccept / [e]dit / [r]egenerate / [c]ancel:[/bold] "
+                "[bold cyan]?[/bold cyan] [bold]\\[a]ccept / \\[e]dit / \\[r]egenerate / \\[c]ancel:[/bold] "
             )
         except (EOFError, KeyboardInterrupt):
             con.print("\nCommit cancelled.")
@@ -274,7 +278,7 @@ def commit_flow(
         choice = raw_input.strip().lower()
         if not choice:
             con.print(
-                "[yellow]Please select an option: [a]ccept, [e]dit, [r]egenerate, or [c]ancel.[/yellow]"
+                "[yellow]Please select an option: \\[a]ccept, \\[e]dit, \\[r]egenerate, or \\[c]ancel.[/yellow]"
             )
             continue
 
@@ -306,6 +310,7 @@ def commit_flow(
                 console=con,
                 secret_store=secret_store,
                 bypass_cache=True,
+                check_budget=False,
             )
             latency_ms = int((time.perf_counter() - t0) * 1000)
 
@@ -318,6 +323,7 @@ def commit_flow(
                 latency_ms=latency_ms,
                 fallback_used=gen_result.is_fallback,
                 estimated_cost_usd=gen_result.estimated_cost_usd,
+                free_tier=active_cfg.free_tier,
             )
 
             current_message = gen_result.text
@@ -331,6 +337,6 @@ def commit_flow(
 
         else:
             con.print(
-                f"[yellow]Invalid selection '{raw_input}'. "
-                "Please choose [a]ccept, [e]dit, [r]egenerate, or [c]ancel.[/yellow]"
+                f"[yellow]Invalid selection '{escape(raw_input)}'. "
+                "Please choose \\[a]ccept, \\[e]dit, \\[r]egenerate, or \\[c]ancel.[/yellow]"
             )
diff --git a/src/gitmate/fallback.py b/src/gitmate/fallback.py
index 28a855f..eb3e4f0 100644
--- a/src/gitmate/fallback.py
+++ b/src/gitmate/fallback.py
@@ -264,6 +264,7 @@ def generate_commit_message(
     bypass_cache: bool = False,
     command: str | None = None,
     hook_mode: bool = False,
+    check_budget: bool = True,
 ) -> GenerationResult:
     """Orchestrate commit message generation with graceful fallback on provider failure."""
     return run_generation_pipeline(
@@ -279,4 +280,5 @@ def generate_commit_message(
         console=console,
         secret_store=secret_store,
         bypass_cache=bypass_cache,
+        check_budget=check_budget,
     )
diff --git a/src/gitmate/orchestrator.py b/src/gitmate/orchestrator.py
index ae475d3..598165d 100644
--- a/src/gitmate/orchestrator.py
+++ b/src/gitmate/orchestrator.py
@@ -17,9 +17,10 @@
 from gitmate.metrics import calculate_cost, get_monthly_spend, record_invocation
 from gitmate.prompt import load_template, render_prompt
 from gitmate.providers.base import LLMProvider, ProviderUnavailable
-from gitmate.providers.cache import CachedProvider, cache_key
+from gitmate.providers.cache import CachedProvider
 from gitmate.providers.gemini import GeminiProvider
 from gitmate.token_budget import (
+    OMITTED_NOTE_PREFIX,
     BudgetStrategy,
     GeminiTokenCounter,
     HeuristicTokenCounter,
@@ -105,6 +106,23 @@ def check_budget_cap(cfg: GitmateConfig, console: Console | None = None) -> None
         )
 
 
+def _prompt_diff_text(diffs: list[FileDiff]) -> str:
+    """Join patches for the prompt, labelling files whose patch text lacks a header.
+
+    Binary files carry no patch and truncated files carry only a size note, so
+    without a label the model would never learn which file they were.
+    """
+    parts: list[str] = []
+    for d in diffs:
+        if d.is_binary:
+            parts.append(f"Binary file {d.path} ({d.status})")
+        elif d.patch_text.startswith(OMITTED_NOTE_PREFIX):
+            parts.append(f"File {d.path} ({d.status}): {d.patch_text}")
+        elif d.patch_text:
+            parts.append(d.patch_text)
+    return "\n".join(parts)
+
+
 def run_generation_pipeline(
     diffs: list[FileDiff],
     cfg: GitmateConfig,
@@ -119,8 +137,13 @@ def run_generation_pipeline(
     secret_store: SecretStore | None = None,
     bypass_cache: bool = False,
     hook_mode: bool = False,
+    check_budget: bool = True,
 ) -> GenerationResult:
-    """Execute unified generation: token budgeting, chunking, caching, LLM call, and fallback."""
+    """Execute unified generation: token budgeting, chunking, caching, LLM call, and fallback.
+
+    Pass check_budget=False only when the caller has already run check_budget_cap,
+    so the 80% warning is not printed twice.
+    """
     if empty_diff_message is not None and not diffs:
         return GenerationResult(
             text=empty_diff_message,
@@ -128,7 +151,8 @@ def run_generation_pipeline(
             model=cfg.model,
         )
 
-    check_budget_cap(cfg, console=console)
+    if check_budget:
+        check_budget_cap(cfg, console=console)
 
     store = secret_store if secret_store is not None else config_mod.KeyringSecretStore()
     api_key = store.get_secret(API_KEY_ACCOUNT) or os.environ.get("GEMINI_API_KEY")
@@ -170,7 +194,7 @@ def run_generation_pipeline(
             decision = budget_mgr.assess(diffs)
         except TokenBudgetError as exc:
             if console is not None:
-                console.print("[yellow]⚠ API unavailable, using template fallback[/yellow]")
+                console.print("[yellow]⚠ Token budgeting failed, using template fallback[/yellow]")
             gen_result = GenerationResult(
                 text=fallback_generator(),
                 is_fallback=True,
@@ -225,12 +249,9 @@ def run_generation_pipeline(
             summary_note = (
                 f"{decision.summary_note}\n\nSummaries of large omitted files:\n{merged_summaries}"
             )
-            patch_chunks = [d.patch_text for d in decision.included_diffs if d.patch_text]
-            diff_text = "\n".join(patch_chunks)
         else:
-            patch_chunks = [d.patch_text for d in decision.included_diffs if d.patch_text]
-            diff_text = "\n".join(patch_chunks)
             summary_note = decision.summary_note
+        diff_text = _prompt_diff_text(decision.included_diffs)
 
         render_kwargs: dict[str, str] = {
             "diff": diff_text,
@@ -241,12 +262,8 @@ def run_generation_pipeline(
 
         prompt_text, _ = render_prompt(template_name, **render_kwargs)
 
-        cache_hit = False
-        if not bypass_cache and isinstance(active_provider, CachedProvider):
-            key = cache_key(prompt_text, cfg.model, active_provider._template_version)
-            cache_hit = active_provider._cache.get(key, default=None) is not None
-
         resp = active_provider.generate(prompt=prompt_text, model=cfg.model)
+        cache_hit = isinstance(active_provider, CachedProvider) and active_provider.last_hit
         if resp.input_tokens is not None:
             total_input_tokens += resp.input_tokens
         if resp.output_tokens is not None:
diff --git a/src/gitmate/pr_summary.py b/src/gitmate/pr_summary.py
index 9ef2a71..ee20419 100644
--- a/src/gitmate/pr_summary.py
+++ b/src/gitmate/pr_summary.py
@@ -93,7 +93,11 @@ def generate_pr_summary(
 
     check_budget_cap(active_cfg, console=con)
 
-    extractor = diff_extractor if diff_extractor is not None else DiffExtractor(repo=repo_dir)
+    extractor = (
+        diff_extractor
+        if diff_extractor is not None
+        else DiffExtractor(repo=repo_dir, extra_ignores=active_cfg.ignore_globs)
+    )
     diffs = extractor.branch_comparison(base=base)
 
     if not diffs:
@@ -115,6 +119,7 @@ def generate_pr_summary(
         console=con,
         secret_store=secret_store,
         bypass_cache=bypass_cache,
+        check_budget=False,  # already checked above
     )
 
     if copy_to_cb:
diff --git a/src/gitmate/providers/cache.py b/src/gitmate/providers/cache.py
index e26e9f2..c2b2f41 100644
--- a/src/gitmate/providers/cache.py
+++ b/src/gitmate/providers/cache.py
@@ -46,6 +46,8 @@ def __init__(
         self._cache = Cache(resolved)
         self._ttl = ttl
         self._template_version = template_version
+        #: True when the most recent generate() was answered from the cache.
+        self.last_hit = False
 
     @property
     def underlying(self) -> LLMProvider:
@@ -56,6 +58,7 @@ def generate(self, prompt: str, model: str) -> LLMResponse:
         """Return a cached response when present, else generate and store."""
         key = cache_key(prompt, model, self._template_version)
         cached: LLMResponse | None = self._cache.get(key, default=None)
+        self.last_hit = cached is not None
         if cached is not None:
             return cached
         response = self._provider.generate(prompt, model)
diff --git a/src/gitmate/token_budget.py b/src/gitmate/token_budget.py
index 9afa6ed..7acbb9a 100644
--- a/src/gitmate/token_budget.py
+++ b/src/gitmate/token_budget.py
@@ -18,6 +18,9 @@
 if TYPE_CHECKING:
     from google import genai
 
+#: Start of the placeholder that replaces a patch dropped for size.
+OMITTED_NOTE_PREFIX = "[patch omitted for size:"
+
 
 class TokenBudgetError(Exception):
     """Base exception for token budgeting and counting errors."""
@@ -264,7 +267,7 @@ def assess(
         current_tokens = total_tokens
 
         for candidate in candidates:
-            note = f"[patch omitted for size: +{candidate.additions}/-{candidate.deletions} lines]"
+            note = f"{OMITTED_NOTE_PREFIX} +{candidate.additions}/-{candidate.deletions} lines]"
             if len(candidate.patch_text) <= len(note):
                 continue
             omitted.append(candidate)
diff --git a/tests/test_committer.py b/tests/test_committer.py
index 52c4768..a2bb037 100644
--- a/tests/test_committer.py
+++ b/tests/test_committer.py
@@ -573,3 +573,50 @@ def test_commit_flow_budget_cap_warning_allows_commit(
     assert provider.call_count == 1
     messages = _get_git_log_messages(git_repo)
     assert messages[0] == "feat: allowed commit"
+
+
+# --- Regression: free-tier spend & Rich markup in the review UI ---
+
+
+def test_free_tier_commit_records_zero_spend(
+    git_repo: Path, metrics_dir: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """Free-tier commits must not be repriced at paid rates when read back."""
+    from gitmate.metrics import get_monthly_spend
+
+    stage_file(git_repo, "file.txt", b"content\n")
+    monkeypatch.setattr(sys.stdin, "isatty", lambda: False)
+    cfg = GitmateConfig(allow_noninteractive_commit=True, free_tier=True)
+
+    assert commit_flow(yes=True, cfg=cfg, provider=FakeProvider()) == 0
+
+    invocations = get_invocations()
+    assert invocations[0].free_tier is True
+    assert get_monthly_spend() == 0.0
+
+
+def test_preview_and_git_output_render_brackets_literally(
+    git_repo: Path, metrics_dir: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """`[...]` in the message, menu, and git output must not be eaten as Rich markup."""
+    stage_file(git_repo, "file.txt", b"content\n")
+    monkeypatch.setattr(sys.stdin, "isatty", lambda: True)
+    con = Console(record=True, width=200)
+    prompts: list[str] = []
+    inputs = iter(["[/oops]", "a"])
+
+    def _input(prompt: str = "") -> str:
+        prompts.append(prompt)
+        return next(inputs)
+
+    monkeypatch.setattr(con, "input", _input)
+    message = "fix(cli): handle list[str] input [skip ci]"
+
+    assert commit_flow(console=con, provider=FakeProvider(message)) == 0
+
+    out = con.export_text()
+    assert message in out  # preview panel
+    assert "Invalid selection '[/oops]'" in out  # no MarkupError
+    assert "[a]ccept, [e]dit, [r]egenerate, or [c]ancel" in out
+    assert "[master " in out or "[main " in out  # git's "[branch hash]" line
+    assert _get_git_log_messages(git_repo)[0] == message
diff --git a/tests/test_orchestrator.py b/tests/test_orchestrator.py
index 570f3eb..2b67c60 100644
--- a/tests/test_orchestrator.py
+++ b/tests/test_orchestrator.py
@@ -158,7 +158,7 @@ def test_generate_commit_message_counter_failure_triggers_fallback() -> None:
     assert result.text == "feat(auth): add login.py"
     assert "Token budgeting failed" in (result.fallback_reason or "")
     console.print.assert_called_once_with(
-        "[yellow]⚠ API unavailable, using template fallback[/yellow]"
+        "[yellow]⚠ Token budgeting failed, using template fallback[/yellow]"
     )
 
 
@@ -333,10 +333,10 @@ def test_generate_commit_message_chunking_success_final_call_fails_preserves_chu
     )
 
 
-def test_generate_commit_message_chunking_with_cached_final_call_preserves_chunk_cost(
+def test_generate_commit_message_fully_cached_rerun_costs_nothing(
     tmp_path: Path,
 ) -> None:
-    """When final prompt hits cache, chunking cost is still preserved rather than zeroed."""
+    """A rerun whose chunk summaries and final prompt are all cached must cost $0."""
     from gitmate.metrics import calculate_cost
     from gitmate.providers.cache import CachedProvider
 
@@ -368,7 +368,7 @@ def test_generate_commit_message_chunking_with_cached_final_call_preserves_chunk
     expected_full_cost = round(chunk_cost + final_cost, 6)
     assert res1.estimated_cost_usd == expected_full_cost
 
-    # 2. Second invocation: final prompt hits cache
+    # 2. Second invocation: chunk summaries and final prompt both hit cache
     res2 = generate_commit_message(
         diffs=_sample_diff(),
         cfg=cfg,
@@ -378,5 +378,86 @@ def test_generate_commit_message_chunking_with_cached_final_call_preserves_chunk
     )
     assert res2.is_fallback is False
     assert res2.cache_hit is True
-    # Final call cost was 0 (cache hit), but chunk_cost MUST still be preserved!
-    assert res2.estimated_cost_usd == chunk_cost
+    # No API tokens were billed on the rerun: cached chunk summaries are excluded.
+    assert res2.estimated_cost_usd == 0.0
+
+
+# --- Regression: file identity in prompt, cache-hit billing, single budget check ---
+
+
+def test_prompt_names_binary_and_truncated_files() -> None:
+    """Binary and size-truncated files must still be identified by path in the prompt."""
+    big = FileDiff(
+        path="src/huge.py",
+        old_path=None,
+        status="modified",
+        additions=900,
+        deletions=0,
+        patch_text="diff --git a/src/huge.py b/src/huge.py\n" + "+x = 1\n" * 900,
+        is_binary=False,
+    )
+    logo = FileDiff(
+        path="assets/logo.png",
+        old_path=None,
+        status="added",
+        additions=0,
+        deletions=0,
+        patch_text="",
+        is_binary=True,
+    )
+    small = _sample_diff()[0]
+    provider = FakeProvider()
+    # Tight window: forces truncation of the big file only.
+    cfg = GitmateConfig(max_context_tokens=2048 + 500 + 400)
+
+    generate_commit_message(
+        diffs=[big, logo, small], cfg=cfg, provider=provider, counter=FakeCounter(token_rate=4)
+    )
+
+    prompt = provider.prompts[-1]
+    assert "File src/huge.py (modified): [patch omitted for size:" in prompt
+    assert "Binary file assets/logo.png (added)" in prompt
+    assert "+class LoginService:" in prompt
+
+
+def test_cached_chunk_summaries_are_not_billed(tmp_path: Path) -> None:
+    """A fully cached rerun of a chunked generation must cost $0."""
+    from gitmate.chunking import summarize_omitted_diffs
+    from gitmate.providers.cache import CachedProvider
+
+    provider = CachedProvider(FakeProvider("summary"), cache_dir=tmp_path)
+    diffs = _sample_diff()
+
+    _, first_in, first_out = summarize_omitted_diffs(diffs, provider, "gemini-3.5-flash-lite")
+    _, again_in, again_out = summarize_omitted_diffs(diffs, provider, "gemini-3.5-flash-lite")
+
+    assert (first_in, first_out) == (150, 25)
+    assert (again_in, again_out) == (0, 0)
+    provider.close()
+
+
+def test_main_call_cache_hit_detected(tmp_path: Path) -> None:
+    from gitmate.providers.cache import CachedProvider
+
+    provider = CachedProvider(FakeProvider(), cache_dir=tmp_path)
+    cfg = GitmateConfig()
+    kwargs = {"diffs": _sample_diff(), "cfg": cfg, "provider": provider, "counter": FakeCounter()}
+
+    assert generate_commit_message(**kwargs).cache_hit is False  # type: ignore[arg-type]
+    second = generate_commit_message(**kwargs)  # type: ignore[arg-type]
+    assert second.cache_hit is True
+    assert second.estimated_cost_usd == 0.0
+    provider.close()
+
+
+def test_check_budget_false_skips_cap(monkeypatch: pytest.MonkeyPatch) -> None:
+    """Callers that already enforced the cap must not trigger a second check."""
+    import gitmate.orchestrator as orch
+
+    calls: list[object] = []
+    monkeypatch.setattr(orch, "check_budget_cap", lambda *a, **k: calls.append(a))
+    common = {"diffs": _sample_diff(), "cfg": GitmateConfig(), "provider": FakeProvider()}
+
+    generate_commit_message(**common, counter=FakeCounter())  # type: ignore[arg-type]
+    generate_commit_message(**common, counter=FakeCounter(), check_budget=False)  # type: ignore[arg-type]
+    assert len(calls) == 1
diff --git a/tests/test_pr_summary.py b/tests/test_pr_summary.py
index 457cb98..adcf98e 100644
--- a/tests/test_pr_summary.py
+++ b/tests/test_pr_summary.py
@@ -207,3 +207,20 @@ def test_pr_summary_gh_cli_error(git_repo: Path) -> None:
             copy_to_cb=False,
             create_pr=True,
         )
+
+
+def test_pr_summary_honours_user_ignore_globs(git_repo: Path, metrics_dir: Path) -> None:
+    """User ignore_globs must filter PR diffs just like they do for commit."""
+    subprocess.run(["git", "checkout", "-b", "feature-y"], cwd=git_repo, check=True)
+    commit_file(git_repo, "hello.py", "print('hello')", "feat: add hello")
+    commit_file(git_repo, "generated/schema.json", "{}", "chore: regen schema")
+
+    provider = FakeProvider()
+    cfg = GitmateConfig(ignore_globs=["generated/"])
+    generate_pr_summary(
+        base="main", cfg=cfg, provider=provider, repo_dir=git_repo, copy_to_cb=False
+    )
+
+    prompt = provider.calls[0][0]
+    assert "hello.py" in prompt
+    assert "schema.json" not in prompt