fix(bark): refine compact briefing template

[IceCodeNew]

Summary

• render Bark briefing items as plain text without Markdown list markers
• merge Bark source IDs that share the same displayed source name
• show each distinct Bark source on its own line without a source-label prefix
• require plain-text LLM briefing fields before publisher-specific rendering

Verification

• prek run --all-files
• mise exec -- uv run --with pytest --with pytest-cov -- pytest --cov --cov-branch --cov-report=xml (1015 passed)
• CodeRabbit CLI review against master (0 issues)

Summary by CodeRabbit

• New Features

  • Source citations now appear as inline numbered references with a separate numbered source list.
  • Duplicate sources with matching names are consolidated into a single citation.

• Improvements

  • Weather briefings use cleaner plain-text formatting.
  • Warnings, conclusions, disaster updates, and advice no longer include redundant Markdown-style bullet prefixes.
  • Generated briefing content is restricted to plain text (no Markdown headings, lists, emphasis, code, or links).

• Bug Fixes

  • Improved newline handling when splitting long plain-text messages for display.

[coderabbitai]

Warning

Review limit reached

@IceCodeNew, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 48 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a817bc27-f12b-461c-b365-624250a8c8a8

📥 Commits

Reviewing files that changed from the base of the PR and between 669f01c and 8dcce6f.

📒 Files selected for processing (2)

• tests/test_bark_publisher.py
• weather_briefing/delivery/bark.py

📝 Walkthrough

Walkthrough

The system prompt now requires selected briefing fields to use plain text without Markdown. Bark rendering uses normalized numbered source attributions, removes embedded bullet prefixes, and updates newline-aware message chunking. Tests cover the revised prompt, rendering, attribution, and splitting behavior.

Changes

Bark plain-text output

Layer / File(s)	Summary
Plain-text output contract weather_briefing/data/system_prompt.txt, tests/test_prompts.py	Prompt constraints and tests require relevant briefing fields to avoid Markdown formatting.
Bark rendering and attribution weather_briefing/delivery/renderers.py, tests/test_render.py	Bark output removes hyphen bullets, uses normalized and merged numbered source references, and renders a numbered source name list.
Bark message chunking weather_briefing/delivery/bark.py, tests/test_bark_publisher.py	Message splitting consumes boundary newlines while preserving newlines that do not align with the split boundary.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• IceCodeNew/weather-briefing#31: Updates the related prompt attribution and conflict expectations.
• IceCodeNew/weather-briefing#108: Introduces the Bark publishing paths modified here.
• IceCodeNew/weather-briefing#110: Modifies related Bark rendering and message-splitting paths.

Suggested labels: 🕐 40+ Minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the PR’s main focus on refining the compact Bark briefing template.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/refine-bark-briefing-template

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[IceCodeNew]

/agentic_review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.83%. Comparing base (067c80c) to head (8dcce6f).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #111   +/-   ##
=======================================
  Coverage   99.83%   99.83%           
=======================================
  Files          93       93           
  Lines       10344    10382   +38     
  Branches      612      615    +3     
=======================================
+ Hits        10327    10365   +38     
  Misses         12       12           
  Partials        5        5           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 46 rules

1. Ambiguous chunk reconstruction ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New

Description

split_plain_message() drops the delimiter newline when splitting at a line boundary, so callers
cannot reliably reconstruct the original body from the returned chunks via simple concatenation. The
tests now use "\n".join(chunks) in one case but "".join(chunks) in another, leaving the intended
contract unclear and easy to misuse.

Code

weather_briefing/delivery/bark.py[R187-190]

+        newline_at = remaining.rfind("\n", earliest_split, limit + 1)
+        if newline_at >= earliest_split:
+            chunks.append(remaining[:newline_at])
+            remaining = remaining[newline_at + 1 :]

Relevance

⭐⭐⭐ High

Team has accepted chunking/splitting correctness fixes; clarifying recomposition semantics prevents
misuse and bugs.

PR-#95
PR-#110

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The splitter explicitly skips the newline on line-boundary splits, so the newline is not present in
any returned chunk; this changes how (or whether) chunks can be recombined. The updated tests
validate newline-free chunks and reconstruct with "\n".join(...) in one test, while another test
still reconstructs via "".join(...), demonstrating inconsistent expectations around recomposition
semantics.

weather_briefing/delivery/bark.py[176-195]
tests/test_bark_publisher.py[260-274]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`split_plain_message()` now removes the `"\n"` character at a chosen line-boundary split (`remaining = remaining[newline_at + 1:]`). This makes the function’s output chunks ambiguous to recombine: depending on whether a split occurred at a newline, neither `"".join(chunks)` nor `"\n".join(chunks)` is universally correct.

