Add batch 1 assess - #3
Conversation
📝 WalkthroughWalkthroughThe PR updates Markdown article triage to use weighted metadata and unit evidence, aligns component and unit serialization with schema expectations, changes DITA readiness to degrade on schema validation failure, adds pipeline progress reporting and validation timeout wiring, and expands unit, contract, and fixture coverage. ChangesClassifier, validation, and readiness changes
Design and planning documents
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)design/2026-06-28-b1-parse-assess-implementation-update.mdmarkdownlint-cli2 wrapper config was not available before execution 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
design/2025-06-27-Direction-and-Notes.md (6)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFilename date appears to be a year off.
The document is dated
2025-06-27but the PR was created on2026-06-28and the implementation update is2026-06-27. Verify whether this should be2026-06-27-Direction-and-Notes.mdto match the cohort and avoid confusion with stale documents.🤖 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 `@design/2025-06-27-Direction-and-Notes.md` at line 1, The document filename appears to use the wrong year, which can make it look stale or misaligned with the cohort. Rename the Markdown file from the 2025-dated version to the 2026-dated version so it matches the PR timeline and the implementation update, and make sure any references to this document use the corrected name consistently.
163-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePhase 4 pre-H2 introduction rule is implemented — verify alignment.
The rule described here matches the
UnitType.introductionbranch in_build_unit(headingless paragraphs without ordered lists). Confirm this section reflects the actual implementation or add a completion marker.🤖 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 `@design/2025-06-27-Direction-and-Notes.md` around lines 163 - 182, The Phase 4 note should match the actual `_build_unit` behavior for headingless pre-H2 content, but it currently reads like an implementation claim without confirmation. Verify the `UnitType.introduction` handling in `_build_unit` and either update the section text to reflect the real rule or add a clear completion marker showing it is implemented, especially around the headingless paragraph/list classification path.
185-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPhase 5 readiness reform partially implemented — clarify scope.
The
degradedstatus for schema validation failure is implemented inDitaReadinessEvaluator, but the broader readiness matrix (partial,blocked,not_attempted) and RAG chunk confidence metadata are not yet built. Mark completed vs. pending items to maintain document accuracy as the system evolves.🤖 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 `@design/2025-06-27-Direction-and-Notes.md` around lines 185 - 236, The Phase 5 readiness section is mixing implemented and not-yet-built behavior, so update the wording to clearly separate what `DitaReadinessEvaluator` already supports (`degraded` for schema-invalid cases) from the still-pending readiness matrix (`partial`, `blocked`, `not_attempted`) and RAG chunk confidence fields. Keep the document accurate by explicitly marking the broader status taxonomy and per-chunk metadata requirements as future work, while preserving the completed `degraded` scope already reflected in `DitaReadinessEvaluator`.
33-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPhase 1 proposes inspectability features not yet implemented.
The
inspect-triageCLI command and detailed JSON triage evidence output (confidence, scores, reasons) are described as deliverables, but the current implementation only exposes_MetadataEvidenceand_ArticleCandidateScoreas internal dataclasses without the rich debug output format shown. Consider adding a "Completed / Pending" marker to each phase so readers know the current implementation boundary.🤖 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 `@design/2025-06-27-Direction-and-Notes.md` around lines 33 - 84, The phase text describes inspect-triage and rich triage evidence as completed deliverables even though they are not implemented yet. Update the phase wording in this document to clearly mark the inspectability items as pending, and add an explicit Completed/Pending status for the phase so readers can distinguish implemented evidence types like MetadataEvidence and ArticleCandidateScore from planned CLI/output features such as structure-parser inspect-triage.
351-451: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMilestone sequence doesn't match actual implementation order.
The PR implemented Milestone 2 (conservative scorer), Milestone 3 (unit classifier expansion), and partial Milestone 5 (readiness degradation) before Milestone 1's inspectability layer. Consider reordering or adding cross-references to the actual PR changes so the roadmap doesn't mislead future implementers about dependencies.
🤖 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 `@design/2025-06-27-Direction-and-Notes.md` around lines 351 - 451, The milestone order in this roadmap does not match the actual dependency order in the implementation. Update the “Practical implementation sequence” section to reflect the real PR flow: reference the conservative scorer and unit classifier work before the honesty/inspectability layer only where appropriate, and add explicit dependency notes between ArticleTriageDecision, UnitEvidence, inspect-triage, and the readiness/status changes so future implementers are not misled. Keep the sequence aligned with the existing milestone labels and key symbols already named in the document.
86-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPhase 2 scoring policy is largely implemented — mark as done or in-progress.
The
min_score,min_margin, and topic fallback described here are active inclassifier.py(_MIN_MARGIN = 3,ArticleSignature.min_score,_score_article_type). However,dominance_ratiois not implemented. Clarify which sub-items are complete to prevent readers from expecting unbuilt features.🤖 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 `@design/2025-06-27-Direction-and-Notes.md` around lines 86 - 125, The Phase 2 scoring policy note is overstating what is implemented, since `classifier.py` already has `_MIN_MARGIN`, `ArticleSignature.min_score`, and `_score_article_type`, but `dominance_ratio` is still not present. Update the note to clearly separate completed items from pending ones, marking the implemented scoring and topic fallback pieces as done/in-progress and calling out `dominance_ratio` as not yet built. Use the existing symbols `ArticleSignature.min_score`, `_MIN_MARGIN`, and `_score_article_type` to anchor the clarification.
🤖 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/structure_parser/readiness/dita.py`:
- Line 21: The schema-validation prerequisite in DITA readiness is only
recording failures, so successful validation is indistinguishable from “never
run.” Update the logic around the DITA readiness tracking so the branch that
handles schema validation success in the readiness/parser flow (including the
code paths referenced by the DITA prerequisite handling) records a positive
result in TargetReadiness.prerequisites_met, while still keeping failures in
prerequisites_missing.
In `@src/structure_parser/structured_markdown/classifier.py`:
- Around line 178-184: The precedence-ranked metadata key collections used by
_infer_article_type_from_metadata are unordered, so first-match selection can
vary across runs. Replace _AUTHORITATIVE_METADATA_KEYS and _SECONDARY_EXACT_KEYS
in structured_markdown/classifier.py with ordered tuples (keeping the intended
key order) and make sure the lookup logic preserves that order when scanning for
article-type metadata.
---
Nitpick comments:
In `@design/2025-06-27-Direction-and-Notes.md`:
- Line 1: The document filename appears to use the wrong year, which can make it
look stale or misaligned with the cohort. Rename the Markdown file from the
2025-dated version to the 2026-dated version so it matches the PR timeline and
the implementation update, and make sure any references to this document use the
corrected name consistently.
- Around line 163-182: The Phase 4 note should match the actual `_build_unit`
behavior for headingless pre-H2 content, but it currently reads like an
implementation claim without confirmation. Verify the `UnitType.introduction`
handling in `_build_unit` and either update the section text to reflect the real
rule or add a clear completion marker showing it is implemented, especially
around the headingless paragraph/list classification path.
- Around line 185-236: The Phase 5 readiness section is mixing implemented and
not-yet-built behavior, so update the wording to clearly separate what
`DitaReadinessEvaluator` already supports (`degraded` for schema-invalid cases)
from the still-pending readiness matrix (`partial`, `blocked`, `not_attempted`)
and RAG chunk confidence fields. Keep the document accurate by explicitly
marking the broader status taxonomy and per-chunk metadata requirements as
future work, while preserving the completed `degraded` scope already reflected
in `DitaReadinessEvaluator`.
- Around line 33-84: The phase text describes inspect-triage and rich triage
evidence as completed deliverables even though they are not implemented yet.
Update the phase wording in this document to clearly mark the inspectability
items as pending, and add an explicit Completed/Pending status for the phase so
readers can distinguish implemented evidence types like MetadataEvidence and
ArticleCandidateScore from planned CLI/output features such as structure-parser
inspect-triage.
- Around line 351-451: The milestone order in this roadmap does not match the
actual dependency order in the implementation. Update the “Practical
implementation sequence” section to reflect the real PR flow: reference the
conservative scorer and unit classifier work before the honesty/inspectability
layer only where appropriate, and add explicit dependency notes between
ArticleTriageDecision, UnitEvidence, inspect-triage, and the readiness/status
changes so future implementers are not misled. Keep the sequence aligned with
the existing milestone labels and key symbols already named in the document.
- Around line 86-125: The Phase 2 scoring policy note is overstating what is
implemented, since `classifier.py` already has `_MIN_MARGIN`,
`ArticleSignature.min_score`, and `_score_article_type`, but `dominance_ratio`
is still not present. Update the note to clearly separate completed items from
pending ones, marking the implemented scoring and topic fallback pieces as
done/in-progress and calling out `dominance_ratio` as not yet built. Use the
existing symbols `ArticleSignature.min_score`, `_MIN_MARGIN`, and
`_score_article_type` to anchor the clarification.
🪄 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
Run ID: 497a9f75-1781-4c8b-84e4-31ed1ec3edb0
📒 Files selected for processing (12)
design/2025-06-27-Direction-and-Notes.mddesign/2026-06-27-parse-assess-implementation-update.mddesign/2026-06-28-assess/assess.mddesign/2026-06-28-assess/azure-stack-acs-differences.md.jsondesign/2026-06-28-assess/azure-stack-arm-templates.md.jsondesign/2026-06-28-assess/azure-stack-compute-overview.md.jsondesign/2026-06-28-assess/azure-stack-connect-azure-stack.md.jsondesign/2026-06-28-assess/azure-stack-connect-vpn.md.jsondesign/2026-06-28-assess/azure-stack-considerations.md.jsonsrc/structure_parser/readiness/dita.pysrc/structure_parser/structured_markdown/classifier.pytests/unit/test_structured_markdown_classifier.py
| - Document must have an H1 title. | ||
| - Article type must be classified (not "unknown"). | ||
| - DITA type mapping must be present. | ||
| - Schema validation must pass (failure → degraded, not blocked). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Record successful schema validation when it is present.
The new branch only writes the failure case into prerequisites_missing. When validation succeeds, TargetReadiness.prerequisites_met still looks the same as "validation was never run", even though Line 21 now treats this as a tracked prerequisite.
Proposed fix
- if doc.validation is not None and not doc.validation.valid:
- missing.append(
- "Schema validation failed; DITA output may not conform to article schema"
- )
+ if doc.validation is not None:
+ if doc.validation.valid:
+ met.append("Schema validation passed")
+ else:
+ missing.append(
+ "Schema validation failed; DITA output may not conform to article schema"
+ )Also applies to: 48-55
🤖 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/structure_parser/readiness/dita.py` at line 21, The schema-validation
prerequisite in DITA readiness is only recording failures, so successful
validation is indistinguishable from “never run.” Update the logic around the
DITA readiness tracking so the branch that handles schema validation success in
the readiness/parser flow (including the code paths referenced by the DITA
prerequisite handling) records a positive result in
TargetReadiness.prerequisites_met, while still keeping failures in
prerequisites_missing.
| # Keys whose values are treated as direct author declarations (weight 10). | ||
| _AUTHORITATIVE_METADATA_KEYS: frozenset[str] = frozenset({"articleType", "article_type"}) | ||
|
|
||
| # Additional exact keys whose values are treated as secondary evidence (weight 6). | ||
| _SECONDARY_EXACT_KEYS: frozenset[str] = frozenset({ | ||
| "type", "topic", "topic_type", "content_type", "document_type", "information_type", | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Python frozenset iteration order strings PYTHONHASHSEED determinism
💡 Result:
In Python, the iteration order of a frozenset is not guaranteed to be deterministic [1][2]. Frozensets, like sets, are unordered collections based on hash tables [2], and their internal organization depends on the hash values of their elements [1][3]. Because Python enables hash randomization (also known as "salting") by default for certain types—including strings, bytes, and datetime objects—the hash values for these types change between different invocations of the Python interpreter [1][2]. This randomization is a security feature designed to prevent algorithmic complexity denial-of-service attacks [1][2]. As a result, the iteration order of a frozenset containing such types can vary from one execution of a program to the next [1][3]. You can control this behavior using the PYTHONHASHSEED environment variable: 1. Determinism across runs: By setting PYTHONHASHSEED to a fixed integer value (e.g., export PYTHONHASHSEED=0), you can force the hash seed to be constant across different runs of the same Python process [1][3]. While this makes the iteration order consistent for a given set of elements, the order remains implementation-dependent and should not be relied upon as a stable property [1][3]. 2. Limitations: Setting PYTHONHASHSEED does not guarantee a specific order; it only ensures that the hash-based ordering remains the same if the hash function and set insertion sequence remain identical [3][4]. If your elements include objects that use identity-based hashing (the default for user-defined classes), the order will still vary across runs even with a fixed hash seed because memory addresses change [4]. Note that while frozenset iteration order remains inherently non-deterministic by design [2], recent updates to Python have improved the determinism of frozenset serialization (marshalling) for the purpose of creating reproducible bytecode (pyc files) [5][6]. However, this does not change the runtime iteration behavior of the object itself [6].
Citations:
- 1: https://stackoverflow.com/questions/52768966/is-iterating-over-the-same-frozenset-guaranteed-always-to-produce-the-items-in-t
- 2: https://stackoverflow.com/questions/3812429/is-pythons-set-stable
- 3: https://stackoverflow.com/questions/3848091/set-iteration-order-varies-from-run-to-run
- 4: https://stackoverflow.com/questions/51948243/why-is-the-order-of-python-sets-not-deterministic-even-when-pythonhashseed-0
- 5: bpo-37596: Make
setandfrozensetmarshalling deterministic python/cpython#27926 - 6: Reproducible pyc: frozenset is not serialized in a deterministic order python/cpython#81777
Use ordered containers for precedence-ranked metadata keys.
_AUTHORITATIVE_METADATA_KEYS and _SECONDARY_EXACT_KEYS are iterated to pick the first matching key, but frozenset iteration over strings is hash-seed dependent. That makes _infer_article_type_from_metadata choose different conflicting metadata values across runs. Switch these to ordered tuples so precedence is deterministic.
🤖 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/structure_parser/structured_markdown/classifier.py` around lines 178 -
184, The precedence-ranked metadata key collections used by
_infer_article_type_from_metadata are unordered, so first-match selection can
vary across runs. Replace _AUTHORITATIVE_METADATA_KEYS and _SECONDARY_EXACT_KEYS
in structured_markdown/classifier.py with ordered tuples (keeping the intended
key order) and make sure the lookup logic preserves that order when scanning for
article-type metadata.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/structure_parser/structured_markdown/component_mapper.py`:
- Around line 73-89: The blockquote/alert mapping in component_mapper is
building markdown only from paragraph text, so nested lists, code blocks, and
other child nodes are lost even though they remain in content. Update the
mapping logic in the alert and blockquote branches of the component creation
code to derive markdown from the already-mapped child components or the original
source slice instead of joining only paragraph content, and remove the synthetic
"> ..." fallback so markdown stays aligned with the actual nested structure.
In `@src/structure_parser/validation/model_validator.py`:
- Around line 130-135: Serialization in `_to_schema_dict()` is dropping empty
required `markdown` values because it uses a truthy check, which removes valid
empty-string fields for components like table cells. Update the
`model_validator` serialization logic to preserve `markdown` when it is `""` by
checking for `None` instead, and keep the existing behavior for missing values.
Use `_to_schema_dict()`, `unit.term`, and the `markdown` field assignment as the
key places to update.
In `@tests/contract/test_unit_schema_contract.py`:
- Around line 176-185: The procedure-dominance assertion in
test_multiple_procedure_units_is_howto is using clean.md, which is also an
authoritative-metadata fixture and can mask regressions in unit-evidence
scoring. Update this test to use a metadata-free fixture in
test_unit_schema_contract.py so ArticleType.howto is asserted based on procedure
units alone, and keep the check tied to parse_file, sc.structured_content, and
UnitType.procedure.
🪄 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
Run ID: 73c44328-b8c6-4e6b-ae24-7bd0bf65d9b3
📒 Files selected for processing (14)
.gitignoredesign/2026-06-27-Direction-and-Notes.mddesign/2026-06-28-Future-Implementation-Tasks.mdmodel/articles/units/unitIntroduction.schema.jsonsrc/structure_parser/resources/model/articles/units/unitIntroduction.schema.jsonsrc/structure_parser/structured_markdown/attribute_mapper.pysrc/structure_parser/structured_markdown/classifier.pysrc/structure_parser/structured_markdown/component_mapper.pysrc/structure_parser/validation/model_validator.pytests/contract/test_schema_round_trip.pytests/contract/test_unit_schema_contract.pytests/fixtures/markdown/nextstep_only.mdtests/fixtures/markdown/procedure_unit.mdtests/fixtures/markdown/topic_mixed.md
💤 Files with no reviewable changes (1)
- design/2026-06-27-Direction-and-Notes.md
✅ Files skipped from review due to trivial changes (3)
- tests/fixtures/markdown/topic_mixed.md
- tests/fixtures/markdown/nextstep_only.md
- src/structure_parser/structured_markdown/attribute_mapper.py
| inner = "\n".join( | ||
| f"> {c.content}" for c in node.children if c.node_type == "paragraph" and c.content | ||
| ) | ||
| md = f"> [!{alert_type.upper()}]\n{inner}" if inner else f"> [!{alert_type.upper()}]" | ||
| return Component( | ||
| component_type=ComponentType.compAlert, | ||
| alert_type=alert_type, | ||
| markdown=md, | ||
| source=span, | ||
| content=children_comps, | ||
| ) | ||
| inner = "\n".join( | ||
| f"> {c.content}" for c in node.children if c.node_type == "paragraph" and c.content | ||
| ) | ||
| return Component( | ||
| component_type=ComponentType.compBlockQuote, | ||
| markdown=inner or "> ...", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Keep markdown faithful to nested quote/list content.
Line 73 and Line 172 only derive markdown from paragraph/inline text, so nested lists or code blocks are preserved in content but disappear from markdown. The "> ..." fallback on Line 89 also invents content for non-paragraph blockquotes. Build markdown from the mapped children (or the raw source slice) so the two representations stay in sync.
Also applies to: 167-175
🤖 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/structure_parser/structured_markdown/component_mapper.py` around lines 73
- 89, The blockquote/alert mapping in component_mapper is building markdown only
from paragraph text, so nested lists, code blocks, and other child nodes are
lost even though they remain in content. Update the mapping logic in the alert
and blockquote branches of the component creation code to derive markdown from
the already-mapped child components or the original source slice instead of
joining only paragraph content, and remove the synthetic "> ..." fallback so
markdown stays aligned with the actual nested structure.
| if unit.procedure_representation is not None: | ||
| u_dict["procedureRepresentation"] = unit.procedure_representation.value | ||
| if unit.term: | ||
| u_dict["term"] = unit.term | ||
|
|
||
| comps = [] | ||
| for comp in unit.content: | ||
| c_dict: dict = {"componentType": comp.component_type.value} | ||
| if comp.markdown: | ||
| c_dict["markdown"] = comp.markdown | ||
| if comp.text: | ||
| c_dict["text"] = comp.text | ||
| comps.append(c_dict) | ||
| u_dict["content"] = comps | ||
| u_dict["content"] = [_component_to_dict(c) for c in unit.content] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve empty required markdown fields during serialization.
Line 173 drops markdown whenever its value is "". That breaks the new mapper behavior for empty-but-valid components such as table cells, where the schema still requires a markdown property. Use a None check here so _to_schema_dict() stops stripping required fields.
Suggested fix
- if c.markdown:
+ if c.markdown is not None:
d["markdown"] = c.markdownAlso applies to: 173-175
🤖 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/structure_parser/validation/model_validator.py` around lines 130 - 135,
Serialization in `_to_schema_dict()` is dropping empty required `markdown`
values because it uses a truthy check, which removes valid empty-string fields
for components like table cells. Update the `model_validator` serialization
logic to preserve `markdown` when it is `""` by checking for `None` instead, and
keep the existing behavior for missing values. Use `_to_schema_dict()`,
`unit.term`, and the `markdown` field assignment as the key places to update.
| def test_multiple_procedure_units_is_howto(self) -> None: | ||
| """A document with multiple ordered-list procedure units should be howto.""" | ||
| doc = parse_file(_MD / "clean.md") | ||
| sc = doc.structured_content | ||
| assert sc is not None | ||
| proc_count = sum(1 for u in sc.content if u.unit_type == UnitType.procedure) | ||
| assert proc_count >= 1, "clean.md should have at least one procedure unit" | ||
| assert sc.article_type == ArticleType.howto, ( | ||
| f"clean.md with procedure units should be howto, got {sc.article_type.value!r}" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a metadata-free fixture for the procedure-dominance assertion.
Lines 178-185 parse clean.md, but this file is also treated below as an authoritative-metadata howto fixture. That means this test still passes if unit-evidence scoring regresses and metadata alone forces ArticleType.howto, so it does not actually verify conservative howto selection.
Suggested test tightening
def test_multiple_procedure_units_is_howto(self) -> None:
"""A document with multiple ordered-list procedure units should be howto."""
- doc = parse_file(_MD / "clean.md")
+ doc = parse_file(_MD / "procedure_dominant.md") # no authoritative articleType metadata
sc = doc.structured_content
assert sc is not None
proc_count = sum(1 for u in sc.content if u.unit_type == UnitType.procedure)
- assert proc_count >= 1, "clean.md should have at least one procedure unit"
+ assert proc_count >= 2, "procedure_dominant.md should have multiple procedure units"
assert sc.article_type == ArticleType.howto, (
- f"clean.md with procedure units should be howto, got {sc.article_type.value!r}"
+ f"procedure_dominant.md should be howto, got {sc.article_type.value!r}"
)📝 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.
| def test_multiple_procedure_units_is_howto(self) -> None: | |
| """A document with multiple ordered-list procedure units should be howto.""" | |
| doc = parse_file(_MD / "clean.md") | |
| sc = doc.structured_content | |
| assert sc is not None | |
| proc_count = sum(1 for u in sc.content if u.unit_type == UnitType.procedure) | |
| assert proc_count >= 1, "clean.md should have at least one procedure unit" | |
| assert sc.article_type == ArticleType.howto, ( | |
| f"clean.md with procedure units should be howto, got {sc.article_type.value!r}" | |
| ) | |
| def test_multiple_procedure_units_is_howto(self) -> None: | |
| """A document with multiple ordered-list procedure units should be howto.""" | |
| doc = parse_file(_MD / "procedure_dominant.md") # no authoritative articleType metadata | |
| sc = doc.structured_content | |
| assert sc is not None | |
| proc_count = sum(1 for u in sc.content if u.unit_type == UnitType.procedure) | |
| assert proc_count >= 2, "procedure_dominant.md should have multiple procedure units" | |
| assert sc.article_type == ArticleType.howto, ( | |
| f"procedure_dominant.md should be howto, got {sc.article_type.value!r}" | |
| ) |
🤖 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/contract/test_unit_schema_contract.py` around lines 176 - 185, The
procedure-dominance assertion in test_multiple_procedure_units_is_howto is using
clean.md, which is also an authoritative-metadata fixture and can mask
regressions in unit-evidence scoring. Update this test to use a metadata-free
fixture in test_unit_schema_contract.py so ArticleType.howto is asserted based
on procedure units alone, and keep the check tied to parse_file,
sc.structured_content, and UnitType.procedure.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/structure_parser/cli.py`:
- Around line 48-49: The _pipeline_config helper is forcing
enable_model_validation to False for every pipe run, which prevents downstream
validation state from being populated. Update _pipeline_config to preserve the
validation setting from _config(debug) instead of overriding it, so
semantic_enricher.py can set doc.validation and readiness/dita.py can correctly
report schema-validation readiness. Use the _pipeline_config and _config symbols
to locate and adjust the pipeline configuration flow.
In `@src/structure_parser/contracts/config.py`:
- Around line 26-32: The `schema_validation_timeout_seconds` config field
currently allows non-positive values to slip through and disable the timeout in
`_schema_validation_timer()`. Update the config boundary in `config.py` for
`schema_validation_timeout_seconds` so only `None` is treated as unbounded and
any `0` or negative value is rejected or normalized before validation begins.
Keep the fix localized to the config field or its validation logic so downstream
schema validation never receives non-positive timeout values.
In `@src/structure_parser/validation/schema_validator.py`:
- Around line 5-6: Guard the SIGALRM timer setup in schema_validator so it only
runs on the main thread, since signal.signal() and setitimer() raise ValueError
in worker threads and currently surface as schema-validation failures. Update
the validation flow around the signal-based timeout logic to check for the main
thread before enabling the timer, and skip that timeout path entirely when not
on the main thread.
🪄 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
Run ID: 1665ea93-2df7-4508-ba1d-fea135e25418
📒 Files selected for processing (8)
design/2026-06-28-b1-parse-assess-implementation-update.mdsrc/structure_parser/application/commands.pysrc/structure_parser/cli.pysrc/structure_parser/contracts/config.pysrc/structure_parser/enrichment/semantic_enricher.pysrc/structure_parser/pipeline/orchestrator.pysrc/structure_parser/validation/model_validator.pysrc/structure_parser/validation/schema_validator.py
| def _pipeline_config(debug: bool = False) -> ParserConfig: | ||
| return _config(debug).model_copy(update={"enable_model_validation": False}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't disable model validation for pipe.
This hard-sets enable_model_validation=False for every pipeline run. Downstream, semantic_enricher.py leaves doc.validation unset when validation is disabled, and readiness/dita.py only degrades on a failed validation result, so the pipe path can no longer reflect schema-validation readiness at all.
Suggested fix
def _pipeline_config(debug: bool = False) -> ParserConfig:
- return _config(debug).model_copy(update={"enable_model_validation": False})
+ return _config(debug)📝 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.
| def _pipeline_config(debug: bool = False) -> ParserConfig: | |
| return _config(debug).model_copy(update={"enable_model_validation": False}) | |
| def _pipeline_config(debug: bool = False) -> ParserConfig: | |
| return _config(debug) |
🤖 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/structure_parser/cli.py` around lines 48 - 49, The _pipeline_config
helper is forcing enable_model_validation to False for every pipe run, which
prevents downstream validation state from being populated. Update
_pipeline_config to preserve the validation setting from _config(debug) instead
of overriding it, so semantic_enricher.py can set doc.validation and
readiness/dita.py can correctly report schema-validation readiness. Use the
_pipeline_config and _config symbols to locate and adjust the pipeline
configuration flow.
| schema_validation_timeout_seconds: int | None = Field( | ||
| default=5, | ||
| description=( | ||
| "Maximum seconds for advisory runtime schema validation. " | ||
| "Set to None to allow unbounded validation." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject non-positive timeout values at the config boundary.
schema_validator._schema_validation_timer() treats <= 0 as “no timeout”, so 0 or -1 silently disable the cap even though this field says only None should do that. Please enforce > 0 here or normalize non-positive values before they reach validation.
🤖 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/structure_parser/contracts/config.py` around lines 26 - 32, The
`schema_validation_timeout_seconds` config field currently allows non-positive
values to slip through and disable the timeout in `_schema_validation_timer()`.
Update the config boundary in `config.py` for
`schema_validation_timeout_seconds` so only `None` is treated as unbounded and
any `0` or negative value is rejected or normalized before validation begins.
Keep the fix localized to the config field or its validation logic so downstream
schema validation never receives non-positive timeout values.
| import signal | ||
| from contextlib import contextmanager |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline src/structure_parser/validation/schema_validator.py --view expanded || true
printf '\n== Relevant file slice ==\n'
sed -n '1,140p' src/structure_parser/validation/schema_validator.py
printf '\n== Search for validate_against_schema usage ==\n'
rg -n "validate_against_schema|_schema_validation_timer|setitimer|SIGALRM|signal\.signal" src tests || true
printf '\n== Threaded signal semantics probe (read-only) ==\n'
python3 - <<'PY'
import signal
import threading
print("has_SIGALRM=", hasattr(signal, "SIGALRM"))
print("main_thread=", threading.current_thread() is threading.main_thread())
def probe():
try:
signal.signal(signal.SIGALRM, lambda *_: None)
print("worker_signal_ok=True")
except Exception as e:
print("worker_signal_ok=False", type(e).__name__, str(e))
t = threading.Thread(target=probe)
t.start()
t.join()
PYRepository: mattbriggs/structured-markdown
Length of output: 7979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '136,180p' src/structure_parser/validation/schema_validator.pyRepository: mattbriggs/structured-markdown
Length of output: 1903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Call sites ==\n'
rg -n "validate_against_schema\(" src tests || true
printf '\n== Threading / executor usage near validation paths ==\n'
rg -n "threading|ThreadPool|ProcessPool|executor|asyncio|to_thread|run_in_executor" src tests || trueRepository: mattbriggs/structured-markdown
Length of output: 630
Skip SIGALRM outside the main thread. signal.signal() / setitimer() raise ValueError in worker threads, and this path turns that into a schema-validation failure. Add a main-thread guard before enabling the timer.
🤖 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/structure_parser/validation/schema_validator.py` around lines 5 - 6,
Guard the SIGALRM timer setup in schema_validator so it only runs on the main
thread, since signal.signal() and setitimer() raise ValueError in worker threads
and currently surface as schema-validation failures. Update the validation flow
around the signal-based timeout logic to check for the main thread before
enabling the timer, and skip that timeout path entirely when not on the main
thread.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests