Add pipeline ability - #1
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a complete folder-based ChangesRepository Pipeline
Article Triage, Classifier, and Docs
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)
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: 12
🧹 Nitpick comments (2)
design/2026-06-27-tech-note-article-triage-note.md (2)
516-533: 📐 Maintainability & Code Quality | 🔵 TrivialMetadataEvidence missing
schema_namefor future evidence contract.Section 5.9's evidence JSON includes
"schema": "artHowto.schema.json", butMetadataEvidence(section 12.2) only hasschema_exists: bool. When the evidence contract is later implemented (section 12.9), the schema filename will be needed. Addschema_name: str | NonetoMetadataEvidencenow to avoid a breaking change later.Also applies to: 292-316
🤖 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/2026-06-27-tech-note-article-triage-note.md` around lines 516 - 533, MetadataEvidence is missing the future schema filename field needed by the evidence contract, so update the MetadataEvidence dataclass to include schema_name: str | None alongside schema_exists. Keep the new field aligned with the existing evidence shape and use the existing symbols MetadataEvidence and TriageDecision as the place to make the change so later JSON evidence can carry the schema name without a breaking contract change.
762-791: 📐 Maintainability & Code Quality | 🔵 TrivialTest plan missing explicit coverage for reconciliation cases 2 and 6.
The fixture table covers cases 1 (implicitly), 3 (
metadata_construction_conflict), 4/5/7 (unknown_fallback,construction_only_*), and 6/7 (metadata_invalid_value). Missing:
- Case 2: Metadata valid, construction weak — e.g.,
articleType: howtowith minimal structure where construction scores don't clear threshold.- Case 6 explicit: Invalid metadata with strong construction —
metadata_invalid_value.mdmay cover this if structure is strong, but the expected outcome says "construction inference or unknown" which is ambiguous.Add explicit fixtures for cases 2 and 6, and ensure the reconciliation unit tests parametrize all 7 cases with exact threshold boundaries (e.g., metadata confidence 0.80, construction winner 0.59, gap 0.19).
🤖 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/2026-06-27-tech-note-article-triage-note.md` around lines 762 - 791, The test plan is missing explicit coverage for reconciliation cases 2 and 6, so add dedicated fixtures and assertions to make those paths unambiguous. Update the fixture table in the test plan with a case-2 example where valid metadata like articleType=howto has weak construction and should keep metadata-driven triage, and a case-6 example where invalid metadata is overridden by strong construction with an exact expected outcome. Also expand tests such as test_triage_reconciliation and test_metadata_triage to parametrize all seven reconciliation cases with clear threshold-boundary expectations, using the existing diagnostic symbols SP-043, SP-044, SP-045, and SP-046 rather than relying on vague fallback behavior.
🤖 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 `@design/2026-06-27-tech-note-article-triage-note.md`:
- Around line 265-273: The conflict-resolution rules are inconsistent between
the summary table and the detailed case in the article, especially around the
metadata-vs-construction decision logic. Update the section that defines the
“metadata valid, construction strongly disagrees” path so it matches the policy
in the table by explicitly stating how to choose the stronger evidence, and
align the detailed case logic in the relevant section with that rule instead of
always preferring construction when the gap threshold is met. Use the section
headings and the conflict-resolution case descriptions as the primary symbols to
locate and reconcile the mismatched guidance.
In `@src/structure_parser/application/commands.py`:
- Around line 221-223: The CSV export path in the command flow is treating
CsvInventoryReporter.write() as if it cannot fail, so report success is logged
even when the write returns PIPE_003. Update the logic around report_path,
CsvInventoryReporter.write(), and _log_report_written() to check the write
result first, and only print/log success when the write succeeds; if it fails,
propagate or return the error instead of exiting 0.
In `@src/structure_parser/cli.py`:
- Around line 156-158: The CLI option for log formatting currently accepts any
string, which lets invalid values silently fall back to text in
_add_log_file_handler(). Update the Typer option on the log_format parameter in
the CLI entrypoint to validate only the supported values (text and jsonl), so
unsupported inputs are rejected at the command boundary instead of being treated
as text.
In `@src/structure_parser/contracts/pipeline.py`:
- Around line 81-90: PipelineConfig currently allows an empty inputs list, which
lets invalid Python callers proceed into discovery instead of failing at
validation time. Update the PipelineConfig model in the pipeline contract to
enforce at least one input on the inputs field, using the existing field
definition so empty lists are rejected before any run begins. Keep the change
localized to the PipelineConfig inputs declaration and align it with the CLI
contract.
In `@src/structure_parser/pipeline/discovery.py`:
- Around line 79-98: Explicit file inputs are being accepted without verifying
they are Markdown, so _add_file() can emit non-Markdown paths like notes.txt as
SourceFormat.markdown. Update discovery._add_file to apply the same
_MARKDOWN_SUFFIXES gate used by folder discovery before creating
DiscoveredSource, and only append when the path matches a Markdown suffix in
addition to the existing include/exclude checks.
In `@src/structure_parser/pipeline/orchestrator.py`:
- Around line 75-85: The PIPE-007 early return in the orchestrator is dropping
discovery and run-diagnostic totals because _make_run_result() only derives
aggregates from files. Update the early-abort path and _make_run_result() so
discovered_count still reflects the parsed sources and run-level diagnostics
still contribute to error_count/warning_count, then ensure PipelineCommand.run()
summary uses those preserved counts for duplicate/overlap/missing-input aborts.
In `@src/structure_parser/pipeline/output.py`:
- Around line 108-123: Duplicate-target detection in the output pipeline is
using the raw Path as the seen key, so case-only differences can bypass PIPE-007
on case-insensitive filesystems. Update the duplicate check in
ParsedDocumentWriter/its target collection logic to normalize target keys using
the output filesystem’s case rules before storing or comparing in seen, and keep
the existing error logging and duplicates list behavior. Add a test around the
duplicate-target detection path that uses case-variant source names to verify
the collision is caught.
In `@src/structure_parser/pipeline/reporting.py`:
- Around line 30-70: The report write failure path in write() is currently easy
to miss because it returns PIPE_003, but the caller in the command flow ignores
that status and still reports success. Update write() and the surrounding
reporting flow so a failed report write is surfaced as a real failure: either
raise from InventoryReportWriter.write or have the caller check the PIPE_003
return, add a run diagnostic, and fail the command with a non-zero exit. Make
sure the fix is applied consistently in InventoryReportWriter.write and the
command handler that invokes it.
In `@tests/integration/test_pipeline_error_inventory.py`:
- Around line 61-62: The integration test assertion in
test_pipeline_error_inventory should verify parser_codes directly instead of
allowing a fallback on status == "parsed". Update the check in the test around
rows[0]["parser_codes"] so it explicitly fails when parser diagnostics are
missing, keeping the assertion tied to the parser_codes field and the test's
intended regression coverage.
In `@tests/unit/test_cli_pipeline_command.py`:
- Around line 63-71: The strict-mode test in test_exit_1_strict_with_warnings is
currently non-deterministic because it accepts either exit code, so it does not
verify PipelineCommand.run()’s strict branch. Make the warning input
deterministic by stubbing the orchestrator result or equivalent in the test
setup, then assert that the strict path returns code 1 when warnings are
present. Use the existing _run helper and PipelineCommand.run as the key symbols
to update the test.
In `@tests/unit/test_pipeline_orchestrator.py`:
- Around line 135-145: The test currently only covers a fully valid discovery
path, so it never proves failure isolation in PipelineOrchestrator.run. Update
test_one_failure_does_not_stop_others to force one parse/write failure by
monkeypatching structure_parser.pipeline.orchestrator.parse_one (or the writer)
while keeping a second valid file in the input set, then assert the good file
still gets processed and stats.discovered_count reflects both discovery and
continued execution.
- Around line 61-62: The assertion in the pipeline orchestrator test is too weak
because it only checks one parsed file, so update the test around result.stats
in test_pipeline_orchestrator to explicitly verify both discovered files are
accounted for. Replace the duplicated parsed_count check with a condition that
reflects two successful parses or otherwise compares parsed_count against
discovered_count so the test fails if either file is not parsed.
---
Nitpick comments:
In `@design/2026-06-27-tech-note-article-triage-note.md`:
- Around line 516-533: MetadataEvidence is missing the future schema filename
field needed by the evidence contract, so update the MetadataEvidence dataclass
to include schema_name: str | None alongside schema_exists. Keep the new field
aligned with the existing evidence shape and use the existing symbols
MetadataEvidence and TriageDecision as the place to make the change so later
JSON evidence can carry the schema name without a breaking contract change.
- Around line 762-791: The test plan is missing explicit coverage for
reconciliation cases 2 and 6, so add dedicated fixtures and assertions to make
those paths unambiguous. Update the fixture table in the test plan with a case-2
example where valid metadata like articleType=howto has weak construction and
should keep metadata-driven triage, and a case-6 example where invalid metadata
is overridden by strong construction with an exact expected outcome. Also expand
tests such as test_triage_reconciliation and test_metadata_triage to parametrize
all seven reconciliation cases with clear threshold-boundary expectations, using
the existing diagnostic symbols SP-043, SP-044, SP-045, and SP-046 rather than
relying on vague fallback behavior.
🪄 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: ebcbf97b-700a-4775-9ec8-45685789d20a
📒 Files selected for processing (24)
design/2026-06-27-pipe-the-pipeline-SRS.mddesign/2026-06-27-tech-note-article-triage-note.mdsrc/structure_parser/application/commands.pysrc/structure_parser/cli.pysrc/structure_parser/contracts/pipeline.pysrc/structure_parser/pipeline/__init__.pysrc/structure_parser/pipeline/discovery.pysrc/structure_parser/pipeline/orchestrator.pysrc/structure_parser/pipeline/output.pysrc/structure_parser/pipeline/reporting.pytests/fixtures/content_repo/guide/configure.mdtests/fixtures/content_repo/guide/install.mdtests/fixtures/content_repo/index.mdtests/fixtures/content_repo/reference/api.mdtests/integration/test_pipeline_cli.pytests/integration/test_pipeline_dry_run.pytests/integration/test_pipeline_end_to_end.pytests/integration/test_pipeline_error_inventory.pytests/unit/test_cli_pipeline_command.pytests/unit/test_pipeline_contracts.pytests/unit/test_pipeline_discovery.pytests/unit/test_pipeline_orchestrator.pytests/unit/test_pipeline_output.pytests/unit/test_pipeline_reporting.py
Comment on lines
+265
to
+273
| | Case | Behavior | | ||
| |---|---| | ||
| | Metadata valid, construction agrees | Select metadata type with high confidence. | | ||
| | Metadata valid, construction weak | Select metadata type with medium confidence and note weak construction evidence. | | ||
| | Metadata valid, construction strongly disagrees | Select the stronger evidence according to policy and emit a conflict diagnostic. | | ||
| | Metadata missing, construction strong | Select construction-inferred type. | | ||
| | Metadata missing, construction weak | Fall back to `topic` or `unknown` based on project policy. | | ||
| | Metadata invalid, construction strong | Select construction-inferred type and emit invalid metadata diagnostic. | | ||
| | Metadata invalid, construction weak | Fall back and emit both metadata and unknown/low-confidence diagnostics. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Inconsistent conflict resolution between section 5.8 and 12.6.
Section 5.8 states: "Metadata valid, construction strongly disagrees → Select the stronger evidence according to policy and emit a conflict diagnostic."
Section 12.6 case 3 hardcodes: always select construction type when gap > threshold, regardless of whether metadata confidence is higher.
If metadata confidence is 1.0 (exact match, schema exists) and construction score is 0.65 with gap 0.25, the design says pick construction (0.65) over metadata (1.0). This contradicts "select the stronger evidence according to policy." Clarify whether the gap threshold should override metadata confidence, or whether case 3 should compare absolute scores and pick the stronger evidence.
Also applies to: 676-684
🤖 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/2026-06-27-tech-note-article-triage-note.md` around lines 265 - 273,
The conflict-resolution rules are inconsistent between the summary table and the
detailed case in the article, especially around the metadata-vs-construction
decision logic. Update the section that defines the “metadata valid,
construction strongly disagrees” path so it matches the policy in the table by
explicitly stating how to choose the stronger evidence, and align the detailed
case logic in the relevant section with that rule instead of always preferring
construction when the gap threshold is met. Use the section headings and the
conflict-resolution case descriptions as the primary symbols to locate and
reconcile the mismatched guidance.
Comment on lines
+221
to
+223
| report_path = config.effective_report_path() | ||
| CsvInventoryReporter().write(result, report_path) | ||
| _log_report_written(report_path) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle CSV write failures before reporting success.
CsvInventoryReporter.write() returns PIPE_003 on OSError, but this path ignores that result and still logs pipeline.report.written, prints the report location, and can exit 0. An unwritable report path would therefore look like a successful pipeline run.
Suggested fix
- CsvInventoryReporter().write(result, report_path)
- _log_report_written(report_path)
+ report_error = CsvInventoryReporter().write(result, report_path)
+ if report_error is not None:
+ return f"Failed to write pipeline report: {report_path} ({report_error})", 1🤖 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/application/commands.py` around lines 221 - 223, The CSV
export path in the command flow is treating CsvInventoryReporter.write() as if
it cannot fail, so report success is logged even when the write returns
PIPE_003. Update the logic around report_path, CsvInventoryReporter.write(), and
_log_report_written() to check the write result first, and only print/log
success when the write succeeds; if it fails, propagate or return the error
instead of exiting 0.
Comment on lines
+156
to
+158
| log_format: Annotated[ | ||
| str, typer.Option("--log-format", help="Log format: text or jsonl.") | ||
| ] = "text", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep outline src/structure_parser/cli.py --view expandedRepository: mattbriggs/structured-markdown
Length of output: 863
🏁 Script executed:
sed -n '136,170p' src/structure_parser/cli.pyRepository: mattbriggs/structured-markdown
Length of output: 1555
🏁 Script executed:
sed -n '248,288p' src/structure_parser/application/commands.pyRepository: mattbriggs/structured-markdown
Length of output: 1859
🏁 Script executed:
rg -n "log_format|jsonl|typer\.Option|Choice|click\.Choice" src/structure_parser -SRepository: mattbriggs/structured-markdown
Length of output: 2424
Reject unsupported --log-format values at the CLI boundary.
--log-format accepts any string, but _add_log_file_handler() only treats jsonl specially and falls back to text for everything else, so typos silently produce the wrong log format.
🤖 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 156 - 158, The CLI option for log
formatting currently accepts any string, which lets invalid values silently fall
back to text in _add_log_file_handler(). Update the Typer option on the
log_format parameter in the CLI entrypoint to validate only the supported values
(text and jsonl), so unsupported inputs are rejected at the command boundary
instead of being treated as text.
Comment on lines
+81
to
+90
| inputs: list[Path] | ||
| output_dir: Path | ||
| report_path: Path | None = None | ||
| include_patterns: list[str] = Field(default_factory=lambda: ["*.md", "*.markdown"]) | ||
| exclude_patterns: list[str] = Field(default_factory=list) | ||
| log_file: Path | None = None | ||
| log_format: str = "text" | ||
| strict: bool = False | ||
| dry_run: bool = False | ||
| parser_config: ParserConfig = Field(default_factory=ParserConfig) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject empty inputs at model-validation time.
PipelineConfig(inputs=[], ...) currently validates, so Python callers can reach discovery with an invalid configuration and get a no-op run plus PIPE-004 instead of failing fast. Enforcing the minimum length here keeps the Python API aligned with the CLI contract.
Proposed fix
-class PipelineConfig(BaseModel):
+class PipelineConfig(BaseModel):
@@
- inputs: list[Path]
+ inputs: list[Path] = Field(min_length=1)📝 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.
Suggested change
| inputs: list[Path] | |
| output_dir: Path | |
| report_path: Path | None = None | |
| include_patterns: list[str] = Field(default_factory=lambda: ["*.md", "*.markdown"]) | |
| exclude_patterns: list[str] = Field(default_factory=list) | |
| log_file: Path | None = None | |
| log_format: str = "text" | |
| strict: bool = False | |
| dry_run: bool = False | |
| parser_config: ParserConfig = Field(default_factory=ParserConfig) | |
| inputs: list[Path] = Field(min_length=1) | |
| output_dir: Path | |
| report_path: Path | None = None | |
| include_patterns: list[str] = Field(default_factory=lambda: ["*.md", "*.markdown"]) | |
| exclude_patterns: list[str] = Field(default_factory=list) | |
| log_file: Path | None = None | |
| log_format: str = "text" | |
| strict: bool = False | |
| dry_run: bool = False | |
| parser_config: ParserConfig = Field(default_factory=ParserConfig) |
🤖 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/pipeline.py` around lines 81 - 90,
PipelineConfig currently allows an empty inputs list, which lets invalid Python
callers proceed into discovery instead of failing at validation time. Update the
PipelineConfig model in the pipeline contract to enforce at least one input on
the inputs field, using the existing field definition so empty lists are
rejected before any run begins. Keep the change localized to the PipelineConfig
inputs declaration and align it with the CLI contract.
Comment on lines
+79
to
+98
| def _add_file( | ||
| self, | ||
| path: Path, | ||
| source_root: Path, | ||
| config: PipelineConfig, | ||
| sources: list[DiscoveredSource], | ||
| ) -> None: | ||
| if not self._matches_include(path.name, config.include_patterns): | ||
| return | ||
| if self._matches_exclude(path.name, config.exclude_patterns): | ||
| return | ||
| relative = path.relative_to(source_root) | ||
| sources.append( | ||
| DiscoveredSource( | ||
| source_root=source_root.resolve(), | ||
| source_path=path.resolve(), | ||
| relative_path=relative, | ||
| source_format=SourceFormat.markdown, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Explicit file inputs can admit non-Markdown files.
_add_file() never checks _MARKDOWN_SUFFIXES, so a caller can pass something like notes.txt with include_patterns=["*"] and it will still be emitted as SourceFormat.markdown. Folder discovery guards against this, but direct-file discovery does not.
Proposed fix
def _add_file(
self,
path: Path,
source_root: Path,
config: PipelineConfig,
sources: list[DiscoveredSource],
) -> None:
+ if path.suffix.lower() not in _MARKDOWN_SUFFIXES:
+ return
if not self._matches_include(path.name, config.include_patterns):
return
if self._matches_exclude(path.name, config.exclude_patterns):
return📝 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.
Suggested change
| def _add_file( | |
| self, | |
| path: Path, | |
| source_root: Path, | |
| config: PipelineConfig, | |
| sources: list[DiscoveredSource], | |
| ) -> None: | |
| if not self._matches_include(path.name, config.include_patterns): | |
| return | |
| if self._matches_exclude(path.name, config.exclude_patterns): | |
| return | |
| relative = path.relative_to(source_root) | |
| sources.append( | |
| DiscoveredSource( | |
| source_root=source_root.resolve(), | |
| source_path=path.resolve(), | |
| relative_path=relative, | |
| source_format=SourceFormat.markdown, | |
| ) | |
| ) | |
| def _add_file( | |
| self, | |
| path: Path, | |
| source_root: Path, | |
| config: PipelineConfig, | |
| sources: list[DiscoveredSource], | |
| ) -> None: | |
| if path.suffix.lower() not in _MARKDOWN_SUFFIXES: | |
| return | |
| if not self._matches_include(path.name, config.include_patterns): | |
| return | |
| if self._matches_exclude(path.name, config.exclude_patterns): | |
| return | |
| relative = path.relative_to(source_root) | |
| sources.append( | |
| DiscoveredSource( | |
| source_root=source_root.resolve(), | |
| source_path=path.resolve(), | |
| relative_path=relative, | |
| source_format=SourceFormat.markdown, | |
| ) | |
| ) |
🤖 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/pipeline/discovery.py` around lines 79 - 98, Explicit
file inputs are being accepted without verifying they are Markdown, so
_add_file() can emit non-Markdown paths like notes.txt as SourceFormat.markdown.
Update discovery._add_file to apply the same _MARKDOWN_SUFFIXES gate used by
folder discovery before creating DiscoveredSource, and only append when the path
matches a Markdown suffix in addition to the existing include/exclude checks.
Comment on lines
+30
to
+70
| def write(self, result: PipelineRunResult, report_path: Path) -> str | None: | ||
| """Write the CSV inventory report for a completed pipeline run. | ||
|
|
||
| :param result: Completed pipeline run result. | ||
| :param report_path: Output CSV path. | ||
| :returns: ``PIPE-003`` error code string on write failure, ``None`` on success. | ||
| :side effects: Creates parent directories and writes a UTF-8 CSV file. | ||
| """ | ||
| try: | ||
| report_path.parent.mkdir(parents=True, exist_ok=True) | ||
| with report_path.open("w", newline="", encoding="utf-8") as fh: | ||
| writer = csv.DictWriter(fh, fieldnames=CSV_FIELDS) | ||
| writer.writeheader() | ||
| for file_result in result.files: | ||
| writer.writerow( | ||
| { | ||
| "run_id": result.run_id, | ||
| "source_root": file_result.source.source_root.as_posix(), | ||
| "source_path": file_result.source.source_path.as_posix(), | ||
| "relative_path": file_result.source.relative_path.as_posix(), | ||
| "target_path": ( | ||
| file_result.target_path.as_posix() | ||
| if file_result.target_path | ||
| else "" | ||
| ), | ||
| "status": file_result.status, | ||
| "parser_codes": ";".join(file_result.parser_codes), | ||
| "pipeline_code": file_result.pipeline_code or "", | ||
| "error_count": file_result.error_count, | ||
| "warning_count": file_result.warning_count, | ||
| "duration_ms": f"{file_result.duration_ms:.1f}", | ||
| } | ||
| ) | ||
| _log.info( | ||
| "pipeline.report.written", | ||
| extra={"report_path": str(report_path), "rows": len(result.files)}, | ||
| ) | ||
| return None | ||
| except OSError as exc: | ||
| _log.error("Failed to write report %s: %s", report_path, exc) | ||
| return PIPE_003 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make report-write failures impossible to ignore.
write() collapses failures to PIPE-003, but the provided src/structure_parser/application/commands.py:203-251 caller does not inspect that return and logs the report as written anyway. A bad report_path therefore loses the inventory artifact while the CLI still looks successful. Either raise here or require callers to turn PIPE-003 into a run diagnostic / non-zero exit.
🤖 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/pipeline/reporting.py` around lines 30 - 70, The report
write failure path in write() is currently easy to miss because it returns
PIPE_003, but the caller in the command flow ignores that status and still
reports success. Update write() and the surrounding reporting flow so a failed
report write is surfaced as a real failure: either raise from
InventoryReportWriter.write or have the caller check the PIPE_003 return, add a
run diagnostic, and fail the command with a non-zero exit. Make sure the fix is
applied consistently in InventoryReportWriter.write and the command handler that
invokes it.
Comment on lines
+61
to
+62
| # Parser codes should be present (e.g. SP-011, SP-020) | ||
| assert rows[0]["parser_codes"] != "" or rows[0]["status"] == "parsed" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert parser_codes directly here.
This currently passes whenever the row status is "parsed", even if the CSV stops exporting parser diagnostics entirely. That makes the test miss the regression it is named for.
Suggested fix
- assert rows[0]["parser_codes"] != "" or rows[0]["status"] == "parsed"
+ assert rows[0]["parser_codes"] != ""📝 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.
Suggested change
| # Parser codes should be present (e.g. SP-011, SP-020) | |
| assert rows[0]["parser_codes"] != "" or rows[0]["status"] == "parsed" | |
| # Parser codes should be present (e.g. SP-011, SP-020) | |
| assert rows[0]["parser_codes"] != "" |
🤖 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/integration/test_pipeline_error_inventory.py` around lines 61 - 62, The
integration test assertion in test_pipeline_error_inventory should verify
parser_codes directly instead of allowing a fallback on status == "parsed".
Update the check in the test around rows[0]["parser_codes"] so it explicitly
fails when parser diagnostics are missing, keeping the assertion tied to the
parser_codes field and the test's intended regression coverage.
Comment on lines
+63
to
+71
| def test_exit_1_strict_with_warnings(self, tmp_path): | ||
| (tmp_path / "docs").mkdir() | ||
| # A file without front matter produces SP-011 (info) and possibly SP-020 | ||
| # (warning). SP-020 is a warning so strict mode should raise exit 1. | ||
| (tmp_path / "docs/nowarn.md").write_text("No title here.\n\nJust text.\n") | ||
| _, code = _run(tmp_path, tmp_path / "docs", strict=True) | ||
| # strict=True: exit 1 if any warnings present | ||
| # (result depends on parser; we just check the code is valid) | ||
| assert code in (0, 1) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This strict-mode test currently proves nothing.
assert code in (0, 1) will pass no matter how PipelineCommand.run() behaves. For a unit test of the strict exit-code branch, make the warning source deterministic (for example by stubbing the orchestrator result) and assert code == 1.
🤖 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/unit/test_cli_pipeline_command.py` around lines 63 - 71, The
strict-mode test in test_exit_1_strict_with_warnings is currently
non-deterministic because it accepts either exit code, so it does not verify
PipelineCommand.run()’s strict branch. Make the warning input deterministic by
stubbing the orchestrator result or equivalent in the test setup, then assert
that the strict path returns code 1 when warnings are present. Use the existing
_run helper and PipelineCommand.run as the key symbols to update the test.
Comment on lines
+61
to
+62
| assert result.stats.discovered_count == 2 | ||
| assert result.stats.parsed_count + result.stats.parsed_count >= 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tighten this assertion so it actually covers both files.
parsed_count + parsed_count >= 1 collapses to parsed_count >= 1, so this still passes if one of the two discovered files fails.
Suggested fix
assert result.stats.discovered_count == 2
- assert result.stats.parsed_count + result.stats.parsed_count >= 1
+ assert len(result.files) == 2
+ assert all(f.status != PipelineFileStatus.failed for f in result.files)📝 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.
Suggested change
| assert result.stats.discovered_count == 2 | |
| assert result.stats.parsed_count + result.stats.parsed_count >= 1 | |
| assert result.stats.discovered_count == 2 | |
| assert len(result.files) == 2 | |
| assert all(f.status != PipelineFileStatus.failed for f in result.files) |
🤖 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/unit/test_pipeline_orchestrator.py` around lines 61 - 62, The assertion
in the pipeline orchestrator test is too weak because it only checks one parsed
file, so update the test around result.stats in test_pipeline_orchestrator to
explicitly verify both discovered files are accounted for. Replace the
duplicated parsed_count check with a condition that reflects two successful
parses or otherwise compares parsed_count against discovered_count so the test
fails if either file is not parsed.
Comment on lines
+135
to
+145
| def test_one_failure_does_not_stop_others(self, tmp_path): | ||
| (tmp_path / "docs").mkdir() | ||
| (tmp_path / "docs/good.md").write_text(_MINIMAL_MD) | ||
| # A file that will be deleted before parsing simulates a race condition. | ||
| # Since the parser returns SP-001 for missing files (not an exception), | ||
| # we test with a missing explicit input path instead. | ||
| config = _make_config(tmp_path, tmp_path / "docs") | ||
| result = PipelineOrchestrator().run(config) | ||
| # Good file should still be discovered and attempted | ||
| assert result.stats.discovered_count >= 1 | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This test never exercises the failure path.
The setup only gives the orchestrator a valid directory with one valid file, so the assertion passes without proving that a failed file is isolated and the next file still runs. A small monkeypatch around structure_parser.pipeline.orchestrator.parse_one or the writer would make this test meaningful.
🤖 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/unit/test_pipeline_orchestrator.py` around lines 135 - 145, The test
currently only covers a fully valid discovery path, so it never proves failure
isolation in PipelineOrchestrator.run. Update
test_one_failure_does_not_stop_others to force one parse/write failure by
monkeypatching structure_parser.pipeline.orchestrator.parse_one (or the writer)
while keeping a second valid file in the input set, then assert the good file
still gets processed and stats.discovered_count reflects both discovery and
continued execution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
structure-parser pipeto process Markdown folders/files into per-file JSON outputs plus a CSV inventory report (custom report path supported).run_pipeline()API for the same repository pipeline flow.--dry-run, include/exclude filters, optional file logging (format selectable), and--strictexit behavior.Bug Fixes
Documentation / Tests