### Issue Context
The updated unit tests demonstrate two different reconstruction expectations (`"\n".join(...)` vs `"".join(...)`), which risks future callers/tests using the wrong recomposition and silently altering content.

### Fix Focus Areas
- weather_briefing/delivery/bark.py[176-195]
- tests/test_bark_publisher.py[260-274]

### What to change
Pick one explicit contract and enforce it consistently:
1) **Lossless split contract (recommended for generic split helpers):** ensure all characters (including the split newline) are preserved in some chunk, and update tests to assert `"".join(chunks) == body` for all inputs.

OR

2) **Delimiter-based contract:** keep the current behavior but **document it in the docstring** (newline-as-boundary, newline removed), and update tests to reconstruct in a single, clearly-defined way (e.g., add a small test helper that recombines chunks per the chosen rule and use it consistently).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Leading newline Bark chunks ✓ Resolved 🐞 Bug ☼ Reliability

Description

Bark’s new source footer is newline-separated, which increases the odds that split_plain_message()
splits exactly at a '\n' and leaves that newline as the first character of the next chunk. This can
cause a Bark notification chunk to start with a blank line and wastes payload budget (occasionally
forcing an extra chunk near the limit).

Code

weather_briefing/delivery/renderers.py[R340-356]

+def _bark_numbered_source_references(
+    result: BriefingResult,
+    source_references: dict[str, str],
+) -> tuple[dict[str, str], str]:
+    numbered_references: dict[str, str] = {}
+    numbers_by_name: dict[str, str] = {}
+    source_lines: list[str] = []
+    for source_id in _ordered_source_ids(result):
+        source_name = " ".join(source_references[source_id].split()) or source_id
+        normalized_name = source_name.casefold()
+        number = numbers_by_name.get(normalized_name)
+        if number is None:
+            number = f"[{len(numbers_by_name) + 1}]"
+            numbers_by_name[normalized_name] = number
+            source_lines.append(f"{number} {source_name}")
+        numbered_references[source_id] = number
+    return numbered_references, "\n".join(source_lines)

Relevance

⭐⭐⭐ High

Team previously accepted Bark newline/whitespace trimming to avoid chunking/payload issues; this is
same reliability class.

PR-#110

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The Bark renderer now produces a multi-line source footer by joining lines with newlines, increasing
message newline density. The Bark splitter slices at the newline index but keeps that newline in
remaining, so the next emitted chunk can begin with \n.

weather_briefing/delivery/renderers.py[340-356]
weather_briefing/delivery/bark.py[176-193]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`split_plain_message()` splits on a newline boundary but keeps the delimiter at the start of the next chunk (`remaining = remaining[split_at:]`). With Bark’s briefing now emitting more newline boundaries (each source on its own line), long briefings are more likely to produce chunks that begin with `\n`, resulting in notifications starting with a blank line and wasting character budget.

## Issue Context
- Bark briefings now end with a multi-line footer via `"\n".join(source_lines)`.
- Bark chunking tries to split at newlines when possible.

## Fix Focus Areas
- weather_briefing/delivery/bark.py[176-193]
- weather_briefing/delivery/renderers.py[340-356]

## Suggested fix approach
- When a newline split is chosen, split *after* the newline when possible (so the next chunk doesn’t start with `\n`).
 - Example: search for `\n` in `remaining` up to `limit` (or `limit-1`), and set `split_at = newline_index + 1` when `newline_index >= 0`.
 - Fall back to `split_at = limit` when no newline is found.
- Add a regression test that constructs a long multi-line body where the chosen split point is a newline and assert no returned chunk starts with `\n`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Markdown bypasses remain ✓ Resolved 🐞 Bug ≡ Correctness

Description

PlainTextString rejects only a limited set of Markdown regex patterns, so common Markdown
constructs (e.g., reference-style links like [Forecast]: https://… or Setext headings like
Forecast\n===) can still pass validation and reach renderers as "plain text". This undermines the
new "must not contain Markdown syntax" contract for user-facing fields.

Code

weather_briefing/llm/schema.py[R13-35]

