Phase 5 — Hardening: size gates, retry, structured logging, quote-based resolution #14

New issue

Closed

opened 2026-06-10 21:23:18 +00:00 by coding-agent-marvin8 · 6 comments

coding-agent-marvin8 commented 2026-06-10 21:23:18 +00:00 (Migrated from codeberg.org)

Copy link

Phase 5 hardening work. Six self-contained items implemented in the same branch via TDD.

Checklist

• PR size gate — WUMING_MAX_DIFF_LINES env var; post a plain comment and exit cleanly when exceeded
• Hunk size cap — truncate_large_hunks() in diff.py; replace oversized hunks with a single stub line before sending to agents
• Approximate token budget — estimated_tokens = len(diff_text) // 4; emit a structured log event per agent call for observability
• HTTP retry with exponential backoff — with_retry() async helper in retry.py; applied to all ForgejoClient and backend HTTP calls; no new runtime deps
• Structured JSON logging — src/wuming/log.py; JSONFormatter (stdlib); replace all print() calls; Woodpecker-friendly
• Quote-based line resolution — quote field on ReviewComment; find_line_by_quote() in diff.py; agents supply verbatim line text; _resolve_positions tries quote-search before trusting the LLM line number

Rationale

• Size gate: prevents sending giant diffs to LLMs, silently burning tokens and hitting context limits
• Hunk cap: ensures a single huge hunk (e.g. a generated file that slipped through path filters) does not blow the context window
• Token budget: gives operators visibility into per-call cost without adding new dependencies
• Retry: makes the tool resilient against transient Forgejo API and LLM API hiccups (rate limits, 503s)
• Structured logging: required for Woodpecker CI log parsers and observability tooling; current print() output is not machine-readable
• Quote resolution: avoids silently dropping comments when the LLM hallucinates a line number; quoting the exact line text is more robust

Phase 5 hardening work. Six self-contained items implemented in the same branch via TDD. ## Checklist - [ ] PR size gate — `WUMING_MAX_DIFF_LINES` env var; post a plain comment and exit cleanly when exceeded - [ ] Hunk size cap — `truncate_large_hunks()` in `diff.py`; replace oversized hunks with a single stub line before sending to agents - [ ] Approximate token budget — `estimated_tokens = len(diff_text) // 4`; emit a structured log event per agent call for observability - [ ] HTTP retry with exponential backoff — `with_retry()` async helper in `retry.py`; applied to all `ForgejoClient` and backend HTTP calls; no new runtime deps - [ ] Structured JSON logging — `src/wuming/log.py`; `JSONFormatter` (stdlib); replace all `print()` calls; Woodpecker-friendly - [ ] Quote-based line resolution — `quote` field on `ReviewComment`; `find_line_by_quote()` in `diff.py`; agents supply verbatim line text; `_resolve_positions` tries quote-search before trusting the LLM line number ## Rationale - **Size gate**: prevents sending giant diffs to LLMs, silently burning tokens and hitting context limits - **Hunk cap**: ensures a single huge hunk (e.g. a generated file that slipped through path filters) does not blow the context window - **Token budget**: gives operators visibility into per-call cost without adding new dependencies - **Retry**: makes the tool resilient against transient Forgejo API and LLM API hiccups (rate limits, 503s) - **Structured logging**: required for Woodpecker CI log parsers and observability tooling; current `print()` output is not machine-readable - **Quote resolution**: avoids silently dropping comments when the LLM hallucinates a line number; quoting the exact line text is more robust

coding-agent-marvin8 commented 2026-06-10 21:23:36 +00:00 (Migrated from codeberg.org)

Copy link

Task 1: PR size gate

Motivation: A very large PR (thousands of changed lines) exceeds the LLM context window, causing silent truncation or API errors. Better to refuse early with a clear message than to produce incomplete or garbled reviews.

New env var: WUMING_MAX_DIFF_LINES (integer, default 0 = disabled). When the raw diff line count exceeds the configured value, WuMing posts a single plain PR comment explaining the PR is too large to review automatically, then returns without posting any inline review.

Key files:

• src/wuming/config.py — add max_diff_lines: int field; parse WUMING_MAX_DIFF_LINES
• src/wuming/forgejo.py — add post_comment(body: str) method (Forgejo issues comments endpoint: POST /api/v1/repos/{owner}/{repo}/issues/{index}/comments); distinct from post_review()
• src/wuming/main.py — after raw_diff = await forgejo.get_diff(), check len(raw_diff.splitlines()) > config.max_diff_lines; if triggered, call await forgejo.post_comment(...) and return

