Compare commits
4 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
e28d16b324 | ||
|
|
bc32047610 | ||
|
|
3a24420d7d | ||
|
|
d14c1c5a56 |
@@ -1,387 +0,0 @@
|
||||
# Morning Review Packet
|
||||
|
||||
Source epic: [EPIC: Morning review packet — Hermes harness features landed 2026-04-21](https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/949)
|
||||
|
||||
## Epic context
|
||||
|
||||
EPIC: Morning review packet — Hermes harness features landed 2026-04-21
|
||||
|
||||
Source: git log on upstream/main since 2026-04-21 00:00 EDT, plus the current local branch `burn/921-poka-yoke-hardcoded-paths` for the branch-only path-guard work.
|
||||
|
||||
Important review note:
|
||||
- Validate upstream-landed features on `upstream/main` or a synced branch.
|
||||
- Validate the path-guard work on `burn/921-poka-yoke-hardcoded-paths`.
|
||||
|
||||
This epic is a morning-review packet: one QA issue per feature cluster, each with concrete acceptance criteria and targeted tests or manual checks.
|
||||
|
||||
## Success criteria
|
||||
- [ ] Every issue has a clear PASS / FAIL outcome.
|
||||
- [ ] Test output or manual evidence is attached to each issue.
|
||||
- [ ] Any drift between upstream/main and forge/main is called out explicitly.
|
||||
|
||||
## Sub-issues
|
||||
### Upstream/main features landed 2026-04-21
|
||||
- [ ] #950 [QA] Verify AI Gateway provider UX + attribution headers
|
||||
- [ ] #951 [QA] Verify transport abstraction + AnthropicTransport wiring
|
||||
- [ ] #952 [QA] Verify CLI voice beep toggle
|
||||
- [ ] #953 [QA] Verify bundled skill scripts run out of the box
|
||||
- [ ] #954 [QA] Verify maps skill guest_house / camp_site / bakery expansion
|
||||
- [ ] #955 [QA] Verify KittenTTS local provider end-to-end
|
||||
- [ ] #956 [QA] Verify numbered keyboard shortcuts for approval + clarify prompts
|
||||
- [ ] #957 [QA] Verify optional adversarial-ux-test skill catalog flow
|
||||
- [ ] #958 [QA] Verify /usage account limits in CLI + gateway
|
||||
- [ ] #959 [QA] Verify OpenCode-Go curated catalog additions
|
||||
- [ ] #960 [QA] Verify patch 'did you mean?' suggestions
|
||||
- [ ] #961 [QA] Verify web dashboard update/restart action buttons
|
||||
|
||||
### Local branch-only work
|
||||
- [ ] #962 [QA] Verify hardcoded-home path guard on burn/921 branch
|
||||
|
||||
## Summary
|
||||
|
||||
| Issue | State | Commits | Tests |
|
||||
| --- | --- | --- | --- |
|
||||
| #950 | open | 5 | 2 |
|
||||
| #951 | open | 2 | 2 |
|
||||
| #952 | open | 1 | 1 |
|
||||
| #953 | open | 1 | 2 |
|
||||
| #954 | open | 1 | 0 |
|
||||
| #955 | open | 2 | 1 |
|
||||
| #956 | open | 1 | 0 |
|
||||
| #957 | open | 1 | 0 |
|
||||
| #958 | open | 2 | 2 |
|
||||
| #959 | open | 1 | 1 |
|
||||
| #960 | open | 2 | 1 |
|
||||
| #961 | closed | 1 | 0 |
|
||||
| #962 | closed | 1 | 1 |
|
||||
|
||||
## #950 — [QA] Verify AI Gateway provider UX + attribution headers
|
||||
|
||||
State: open
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/950
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `b11753879` — attribution default_headers for ai-gateway provider
|
||||
- `700437440` — curated picker with live pricing
|
||||
- `ac26a460f` — promote ai-gateway in provider picker ordering
|
||||
- `5bb2d11b0` — auto-promote free Moonshot models
|
||||
- `29f57ec95` — Vercel deep-link for API key creation
|
||||
|
||||
### Targeted tests
|
||||
- `tests/hermes_cli/test_ai_gateway_models.py`
|
||||
- `tests/run_agent/test_provider_attribution_headers.py`
|
||||
|
||||
### Tasks
|
||||
- [ ] Open `hermes model` and verify `ai-gateway` appears near the top.
|
||||
- [ ] Verify live pricing appears in the picker.
|
||||
- [ ] Verify free Moonshot models are promoted.
|
||||
- [ ] Trigger API-key setup flow and verify the Vercel deep link.
|
||||
- [ ] Send one ai-gateway request and verify attribution headers are attached.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] UI ordering and pricing match the landed behavior.
|
||||
- [ ] Attribution headers are present on ai-gateway requests.
|
||||
- [ ] Targeted tests pass.
|
||||
|
||||
## #951 — [QA] Verify transport abstraction + AnthropicTransport wiring
|
||||
|
||||
State: open
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/951
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `7ab5eebd0` — transport types + Anthropic normalize migration
|
||||
- `731f4fbae` — transport ABC + AnthropicTransport wired to all paths
|
||||
|
||||
### Targeted tests
|
||||
- `tests/agent/transports/test_types.py`
|
||||
- `tests/agent/test_anthropic_normalize_v2.py`
|
||||
|
||||
### Tasks
|
||||
- [ ] Verify plain-text Anthropic responses normalize correctly.
|
||||
- [ ] Verify tool-call responses preserve IDs, names, and arguments.
|
||||
- [ ] Verify reasoning/thinking is preserved separately from visible content.
|
||||
- [ ] Verify finish_reason mapping remains correct across paths.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] Normalized response shape is stable.
|
||||
- [ ] Tool-call and reasoning payloads survive normalization.
|
||||
- [ ] Targeted tests pass.
|
||||
|
||||
## #952 — [QA] Verify CLI voice beep toggle
|
||||
|
||||
State: open
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/952
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `b48ea41d2` — voice: add CLI beep toggle
|
||||
|
||||
### Targeted tests
|
||||
- `tests/tools/test_voice_cli_integration.py`
|
||||
|
||||
### Tasks
|
||||
- [ ] Enable the beep option in config and confirm voice mode emits the beep.
|
||||
- [ ] Disable the option and confirm the same path is silent.
|
||||
- [ ] Verify voice mode still strips markdown before speech output.
|
||||
- [ ] Verify voice mode does not pollute conversation history with TTS-only text.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] Beep behavior is actually toggled by config.
|
||||
- [ ] Existing voice/TTS integration behavior is not regressed.
|
||||
- [ ] Targeted tests pass.
|
||||
|
||||
## #953 — [QA] Verify bundled skill scripts run out of the box
|
||||
|
||||
State: open
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/953
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `328223576` — make bundled skill scripts runnable out of the box
|
||||
|
||||
### Targeted tests
|
||||
- `tests/agent/test_skill_commands.py`
|
||||
- `tests/tools/test_local_shell_init.py`
|
||||
|
||||
### Tasks
|
||||
- [ ] Pick a bundled skill that ships a script and run it without manual chmod/PATH surgery.
|
||||
- [ ] Verify local terminal execution resolves the installed skill script correctly.
|
||||
- [ ] Verify local shell init still behaves correctly.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] Bundled skill scripts execute from the installed skill location with no manual prep.
|
||||
- [ ] Local shell init remains healthy.
|
||||
- [ ] Targeted tests pass.
|
||||
|
||||
## #954 — [QA] Verify maps skill guest_house / camp_site / bakery expansion
|
||||
|
||||
State: open
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/954
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `c5a814b23` — maps: add guest_house, camp_site, and dual-key bakery lookup
|
||||
|
||||
### Tasks
|
||||
- [ ] Use the maps skill to search for a guest house in a known populated area.
|
||||
- [ ] Use the maps skill to search for a camp site in a known populated area.
|
||||
- [ ] Use the maps skill to search for a bakery and verify both supported keys resolve correctly.
|
||||
- [ ] Confirm results are sensible and non-empty.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] All three place types resolve correctly.
|
||||
- [ ] Bakery lookup works through both supported keys.
|
||||
- [ ] Manual evidence is attached in the issue.
|
||||
|
||||
## #955 — [QA] Verify KittenTTS local provider end-to-end
|
||||
|
||||
State: open
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/955
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `1830ebfc5` — add KittenTTS provider
|
||||
- `2d7ff9c5b` — complete KittenTTS integration across tools/setup/docs/tests
|
||||
|
||||
### Targeted tests
|
||||
- `tests/tools/test_tts_kittentts.py`
|
||||
|
||||
### Tasks
|
||||
- [ ] Configure TTS to use `kittentts`.
|
||||
- [ ] Generate speech to `.wav` and verify playable output.
|
||||
- [ ] Verify voice / speed / cleaned text are passed correctly.
|
||||
- [ ] Generate repeated requests and verify model caching behavior.
|
||||
- [ ] Generate a non-wav output and verify ffmpeg conversion path.
|
||||
- [ ] Verify missing-package behavior returns a helpful error.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] KittenTTS works end-to-end when installed.
|
||||
- [ ] Failure mode is operator-friendly when not installed.
|
||||
- [ ] Targeted tests pass.
|
||||
|
||||
## #956 — [QA] Verify numbered keyboard shortcuts for approval + clarify prompts
|
||||
|
||||
State: open
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/956
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `d1ed6f4fb` — CLI: add numbered keyboard shortcuts to approval and clarify prompts
|
||||
|
||||
### Tasks
|
||||
- [ ] Trigger an approval prompt and choose an option with number keys.
|
||||
- [ ] Trigger a clarify prompt and choose an option with number keys.
|
||||
- [ ] Verify the correct option is submitted both times.
|
||||
- [ ] Verify normal keyboard navigation still works.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] Number-key selection works for both prompt types.
|
||||
- [ ] Legacy keyboard navigation is not broken.
|
||||
- [ ] Manual evidence is attached in the issue.
|
||||
|
||||
## #957 — [QA] Verify optional adversarial-ux-test skill catalog flow
|
||||
|
||||
State: open
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/957
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `e50e7f11b` — skills: add adversarial-ux-test optional skill
|
||||
|
||||
### Tasks
|
||||
- [ ] Verify the optional skill appears in the optional skill catalog.
|
||||
- [ ] Install or enable the skill.
|
||||
- [ ] Load it successfully through Hermes.
|
||||
- [ ] Disable or remove it and verify catalog state updates cleanly.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] Catalog listing is correct.
|
||||
- [ ] Install / load / disable lifecycle works cleanly.
|
||||
- [ ] Manual evidence is attached in the issue.
|
||||
|
||||
## #958 — [QA] Verify /usage account limits in CLI + gateway
|
||||
|
||||
State: open
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/958
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `8a11b0a20` — per-provider account limits module
|
||||
- `bcc5d7b67` — append account limits section in CLI and gateway
|
||||
|
||||
### Targeted tests
|
||||
- `tests/test_account_usage.py`
|
||||
- `tests/gateway/test_usage_command.py`
|
||||
|
||||
### Tasks
|
||||
- [ ] Run `/usage` in CLI for a provider with account limits.
|
||||
- [ ] Verify provider, remaining quota, total limit, and reset window render correctly.
|
||||
- [ ] Run `/usage` through the gateway and verify the same section appears.
|
||||
- [ ] Verify zero-value cache read/write sections stay hidden when appropriate.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] CLI and gateway both show the landed account-limits section correctly.
|
||||
- [ ] Targeted tests pass.
|
||||
|
||||
## #959 — [QA] Verify OpenCode-Go curated catalog additions
|
||||
|
||||
State: open
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/959
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `4fea1769d` — opencode-go: add Kimi K2.6 and Qwen3.5/3.6 Plus to curated catalog
|
||||
|
||||
### Targeted tests
|
||||
- `tests/hermes_cli/test_opencode_go_in_model_list.py`
|
||||
|
||||
### Tasks
|
||||
- [ ] With valid OpenCode-Go credentials, open `hermes model`.
|
||||
- [ ] Verify Kimi K2.6 appears.
|
||||
- [ ] Verify Qwen 3.5 Plus and 3.6 Plus appear.
|
||||
- [ ] Unset credentials and verify the provider/catalog hides correctly.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] New curated models are present when credentials exist.
|
||||
- [ ] Catalog visibility still respects credential gating.
|
||||
- [ ] Targeted tests pass.
|
||||
|
||||
## #960 — [QA] Verify patch 'did you mean?' suggestions
|
||||
|
||||
State: open
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/960
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `15abf4ed8` — add `did you mean?` feedback when patch fails to match
|
||||
- `5e6427a42` — gate it to true no-match cases and extend to v4a / skill_manage
|
||||
|
||||
### Targeted tests
|
||||
- `tests/tools/test_fuzzy_match.py`
|
||||
|
||||
### Tasks
|
||||
- [ ] Intentionally run a replace/patch with a near-miss `old_string`.
|
||||
- [ ] Verify the tool suggests a useful nearby line/context.
|
||||
- [ ] Verify suggestions only appear on true no-match failures.
|
||||
- [ ] Verify the behavior also works via file tools, v4a patching, and skill_manage.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] Suggestion quality is helpful, not noisy.
|
||||
- [ ] Suggestions are correctly gated to no-match cases.
|
||||
- [ ] Targeted tests pass.
|
||||
|
||||
## #961 — [QA] Verify web dashboard update/restart action buttons
|
||||
|
||||
State: closed
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/961
|
||||
|
||||
### Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
### Commits
|
||||
- `fc21c1420` — add buttons to update Hermes and restart gateway
|
||||
|
||||
### Files touched
|
||||
- `web/src/pages/StatusPage.tsx`
|
||||
- `web/src/lib/api.ts`
|
||||
- `web/src/i18n/en.ts`
|
||||
|
||||
### Tasks
|
||||
- [ ] Open the Web UI status page and verify both buttons are present.
|
||||
- [ ] Click Restart Gateway in a safe environment and verify running/output/success-or-failure states render.
|
||||
- [ ] Click Update Hermes and verify the same action lifecycle.
|
||||
- [ ] Verify the page remains responsive while actions are running.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] Both action buttons are present and wired.
|
||||
- [ ] Action status polling and result rendering work end-to-end.
|
||||
- [ ] Manual evidence is attached in the issue.
|
||||
|
||||
## #962 — [QA] Verify hardcoded-home path guard on burn/921 branch
|
||||
|
||||
State: closed
|
||||
URL: https://forge.alexanderwhitestone.com/Timmy_Foundation/hermes-agent/issues/962
|
||||
|
||||
### Branch / checkout
|
||||
- Validate specifically on `burn/921-poka-yoke-hardcoded-paths` (not upstream/main).
|
||||
|
||||
### Commits
|
||||
- `5dcb90531` — Poka-yoke: prevent hardcoded home-directory paths
|
||||
|
||||
### Targeted tests
|
||||
- `tests/test_path_guard.py`
|
||||
|
||||
### Tasks
|
||||
- [ ] Verify hardcoded `/Users/...` paths are rejected.
|
||||
- [ ] Verify hardcoded `~/.hermes/...` paths are rejected in guarded contexts.
|
||||
- [ ] Verify valid relative paths still pass.
|
||||
- [ ] Verify appropriate absolute paths still pass where intended.
|
||||
- [ ] Verify linting catches violations in non-test files.
|
||||
|
||||
### Acceptance criteria
|
||||
- [ ] Guard blocks the dangerous patterns and preserves allowed ones.
|
||||
- [ ] Targeted tests pass.
|
||||
@@ -1,301 +0,0 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Build a morning review packet from a Gitea epic and its child QA issues.
|
||||
|
||||
This script fetches a parent epic plus its sub-issues, extracts the structured
|
||||
sections from each QA issue body, and renders a single markdown packet suitable
|
||||
for morning review.
|
||||
|
||||
Usage:
|
||||
python scripts/morning_review_packet.py --epic-number 949
|
||||
python scripts/morning_review_packet.py --epic-number 949 --children 950-962
|
||||
python scripts/morning_review_packet.py --epic-number 949 --output docs/review_packets/hermes-harness-2026-04-21.md
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import json
|
||||
import os
|
||||
import re
|
||||
import urllib.request
|
||||
from dataclasses import dataclass, field
|
||||
from pathlib import Path
|
||||
from typing import Iterable
|
||||
|
||||
DEFAULT_BASE_URL = "https://forge.alexanderwhitestone.com"
|
||||
DEFAULT_OWNER = "Timmy_Foundation"
|
||||
DEFAULT_REPO = "hermes-agent"
|
||||
DEFAULT_TOKEN_PATH = Path.home() / ".config" / "gitea" / "token"
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class CommitEvidence:
|
||||
sha: str
|
||||
summary: str
|
||||
|
||||
|
||||
@dataclass
|
||||
class ReviewIssue:
|
||||
number: int
|
||||
title: str
|
||||
state: str
|
||||
url: str
|
||||
comments: int = 0
|
||||
parent_issue: int | None = None
|
||||
checkout_notes: list[str] = field(default_factory=list)
|
||||
commits: list[CommitEvidence] = field(default_factory=list)
|
||||
targeted_tests: list[str] = field(default_factory=list)
|
||||
files_touched: list[str] = field(default_factory=list)
|
||||
tasks: list[str] = field(default_factory=list)
|
||||
acceptance_criteria: list[str] = field(default_factory=list)
|
||||
|
||||
|
||||
def parse_issue_number_spec(spec: str) -> list[int]:
|
||||
"""Parse a comma-separated issue list like ``950-952,955,962``."""
|
||||
numbers: list[int] = []
|
||||
seen: set[int] = set()
|
||||
for chunk in (part.strip() for part in spec.split(",")):
|
||||
if not chunk:
|
||||
continue
|
||||
if "-" in chunk:
|
||||
start_str, end_str = (part.strip() for part in chunk.split("-", 1))
|
||||
start = int(start_str)
|
||||
end = int(end_str)
|
||||
if end < start:
|
||||
raise ValueError(f"Invalid descending issue range: {chunk}")
|
||||
for number in range(start, end + 1):
|
||||
if number not in seen:
|
||||
numbers.append(number)
|
||||
seen.add(number)
|
||||
else:
|
||||
number = int(chunk)
|
||||
if number not in seen:
|
||||
numbers.append(number)
|
||||
seen.add(number)
|
||||
return numbers
|
||||
|
||||
|
||||
def _parse_sections(body: str) -> dict[str, list[str]]:
|
||||
sections: dict[str, list[str]] = {}
|
||||
current: str | None = None
|
||||
for raw_line in body.splitlines():
|
||||
line = raw_line.rstrip()
|
||||
if line.startswith("## "):
|
||||
current = line[3:].strip()
|
||||
sections[current] = []
|
||||
continue
|
||||
if current is not None:
|
||||
sections[current].append(line)
|
||||
return sections
|
||||
|
||||
|
||||
def _clean_bullet(line: str) -> str | None:
|
||||
stripped = line.strip()
|
||||
if not stripped:
|
||||
return None
|
||||
stripped = re.sub(r"^-\s*\[(?: |x|X)\]\s*", "", stripped)
|
||||
stripped = re.sub(r"^-\s*", "", stripped)
|
||||
return stripped.strip() or None
|
||||
|
||||
|
||||
def _extract_bullets(lines: Iterable[str]) -> list[str]:
|
||||
items: list[str] = []
|
||||
for line in lines:
|
||||
cleaned = _clean_bullet(line)
|
||||
if cleaned:
|
||||
items.append(cleaned)
|
||||
return items
|
||||
|
||||
|
||||
def _extract_parent_issue(body: str, sections: dict[str, list[str]]) -> int | None:
|
||||
parent_lines = sections.get("Parent", [])
|
||||
for line in parent_lines:
|
||||
match = re.search(r"#(\d+)", line)
|
||||
if match:
|
||||
return int(match.group(1))
|
||||
match = re.search(r"Linked to Epic\s+#(\d+)", body, flags=re.IGNORECASE)
|
||||
if match:
|
||||
return int(match.group(1))
|
||||
return None
|
||||
|
||||
|
||||
def _extract_commits(lines: Iterable[str]) -> list[CommitEvidence]:
|
||||
commits: list[CommitEvidence] = []
|
||||
for item in _extract_bullets(lines):
|
||||
match = re.match(r"`([^`]+)`\s*(.*)", item)
|
||||
if match:
|
||||
commits.append(CommitEvidence(sha=match.group(1).strip(), summary=match.group(2).strip()))
|
||||
else:
|
||||
commits.append(CommitEvidence(sha="", summary=item))
|
||||
return commits
|
||||
|
||||
|
||||
def _strip_backticks(items: Iterable[str]) -> list[str]:
|
||||
cleaned: list[str] = []
|
||||
for item in items:
|
||||
cleaned.append(item.replace("`", "").strip())
|
||||
return cleaned
|
||||
|
||||
|
||||
def discover_child_issue_numbers(epic_body: str) -> list[int]:
|
||||
"""Discover sub-issue numbers from an epic body."""
|
||||
sections = _parse_sections(epic_body)
|
||||
sub_lines = sections.get("Sub-issues")
|
||||
if not sub_lines:
|
||||
return []
|
||||
numbers: list[int] = []
|
||||
seen: set[int] = set()
|
||||
for line in sub_lines:
|
||||
for match in re.finditer(r"#(\d+)", line):
|
||||
number = int(match.group(1))
|
||||
if number not in seen:
|
||||
numbers.append(number)
|
||||
seen.add(number)
|
||||
return numbers
|
||||
|
||||
|
||||
def parse_child_issue(issue: dict) -> ReviewIssue:
|
||||
body = issue.get("body") or ""
|
||||
sections = _parse_sections(body)
|
||||
commit_lines = sections.get("Commits landed today", []) or sections.get("Commit landed today", [])
|
||||
|
||||
return ReviewIssue(
|
||||
number=int(issue["number"]),
|
||||
title=issue.get("title") or "",
|
||||
state=(issue.get("state") or "unknown").lower(),
|
||||
url=issue.get("html_url") or issue.get("url") or "",
|
||||
comments=int(issue.get("comments") or 0),
|
||||
parent_issue=_extract_parent_issue(body, sections),
|
||||
checkout_notes=_extract_bullets(sections.get("Branch / checkout", [])),
|
||||
commits=_extract_commits(commit_lines),
|
||||
targeted_tests=_strip_backticks(_extract_bullets(sections.get("Targeted tests", []))),
|
||||
files_touched=_strip_backticks(_extract_bullets(sections.get("Files touched", []))),
|
||||
tasks=_extract_bullets(sections.get("Tasks", [])),
|
||||
acceptance_criteria=_extract_bullets(sections.get("Acceptance Criteria", [])),
|
||||
)
|
||||
|
||||
|
||||
def build_packet_markdown(epic_issue: dict, child_issues: list[ReviewIssue]) -> str:
|
||||
title = epic_issue.get("title") or f"Epic #{epic_issue.get('number')}"
|
||||
url = epic_issue.get("html_url") or epic_issue.get("url") or ""
|
||||
body = epic_issue.get("body") or ""
|
||||
children = sorted(child_issues, key=lambda item: item.number)
|
||||
|
||||
lines: list[str] = []
|
||||
lines.append("# Morning Review Packet")
|
||||
lines.append("")
|
||||
lines.append(f"Source epic: [{title}]({url})")
|
||||
lines.append("")
|
||||
lines.append("## Epic context")
|
||||
lines.append("")
|
||||
lines.append(title)
|
||||
lines.append("")
|
||||
for line in body.splitlines():
|
||||
if line.strip():
|
||||
lines.append(line)
|
||||
else:
|
||||
lines.append("")
|
||||
lines.append("")
|
||||
lines.append("## Summary")
|
||||
lines.append("")
|
||||
lines.append("| Issue | State | Commits | Tests |")
|
||||
lines.append("| --- | --- | --- | --- |")
|
||||
for child in children:
|
||||
lines.append(
|
||||
f"| #{child.number} | {child.state} | {len(child.commits)} | {len(child.targeted_tests)} |"
|
||||
)
|
||||
lines.append("")
|
||||
|
||||
for child in children:
|
||||
lines.append(f"## #{child.number} — {child.title}")
|
||||
lines.append("")
|
||||
lines.append(f"State: {child.state}")
|
||||
lines.append(f"URL: {child.url}")
|
||||
lines.append("")
|
||||
if child.checkout_notes:
|
||||
lines.append("### Branch / checkout")
|
||||
for note in child.checkout_notes:
|
||||
lines.append(f"- {note}")
|
||||
lines.append("")
|
||||
if child.commits:
|
||||
lines.append("### Commits")
|
||||
for commit in child.commits:
|
||||
if commit.sha:
|
||||
lines.append(f"- `{commit.sha}` — {commit.summary}")
|
||||
else:
|
||||
lines.append(f"- {commit.summary}")
|
||||
lines.append("")
|
||||
if child.targeted_tests:
|
||||
lines.append("### Targeted tests")
|
||||
for test_path in child.targeted_tests:
|
||||
lines.append(f"- `{test_path}`")
|
||||
lines.append("")
|
||||
if child.files_touched:
|
||||
lines.append("### Files touched")
|
||||
for file_path in child.files_touched:
|
||||
lines.append(f"- `{file_path}`")
|
||||
lines.append("")
|
||||
if child.tasks:
|
||||
lines.append("### Tasks")
|
||||
for task in child.tasks:
|
||||
lines.append(f"- [ ] {task}")
|
||||
lines.append("")
|
||||
if child.acceptance_criteria:
|
||||
lines.append("### Acceptance criteria")
|
||||
for item in child.acceptance_criteria:
|
||||
lines.append(f"- [ ] {item}")
|
||||
lines.append("")
|
||||
|
||||
return "\n".join(lines).rstrip() + "\n"
|
||||
|
||||
|
||||
def _resolve_token(explicit_token: str | None = None) -> str:
|
||||
if explicit_token:
|
||||
return explicit_token.strip()
|
||||
env_token = os.getenv("GITEA_TOKEN")
|
||||
if env_token:
|
||||
return env_token.strip()
|
||||
if DEFAULT_TOKEN_PATH.exists():
|
||||
return DEFAULT_TOKEN_PATH.read_text().strip()
|
||||
raise FileNotFoundError(f"No Gitea token found. Set GITEA_TOKEN or create {DEFAULT_TOKEN_PATH}")
|
||||
|
||||
|
||||
def fetch_issue(base_url: str, owner: str, repo: str, number: int, token: str) -> dict:
|
||||
url = f"{base_url.rstrip('/')}/api/v1/repos/{owner}/{repo}/issues/{number}"
|
||||
request = urllib.request.Request(url, headers={"Authorization": f"token {token}"})
|
||||
with urllib.request.urlopen(request, timeout=30) as response:
|
||||
return json.loads(response.read().decode())
|
||||
|
||||
|
||||
def collect_child_issues(base_url: str, owner: str, repo: str, epic_issue: dict, token: str, children_spec: str | None = None) -> list[dict]:
|
||||
numbers = parse_issue_number_spec(children_spec) if children_spec else discover_child_issue_numbers(epic_issue.get("body") or "")
|
||||
return [fetch_issue(base_url, owner, repo, number, token) for number in numbers]
|
||||
|
||||
|
||||
def main(argv: list[str] | None = None) -> int:
|
||||
parser = argparse.ArgumentParser(description="Build a markdown morning review packet from a Gitea epic")
|
||||
parser.add_argument("--base-url", default=DEFAULT_BASE_URL)
|
||||
parser.add_argument("--owner", default=DEFAULT_OWNER)
|
||||
parser.add_argument("--repo", default=DEFAULT_REPO)
|
||||
parser.add_argument("--epic-number", type=int, required=True)
|
||||
parser.add_argument("--children", help="Explicit issue list/ranges, e.g. 950-962")
|
||||
parser.add_argument("--token", help="Gitea token (defaults to GITEA_TOKEN or ~/.config/gitea/token)")
|
||||
parser.add_argument("--output", help="Write markdown packet to this path instead of stdout")
|
||||
args = parser.parse_args(argv)
|
||||
|
||||
token = _resolve_token(args.token)
|
||||
epic_issue = fetch_issue(args.base_url, args.owner, args.repo, args.epic_number, token)
|
||||
child_issue_dicts = collect_child_issues(args.base_url, args.owner, args.repo, epic_issue, token, args.children)
|
||||
packet = build_packet_markdown(epic_issue, [parse_child_issue(issue) for issue in child_issue_dicts])
|
||||
|
||||
if args.output:
|
||||
output_path = Path(args.output)
|
||||
output_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
output_path.write_text(packet)
|
||||
else:
|
||||
print(packet, end="")
|
||||
return 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
raise SystemExit(main())
|
||||
@@ -1,162 +0,0 @@
|
||||
from pathlib import Path
|
||||
import sys
|
||||
|
||||
SCRIPT_DIR = Path(__file__).resolve().parents[1] / "scripts"
|
||||
sys.path.insert(0, str(SCRIPT_DIR))
|
||||
|
||||
import morning_review_packet as mrp
|
||||
|
||||
|
||||
EPIC_BODY = """Source: git log on upstream/main since 2026-04-21 00:00 EDT.
|
||||
|
||||
## Success criteria
|
||||
- [ ] Every issue has a clear PASS / FAIL outcome.
|
||||
|
||||
## Sub-issues
|
||||
- [ ] #950 [QA] Verify AI Gateway provider UX + attribution headers
|
||||
- [ ] #951 [QA] Verify transport abstraction + AnthropicTransport wiring
|
||||
- [x] #962 [QA] Verify hardcoded-home path guard on burn/921 branch
|
||||
"""
|
||||
|
||||
|
||||
CHILD_BODY_PLURAL = """## Parent
|
||||
#949
|
||||
|
||||
## Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
## Commits landed today
|
||||
- `b11753879` attribution default_headers for ai-gateway provider
|
||||
- `700437440` curated picker with live pricing
|
||||
|
||||
## Targeted tests
|
||||
- `tests/hermes_cli/test_ai_gateway_models.py`
|
||||
- `tests/run_agent/test_provider_attribution_headers.py`
|
||||
|
||||
## Tasks
|
||||
- [ ] Verify the picker ordering.
|
||||
- [ ] Verify attribution headers.
|
||||
|
||||
## Acceptance Criteria
|
||||
- [ ] Picker shows AI Gateway prominently.
|
||||
- [ ] Headers appear on OpenRouter calls.
|
||||
"""
|
||||
|
||||
|
||||
CHILD_BODY_SINGULAR = """## Parent
|
||||
#949
|
||||
|
||||
## Branch / checkout
|
||||
- Validate on `upstream/main` or an equivalent synced checkout.
|
||||
|
||||
## Commit landed today
|
||||
- `fc21c1420` add buttons to update Hermes and restart gateway
|
||||
|
||||
## Files touched
|
||||
- `web/src/pages/StatusPage.tsx`
|
||||
- `web/src/lib/api.ts`
|
||||
- `web/src/i18n/en.ts`
|
||||
|
||||
## Tasks
|
||||
- [ ] Open the Web UI status page and verify both buttons are present.
|
||||
- [ ] Click Restart Gateway in a safe environment.
|
||||
"""
|
||||
|
||||
|
||||
def test_discover_child_issue_numbers_from_epic_body():
|
||||
assert mrp.discover_child_issue_numbers(EPIC_BODY) == [950, 951, 962]
|
||||
|
||||
|
||||
def test_parse_issue_number_spec_supports_ranges_and_lists():
|
||||
assert mrp.parse_issue_number_spec("950-952,955,962") == [950, 951, 952, 955, 962]
|
||||
|
||||
|
||||
def test_parse_child_issue_extracts_structured_sections():
|
||||
issue = {
|
||||
"number": 950,
|
||||
"title": "[QA] Verify AI Gateway provider UX + attribution headers",
|
||||
"state": "open",
|
||||
"html_url": "https://forge.example/950",
|
||||
"comments": 0,
|
||||
"body": CHILD_BODY_PLURAL,
|
||||
}
|
||||
|
||||
parsed = mrp.parse_child_issue(issue)
|
||||
|
||||
assert parsed.number == 950
|
||||
assert parsed.parent_issue == 949
|
||||
assert parsed.checkout_notes == ["Validate on `upstream/main` or an equivalent synced checkout."]
|
||||
assert [c.sha for c in parsed.commits] == ["b11753879", "700437440"]
|
||||
assert parsed.targeted_tests == [
|
||||
"tests/hermes_cli/test_ai_gateway_models.py",
|
||||
"tests/run_agent/test_provider_attribution_headers.py",
|
||||
]
|
||||
assert parsed.tasks == [
|
||||
"Verify the picker ordering.",
|
||||
"Verify attribution headers.",
|
||||
]
|
||||
assert parsed.acceptance_criteria == [
|
||||
"Picker shows AI Gateway prominently.",
|
||||
"Headers appear on OpenRouter calls.",
|
||||
]
|
||||
|
||||
|
||||
def test_parse_child_issue_handles_singular_commit_heading_and_files_touched():
|
||||
issue = {
|
||||
"number": 961,
|
||||
"title": "[QA] Verify web dashboard update/restart action buttons",
|
||||
"state": "closed",
|
||||
"html_url": "https://forge.example/961",
|
||||
"comments": 16,
|
||||
"body": CHILD_BODY_SINGULAR,
|
||||
}
|
||||
|
||||
parsed = mrp.parse_child_issue(issue)
|
||||
|
||||
assert [c.sha for c in parsed.commits] == ["fc21c1420"]
|
||||
assert parsed.files_touched == [
|
||||
"web/src/pages/StatusPage.tsx",
|
||||
"web/src/lib/api.ts",
|
||||
"web/src/i18n/en.ts",
|
||||
]
|
||||
assert parsed.tasks == [
|
||||
"Open the Web UI status page and verify both buttons are present.",
|
||||
"Click Restart Gateway in a safe environment.",
|
||||
]
|
||||
|
||||
|
||||
def test_build_packet_markdown_renders_summary_and_details():
|
||||
epic_issue = {
|
||||
"number": 949,
|
||||
"title": "EPIC: Morning review packet — Hermes harness features landed 2026-04-21",
|
||||
"state": "open",
|
||||
"html_url": "https://forge.example/949",
|
||||
"body": EPIC_BODY,
|
||||
}
|
||||
child_a = mrp.parse_child_issue({
|
||||
"number": 950,
|
||||
"title": "[QA] Verify AI Gateway provider UX + attribution headers",
|
||||
"state": "open",
|
||||
"html_url": "https://forge.example/950",
|
||||
"comments": 0,
|
||||
"body": CHILD_BODY_PLURAL,
|
||||
})
|
||||
child_b = mrp.parse_child_issue({
|
||||
"number": 961,
|
||||
"title": "[QA] Verify web dashboard update/restart action buttons",
|
||||
"state": "closed",
|
||||
"html_url": "https://forge.example/961",
|
||||
"comments": 16,
|
||||
"body": CHILD_BODY_SINGULAR,
|
||||
})
|
||||
|
||||
markdown = mrp.build_packet_markdown(epic_issue, [child_a, child_b])
|
||||
|
||||
assert "# Morning Review Packet" in markdown
|
||||
assert "EPIC: Morning review packet — Hermes harness features landed 2026-04-21" in markdown
|
||||
assert "| #950 | open | 2 | 2 |" in markdown
|
||||
assert "| #961 | closed | 1 | 0 |" in markdown
|
||||
assert "## #950 — [QA] Verify AI Gateway provider UX + attribution headers" in markdown
|
||||
assert "## #961 — [QA] Verify web dashboard update/restart action buttons" in markdown
|
||||
assert "`b11753879` — attribution default_headers for ai-gateway provider" in markdown
|
||||
assert "`web/src/pages/StatusPage.tsx`" in markdown
|
||||
@@ -148,3 +148,184 @@ class TestStrategyNameSurfaced:
|
||||
assert count == 0
|
||||
assert strategy is None
|
||||
assert err is not None
|
||||
|
||||
|
||||
class TestEscapeDriftGuard:
|
||||
"""Tests for the escape-drift guard that catches bash/JSON serialization
|
||||
artifacts where an apostrophe gets prefixed with a spurious backslash
|
||||
in tool-call transport.
|
||||
"""
|
||||
|
||||
def test_drift_blocked_apostrophe(self):
|
||||
"""File has ', old_string and new_string both have \\' — classic
|
||||
tool-call drift. Guard must block with a helpful error instead of
|
||||
writing \\' literals into source code."""
|
||||
content = "x = \"hello there\"\n"
|
||||
# Simulate transport-corrupted old_string and new_string where an
|
||||
# apostrophe-like context got prefixed with a backslash. The content
|
||||
# itself has no apostrophe, but both strings do — matching via
|
||||
# whitespace/anchor strategies would otherwise succeed.
|
||||
old_string = "x = \"hello there\" # don\\'t edit\n"
|
||||
new_string = "x = \"hi there\" # don\\'t edit\n"
|
||||
# This particular pair won't match anything, so it exits via
|
||||
# no-match path. Build a case where a non-exact strategy DOES match.
|
||||
content = "line\n x = 1\nline"
|
||||
old_string = "line\n x = \\'a\\'\nline"
|
||||
new_string = "line\n x = \\'b\\'\nline"
|
||||
new, count, strategy, err = fuzzy_find_and_replace(content, old_string, new_string)
|
||||
assert count == 0
|
||||
assert err is not None and "Escape-drift" in err
|
||||
assert "backslash" in err.lower()
|
||||
assert new == content # file untouched
|
||||
|
||||
def test_drift_blocked_double_quote(self):
|
||||
"""Same idea but with \\" drift instead of \\'."""
|
||||
content = 'line\n x = 1\nline'
|
||||
old_string = 'line\n x = \\"a\\"\nline'
|
||||
new_string = 'line\n x = \\"b\\"\nline'
|
||||
new, count, strategy, err = fuzzy_find_and_replace(content, old_string, new_string)
|
||||
assert count == 0
|
||||
assert err is not None and "Escape-drift" in err
|
||||
|
||||
def test_drift_allowed_when_file_genuinely_has_backslash_escapes(self):
|
||||
"""If the file already contains \\' (e.g. inside an existing escaped
|
||||
string), the model is legitimately preserving it. Guard must NOT
|
||||
fire."""
|
||||
content = "line\n x = \\'a\\'\nline"
|
||||
old_string = "line\n x = \\'a\\'\nline"
|
||||
new_string = "line\n x = \\'b\\'\nline"
|
||||
new, count, strategy, err = fuzzy_find_and_replace(content, old_string, new_string)
|
||||
assert err is None
|
||||
assert count == 1
|
||||
assert "\\'b\\'" in new
|
||||
|
||||
def test_drift_allowed_on_exact_match(self):
|
||||
"""Exact matches bypass the drift guard entirely — if the file
|
||||
really contains the exact bytes old_string specified, it's not
|
||||
drift."""
|
||||
content = "hello \\'world\\'"
|
||||
new, count, strategy, err = fuzzy_find_and_replace(
|
||||
content, "hello \\'world\\'", "hello \\'there\\'"
|
||||
)
|
||||
assert err is None
|
||||
assert count == 1
|
||||
assert strategy == "exact"
|
||||
|
||||
def test_drift_allowed_when_adding_escaped_strings(self):
|
||||
"""Model is adding new content with \\' that wasn't in the original.
|
||||
old_string has no \\', so guard doesn't fire."""
|
||||
content = "line1\nline2\nline3"
|
||||
old_string = "line1\nline2\nline3"
|
||||
new_string = "line1\nprint(\\'added\\')\nline2\nline3"
|
||||
new, count, strategy, err = fuzzy_find_and_replace(content, old_string, new_string)
|
||||
assert err is None
|
||||
assert count == 1
|
||||
assert "\\'added\\'" in new
|
||||
|
||||
def test_no_drift_check_when_new_string_lacks_suspect_chars(self):
|
||||
"""Fast-path: if new_string has no \\' or \\", guard must not
|
||||
fire even on fuzzy match."""
|
||||
content = "def foo():\n pass" # extra space ignored by line_trimmed
|
||||
old_string = "def foo():\n pass"
|
||||
new_string = "def bar():\n return 1"
|
||||
new, count, strategy, err = fuzzy_find_and_replace(content, old_string, new_string)
|
||||
assert err is None
|
||||
assert count == 1
|
||||
|
||||
|
||||
class TestFindClosestLines:
|
||||
def setup_method(self):
|
||||
from tools.fuzzy_match import find_closest_lines
|
||||
self.find_closest_lines = find_closest_lines
|
||||
|
||||
def test_finds_similar_line(self):
|
||||
content = "def foo():\n pass\ndef bar():\n return 1\n"
|
||||
result = self.find_closest_lines("def baz():", content)
|
||||
assert "def foo" in result or "def bar" in result
|
||||
|
||||
def test_returns_empty_for_no_match(self):
|
||||
content = "completely different content here"
|
||||
result = self.find_closest_lines("xyzzy_no_match_possible_!!!", content)
|
||||
assert result == ""
|
||||
|
||||
def test_returns_empty_for_empty_inputs(self):
|
||||
assert self.find_closest_lines("", "some content") == ""
|
||||
assert self.find_closest_lines("old string", "") == ""
|
||||
|
||||
def test_includes_context_lines(self):
|
||||
content = "line1\nline2\ndef target():\n pass\nline5\n"
|
||||
result = self.find_closest_lines("def target():", content)
|
||||
assert "target" in result
|
||||
|
||||
def test_includes_line_numbers(self):
|
||||
content = "line1\nline2\ndef foo():\n pass\n"
|
||||
result = self.find_closest_lines("def foo():", content)
|
||||
# Should include line numbers in format "N| content"
|
||||
assert "|" in result
|
||||
|
||||
|
||||
class TestFormatNoMatchHint:
|
||||
"""Gating tests for format_no_match_hint — the shared helper that decides
|
||||
whether a 'Did you mean?' snippet should be appended to an error.
|
||||
"""
|
||||
|
||||
def setup_method(self):
|
||||
from tools.fuzzy_match import format_no_match_hint
|
||||
self.fmt = format_no_match_hint
|
||||
|
||||
def test_fires_on_could_not_find_with_match(self):
|
||||
"""Classic no-match: similar content exists → hint fires."""
|
||||
content = "def foo():\n pass\ndef bar():\n pass\n"
|
||||
result = self.fmt(
|
||||
"Could not find a match for old_string in the file",
|
||||
0, "def baz():", content,
|
||||
)
|
||||
assert "Did you mean" in result
|
||||
assert "foo" in result or "bar" in result
|
||||
|
||||
def test_silent_on_ambiguous_match_error(self):
|
||||
"""'Found N matches' is not a missing-match failure — no hint."""
|
||||
content = "aaa bbb aaa\n"
|
||||
result = self.fmt(
|
||||
"Found 2 matches for old_string. Provide more context to make it unique, or use replace_all=True.",
|
||||
0, "aaa", content,
|
||||
)
|
||||
assert result == ""
|
||||
|
||||
def test_silent_on_escape_drift_error(self):
|
||||
"""Escape-drift errors are intentional blocks — hint would mislead."""
|
||||
content = "x = 1\n"
|
||||
result = self.fmt(
|
||||
"Escape-drift detected: old_string and new_string contain the literal sequence '\\\\''...",
|
||||
0, "x = \\'1\\'", content,
|
||||
)
|
||||
assert result == ""
|
||||
|
||||
def test_silent_on_identical_strings(self):
|
||||
"""old_string == new_string — hint irrelevant."""
|
||||
result = self.fmt(
|
||||
"old_string and new_string are identical",
|
||||
0, "foo", "foo bar\n",
|
||||
)
|
||||
assert result == ""
|
||||
|
||||
def test_silent_when_match_count_nonzero(self):
|
||||
"""If match succeeded, we shouldn't be in the error path — defense in depth."""
|
||||
result = self.fmt(
|
||||
"Could not find a match for old_string in the file",
|
||||
1, "foo", "foo bar\n",
|
||||
)
|
||||
assert result == ""
|
||||
|
||||
def test_silent_on_none_error(self):
|
||||
"""No error at all — no hint."""
|
||||
result = self.fmt(None, 0, "foo", "bar\n")
|
||||
assert result == ""
|
||||
|
||||
def test_silent_when_no_similar_content(self):
|
||||
"""Even for a valid no-match error, skip hint when nothing similar exists."""
|
||||
result = self.fmt(
|
||||
"Could not find a match for old_string in the file",
|
||||
0, "totally_unique_xyzzy_qux", "abc\nxyz\n",
|
||||
)
|
||||
assert result == ""
|
||||
|
||||
114
tests/tools/test_patch_did_you_mean.py
Normal file
114
tests/tools/test_patch_did_you_mean.py
Normal file
@@ -0,0 +1,114 @@
|
||||
import json
|
||||
import os
|
||||
import textwrap
|
||||
from pathlib import Path
|
||||
|
||||
import tools.skill_manager_tool as skill_manager_tool
|
||||
from tools.file_tools import patch_tool
|
||||
from tools.skill_manager_tool import _create_skill, _patch_skill
|
||||
|
||||
|
||||
def _disable_patch_tool_guards(monkeypatch):
|
||||
monkeypatch.setattr("tools.file_tools._check_sensitive_path", lambda _path: None)
|
||||
monkeypatch.setattr("tools.file_tools._check_file_staleness", lambda _path, _task_id: None)
|
||||
monkeypatch.setattr("tools.file_tools._log_and_check_conflict", lambda _path, _task_id, _action: None)
|
||||
|
||||
|
||||
def test_patch_tool_replace_no_match_shows_rich_hint_without_legacy_hint(tmp_path, monkeypatch):
|
||||
_disable_patch_tool_guards(monkeypatch)
|
||||
sample = tmp_path / "sample.py"
|
||||
sample.write_text("def foo():\n return 1\n\ndef bar():\n return 2\n", encoding="utf-8")
|
||||
|
||||
raw = patch_tool(
|
||||
mode="replace",
|
||||
path=str(sample),
|
||||
old_string="def barycentric():",
|
||||
new_string="def barycentric_new():",
|
||||
task_id="qa960-replace-rich-hint",
|
||||
)
|
||||
|
||||
result = json.loads(raw)
|
||||
assert result["success"] is False
|
||||
assert "Could not find a match" in result["error"]
|
||||
assert "Did you mean one of these sections?" in result["error"]
|
||||
assert "def bar():" in result["error"] or "def foo():" in result["error"]
|
||||
assert "[Hint:" not in raw
|
||||
|
||||
|
||||
def test_patch_tool_replace_ambiguous_error_does_not_show_did_you_mean(tmp_path, monkeypatch):
|
||||
_disable_patch_tool_guards(monkeypatch)
|
||||
sample = tmp_path / "sample.py"
|
||||
sample.write_text("aaa\nbbb\naaa\n", encoding="utf-8")
|
||||
|
||||
raw = patch_tool(
|
||||
mode="replace",
|
||||
path=str(sample),
|
||||
old_string="aaa",
|
||||
new_string="ccc",
|
||||
task_id="qa960-replace-ambiguous",
|
||||
)
|
||||
|
||||
result = json.loads(raw)
|
||||
assert result["success"] is False
|
||||
assert "Found 2 matches" in result["error"]
|
||||
assert "Did you mean one of these sections?" not in result["error"]
|
||||
assert "[Hint:" not in raw
|
||||
|
||||
|
||||
def test_patch_tool_v4a_no_match_shows_rich_hint(tmp_path, monkeypatch):
|
||||
_disable_patch_tool_guards(monkeypatch)
|
||||
sample = tmp_path / "sample.py"
|
||||
sample.write_text("def foo():\n return 1\n", encoding="utf-8")
|
||||
|
||||
patch = textwrap.dedent(
|
||||
f"""\
|
||||
*** Begin Patch
|
||||
*** Update File: {sample}
|
||||
@@
|
||||
-def barycentric():
|
||||
+def barycentric_new():
|
||||
*** End Patch
|
||||
"""
|
||||
)
|
||||
|
||||
raw = patch_tool(mode="patch", patch=patch, task_id="qa960-v4a-rich-hint")
|
||||
result = json.loads(raw)
|
||||
assert result["success"] is False
|
||||
assert "Patch validation failed" in result["error"]
|
||||
assert "Did you mean one of these sections?" in result["error"]
|
||||
assert "def foo():" in result["error"]
|
||||
|
||||
|
||||
def test_skill_patch_no_match_shows_rich_hint(tmp_path, monkeypatch):
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
skills_dir = tmp_path / "skills"
|
||||
skills_dir.mkdir(parents=True, exist_ok=True)
|
||||
monkeypatch.setattr(skill_manager_tool, "SKILLS_DIR", skills_dir)
|
||||
monkeypatch.setattr(skill_manager_tool, "_security_scan_skill", lambda _skill_dir: None)
|
||||
|
||||
_create_skill(
|
||||
"qa-skill",
|
||||
textwrap.dedent(
|
||||
"""\
|
||||
---
|
||||
name: qa-skill
|
||||
description: test
|
||||
---
|
||||
|
||||
Step 1: Do the thing.
|
||||
Step 2: Verify the thing.
|
||||
"""
|
||||
),
|
||||
)
|
||||
|
||||
result = _patch_skill(
|
||||
"qa-skill",
|
||||
"Step 1: Do the production rollout.",
|
||||
"Step 1: Updated.",
|
||||
)
|
||||
|
||||
assert result["success"] is False
|
||||
assert "Could not find a match" in result["error"]
|
||||
assert "Did you mean one of these sections?" in result["error"]
|
||||
assert "Step 1: Do the thing." in result["error"]
|
||||
assert "file_preview" in result
|
||||
@@ -757,12 +757,14 @@ class ShellFileOperations(FileOperations):
|
||||
content, old_string, new_string, replace_all
|
||||
)
|
||||
|
||||
if error:
|
||||
return PatchResult(error=error)
|
||||
|
||||
if match_count == 0:
|
||||
return PatchResult(error=f"Could not find match for old_string in {path}")
|
||||
|
||||
if error or match_count == 0:
|
||||
err_msg = error or f"Could not find match for old_string in {path}"
|
||||
try:
|
||||
from tools.fuzzy_match import format_no_match_hint
|
||||
err_msg += format_no_match_hint(err_msg, match_count, old_string, content)
|
||||
except Exception:
|
||||
pass
|
||||
return PatchResult(error=err_msg)
|
||||
# Write back
|
||||
write_result = self.write_file(path, new_content)
|
||||
if write_result.error:
|
||||
|
||||
@@ -8,6 +8,7 @@ import os
|
||||
import threading
|
||||
import time
|
||||
from pathlib import Path
|
||||
from typing import Any, Dict, Optional
|
||||
from tools.binary_extensions import has_binary_extension
|
||||
from tools.file_operations import ShellFileOperations
|
||||
from agent.redact import redact_sensitive_text
|
||||
@@ -690,8 +691,11 @@ def patch_tool(mode: str = "replace", path: str = None, old_string: str = None,
|
||||
result_json = json.dumps(result_dict, ensure_ascii=False)
|
||||
# Hint when old_string not found — saves iterations where the agent
|
||||
# retries with stale content instead of re-reading the file.
|
||||
# Suppressed when patch_replace already attached a rich "Did you mean?"
|
||||
# snippet (which is strictly more useful than the generic hint).
|
||||
if result_dict.get("error") and "Could not find" in str(result_dict["error"]):
|
||||
result_json += "\n\n[Hint: old_string not found. Use read_file to verify the current content, or search_files to locate the text.]"
|
||||
if "Did you mean one of these sections?" not in str(result_dict["error"]):
|
||||
result_json += "\n\n[Hint: old_string not found. Use read_file to verify the current content, or search_files to locate the text.]"
|
||||
return result_json
|
||||
except Exception as e:
|
||||
return tool_error(str(e))
|
||||
|
||||
@@ -93,6 +93,21 @@ def fuzzy_find_and_replace(content: str, old_string: str, new_string: str,
|
||||
f"Provide more context to make it unique, or use replace_all=True."
|
||||
)
|
||||
|
||||
# Escape-drift guard: when the matched strategy is NOT `exact`,
|
||||
# we matched via some form of normalization. If new_string
|
||||
# contains shell/JSON-style escape sequences (\\' or \\") that
|
||||
# would be written literally into the file but the matched
|
||||
# region of the file has no such sequences, this is almost
|
||||
# certainly tool-call serialization drift — the model typed
|
||||
# an apostrophe/quote and the transport added a stray
|
||||
# backslash. Writing new_string as-is would corrupt the file.
|
||||
# Block with a helpful error so the model re-reads and retries
|
||||
# instead of the caller silently persisting garbage (or not).
|
||||
if strategy_name != "exact":
|
||||
drift_err = _detect_escape_drift(content, matches, old_string, new_string)
|
||||
if drift_err:
|
||||
return content, 0, None, drift_err
|
||||
|
||||
# Perform replacement
|
||||
new_content = _apply_replacements(content, matches, new_string)
|
||||
return new_content, len(matches), strategy_name, None
|
||||
@@ -101,6 +116,46 @@ def fuzzy_find_and_replace(content: str, old_string: str, new_string: str,
|
||||
return content, 0, None, "Could not find a match for old_string in the file"
|
||||
|
||||
|
||||
def _detect_escape_drift(content: str, matches: List[Tuple[int, int]],
|
||||
old_string: str, new_string: str) -> Optional[str]:
|
||||
"""Detect tool-call escape-drift artifacts in new_string.
|
||||
|
||||
Looks for ``\\'`` or ``\\"`` sequences that are present in both
|
||||
old_string and new_string (i.e. the model copy-pasted them as "context"
|
||||
it intended to preserve) but don't exist in the matched region of the
|
||||
file. That pattern indicates the transport layer inserted spurious
|
||||
shell-style escapes around apostrophes or quotes — writing new_string
|
||||
verbatim would literally insert ``\\'`` into source code.
|
||||
|
||||
Returns an error string if drift is detected, None otherwise.
|
||||
"""
|
||||
# Cheap pre-check: bail out unless new_string actually contains a
|
||||
# suspect escape sequence. This keeps the guard free for all the
|
||||
# common, correct cases.
|
||||
if "\\'" not in new_string and '\\"' not in new_string:
|
||||
return None
|
||||
|
||||
# Aggregate matched regions of the file — that's what new_string will
|
||||
# replace. If the suspect escapes are present there already, the
|
||||
# model is genuinely preserving them (valid for some languages /
|
||||
# escaped strings); accept the patch.
|
||||
matched_regions = "".join(content[start:end] for start, end in matches)
|
||||
|
||||
for suspect in ("\\'", '\\"'):
|
||||
if suspect in new_string and suspect in old_string and suspect not in matched_regions:
|
||||
plain = suspect[1] # "'" or '"'
|
||||
return (
|
||||
f"Escape-drift detected: old_string and new_string contain "
|
||||
f"the literal sequence {suspect!r} but the matched region of "
|
||||
f"the file does not. This is almost always a tool-call "
|
||||
f"serialization artifact where an apostrophe or quote got "
|
||||
f"prefixed with a spurious backslash. Re-read the file with "
|
||||
f"read_file and pass old_string/new_string without "
|
||||
f"backslash-escaping {plain!r} characters."
|
||||
)
|
||||
return None
|
||||
|
||||
|
||||
def _apply_replacements(content: str, matches: List[Tuple[int, int]], new_string: str) -> str:
|
||||
"""
|
||||
Apply replacements at the given positions.
|
||||
@@ -564,3 +619,86 @@ def _map_normalized_positions(original: str, normalized: str,
|
||||
original_matches.append((orig_start, min(orig_end, len(original))))
|
||||
|
||||
return original_matches
|
||||
|
||||
|
||||
def find_closest_lines(old_string: str, content: str, context_lines: int = 2, max_results: int = 3) -> str:
|
||||
"""Find lines in content most similar to old_string for "did you mean?" feedback.
|
||||
|
||||
Returns a formatted string showing the closest matching lines with context,
|
||||
or empty string if no useful match is found.
|
||||
"""
|
||||
if not old_string or not content:
|
||||
return ""
|
||||
|
||||
old_lines = old_string.splitlines()
|
||||
content_lines = content.splitlines()
|
||||
|
||||
if not old_lines or not content_lines:
|
||||
return ""
|
||||
|
||||
# Use first line of old_string as anchor for search
|
||||
anchor = old_lines[0].strip()
|
||||
if not anchor:
|
||||
# Try second line if first is blank
|
||||
candidates = [l.strip() for l in old_lines if l.strip()]
|
||||
if not candidates:
|
||||
return ""
|
||||
anchor = candidates[0]
|
||||
|
||||
# Score each line in content by similarity to anchor
|
||||
scored = []
|
||||
for i, line in enumerate(content_lines):
|
||||
stripped = line.strip()
|
||||
if not stripped:
|
||||
continue
|
||||
ratio = SequenceMatcher(None, anchor, stripped).ratio()
|
||||
if ratio > 0.3:
|
||||
scored.append((ratio, i))
|
||||
|
||||
if not scored:
|
||||
return ""
|
||||
|
||||
# Take top matches
|
||||
scored.sort(key=lambda x: -x[0])
|
||||
top = scored[:max_results]
|
||||
|
||||
parts = []
|
||||
seen_ranges = set()
|
||||
for _, line_idx in top:
|
||||
start = max(0, line_idx - context_lines)
|
||||
end = min(len(content_lines), line_idx + len(old_lines) + context_lines)
|
||||
key = (start, end)
|
||||
if key in seen_ranges:
|
||||
continue
|
||||
seen_ranges.add(key)
|
||||
snippet = "\n".join(
|
||||
f"{start + j + 1:4d}| {content_lines[start + j]}"
|
||||
for j in range(end - start)
|
||||
)
|
||||
parts.append(snippet)
|
||||
|
||||
if not parts:
|
||||
return ""
|
||||
|
||||
return "\n---\n".join(parts)
|
||||
|
||||
|
||||
def format_no_match_hint(error: Optional[str], match_count: int,
|
||||
old_string: str, content: str) -> str:
|
||||
"""Return a '\\n\\nDid you mean...' snippet for plain no-match errors.
|
||||
|
||||
Gated so the hint only fires for actual "old_string not found" failures.
|
||||
Ambiguous-match ("Found N matches"), escape-drift, and identical-strings
|
||||
errors all have ``match_count == 0`` but a "did you mean?" snippet would
|
||||
be misleading — those failed for unrelated reasons.
|
||||
|
||||
Returns an empty string when there's nothing useful to append.
|
||||
"""
|
||||
if match_count != 0:
|
||||
return ""
|
||||
if not error or not error.startswith("Could not find"):
|
||||
return ""
|
||||
hint = find_closest_lines(old_string, content)
|
||||
if not hint:
|
||||
return ""
|
||||
return "\n\nDid you mean one of these sections?\n" + hint
|
||||
|
||||
@@ -290,10 +290,16 @@ def _validate_operations(
|
||||
)
|
||||
if count == 0:
|
||||
label = f"'{hunk.context_hint}'" if hunk.context_hint else "(no hint)"
|
||||
errors.append(
|
||||
msg = (
|
||||
f"{op.file_path}: hunk {label} not found"
|
||||
+ (f" — {match_error}" if match_error else "")
|
||||
)
|
||||
try:
|
||||
from tools.fuzzy_match import format_no_match_hint
|
||||
msg += format_no_match_hint(match_error, count, search_pattern, simulated)
|
||||
except Exception:
|
||||
pass
|
||||
errors.append(msg)
|
||||
else:
|
||||
# Advance simulation so subsequent hunks validate correctly.
|
||||
# Reuse the result from the call above — no second fuzzy run.
|
||||
@@ -537,7 +543,13 @@ def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]:
|
||||
error = None
|
||||
|
||||
if error:
|
||||
return False, f"Could not apply hunk: {error}"
|
||||
err_msg = f"Could not apply hunk: {error}"
|
||||
try:
|
||||
from tools.fuzzy_match import format_no_match_hint
|
||||
err_msg += format_no_match_hint(error, 0, search_pattern, new_content)
|
||||
except Exception:
|
||||
pass
|
||||
return False, err_msg
|
||||
else:
|
||||
# Addition-only hunk (no context or removed lines).
|
||||
# Insert at the location indicated by the context hint, or at end of file.
|
||||
|
||||
@@ -575,9 +575,15 @@ def _patch_skill(
|
||||
if match_error:
|
||||
# Show a short preview of the file so the model can self-correct
|
||||
preview = content[:500] + ("..." if len(content) > 500 else "")
|
||||
err_msg = match_error
|
||||
try:
|
||||
from tools.fuzzy_match import format_no_match_hint
|
||||
err_msg += format_no_match_hint(match_error, match_count, old_string, content)
|
||||
except Exception:
|
||||
pass
|
||||
return {
|
||||
"success": False,
|
||||
"error": match_error,
|
||||
"error": err_msg,
|
||||
"file_preview": preview,
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user