+_MARKDOWN_PATTERNS = (
+    re.compile(r"(?m)^\s{0,3}(?:#{1,6}|[-*+>]|\d+[.)])\s+"),
+    re.compile(r"(?m)^\s*(?:-{3,}|\*{3,}|_{3,})\s*$"),
+    re.compile(r"```|~~~"),
+    re.compile(r"\[[^\]\n]+\]\([^)\n]+\)"),
+    re.compile(r"(?P<delimiter>\*\*|__)(?=\S).+?(?<=\S)(?P=delimiter)"),
+    re.compile(r"(?<!\*)\*(?!\*)(?=\S)[^*\n]+?(?<=\S)\*(?!\*)"),
+    re.compile(r"(?<![\w_])_(?!_)(?=\S)[^_\n]+?(?<=\S)_(?![\w_])"),
+    re.compile(r"~~(?=\S).+?(?<=\S)~~"),
+    re.compile(r"`[^`\n]+`"),
+)
+

def _non_empty(value: str) -> str:
    if not value.strip():
        raise ValueError("must not be empty")
    return value


+def _plain_text(value: str) -> str:
+    if any(pattern.search(value) for pattern in _MARKDOWN_PATTERNS):
+        raise ValueError("must not contain Markdown syntax")
+    return value

Relevance

⭐⭐⭐ High

Matches PR intent to enforce plain-text contract; likely to add patterns/tests for remaining
Markdown bypasses.

PR-#110

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The schema claims to reject Markdown via _plain_text, but the pattern list is limited to a few
Markdown forms; reference-link definitions and Setext headings are not among the implemented
patterns, so they will not be rejected by any(pattern.search(value) ...).

weather_briefing/llm/schema.py[13-35]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PlainTextString` is intended to enforce “no Markdown” in user-facing LLM fields, but the current `_MARKDOWN_PATTERNS` only catches a subset of Markdown. As a result, some Markdown-like constructs can slip through schema validation and be rendered verbatim downstream.

## Issue Context
The validator currently checks headings (ATX), lists, blockquotes, horizontal rules, fenced code markers, inline links, emphasis, strikethrough, and inline code. It does **not** detect other common Markdown forms such as:
- reference-style link definitions: `(?m)^\s*\[[^\]]+\]:\s+\S+`
- Setext headings: `Title\n===` / `Title\n---`
- (Optional) autolinks like `<https://example.com>` if those should be disallowed

## Fix Focus Areas
- weather_briefing/llm/schema.py[13-35]

## Suggested implementation notes
- Add additional regex patterns for reference links and Setext headings.
- Add unit tests mirroring the new parametrized tests to assert these are rejected for `headline` and `*.text`/warning fields.
- Keep patterns conservative to avoid false positives for normal punctuation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

4. Blank source names merge ✓ Resolved 🐞 Bug ≡ Correctness

Description

_bark_numbered_source_references() deduplicates Bark sources by normalized display name but does not
handle empty/whitespace-only names, causing distinct sources to collapse to one number and emitting
malformed footer lines (e.g., "[1] "). This can produce ambiguous/incorrect attributions and can
introduce trailing spaces that inflate Bark message length calculations.

Code

weather_briefing/delivery/renderers.py[R348-355]

+        source_name = " ".join(source_references[source_id].split())
+        normalized_name = source_name.casefold()
+        number = numbers_by_name.get(normalized_name)
+        if number is None:
+            number = f"[{len(numbers_by_name) + 1}]"
+            numbers_by_name[normalized_name] = number
+            source_lines.append(f"{number} {source_name}")
+        numbered_references[source_id] = number

Relevance

⭐⭐⭐ High

They’ve accepted Bark whitespace/length hardening and edge-case guards; empty-name handling fits
same reliability pattern.

PR-#110
PR-#95

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The Bark numbering helper uses a normalized display-name key (casefold() of whitespace-compacted
source name) to merge IDs, but does not guard against the empty-string case; and SourceDocument
names are not enforced to be non-empty, making this state possible.

weather_briefing/delivery/renderers.py[340-356]
weather_briefing/delivery/renderers.py[206-238]
weather_briefing/models.py[101-117]
PR-#110

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`_bark_numbered_source_references()` collapses sources by display name, but if a `SourceDocument.name` is blank/whitespace, the normalized name becomes empty and multiple distinct sources merge into the same citation number; the footer line also becomes `"[n] "`.

### Issue Context
`SourceDocument.name` is not validated as non-empty, and Bark currently builds `source_references` directly from `document.name` (URLs omitted). This makes the blank-name case representable and user-visible.

### Fix Focus Areas
- weather_briefing/delivery/renderers.py[340-356]
- weather_briefing/delivery/renderers.py[206-238]
- weather_briefing/models.py[101-117]

### Suggested fix
- In `_bark_numbered_source_references()`, after whitespace compaction, add a fallback like:
 - `display_name = source_name or source_id` (or another stable placeholder that does not collapse distinct sources).
 - Use `display_name` for both `normalized_name` and the `source_lines.append()` formatting.