Approach: Add the gate as the very first check after fetching the diff, before any parsing or agent dispatch. The plain comment (not a code review) uses the issues API so it always appears even when no diff positions exist.

## Task 1: PR size gate **Motivation**: A very large PR (thousands of changed lines) exceeds the LLM context window, causing silent truncation or API errors. Better to refuse early with a clear message than to produce incomplete or garbled reviews. **New env var**: `WUMING_MAX_DIFF_LINES` (integer, default `0` = disabled). When the raw diff line count exceeds the configured value, WuMing posts a single plain PR comment explaining the PR is too large to review automatically, then returns without posting any inline review. **Key files**: - `src/wuming/config.py` — add `max_diff_lines: int` field; parse `WUMING_MAX_DIFF_LINES` - `src/wuming/forgejo.py` — add `post_comment(body: str)` method (Forgejo issues comments endpoint: `POST /api/v1/repos/{owner}/{repo}/issues/{index}/comments`); distinct from `post_review()` - `src/wuming/main.py` — after `raw_diff = await forgejo.get_diff()`, check `len(raw_diff.splitlines()) > config.max_diff_lines`; if triggered, call `await forgejo.post_comment(...)` and return **Approach**: Add the gate as the very first check after fetching the diff, before any parsing or agent dispatch. The plain comment (not a code review) uses the issues API so it always appears even when no diff positions exist.

coding-agent-marvin8 commented 2026-06-10 21:23:49 +00:00 (Migrated from codeberg.org)

Copy link

Task 2: Hunk size cap

Motivation: A single oversized hunk (e.g. 2000-line auto-generated file that slipped through skip-path filters) sends an enormous block of text to the LLM, consuming most or all of the available context and leaving no room for the actual review prompt. Truncating it with a clear stub preserves the diff structure while keeping the payload manageable.

Key file: src/wuming/diff.py

New function:

def truncate_large_hunks(file_diffs: list[FileDiff], max_lines: int = 300) -> list[FileDiff]

Any Hunk whose lines tuple exceeds max_lines is replaced with a single +-prefixed stub line:

[... N lines truncated — hunk too large for review ...]

The stub starts with + so build_position_map and _format_diff_text treat it as a valid added line with a new-file line number. The function is pure — it returns a new list without mutating the input.

Wire-up: Called in src/wuming/main.py immediately after filter_diff(), before diffs are handed to the router.

## Task 2: Hunk size cap **Motivation**: A single oversized hunk (e.g. 2000-line auto-generated file that slipped through skip-path filters) sends an enormous block of text to the LLM, consuming most or all of the available context and leaving no room for the actual review prompt. Truncating it with a clear stub preserves the diff structure while keeping the payload manageable. **Key file**: `src/wuming/diff.py` **New function**: ```python def truncate_large_hunks(file_diffs: list[FileDiff], max_lines: int = 300) -> list[FileDiff] ``` Any `Hunk` whose `lines` tuple exceeds `max_lines` is replaced with a single `+`-prefixed stub line: ``` [... N lines truncated — hunk too large for review ...] ``` The stub starts with `+` so `build_position_map` and `_format_diff_text` treat it as a valid added line with a new-file line number. The function is pure — it returns a new list without mutating the input. **Wire-up**: Called in `src/wuming/main.py` immediately after `filter_diff()`, before diffs are handed to the router.

coding-agent-marvin8 commented 2026-06-10 21:24:01 +00:00 (Migrated from codeberg.org)

Copy link

Task 3: Approximate token budget per agent call

Motivation: There is currently no visibility into how much of an LLM's context window each agent call consumes. Logging the estimate gives operators early warning when a diff is approaching context limits, without requiring a paid token-counting API call.

Formula: estimated_tokens = len(diff_text) // 4 (chars / 4 is the standard rough approximation for English-like code text).

Key file: src/wuming/agents/base.py

Change: In BaseAgent.review(), after diff_text = _format_diff_text(matching), emit:

logger.info("token budget", extra={"agent": self.name, "estimated_tokens": estimated_tokens})

This is a purely observational addition — no capping, no early exit. The log event uses the structured JSON logger added in Task 5 so it is machine-readable in Woodpecker.

