Isolate tracing from wrap domain logic (#309) - #412
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
WalkthroughRoute inline tokenisation, span construction, and fragment classification through an observer event model. Adapt tracing through ChangesInline observer pipeline
Sequence Diagram(s)sequenceDiagram
participant wrap_preserving_code_observed
participant segment_inline_observed
participant InlineFragment
participant TracingObserver
wrap_preserving_code_observed->>segment_inline_observed: tokenize text with observer
segment_inline_observed->>TracingObserver: emit parsing events
wrap_preserving_code_observed->>InlineFragment: classify grouped spans
InlineFragment->>TracingObserver: emit classification events
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d403b2519b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(observer) = observer.as_deref_mut() { | ||
| observer.observe(Event::FootnoteReferenceParsed { | ||
| token_length: text[start..text_end].chars().count(), | ||
| }); |
There was a problem hiding this comment.
When wrapping runs without a DEBUG subscriber, ParagraphWriter still supplies Some(TracingObserver), so this block performs chars().count() for every parsed footnote; links do the same at lines 50-53, and fragment snippets are also built before the adapter checks the active level. These computations were previously inside tracing::enabled! guards, so long inline tokens now incur unnecessary full scans even when no event is emitted. Gate observer creation at the wrapping boundary or defer field computation until after the level check, as required by docs/developers-guide.md:608-612.
AGENTS.md reference: AGENTS.md:L38-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Current PR head =='
git rev-parse HEAD
git status --short
printf '%s\n' '== Relevant module outlines =='
ast-grep outline src/wrap/paragraph.rs --items all
ast-grep outline src/wrap/tracing_adapter.rs --items all
ast-grep outline src/wrap/observer.rs --items all
ast-grep outline src/wrap/tokenize/parsing.rs --items all
printf '%s\n' '== Observer construction and observed wrapping call sites =='
rg -n -C 4 --type rust \
'TracingObserver|wrap_preserving_code_observed|segment_inline_observed|parse_link_or_image\s*\(' \
src
printf '%s\n' '== Event variants, payload construction, and adapter level guards =='
rg -n -C 5 --type rust \
'FootnoteReferenceParsed|LinkOrImageParsed|FragmentClassified|FootnoteLabelRecognized|enabled!\s*\(|Level::' \
src/wrap
printf '%s\n' '== Diagnostic-gating guidance =='
rg -n -C 5 '608|tracing::enabled|enabled!' docs/developers-guide.md AGENTS.md 2>/dev/null || true
printf '%s\n' '== Focused source excerpts =='
for f in src/wrap/paragraph.rs src/wrap/tracing_adapter.rs src/wrap/observer.rs src/wrap/inline/fragment.rs; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,260p' "$f"
fi
doneLength of output: 66608
@leynos, this is resolved in the latest commit.
Trace the dataflow: ParagraphWriter still installs TracingObserver, but parse_link_or_image now emits only borrowed token slices. TracingObserver performs chars().count() and fragment snippet construction only inside tracing::enabled!-guarded match arms. This matches the documented performance discipline in docs/developers-guide.md.
I cannot resolve this thread via the API because it was authored by chatgpt-codex-connector[bot], not coderabbitai; GitHub resolution is restricted to this reviewer’s own comments.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== PR head =='
git rev-parse HEAD
echo '== Code graph: observer construction, event production, and tracing emission =='
ast-grep outline src/wrap --items all --type function,struct,enum,trait --match 'TracingObserver|Observer|parse_link_or_image|find_footnote_end|new_observed|wrap_preserving_code_observed' || true
echo '== Relevant call sites and costly diagnostic expressions =='
rg -n -C 5 --type rust \
'TracingObserver|FootnoteReferenceParsed|LinkOrImageParsed|FragmentClassified|chars\(\)\.count\(\)|snippet|tracing::enabled!\s*\(|Level::' \
src/wrap
echo '== Focused current implementations =='
for file in src/wrap/observer.rs src/wrap/tracing_adapter.rs src/wrap/paragraph.rs src/wrap/inline/fragment.rs src/wrap/tokenize/parsing.rs; do
echo "--- ${file}"
sed -n '1,280p' "$file"
done
echo '== Review thread metadata for the supplied comment =='
gh api graphql -F owner='leynos' -F name='mdtablefix' -F number=412 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 20) {
nodes {
id
databaseId
author { login }
body
url
}
}
}
}
}
}
}' | jq -r '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .url == "https://github.com/leynos/mdtablefix/pull/412#discussion_r3651562616"))
| {id, isResolved, comments: [.comments.nodes[] | {author: .author.login, url, body}]}
'Length of output: 71433
@leynos, treat this finding as resolved at 695c804ff319738c0126f8e7652d76adf111248f.
The current dataflow defers expensive diagnostics correctly:
src/wrap/tokenize/parsing.rsemits borrowed token slices rather than callingchars().count().src/wrap/inline/fragment.rsemits borrowed fragment text rather than constructing a snippet.src/wrap/tracing_adapter.rsperformstoken.chars().count()andtrace_text_snippet(token)only insidetracing::enabled!-guarded match arms.
Do not require further code changes for this comment. I cannot resolve the GitHub thread through the API because its original review comment belongs to chatgpt-codex-connector, not coderabbitai; resolve it from the PR UI or as that reviewer.
| pub(super) fn wrap_preserving_code_observed( | ||
| text: &str, | ||
| width: usize, | ||
| observer: &mut Option<&mut dyn crate::wrap::observer::Observer>, | ||
| ) -> Vec<String> { |
There was a problem hiding this comment.
Adding the observed wrapper entry point expands src/wrap/inline.rs to 405 lines, exceeding the repository's hard 400-line limit. Move the observer-facing orchestration or another coherent unit into a focused module rather than leaving this file over the documented maintainability threshold.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Checked-out PR state =="
git rev-parse HEAD
git status --short
printf '\n== src/wrap/inline.rs line count ==\n'
wc -l src/wrap/inline.rs
printf '\n== Codegraph outline: inline module ==\n'
ast-grep outline src/wrap/inline.rs --items all --view expanded
printf '\n== Extracted wrapping module presence and outline ==\n'
if [ -f src/wrap/inline/wrapping.rs ]; then
wc -l src/wrap/inline/wrapping.rs
ast-grep outline src/wrap/inline/wrapping.rs --items all --view expanded
else
echo "src/wrap/inline/wrapping.rs is absent"
fi
printf '\n== Relevant module declarations and re-exports ==\n'
rg -n -C 2 'mod wrapping|wrapping::wrap_preserving_code|wrap_preserving_code_observed' src/wrap/inline.rs
printf '\n== PR head and matching review threads ==\n'
gh pr view 412 --repo leynos/mdtablefix --json headRefOid,url \
--jq '{url, headRefOid}'
gh api graphql -F owner='leynos' -F name='mdtablefix' -F number=412 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
path
line
originalLine
comments(first: 20) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}' | jq -r '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.path == "src/wrap/inline.rs")
| select([.comments.nodes[].body] | join("\n") | contains("Split inline.rs back below the 400-line cap"))
| {id, isResolved, path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, url, body}]}
'Length of output: 10885
@leynos, the finding is resolved in 9d6de44b72d2dc17e286a9b3f9d0dadf964b5a01.
src/wrap/inline.rs is now 48 lines. The observer-facing wrapping orchestration was coherently extracted into src/wrap/inline/wrapping.rs (388 lines), while inline.rs retains module declarations and focused re-exports.
I cannot resolve this specific GitHub thread through the API because its original comment author is chatgpt-codex-connector, not coderabbitai; resolve it from the PR UI or through that reviewer’s integration.
🧬 Code Graph Analysis Results
File: src/wrap/inline.rs
No additional relevant snippets beyond module/predicate re-exports and public wrapping entry points.
File: src/wrap/inline/predicates.rs
Lines 14-16
pub(in crate::wrap::inline) fn is_opening_punct(c: char) -> bool {
matches!(c, '(' | '[' | '"' ) || "“‘（［【《「『".contains(c)
}Lines 18-24
pub(in crate::wrap::inline) fn is_trailing_punct(c: char) -> bool {
// ASCII closers + common Unicode closers and word-final punctuation
matches!(
c,
'.' | ',' | ';' | ':' | '!' | '?' | ')' | ']' | '"' | '\''
) || "…—–»›）］】》」』、。，：；！？”.’".contains(c)
}Lines 109-121
pub(in crate::wrap::inline) fn looks_like_footnote_ref(
token: &str,
observer: &mut ObserverHandle<'_>,
) -> bool {
let result = token
.strip_prefix("[^")
.and_then(|label| label.strip_suffix(']'))
.is_some_and(|label| !label.is_empty());
if let Some(observer) = observer.as_deref_mut() {
observer.observe(Event::FootnoteRefChecked { token, result });
}
result
}Lines 124-133
pub(in crate::wrap::inline) fn ends_with_footnote_ref(
token: &str,
observer: &mut ObserverHandle<'_>,
) -> bool {
let Some(start) = token.rfind("[^") else {
return false;
};
looks_like_footnote_ref(&token[start..], observer)
}Lines 136-138
pub(in crate::wrap::inline) fn is_whitespace_token(token: &str) -> bool {
token.chars().all(char::is_whitespace)
}Lines 141-143
pub(in crate::wrap::inline) fn is_inline_code_token(token: &str) -> bool {
token.len() > 1 && token.starts_with('`') && token.ends_with('`')
}Lines 190-205
pub(in crate::wrap::inline) fn fragment_is_link(text: &str) -> bool {
if looks_like_link(text) {
return true;
}
let mut candidate = link_text_after_leading_openers(text);
while !candidate.is_empty() {
if looks_like_link(candidate) {
return true;
}
let Some(next) = strip_outer_link_wrapper_suffix(candidate) else {
break;
};
candidate = next;
}
false
}File: src/wrap/inline/wrapping.rs
Lines 119-121
pub(in crate::wrap) fn determine_token_span(tokens: &[String], start: usize) -> (usize, usize) {
determine_token_span_observed(tokens, start, &mut None)
}(Wrap-related helper; calls determine_token_span_observed(tokens, start, ...) and returns (start_idx, end_idx).)
Lines 325-328
pub(in crate::wrap) fn wrap_preserving_code(text: &str, width: usize) -> Vec<String> {
let mut observer = crate::wrap::observer::NoOpObserver;
wrap_preserving_code_observed(text, width, &mut Some(&mut observer))
}Lines 330-388
pub(in crate::wrap) fn wrap_preserving_code_observed(
text: &str,
width: usize,
observer: &mut ObserverHandle<'_>,
) -> Vec<String> {
let tokens = tokenize::segment_inline_observed(text, observer);
if tokens.is_empty() {
return Vec::new();
}
let tokens = normalize_footnote_ref_spacing(&tokens, observer);
let fragments = build_fragments(&tokens, observer);
let mut lines = Vec::new();
let mut buffer: Vec<InlineFragment> = Vec::new();
for fragment in fragments {
if should_flush_boundary_link(&lines, &buffer, &fragment, width) {
lines.push(render_line(&buffer, false, !lines.is_empty()));
buffer.clear();
if fragment.is_whitespace() {
continue;
}
}
buffer.push(fragment);
let wrapped = wrap_first_fit(&buffer, &[width_as_f64(width)]);
let raw_lines = wrapped.iter().map(|line| line.to_vec()).collect::<Vec<_>>();
let mut grouped_lines = merge_whitespace_only_lines(&raw_lines, width);
rebalance_atomic_tails(&mut grouped_lines, width);
if grouped_lines.len() == 1 {
continue;
}
if let Some((link_line, remaining_line)) = grouped_lines
.get(grouped_lines.len() - 2)
.zip(grouped_lines.last())
.and_then(|(previous, line)| split_boundary_link_line(previous, line, width))
{
for line in &grouped_lines[..grouped_lines.len() - 1] {
lines.push(render_line(line, false, !lines.is_empty()));
}
lines.push(render_line(&link_line, false, !lines.is_empty()));
buffer = remaining_line;
continue;
}
for line in &grouped_lines[..grouped_lines.len() - 1] {
lines.push(render_line(line, false, !lines.is_empty()));
}
buffer = grouped_lines.pop().unwrap_or_default();
}
if !buffer.is_empty() {
lines.push(render_line(&buffer, true, !lines.is_empty()));
}
lines
}(Main observer-aware inline wrapping pipeline: tokenizes with segment_inline_observed, normalizes footnote spacing, builds fragments, wraps by first-fit, groups/merges whitespace-only lines, rebalances atomic tails, handles boundary links via should_flush_boundary_link and split_boundary_link_line, and renders final Vec<String> lines.)
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="leynos/mdtablefix"
needle="Split inline.rs back below the 400-line cap"
echo "== PR head and checked-out commit =="
gh pr view 412 --repo "$repo" --json headRefOid,url --jq '{url, headRefOid}'
git rev-parse HEAD
echo
echo "== Relevant file sizes =="
wc -l src/wrap/inline.rs
if [ -f src/wrap/inline/wrapping.rs ]; then
wc -l src/wrap/inline/wrapping.rs
fi
echo
echo "== Codegraph: facade module =="
ast-grep outline src/wrap/inline.rs --items all --view expanded
echo
echo "== Codegraph: extracted wrapping orchestration =="
if [ -f src/wrap/inline/wrapping.rs ]; then
ast-grep outline src/wrap/inline/wrapping.rs --items all --view expanded
else
echo "src/wrap/inline/wrapping.rs is absent"
fi
echo
echo "== Facade-to-extracted-module links =="
rg -n -C 2 'mod wrapping|wrapping::wrap_preserving_code|wrap_preserving_code_observed' \
src/wrap/inline.rs src/wrap/inline/wrapping.rs
echo
echo "== Matching review thread =="
gh api graphql -F owner='leynos' -F name='mdtablefix' -F number=412 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
path
line
originalLine
comments(first: 30) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}' | jq --arg needle "$needle" '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.path == "src/wrap/inline.rs")
| select(any(.comments.nodes[]; .body | contains($needle)))
| {
id, isResolved, path, line, originalLine,
comments: [.comments.nodes[] | {author: .author.login, url, body}]
}
'Length of output: 18793
@leynos, treat this finding as resolved in the latest commit (695c804ff319738c0126f8e7652d76adf111248f).
Codegraph analysis confirms that:
src/wrap/inline.rsis now 48 lines, below the 400-line cap.- The observer-aware orchestration lives coherently in
src/wrap/inline/wrapping.rs, which is 388 lines. - The facade declares
mod wrappingand re-exportswrap_preserving_code_observed, preserving the existing inline module surface.
Resolve thread PRRT_kwDOO7HqGM6Tz9LE through the GitHub PR UI or the chatgpt-codex-connector integration. I cannot resolve it through the API because that integration authored the original review comment.
🧬 Code Graph Analysis Results
src/wrap/inline/predicates.rs
Lines 14-16
pub(in crate::wrap::inline) fn is_opening_punct(c: char) -> bool {
matches!(c, '(' | '[' | '"') || "“‘（［【《「『".contains(c)
}Lines 18-24
pub(in crate::wrap::inline) fn is_trailing_punct(c: char) -> bool {
// ASCII closers + common Unicode closers and word-final punctuation
matches!(
c,
'.' | ',' | ';' | ':' | '!' | '?' | ')' | ']' | '"' | '\''
) || "…—–»›）］】》」』、。，：；！？”.’".contains(c)
}Lines 109-121
pub(in crate::wrap::inline) fn looks_like_footnote_ref(
token: &str,
observer: &mut ObserverHandle<'_>,
) -> bool {
let result = token
.strip_prefix("[^")
.and_then(|label| label.strip_suffix(']'))
.is_some_and(|label| !label.is_empty());
if let Some(observer) = observer.as_deref_mut() {
observer.observe(Event::FootnoteRefChecked { token, result });
}
result
}Lines 124-133
pub(in crate::wrap::inline) fn ends_with_footnote_ref(
token: &str,
observer: &mut ObserverHandle<'_>,
) -> bool {
let Some(start) = token.rfind("[^") else {
return false;
};
looks_like_footnote_ref(&token[start..], observer)
}Lines 136-138
pub(in crate::wrap::inline) fn is_whitespace_token(token: &str) -> bool {
token.chars().all(char::is_whitespace)
}Lines 141-143
pub(in crate::wrap::inline) fn is_inline_code_token(token: &str) -> bool {
token.len() > 1 && token.starts_with('`') && token.ends_with('`')
}Lines 190-205
pub(in crate::wrap::inline) fn fragment_is_link(text: &str) -> bool {
if looks_like_link(text) {
return true;
}
let mut candidate = link_text_after_leading_openers(text);
while !candidate.is_empty() {
if looks_like_link(candidate) {
return true;
}
let Some(next) = strip_outer_link_wrapper_suffix(candidate) else {
break;
};
candidate = next;
}
false
}src/wrap/inline/wrapping.rs
Lines 119-121
pub(in crate::wrap) fn determine_token_span(tokens: &[String], start: usize) -> (usize, usize) {
determine_token_span_observed(tokens, start, &mut None)
}Lines 325-328
pub(in crate::wrap) fn wrap_preserving_code(text: &str, width: usize) -> Vec<String> {
let mut observer = crate::wrap::observer::NoOpObserver;
wrap_preserving_code_observed(text, width, &mut Some(&mut observer))
}Lines 330-388 (summary)
/// fn wrap_preserving_code_observed(text: &str, width: usize, observer: &mut ObserverHandle<'_>) -> Vec<String>
/// - Tokenizes input with `tokenize::segment_inline_observed(text, observer)`.
/// - Returns `Vec::new()` if tokenization yields no tokens.
/// - Applies footnote-ref spacing normalization: `normalize_footnote_ref_spacing(&tokens, observer)`.
/// - Builds inline fragments via `build_fragments(&tokens, observer)`.
/// - Iteratively fits fragments into lines using `wrap_first_fit(&buffer, &[width_as_f64(width)])`.
/// - Merges/adjusts line groupings:
/// - `merge_whitespace_only_lines(&raw_lines, width)`
/// - `rebalance_atomic_tails(&mut grouped_lines, width)`
/// - Handles special cases for boundary links using:
/// - `should_flush_boundary_link(...)`
/// - `split_boundary_link_line(previous, line, width)`
/// - Renders finished lines using `render_line(..., keep_inline_code=true/false?, is_not_first_line?)`.
/// - Returns accumulated `lines`, pushing the last buffered line if non-empty.
/// - No explicit error handling; control flow is driven by fragment boundaries and line grouping results.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/wrap/inline/span_helpers.rs (1)
67-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree helper functions can never report to an active observer.
date_token_span,should_couple_whitespace, andmatches_footnote_ref_spacingall call footnote-ref predicates with a hardcoded&mut Nonebecause none of them accept anobserverparameter — even though each is invoked from a context (determine_token_span_observed,wrap_preserving_code_observed) that holds a live observer. The single root cause is these three signatures not threading the caller's observer through.
src/wrap/inline/span_helpers.rs#L67-L86: add anobserver: &mut Option<&mut dyn Observer>parameter todate_token_spanand forward it intotry_couple_footnote_referenceinstead of&mut None.src/wrap/inline/span_helpers.rs#L141-L162: add anobserverparameter toshould_couple_whitespaceand forward it intolooks_like_footnote_refinstead of&mut None.src/wrap/inline/normalize.rs#L43-L50: add anobserverparameter tomatches_footnote_ref_spacing(and its callernormalize_footnote_ref_spacing) and forward it instead of&mut None.🤖 Prompt for 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. In `@src/wrap/inline/span_helpers.rs` around lines 67 - 86, Thread the live observer through all three helper paths: in src/wrap/inline/span_helpers.rs:67-86, add an observer parameter to date_token_span and pass it to try_couple_footnote_reference; in src/wrap/inline/span_helpers.rs:141-162, add the parameter to should_couple_whitespace and pass it to looks_like_footnote_ref; in src/wrap/inline/normalize.rs:43-50, add it to matches_footnote_ref_spacing and normalize_footnote_ref_spacing, forwarding it instead of &mut None. Update all callers, including determine_token_span_observed and wrap_preserving_code_observed.src/wrap/inline.rs (1)
338-402: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit
src/wrap/inline.rsinto a submodule.
Keep the observer-aware wrapping helpers (determine_token_span_observed,build_fragments,wrap_preserving_code_observed) out of this file sosrc/wrap/inline.rsstays within the 400-line cap; it is already at 405 lines.🤖 Prompt for 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. In `@src/wrap/inline.rs` around lines 338 - 402, Split the observer-aware wrapping helpers determine_token_span_observed, build_fragments, and wrap_preserving_code_observed into a dedicated inline submodule, updating visibility and imports so existing callers and tests continue to work. Keep inline.rs focused on the remaining public wrapping API and under the 400-line limit.Source: Path instructions
🤖 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 `@src/wrap/inline.rs`:
- Line 258: Import FragmentKind from crate::wrap::observer in the relevant
scope, then update both call sites around the line-length checks to use
FragmentKind::Link instead of the fully qualified path.
- Around line 60-64: Introduce a shared pub(crate) ObserverHandle<'a> alias in
observer.rs for the optional mutable Observer reference, then update
initial_token_span in src/wrap/inline.rs#L60-L64, new_observed in
src/wrap/inline/fragment.rs#L51-L51, looks_like_footnote_ref in
src/wrap/inline/predicates.rs#L109-L111, and try_couple_footnote_reference in
src/wrap/inline/span_helpers.rs#L223-L223 to accept &mut ObserverHandle<'_>,
adding the necessary imports and replacing the repeated verbose types.
In `@src/wrap/observer.rs`:
- Around line 51-56: Make NoOpObserver test-only by adding #[cfg(test)] to the
struct and removing its dead_code allowance, while preserving its existing
documentation and visibility.
In `@src/wrap/paragraph.rs`:
- Around line 167-171: Extract the shared TracingObserver setup and
wrap_preserving_code_observed call from wrap_with_prefix and
append_wrapped_with_prefix_width into a small private helper, then update both
call sites to use it while preserving their existing text and width arguments.
---
Outside diff comments:
In `@src/wrap/inline.rs`:
- Around line 338-402: Split the observer-aware wrapping helpers
determine_token_span_observed, build_fragments, and
wrap_preserving_code_observed into a dedicated inline submodule, updating
visibility and imports so existing callers and tests continue to work. Keep
inline.rs focused on the remaining public wrapping API and under the 400-line
limit.
In `@src/wrap/inline/span_helpers.rs`:
- Around line 67-86: Thread the live observer through all three helper paths: in
src/wrap/inline/span_helpers.rs:67-86, add an observer parameter to
date_token_span and pass it to try_couple_footnote_reference; in
src/wrap/inline/span_helpers.rs:141-162, add the parameter to
should_couple_whitespace and pass it to looks_like_footnote_ref; in
src/wrap/inline/normalize.rs:43-50, add it to matches_footnote_ref_spacing and
normalize_footnote_ref_spacing, forwarding it instead of &mut None. Update all
callers, including determine_token_span_observed and
wrap_preserving_code_observed.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aaafb396-6a69-4f4c-b664-83026f14d8ca
📒 Files selected for processing (18)
src/wrap.rssrc/wrap/inline.rssrc/wrap/inline/fragment.rssrc/wrap/inline/normalize.rssrc/wrap/inline/postprocess.rssrc/wrap/inline/postprocess_tests.rssrc/wrap/inline/predicate_tracing_tests.rssrc/wrap/inline/predicates.rssrc/wrap/inline/span_helper_tracing_tests.rssrc/wrap/inline/span_helpers.rssrc/wrap/inline/tests.rssrc/wrap/inline/tracing_events.rssrc/wrap/observer.rssrc/wrap/paragraph.rssrc/wrap/tokenize/mod.rssrc/wrap/tokenize/parsing.rssrc/wrap/tokenize/parsing_tests.rssrc/wrap/tracing_adapter.rs
💤 Files with no reviewable changes (3)
- src/wrap/inline/predicate_tracing_tests.rs
- src/wrap/inline/span_helper_tracing_tests.rs
- src/wrap/inline/tracing_events.rs
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD~1)"
printf '%s\n' '=== Changed files ==='
git diff --name-status "$base" HEAD
printf '%s\n' '=== Recent commits ==='
git log --oneline -12
printf '%s\n' '=== Observer, adapter, predicate test evidence ==='
for f in src/wrap/observer.rs src/wrap/tracing_adapter.rs src/wrap/inline/predicates.rs; do
if [ -f "$f" ]; then
printf '\n--- %s outline ---\n' "$f"
ast-grep outline "$f" --items all || true
printf '\n--- %s relevant lines ---\n' "$f"
rg -n -C 3 \
-e 'NoOpObserver' \
-e 'allow\(dead_code\)' \
-e 'FootnoteRefChecked' \
-e 'footnote reference checked' \
-e 'looks_like_footnote_ref' \
-e 'ends_with_footnote_ref' \
-e 'enabled!' \
"$f" || true
fi
done
printf '%s\n' '=== Benchmark registration and sources ==='
rg -n -i -C 3 \
-e '\[\[bench\]\]' \
-e 'criterion|divan|iai' \
-e 'bench' \
Cargo.toml Makefile docs README.md 2>/dev/null || true
fd -t f . -E .git -E target | rg -i '(^|/)(benches|benchmarks)/|bench.*\.(rs|md)$' || true
printf '%s\n' '=== Documentation evidence ==='
for f in docs/developers-guide.md docs/architecture.md docs/contents.md docs/adrs/0006-observer-boundary-for-tracing.md; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
rg -n -i -C 2 \
-e 'observer|tracing|benchmark|performance|disabled' \
"$f" || true
fi
doneLength of output: 29107 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Benchmark implementation outline ==='
ast-grep outline benches/wrap_observer.rs --items all
ast-grep outline src/wrap/bench_internals.rs --items all
ast-grep outline tests/bench_fixtures.rs --items all
printf '%s\n' '=== Benchmark implementation ==='
cat -n benches/wrap_observer.rs
printf '%s\n' '=== Benchmark internals ==='
cat -n src/wrap/bench_internals.rs
printf '%s\n' '=== Fixture tests ==='
cat -n tests/bench_fixtures.rs
printf '%s\n' '=== Makefile validation targets ==='
sed -n '1,90p' Makefile
printf '%s\n' '=== Bench feature/module wiring ==='
rg -n -C 3 \
-e 'bench-internals' \
-e 'bench_internals' \
-e 'wrap_text_realistic_document' \
-e 'observer_none' \
-e 'tracing_observer_disabled' \
src benches tests Cargo.toml MakefileLength of output: 19199
Run the full validation suite before merging; this sandbox can inspect source but cannot execute repository code: make check-fmt
make lint
make typecheck
make test
make markdownlint
make nixie
RUSTFLAGS="-D warnings" make benchTreat any warning or failure from these commands as blocking. |
Move inline classification and parsing observations into domain events.\n\nKeep tracing integration at the paragraph-facing boundary so domain code\ncan be called without a subscriber or vendor-specific guards.
Refine the tracing-isolation work so the inline-wrapping domain stays free of vendor-specific tracing and the observer port is documented. - Add a `pub(crate) ObserverHandle<'a>` alias and replace the repeated `&mut Option<&mut dyn Observer>` signatures across the wrap modules. - Gate `NoOpObserver` behind `#[cfg(test)]` and drop its `dead_code` allowance; only test paths construct it. - Move the observer-aware orchestration (span grouping, fragment building, line fitting) into `wrap/inline/wrapping.rs`, keeping `inline.rs` on the public surface and both files under the 400-line limit; import `FragmentKind` rather than fully qualifying it. - Thread the live observer through `date_token_span`, `should_couple_whitespace`, and footnote-ref normalization instead of discarding it via `&mut None`. - Extract the shared `TracingObserver` setup in `paragraph.rs` into a `wrap_observed` helper. - Make `Event` variants carry borrowed `&str` slices and move all derived work (`chars().count()`, snippet truncation) into `TracingObserver` behind its level gate, so a disabled subscriber pays only a branch on the hot path. - Restore date diagnostics through a new `DateSequenceMatched` event emitted from `date_token_span`. - Add traced tests for `FootnoteRefChecked` and `DateSequenceMatched`; tighten the checklist-span proptest closing-fence assertion so it no longer collides with the opening fence. - Document the observer boundary: developers-guide observability section and event/log mapping, new ADR 0006, and architecture notes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Prove that the observer/tracing adapter adds no derived-payload work on the inline wrapping hot path when tracing is disabled, and establish a baseline for large realistic wrapping workloads. - Add Criterion (minimal features) as a dev-dependency and register a `wrap_observer` bench target gated on a new `bench-internals` feature. - Add `src/wrap/bench_internals.rs`: feature-gated, `#[doc(hidden)]` shims exposing the crate-internal `wrap_preserving_code_observed` for the observer-`None` and `TracingObserver`-disabled paths, plus deterministic large fixtures mixing prose, links, inline code, and footnote references. Never compiled into production builds. - Benchmark the public `wrap_text` document path and the inline path with the observer disabled (`None`) versus `TracingObserver` with no subscriber installed; the latter two must match. - Add `tests/bench_fixtures.rs` proving the fixtures exercise links, code spans, and footnote references and that wrapping preserves them (content assertions only, no timing). - Add a `make bench` target and a developer-guide section documenting the command and the disabled-tracing performance invariant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reconcile the observer/tracing work with main's restructuring of the paragraph module (now split into hard_break, pending, spanning_code, and tail_reflow submodules) after rebasing onto origin/main. - Make `wrap_preserving_code` the production inline entry point again, wiring up a `TracingObserver` internally, so main's new call sites in `paragraph/spanning_code.rs` and `paragraph/tail_reflow.rs` route through the observer boundary. This replaces the branch's test-only `wrap_preserving_code` and the redundant `paragraph::wrap_observed` helper, which is removed. - Gate the `wrap_preserving_code_observed` re-export behind the `bench-internals` feature, its only remaining external caller. - Rebuild Cargo.lock from main's, re-adding the criterion dev-dependency. - Repair an architecture.md merge artifact that mangled the footnote before/after example, and drop stray blank lines flagged by markdownlint in the merged docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
695c804 to
84b2a9c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@docs/developers-guide.md`:
- Around line 722-727: Update the benchmark documentation in the comparison of
observer_none and tracing_observer_disabled to state that disabled tracing still
incurs dynamic dispatch, event matching, and a per-event tracing::enabled!
check. Require that no derived-payload work occurs and that overhead remains
bounded, rather than claiming the two cases must have equal timings.
In `@Makefile`:
- Around line 22-24: Update the Makefile bench target’s cargo invocation to
propagate the existing RUSTFLAGS with -D warnings, ensuring benchmark
compilation fails on warnings while preserving the current features and
wrap_observer benchmark selection.
In `@src/wrap/inline/fragment.rs`:
- Around line 167-169: Pass &mut None to all three looks_like_footnote_ref and
ends_with_footnote_ref probes in src/wrap/inline/fragment.rs lines 167-169,
preserving FragmentClassified as the sole event for the fragment. Also pass &mut
None to both looks_like_footnote_ref calls in src/wrap/inline/normalize.rs lines
50-54 so speculative scans do not emit duplicate observer events.
In `@src/wrap/inline/predicates.rs`:
- Around line 260-264: Rename footnote_ref_check_does_not_require_subscriber to
reflect that it only checks the no-observer path. Add a separate test for the
adapter-without-subscriber scenario by constructing Some(TracingObserver)
outside #[traced_test], then assert looks_like_footnote_ref handles both
footnote and plain inputs without a subscriber.
In `@src/wrap/inline/wrapping.rs`:
- Around line 338-342: Add Rustdoc (`///`) directly above
`wrap_preserving_code_observed` documenting that events are emitted only when
the observer handle is `Some`, and that the adapter is responsible for derived
work. Match the contract detail and style of the sibling `wrap_preserving_code`
documentation.
In `@src/wrap/tracing_adapter.rs`:
- Around line 62-67: Stop emitting Markdown token contents from the
Event::FragmentClassified tracing branch: replace the token snippet field with
non-sensitive metadata such as token_length, kind, and truncated, and remove any
now-unused snippet handling. Update
docs/adrs/0006-observer-boundary-for-tracing.md lines 39-46 to document
metadata-only diagnostics, docs/architecture.md lines 625-632 to remove the
bounded-snippet claim, docs/developers-guide.md lines 678-685 to replace the
raw-token example, and docs/developers-guide.md lines 753-764 to remove the
token field from the event mapping.
In `@tests/bench_fixtures.rs`:
- Around line 31-49: Update
realistic_document_fixture_covers_and_preserves_constructs to validate every
generated construct rather than only index 0. Iterate across all generated
paragraph/clause indices and assert each corresponding code span, link, and
footnote reference is present in both source and wrapped document checks,
preserving the existing failure context.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3b447183-25d2-4747-a395-6f637d226f5b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.tomlMakefilebenches/wrap_observer.rsdocs/adrs/0006-observer-boundary-for-tracing.mddocs/architecture.mddocs/contents.mddocs/developers-guide.mdsrc/wrap.rssrc/wrap/bench_internals.rssrc/wrap/inline.rssrc/wrap/inline/fragment.rssrc/wrap/inline/normalize.rssrc/wrap/inline/postprocess.rssrc/wrap/inline/postprocess_tests.rssrc/wrap/inline/predicate_tracing_tests.rssrc/wrap/inline/predicates.rssrc/wrap/inline/span_helper_tracing_tests.rssrc/wrap/inline/span_helpers.rssrc/wrap/inline/tests.rssrc/wrap/inline/tracing_events.rssrc/wrap/inline/wrapping.rssrc/wrap/observer.rssrc/wrap/paragraph.rssrc/wrap/tokenize/mod.rssrc/wrap/tokenize/parsing.rssrc/wrap/tokenize/parsing_tests.rssrc/wrap/tracing_adapter.rstests/bench_fixtures.rstests/wrap_properties.rs
💤 Files with no reviewable changes (4)
- src/wrap/inline/predicate_tracing_tests.rs
- src/wrap/inline/tracing_events.rs
- src/wrap/inline/span_helper_tracing_tests.rs
- src/wrap/paragraph.rs
| The last two cases should measure the same: with tracing disabled, the derived | ||
| Unicode length counts and snippet truncation must stay inside `TracingObserver` | ||
| behind its `tracing::enabled!` gates, so the adapter adds no per-event work | ||
| beyond one branch. A widening gap between `observer_none` and | ||
| `tracing_observer_disabled` signals that derived-payload work has leaked back | ||
| onto the hot path. |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Describe the disabled-tracing benchmark overhead accurately.
observer_none avoids Observer::observe; tracing_observer_disabled still performs
dynamic dispatch, event matching, and a tracing::enabled! check per event. Require
no derived-payload work and bounded overhead, rather than equal timings.
🤖 Prompt for 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.
In `@docs/developers-guide.md` around lines 722 - 727, Update the benchmark
documentation in the comparison of observer_none and tracing_observer_disabled
to state that disabled tracing still incurs dynamic dispatch, event matching,
and a per-event tracing::enabled! check. Require that no derived-payload work
occurs and that overhead remains bounded, rather than claiming the two cases
must have equal timings.
| bench: ## Run the observer-boundary wrapping benchmarks | ||
| $(CARGO) bench --features bench-internals --bench wrap_observer | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make bench fail on warnings.
This target invokes cargo bench without -D warnings, so benchmark-only warnings can pass and the stated warnings-as-errors validation requirement is not enforced. Propagate RUSTFLAGS with -D warnings, or enforce the equivalent in CI.
Proposed fix
bench: ## Run the observer-boundary wrapping benchmarks
- $(CARGO) bench --features bench-internals --bench wrap_observer
+ RUSTFLAGS="$(RUSTFLAGS) -D warnings" \
+ $(CARGO) bench --features bench-internals --bench wrap_observer🤖 Prompt for 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.
In `@Makefile` around lines 22 - 24, Update the Makefile bench target’s cargo
invocation to propagate the existing RUSTFLAGS with -D warnings, ensuring
benchmark compilation fails on warnings while preserving the current features
and wrap_observer benchmark selection.
| } else if looks_like_footnote_ref(text, observer) | ||
| || looks_like_footnote_ref(trimmed, observer) | ||
| || ends_with_footnote_ref(text, observer) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Speculative predicate probes are handed the live observer, so one decision yields several events. looks_like_footnote_ref emits FootnoteRefChecked on every call, and both sites call it repeatedly on variants of the same token merely to reach a verdict. The result is a trace stream where record count depends on short-circuiting and scan order rather than on anything a reader cares about, plus needless dynamic dispatch per token. Observe decisions, not probes.
src/wrap/inline/fragment.rs#L167-L169: pass&mut Noneto the three footnote probes and let the existingFragmentClassifiedevent at Lines 54-59 be the single record for that fragment.src/wrap/inline/normalize.rs#L50-L54: pass&mut Noneto bothlooks_like_footnote_refcalls, so thefirst_matchscan and the rewrite loop stop reporting the same window twice.
📍 Affects 2 files
src/wrap/inline/fragment.rs#L167-L169(this comment)src/wrap/inline/normalize.rs#L50-L54
🤖 Prompt for 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.
In `@src/wrap/inline/fragment.rs` around lines 167 - 169, Pass &mut None to all
three looks_like_footnote_ref and ends_with_footnote_ref probes in
src/wrap/inline/fragment.rs lines 167-169, preserving FragmentClassified as the
sole event for the fragment. Also pass &mut None to both looks_like_footnote_ref
calls in src/wrap/inline/normalize.rs lines 50-54 so speculative scans do not
emit duplicate observer events.
| #[test] | ||
| fn footnote_ref_check_does_not_require_subscriber() { | ||
| assert!(looks_like_footnote_ref("[^1]", &mut None)); | ||
| assert!(!looks_like_footnote_ref("plain", &mut None)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test does not exercise the "no subscriber" case it claims.
&mut None skips the observer entirely, so nothing ever reaches tracing. The invariant advertised in the module doc — that the predicates behave with the adapter attached but no subscriber installed — needs Some(TracingObserver) outside #[traced_test]. Rename this one to reflect what it checks, and add the adapter-without-subscriber case.
🧪 Proposed test split
#[test]
- fn footnote_ref_check_does_not_require_subscriber() {
+ fn footnote_ref_check_works_without_observer() {
assert!(looks_like_footnote_ref("[^1]", &mut None));
assert!(!looks_like_footnote_ref("plain", &mut None));
}
+
+ #[test]
+ fn footnote_ref_check_does_not_require_subscriber() {
+ let mut observer = TracingObserver;
+ assert!(looks_like_footnote_ref("[^1]", &mut Some(&mut observer)));
+ assert!(!looks_like_footnote_ref("plain", &mut Some(&mut observer)));
+ }🤖 Prompt for 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.
In `@src/wrap/inline/predicates.rs` around lines 260 - 264, Rename
footnote_ref_check_does_not_require_subscriber to reflect that it only checks
the no-observer path. Add a separate test for the adapter-without-subscriber
scenario by constructing Some(TracingObserver) outside #[traced_test], then
assert looks_like_footnote_ref handles both footnote and plain inputs without a
subscriber.
| pub(in crate::wrap) fn wrap_preserving_code_observed( | ||
| text: &str, | ||
| width: usize, | ||
| observer: &mut ObserverHandle<'_>, | ||
| ) -> Vec<String> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Document wrap_preserving_code_observed.
Its sibling wrap_preserving_code carries a full contract description, yet the observer-threaded entry point — the one re-exported to benches under bench-internals and the subject of ADR 0006 — has no rustdoc at all. State the observer contract: events are emitted only when the handle is Some, and derived work is the adapter's responsibility.
As per coding guidelines, "Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc".
📝 Proposed doc comment
+/// Wraps inline Markdown `text` while forwarding classification events to
+/// `observer`.
+///
+/// `width` is measured in Unicode display columns. Events are emitted only
+/// when `observer` is `Some`; the handle carries no derived work, so any
+/// formatting or level gating belongs to the adapter behind it. Passing
+/// `&mut None` disables observation entirely and is the zero-overhead path
+/// exercised by the benchmarks.
pub(in crate::wrap) fn wrap_preserving_code_observed(
text: &str,
width: usize,
observer: &mut ObserverHandle<'_>,
) -> Vec<String> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(in crate::wrap) fn wrap_preserving_code_observed( | |
| text: &str, | |
| width: usize, | |
| observer: &mut ObserverHandle<'_>, | |
| ) -> Vec<String> { | |
| /// Wraps inline Markdown `text` while forwarding classification events to | |
| /// `observer`. | |
| /// | |
| /// `width` is measured in Unicode display columns. Events are emitted only | |
| /// when `observer` is `Some`; the handle carries no derived work, so any | |
| /// formatting or level gating belongs to the adapter behind it. Passing | |
| /// `&mut None` disables observation entirely and is the zero-overhead path | |
| /// exercised by the benchmarks. | |
| pub(in crate::wrap) fn wrap_preserving_code_observed( | |
| text: &str, | |
| width: usize, | |
| observer: &mut ObserverHandle<'_>, | |
| ) -> Vec<String> { |
🤖 Prompt for 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.
In `@src/wrap/inline/wrapping.rs` around lines 338 - 342, Add Rustdoc (`///`)
directly above `wrap_preserving_code_observed` documenting that events are
emitted only when the observer handle is `Some`, and that the adapter is
responsible for derived work. Match the contract detail and style of the sibling
`wrap_preserving_code` documentation.
Source: Coding guidelines
| Event::FragmentClassified { token, kind } | ||
| if tracing::enabled!(tracing::Level::DEBUG) => | ||
| { | ||
| let (snippet, truncated) = trace_text_snippet(token); | ||
| debug!(token = %snippet, truncated, kind = ?kind, "fragment classified"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Stop emitting raw Markdown through tracing.
Bounding a snippet limits volume, not sensitivity: arbitrary Markdown can contain
credentials or personal data, and debug events are commonly exported and retained.
src/wrap/tracing_adapter.rs#L62-L67: replacetoken = %snippetwith
non-sensitive metadata such astoken_length,kind, andtruncated.docs/adrs/0006-observer-boundary-for-tracing.md#L39-L46: remove the
bounded-snippet policy and document metadata-only diagnostics.docs/architecture.md#L625-L632: remove the bounded-snippet claim.docs/developers-guide.md#L678-L685: replace the raw-token logging example.docs/developers-guide.md#L753-L764: remove thetokenfield from the event
mapping.
📍 Affects 4 files
src/wrap/tracing_adapter.rs#L62-L67(this comment)docs/adrs/0006-observer-boundary-for-tracing.md#L39-L46docs/architecture.md#L625-L632docs/developers-guide.md#L678-L685docs/developers-guide.md#L753-L764
🤖 Prompt for 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.
In `@src/wrap/tracing_adapter.rs` around lines 62 - 67, Stop emitting Markdown
token contents from the Event::FragmentClassified tracing branch: replace the
token snippet field with non-sensitive metadata such as token_length, kind, and
truncated, and remove any now-unused snippet handling. Update
docs/adrs/0006-observer-boundary-for-tracing.md lines 39-46 to document
metadata-only diagnostics, docs/architecture.md lines 625-632 to remove the
bounded-snippet claim, docs/developers-guide.md lines 678-685 to replace the
raw-token example, and docs/developers-guide.md lines 753-764 to remove the
token field from the event mapping.
Source: Coding guidelines
| #[test] | ||
| fn realistic_document_fixture_covers_and_preserves_constructs() { | ||
| let document = realistic_markdown_document(); | ||
| let source = document.join("\n"); | ||
| assert_covers_link_code_footnote(&source, "the document fixture"); | ||
|
|
||
| let wrapped = wrap_text(&document, BENCH_WIDTH).join("\n"); | ||
| assert_covers_link_code_footnote(&wrapped, "the wrapped document"); | ||
| // The specific constructs survive wrapping intact. | ||
| assert!( | ||
| wrapped.contains("`inline-code-0`"), | ||
| "code span split: {wrapped}" | ||
| ); | ||
| assert!( | ||
| wrapped.contains("(https://example.com/crate/0)"), | ||
| "link split: {wrapped}" | ||
| ); | ||
| assert!(wrapped.contains("[^note0]"), "footnote split: {wrapped}"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert every generated construct, not only the first one.
The fixtures generate 60 paragraphs and 120 clauses, but Lines 40-48 and Lines 58-63 only verify the 0 item. A regression affecting later links, code spans, or footnote references will pass. Iterate over every generated index, or compare the complete expected construct set with the wrapped output in both tests.
Also applies to: 51-64
🤖 Prompt for 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.
In `@tests/bench_fixtures.rs` around lines 31 - 49, Update
realistic_document_fixture_covers_and_preserves_constructs to validate every
generated construct rather than only index 0. Iterate across all generated
paragraph/clause indices and assert each corresponding code span, link, and
footnote reference is present in both source and wrapped document checks,
preserving the existing failure context.
Source: Coding guidelines
2 participants
Summary
Closes #309
Validation
make check-fmtmake lintmake typecheckmake testcoderabbit review --agentReferences