- Optionally add a targeted test for a blank/whitespace `SourceDocument.name` to ensure citations do not merge and the footer line has no dangling space.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Unbounded regex scan cost ✓ Resolved 🐞 Bug ➹ Performance

Description

The new _plain_text validator runs multiple regex searches over each user-facing string field
without any explicit max-length bound, so unusually large LLM outputs will incur repeated full-text
scans during schema validation. This is avoidable overhead and can amplify cost when validation
failures trigger retries/repairs.

Code

weather_briefing/llm/schema.py[R32-35]

+def _plain_text(value: str) -> str:
+    if any(pattern.search(value) for pattern in _MARKDOWN_PATTERNS):
+        raise ValueError("must not contain Markdown syntax")
+    return value

Relevance

⭐⭐ Medium

Perf concern is plausible but speculative; team sometimes adds guards for pathological inputs, but
may rely on token limits.

PR-#95
PR-#110

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

_plain_text scans text with multiple regex patterns, and the structured output model does not
impose max-length constraints on these strings; parse_result validates the raw payload directly,
so the scan cost grows with input size.

weather_briefing/llm/schema.py[32-35]
weather_briefing/llm/schema.py[72-82]
weather_briefing/llm/result.py[16-24]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_plain_text` performs multiple regex searches on every validated string, but there are no explicit per-field length limits in the Pydantic schema. Very large strings will cause repeated scanning work during validation.

## Issue Context
Even if LLMs are *expected* to be concise, adding a hard ceiling for user-facing fields provides a deterministic bound on validation work and prevents pathological payloads from consuming unnecessary CPU.

## Fix Focus Areas
- weather_briefing/llm/schema.py[32-35]
- weather_briefing/llm/schema.py[72-82]

## Suggested implementation notes
- Introduce `MAX_PLAINTEXT_CHARS` (or similar) and apply via `Annotated[str, Field(max_length=...)]` (or by adding a length-check `AfterValidator`) to `headline`, `SourcedTextPayload.text`, and warning `title/status/detail`.
- Add/adjust tests to confirm overly long values fail validation with a clear error location.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Plain-text rule unenforced ✓ Resolved 🐞 Bug ☼ Reliability

Description

The system prompt now requires several LLM text fields to be plain text (no Markdown), but the
structured output schema only validates non-empty strings and does not reject Markdown-like syntax.
If the model violates the prompt, renderers (including Bark) will pass the Markdown through
verbatim, defeating the new formatting guarantees.

Code

weather_briefing/data/system_prompt.txt[R19-20]

+headline、conclusions[].text、active_warnings 中的 title、status、detail、disaster_tracking[].text
+以及 advice[].text 只能包含纯文本，不得使用 Markdown 标题、列表、强调、代码或链接语法；章节标题和项目符号由发布端统一渲染。

Relevance

⭐⭐ Medium

Team hardens LLM contract boundaries, but no precedent for schema-level Markdown rejection; may stay
prompt-only.

PR-#101
PR-#102

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The prompt explicitly adds a no-Markdown requirement, but the schema only enforces that text fields
are non-empty; renderers then use these text values directly, so any Markdown that slips past the
prompt will be emitted.

weather_briefing/data/system_prompt.txt[17-21]
weather_briefing/llm/schema.py[13-63]
weather_briefing/delivery/renderers.py[288-300]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The prompt now declares a strict contract: `headline`, `conclusions[].text`, warning fields, `disaster_tracking[].text`, and `advice[].text` must be plain text and must not use Markdown. However, schema validation currently accepts any non-empty string, so Markdown can still enter the system when the LLM misbehaves.

### Issue Context
Renderers output `item.text` verbatim in multiple places (including Bark’s compact renderer). The new contract is therefore not guaranteed at runtime.

### Fix Focus Areas
- weather_briefing/data/system_prompt.txt[17-21]
- weather_briefing/llm/schema.py[13-63]
- weather_briefing/delivery/renderers.py[288-300]

### Suggested fix
- Introduce a stricter `PlainTextString` pydantic type (e.g., `AfterValidator`) for the fields covered by the prompt rule.
- Keep the detector narrowly scoped to high-signal Markdown constructs (e.g., fenced code blocks ```; Markdown links `[]()`; line-start list markers like `- ` / `* ` / `1. `; ATX headings `# `) to reduce false positives.
- On violation, raise an actionable `LLMError` so the retry loop can request a corrected output.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 8dcce6f