## Task 3: Approximate token budget per agent call **Motivation**: There is currently no visibility into how much of an LLM's context window each agent call consumes. Logging the estimate gives operators early warning when a diff is approaching context limits, without requiring a paid token-counting API call. **Formula**: `estimated_tokens = len(diff_text) // 4` (chars / 4 is the standard rough approximation for English-like code text). **Key file**: `src/wuming/agents/base.py` **Change**: In `BaseAgent.review()`, after `diff_text = _format_diff_text(matching)`, emit: ```python logger.info("token budget", extra={"agent": self.name, "estimated_tokens": estimated_tokens}) ``` This is a purely observational addition — no capping, no early exit. The log event uses the structured JSON logger added in Task 5 so it is machine-readable in Woodpecker.

coding-agent-marvin8 commented 2026-06-10 21:24:17 +00:00 (Migrated from codeberg.org)

Copy link

Task 4: HTTP retry with exponential backoff

Motivation: Transient network failures, 503s from overloaded LLM APIs, and 429 rate-limit responses currently crash the entire review run. A simple retry loop with exponential backoff handles the common case without adding runtime dependencies.

New file: src/wuming/retry.py

New function:

async def with_retry(
    fn: Callable[[], Awaitable[T]],
    *,
    max_attempts: int = 3,
    base_delay: float = 1.0,
) -> T

Retry policy:

• Retries on httpx2.TransportError (network-level)
• Retries on httpx2.HTTPStatusError with status >= 500 (server error)
• Retries on httpx2.HTTPStatusError with status == 429 (rate-limited)
• Does not retry on 4xx other than 429 (client errors are not transient)
• Does not retry on non-httpx2 exceptions
• Delay before attempt k: base_delay * 2**(k-1) seconds via asyncio.sleep
• After max_attempts exhausted, re-raises the last exception

Applied to:

• All five call sites in src/wuming/forgejo.py (get_diff, get_pr_head_sha, list_review_comment_bodies, post_review, post_comment)
• The complete() method in each of deepseek.py, anthropic.py, ollama.py

No new runtime dependencies — uses only asyncio and httpx2 (already present).

## Task 4: HTTP retry with exponential backoff **Motivation**: Transient network failures, 503s from overloaded LLM APIs, and 429 rate-limit responses currently crash the entire review run. A simple retry loop with exponential backoff handles the common case without adding runtime dependencies. **New file**: `src/wuming/retry.py` **New function**: ```python async def with_retry( fn: Callable[[], Awaitable[T]], *, max_attempts: int = 3, base_delay: float = 1.0, ) -> T ``` Retry policy: - Retries on `httpx2.TransportError` (network-level) - Retries on `httpx2.HTTPStatusError` with `status >= 500` (server error) - Retries on `httpx2.HTTPStatusError` with `status == 429` (rate-limited) - Does **not** retry on 4xx other than 429 (client errors are not transient) - Does **not** retry on non-httpx2 exceptions - Delay before attempt `k`: `base_delay * 2**(k-1)` seconds via `asyncio.sleep` - After `max_attempts` exhausted, re-raises the last exception **Applied to**: - All five call sites in `src/wuming/forgejo.py` (`get_diff`, `get_pr_head_sha`, `list_review_comment_bodies`, `post_review`, `post_comment`) - The `complete()` method in each of `deepseek.py`, `anthropic.py`, `ollama.py` No new runtime dependencies — uses only `asyncio` and `httpx2` (already present).

coding-agent-marvin8 commented 2026-06-10 21:24:35 +00:00 (Migrated from codeberg.org)

Copy link

Task 5: Structured JSON logging

Motivation: All current log/status output uses print(), which is plain text and invisible to Woodpecker's structured log parsing. Replacing it with newline-delimited JSON enables log aggregation, filtering by level, and per-agent metrics — without adding a third-party logging library.

New file: src/wuming/log.py

Key classes/functions:

class JSONFormatter(logging.Formatter):
    # format() → single JSON line per record
    # Always emits: {"ts": "<ISO8601>", "level": "INFO", "msg": "..."}
    # Merges in any extra={"..": ".."} kwargs at the top level

def setup_logging() -> logging.Logger:
    # Attaches a StreamHandler(stderr) with JSONFormatter to the "wuming" logger
    # Idempotent — safe to call multiple times
    # Returns the "wuming" logger

Stdlib only: datetime, json, logging.

Migration: Replace every print() and print(..., file=sys.stderr) call in main.py, router.py, and agents/base.py with logger.info/warning/error. Call setup_logging() at the top of _main(). Each module declares a module-level logger = logging.getLogger("wuming.<module>").