Results up to commit b0056e7 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Blank source names merge ✓ Resolved 🐞 Bug ≡ Correctness

Description

_bark_numbered_source_references() deduplicates Bark sources by normalized display name but does not
handle empty/whitespace-only names, causing distinct sources to collapse to one number and emitting
malformed footer lines (e.g., "[1] "). This can produce ambiguous/incorrect attributions and can
introduce trailing spaces that inflate Bark message length calculations.

Code

weather_briefing/delivery/renderers.py[R348-355]

+        source_name = " ".join(source_references[source_id].split())
+        normalized_name = source_name.casefold()
+        number = numbers_by_name.get(normalized_name)
+        if number is None:
+            number = f"[{len(numbers_by_name) + 1}]"
+            numbers_by_name[normalized_name] = number
+            source_lines.append(f"{number} {source_name}")
+        numbered_references[source_id] = number

Relevance

⭐⭐⭐ High

They’ve accepted Bark whitespace/length hardening and edge-case guards; empty-name handling fits
same reliability pattern.

PR-#110
PR-#95

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The Bark numbering helper uses a normalized display-name key (casefold() of whitespace-compacted
source name) to merge IDs, but does not guard against the empty-string case; and SourceDocument
names are not enforced to be non-empty, making this state possible.

weather_briefing/delivery/renderers.py[340-356]
weather_briefing/delivery/renderers.py[206-238]
weather_briefing/models.py[101-117]
PR-#110

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`_bark_numbered_source_references()` collapses sources by display name, but if a `SourceDocument.name` is blank/whitespace, the normalized name becomes empty and multiple distinct sources merge into the same citation number; the footer line also becomes `"[n] "`.

### Issue Context
`SourceDocument.name` is not validated as non-empty, and Bark currently builds `source_references` directly from `document.name` (URLs omitted). This makes the blank-name case representable and user-visible.

### Fix Focus Areas
- weather_briefing/delivery/renderers.py[340-356]
- weather_briefing/delivery/renderers.py[206-238]
- weather_briefing/models.py[101-117]

### Suggested fix
- In `_bark_numbered_source_references()`, after whitespace compaction, add a fallback like:
 - `display_name = source_name or source_id` (or another stable placeholder that does not collapse distinct sources).
 - Use `display_name` for both `normalized_name` and the `source_lines.append()` formatting.
- Optionally add a targeted test for a blank/whitespace `SourceDocument.name` to ensure citations do not merge and the footer line has no dangling space.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Plain-text rule unenforced ✓ Resolved 🐞 Bug ☼ Reliability

Description

The system prompt now requires several LLM text fields to be plain text (no Markdown), but the
structured output schema only validates non-empty strings and does not reject Markdown-like syntax.
If the model violates the prompt, renderers (including Bark) will pass the Markdown through
verbatim, defeating the new formatting guarantees.

Code

weather_briefing/data/system_prompt.txt[R19-20]

+headline、conclusions[].text、active_warnings 中的 title、status、detail、disaster_tracking[].text
+以及 advice[].text 只能包含纯文本，不得使用 Markdown 标题、列表、强调、代码或链接语法；章节标题和项目符号由发布端统一渲染。

Relevance

⭐⭐ Medium

Team hardens LLM contract boundaries, but no precedent for schema-level Markdown rejection; may stay
prompt-only.

PR-#101
PR-#102

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The prompt explicitly adds a no-Markdown requirement, but the schema only enforces that text fields
are non-empty; renderers then use these text values directly, so any Markdown that slips past the
prompt will be emitted.

weather_briefing/data/system_prompt.txt[17-21]
weather_briefing/llm/schema.py[13-63]
weather_briefing/delivery/renderers.py[288-300]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The prompt now declares a strict contract: `headline`, `conclusions[].text`, warning fields, `disaster_tracking[].text`, and `advice[].text` must be plain text and must not use Markdown. However, schema validation currently accepts any non-empty string, so Markdown can still enter the system when the LLM misbehaves.

### Issue Context
Renderers output `item.text` verbatim in multiple places (including Bark’s compact renderer). The new contract is therefore not guaranteed at runtime.

### Fix Focus Areas
- weather_briefing/data/system_prompt.txt[17-21]
- weather_briefing/llm/schema.py[13-63]
- weather_briefing/delivery/renderers.py[288-300]

### Suggested fix
- Introduce a stricter `PlainTextString` pydantic type (e.g., `AfterValidator`) for the fields covered by the prompt rule.
- Keep the detector narrowly scoped to high-signal Markdown constructs (e.g., fenced code blocks ```; Markdown links `[]()`; line-start list markers like `- ` / `* ` / `1. `; ATX headings `# `) to reduce false positives.
- On violation, raise an actionable `LLMError` so the retry loop can request a corrected output.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit c3edd8d ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Markdown bypasses remain ✓ Resolved 🐞 Bug ≡ Correctness

Description

PlainTextString rejects only a limited set of Markdown regex patterns, so common Markdown
constructs (e.g., reference-style links like [Forecast]: https://… or Setext headings like
Forecast\n===) can still pass validation and reach renderers as "plain text". This undermines the
new "must not contain Markdown syntax" contract for user-facing fields.

Code

weather_briefing/llm/schema.py[R13-35]

+_MARKDOWN_PATTERNS = (
+    re.compile(r"(?m)^\s{0,3}(?:#{1,6}|[-*+>]|\d+[.)])\s+"),
+    re.compile(r"(?m)^\s*(?:-{3,}|\*{3,}|_{3,})\s*$"),
+    re.compile(r"```|~~~"),
+    re.compile(r"\[[^\]\n]+\]\([^)\n]+\)"),
+    re.compile(r"(?P<delimiter>\*\*|__)(?=\S).+?(?<=\S)(?P=delimiter)"),
+    re.compile(r"(?<!\*)\*(?!\*)(?=\S)[^*\n]+?(?<=\S)\*(?!\*)"),
+    re.compile(r"(?<![\w_])_(?!_)(?=\S)[^_\n]+?(?<=\S)_(?![\w_])"),
+    re.compile(r"~~(?=\S).+?(?<=\S)~~"),
+    re.compile(r"`[^`\n]+`"),
+)
+

def _non_empty(value: str) -> str:
    if not value.strip():
        raise ValueError("must not be empty")
    return value


+def _plain_text(value: str) -> str:
+    if any(pattern.search(value) for pattern in _MARKDOWN_PATTERNS):
+        raise ValueError("must not contain Markdown syntax")
+    return value

Relevance

⭐⭐⭐ High

Matches PR intent to enforce plain-text contract; likely to add patterns/tests for remaining
Markdown bypasses.

PR-#110

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The schema claims to reject Markdown via _plain_text, but the pattern list is limited to a few
Markdown forms; reference-link definitions and Setext headings are not among the implemented
patterns, so they will not be rejected by any(pattern.search(value) ...).

weather_briefing/llm/schema.py[13-35]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PlainTextString` is intended to enforce “no Markdown” in user-facing LLM fields, but the current `_MARKDOWN_PATTERNS` only catches a subset of Markdown. As a result, some Markdown-like constructs can slip through schema validation and be rendered verbatim downstream.

## Issue Context
The validator currently checks headings (ATX), lists, blockquotes, horizontal rules, fenced code markers, inline links, emphasis, strikethrough, and inline code. It does **not** detect other common Markdown forms such as:
- reference-style link definitions: `(?m)^\s*\[[^\]]+\]:\s+\S+`
- Setext headings: `Title\n===` / `Title\n---`
- (Optional) autolinks like `<https://example.com>` if those should be disallowed

## Fix Focus Areas
- weather_briefing/llm/schema.py[13-35]

## Suggested implementation notes
- Add additional regex patterns for reference links and Setext headings.
- Add unit tests mirroring the new parametrized tests to assert these are rejected for `headline` and `*.text`/warning fields.
- Keep patterns conservative to avoid false positives for normal punctuation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unbounded regex scan cost ✓ Resolved 🐞 Bug ➹ Performance

Description

The new _plain_text validator runs multiple regex searches over each user-facing string field
without any explicit max-length bound, so unusually large LLM outputs will incur repeated full-text
scans during schema validation. This is avoidable overhead and can amplify cost when validation
failures trigger retries/repairs.

Code

weather_briefing/llm/schema.py[R32-35]

+def _plain_text(value: str) -> str:
+    if any(pattern.search(value) for pattern in _MARKDOWN_PATTERNS):
+        raise ValueError("must not contain Markdown syntax")
+    return value

Relevance

⭐⭐ Medium

Perf concern is plausible but speculative; team sometimes adds guards for pathological inputs, but
may rely on token limits.

PR-#95
PR-#110

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

_plain_text scans text with multiple regex patterns, and the structured output model does not
impose max-length constraints on these strings; parse_result validates the raw payload directly,
so the scan cost grows with input size.