## Task 5: Structured JSON logging **Motivation**: All current log/status output uses `print()`, which is plain text and invisible to Woodpecker's structured log parsing. Replacing it with newline-delimited JSON enables log aggregation, filtering by level, and per-agent metrics — without adding a third-party logging library. **New file**: `src/wuming/log.py` **Key classes/functions**: ```python class JSONFormatter(logging.Formatter): # format() → single JSON line per record # Always emits: {"ts": "<ISO8601>", "level": "INFO", "msg": "..."} # Merges in any extra={"..": ".."} kwargs at the top level def setup_logging() -> logging.Logger: # Attaches a StreamHandler(stderr) with JSONFormatter to the "wuming" logger # Idempotent — safe to call multiple times # Returns the "wuming" logger ``` Stdlib only: `datetime`, `json`, `logging`. **Migration**: Replace every `print()` and `print(..., file=sys.stderr)` call in `main.py`, `router.py`, and `agents/base.py` with `logger.info/warning/error`. Call `setup_logging()` at the top of `_main()`. Each module declares a module-level `logger = logging.getLogger("wuming.<module>")`.

coding-agent-marvin8 commented 2026-06-10 21:24:53 +00:00 (Migrated from codeberg.org)

Copy link

Task 6: Quote-based line resolution

Motivation: LLMs frequently hallucinate line numbers, causing valid review comments to be silently dropped during position resolution. Asking the LLM to also quote the exact verbatim text of the line it is commenting on allows us to locate the correct diff position by text search, falling back to the numeric line only when the quote is absent.

Changes:

src/wuming/comments.py

• Add quote: str | None = None as the last field on ReviewComment

src/wuming/diff.py

def find_line_by_quote(file_diffs: list[FileDiff], path: str, quote: str) -> int | None

Strips the leading +/ prefix from each non-deleted hunk line, compares to quote (exact, case-sensitive), and returns the new-file line number of the first match. Deleted (-) lines are skipped. Returns None if not found.

src/wuming/agents/base.py

• _parse_response(): extract item.get("quote") (optional, default None)
• review(): module-level logger emits token budget (Task 3)

Agent system prompts (code.py, config_agent.py, docs.py, shell.py)

• Add "quote" field to the expected JSON schema: "the exact verbatim text of the line, without the leading +/- prefix; omit or set to null if uncertain"

src/wuming/main.py — _resolve_positions()

• Gains a file_diffs: list[FileDiff] parameter
• If comment.quote is set: call find_line_by_quote() first; use the returned line number for the position_maps lookup; fall back to comment.line if the quote search returns None

## Task 6: Quote-based line resolution **Motivation**: LLMs frequently hallucinate line numbers, causing valid review comments to be silently dropped during position resolution. Asking the LLM to also quote the exact verbatim text of the line it is commenting on allows us to locate the correct diff position by text search, falling back to the numeric line only when the quote is absent. **Changes**: `src/wuming/comments.py` - Add `quote: str | None = None` as the last field on `ReviewComment` `src/wuming/diff.py` ```python def find_line_by_quote(file_diffs: list[FileDiff], path: str, quote: str) -> int | None ``` Strips the leading `+`/` ` prefix from each non-deleted hunk line, compares to `quote` (exact, case-sensitive), and returns the new-file line number of the first match. Deleted (`-`) lines are skipped. Returns `None` if not found. `src/wuming/agents/base.py` - `_parse_response()`: extract `item.get("quote")` (optional, default `None`) - `review()`: module-level logger emits token budget (Task 3) Agent system prompts (`code.py`, `config_agent.py`, `docs.py`, `shell.py`) - Add `"quote"` field to the expected JSON schema: *"the exact verbatim text of the line, without the leading +/- prefix; omit or set to null if uncertain"* `src/wuming/main.py` — `_resolve_positions()` - Gains a `file_diffs: list[FileDiff]` parameter - If `comment.quote` is set: call `find_line_by_quote()` first; use the returned line number for the `position_maps` lookup; fall back to `comment.line` if the quote search returns `None`

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

pages

feat/issue-57-ci-container-publish

feat/issue-53-review-quality

0.8.0

0.7.0

0.6.2

0.6.1

0.6.0

0.5.0

0.4.0

0.3.2

0.3.1

0.3.0

0.2.0

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference

marvin8/wuming#14

No description provided.