weather_briefing/llm/schema.py[32-35]
weather_briefing/llm/schema.py[72-82]
weather_briefing/llm/result.py[16-24]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_plain_text` performs multiple regex searches on every validated string, but there are no explicit per-field length limits in the Pydantic schema. Very large strings will cause repeated scanning work during validation.

## Issue Context
Even if LLMs are *expected* to be concise, adding a hard ceiling for user-facing fields provides a deterministic bound on validation work and prevents pathological payloads from consuming unnecessary CPU.

## Fix Focus Areas
- weather_briefing/llm/schema.py[32-35]
- weather_briefing/llm/schema.py[72-82]

## Suggested implementation notes
- Introduce `MAX_PLAINTEXT_CHARS` (or similar) and apply via `Annotated[str, Field(max_length=...)]` (or by adding a length-check `AfterValidator`) to `headline`, `SourcedTextPayload.text`, and warning `title/status/detail`.
- Add/adjust tests to confirm overly long values fail validation with a clear error location.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 22f0d3a ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Leading newline Bark chunks ✓ Resolved 🐞 Bug ☼ Reliability

Description

Bark’s new source footer is newline-separated, which increases the odds that split_plain_message()
splits exactly at a '\n' and leaves that newline as the first character of the next chunk. This can
cause a Bark notification chunk to start with a blank line and wastes payload budget (occasionally
forcing an extra chunk near the limit).

Code

weather_briefing/delivery/renderers.py[R340-356]

+def _bark_numbered_source_references(
+    result: BriefingResult,
+    source_references: dict[str, str],
+) -> tuple[dict[str, str], str]:
+    numbered_references: dict[str, str] = {}
+    numbers_by_name: dict[str, str] = {}
+    source_lines: list[str] = []
+    for source_id in _ordered_source_ids(result):
+        source_name = " ".join(source_references[source_id].split()) or source_id
+        normalized_name = source_name.casefold()
+        number = numbers_by_name.get(normalized_name)
+        if number is None:
+            number = f"[{len(numbers_by_name) + 1}]"
+            numbers_by_name[normalized_name] = number
+            source_lines.append(f"{number} {source_name}")
+        numbered_references[source_id] = number
+    return numbered_references, "\n".join(source_lines)

Relevance

⭐⭐⭐ High

Team previously accepted Bark newline/whitespace trimming to avoid chunking/payload issues; this is
same reliability class.

PR-#110

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The Bark renderer now produces a multi-line source footer by joining lines with newlines, increasing
message newline density. The Bark splitter slices at the newline index but keeps that newline in
remaining, so the next emitted chunk can begin with \n.

weather_briefing/delivery/renderers.py[340-356]
weather_briefing/delivery/bark.py[176-193]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`split_plain_message()` splits on a newline boundary but keeps the delimiter at the start of the next chunk (`remaining = remaining[split_at:]`). With Bark’s briefing now emitting more newline boundaries (each source on its own line), long briefings are more likely to produce chunks that begin with `\n`, resulting in notifications starting with a blank line and wasting character budget.

## Issue Context
- Bark briefings now end with a multi-line footer via `"\n".join(source_lines)`.
- Bark chunking tries to split at newlines when possible.

## Fix Focus Areas
- weather_briefing/delivery/bark.py[176-193]
- weather_briefing/delivery/renderers.py[340-356]

## Suggested fix approach
- When a newline split is chosen, split *after* the newline when possible (so the next chunk doesn’t start with `\n`).
 - Example: search for `\n` in `remaining` up to `limit` (or `limit-1`), and set `split_at = newline_index + 1` when `newline_index >= 0`.
 - Fall back to `split_at = limit` when no newline is found.
- Add a regression test that constructs a long multi-line body where the chosen split point is a newline and assert no returned chunk starts with `\n`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

PR Summary by Qodo

Refine Bark compact briefing formatting and source citation deduping

🐞 Bug fix ✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Render Bark briefings as plain text (no Markdown-style list markers).
• Deduplicate/normalize Bark source citations and render each source on its own line.
• Tighten Bark message splitting semantics to consume boundary newlines and avoid empty chunks.

Diagram

graph TD
P[/"System prompt"/] --> L(["LLM"]) --> R[["BriefingResult JSON"]] --> T["BarkTextRenderer"] --> S["split_plain_message"] --> A(["Bark API"])
subgraph Legend
  direction LR
  _cfg[/"Prompt/config"/] ~~~ _proc["Code path"] ~~~ _ext(["External service"])
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Renderer-side Markdown sanitization (instead of plain-text requirement)

• ➕ More tolerant of occasional LLM formatting drift; fewer hard failures upstream
• ➕ Centralizes robustness in one layer (renderers)
• ➖ Hard to fully and safely strip all Markdown edge-cases without altering meaning
• ➖ Encourages silent degradation; citations/section formatting can become inconsistent

2. Deduplicate sources by source_id only (no display-name merging)

• ➕ Avoids false merges when different sources share a similar name
• ➕ Preserves provenance fidelity end-to-end
• ➖ Produces noisier Bark footers (duplicate lines) when the same provider appears under multiple IDs
• ➖ Worse UX for compact notifications where space is constrained

3. Preserve boundary newlines during splitting

• ➕ Keeps exact original text reconstruction via join(chunks)
• ➕ Avoids subtle changes to whitespace semantics
• ➖ Creates display artifacts (chunks starting with '\n') and can yield empty trailing chunks
• ➖ Worse presentation in Bark where each chunk is displayed independently

Recommendation: Current approach is a good fit for Bark’s compact, display-first constraints: enforce plain-text at the generation boundary (prompt) and render consistently per publisher, while deduping citations by normalized display name to reduce footer noise. The newline-consuming split semantics are also preferable for chunked display (no leading/trailing newlines, no empty chunks), and the expanded test coverage makes these behavioral changes safer to review.

Files changed (6) +103 / -28

Enhancement (1) +31 / -9

renderers.pyMake Bark renderer fully plain-text and dedupe sources by normalized name +31/-9

Make Bark renderer fully plain-text and dedupe sources by normalized name

• Updates BarkTextRenderer to remove Markdown-style prefixes for items, render the source list as one source per line without a 'Sources:' label, and introduces Bark-specific source numbering that merges multiple source_ids sharing the same normalized display name (with a fallback to source_id for blank names).

weather_briefing/delivery/renderers.py

Bug fix (1) +10 / -7

bark.pyConsume newline split boundaries and avoid empty trailing chunks +10/-7

Consume newline split boundaries and avoid empty trailing chunks

• Refines split_plain_message to split on newline boundaries when available but drop the boundary newline from output chunks, and to omit appending an empty final chunk when the body ends with a boundary newline.

weather_briefing/delivery/bark.py

Tests (3) +60 / -12

test_bark_publisher.pyUpdate tests for newline-consuming Bark message splitting +13/-6

Update tests for newline-consuming Bark message splitting

• Renames and expands split_plain_message tests to assert boundary newlines are consumed, trailing newline does not produce an empty chunk, and internal newlines are preserved within chunks when not used as split points.

tests/test_bark_publisher.py

test_prompts.pyAssert prompt forbids Markdown in briefing fields +2/-0

Assert prompt forbids Markdown in briefing fields

• Adds prompt assertions to verify the system prompt explicitly requires plain-text output and forbids Markdown formatting in briefing fields.

tests/test_prompts.py

test_render.pyAlign Bark renderer tests with plain-text formatting and source deduping +45/-6

Align Bark renderer tests with plain-text formatting and source deduping

• Updates BarkTextRenderer expectations to remove Markdown list markers, render sources as a newline-separated list, and adds coverage for merging sources with the same display name while keeping distinct blank-name sources separate.

tests/test_render.py

Other (1) +2 / -0

system_prompt.txtRequire plain-text briefing fields (no Markdown) at generation time +2/-0

Require plain-text briefing fields (no Markdown) at generation time

• Extends the system prompt to require headline and item text fields to contain only plain text, explicitly disallowing Markdown headings/lists/emphasis/code/links and reserving section titles/bullets for publisher rendering.

weather_briefing/data/system_prompt.txt

[qodo-code-review]

Code review by qodo was updated up to the latest commit b0056e7

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit c3edd8d

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit dd5a225

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 22f0d3a

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 7aa593a

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 669f01c

[qodo-code-review]

Code review by qodo was updated up to the latest commit 669f01c

[IceCodeNew]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@weather_briefing/delivery/bark.py`:
- Around line 187-193: Update the chunk-splitting logic around the newline
boundary and final chunk append so an empty remaining string is not added to the
output. Preserve the existing newline-preferred and hard-limit splitting
behavior, while ensuring inputs ending exactly at the selected boundary produce
no trailing empty chunk before publish.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5297fcc7-0f8f-49a3-9375-a96632149e14

📥 Commits

Reviewing files that changed from the base of the PR and between b0056e7 and 669f01c.

📒 Files selected for processing (4)

• tests/test_bark_publisher.py
• tests/test_render.py
• weather_briefing/delivery/bark.py
• weather_briefing/delivery/renderers.py

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/test_render.py
• weather_briefing/delivery/renderers.py

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8dcce6f