Repository navigation
feat: structured error types for LLM self-correction - #13
Merged
Merged
Conversation
- Remove OpenSpec markers from CLAUDE.md and AGENTS.md - Remove legacy openspec/AGENTS.md (replaced by config.yaml) - Add openspec/config.yaml with spec-driven schema - Add .claude/skills and .claude/commands/opsx from openspec init
New structured-errors capability extending the HashMismatchError pattern to TextNotFoundError, ParagraphIndexError, and BatchOperationError so LLM callers can self-correct in-loop.
Extends the HashMismatchError structured-field pattern to the rest of the LLM-facing error surface so agents can recover in-loop without re-reading the document or regex-parsing messages. - TextNotFoundError now carries search_text, paragraph_ref, paragraph_preview (80-char cap, matching HashMismatchError) and occurrence/total_occurrences for nth-match failures. The message embeds the current paragraph content when the search was scoped. - ParagraphIndexError (new, DocxEditError subclass) replaces stdlib IndexError on out-of-range paragraph refs. Carries index and total_paragraphs. BREAKING. - BatchOperationError (new, DocxEditError subclass) replaces ValueError on batch validation paths only. batch_edit also wraps _apply_single_edit so inner field-level failures carry operation_index. Non-batch ValueError paths unchanged. BREAKING. Both new classes inherit from DocxEditError, so existing 'except DocxEditError' catch-all handlers still match. skills/docx/SKILL.md gains a recovery table documenting each structured error's fields and the retry pattern agents should follow. See openspec/changes/add-structured-error-types for the full spec.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR introduces OpenSpec workflow documentation defining AI assistant behavior for proposing, implementing, and archiving experimental changes, alongside a significant enhancement to docx_editor's exception surface by implementing structured, LLM-recoverable error types with standardized recovery fields and adaptive messaging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
=======================================
+ Coverage 93.6% 93.8% +0.2%
=======================================
Files 10 10
Lines 2133 2170 +37
Branches 485 491 +6
=======================================
+ Hits 1998 2037 +39
+ Misses 35 34 -1
+ Partials 100 99 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docx_editor/track_changes.py (2)
237-240:⚠️ Potential issue | 🟡 MinorStale docstring —
rewrite_paragraphno longer raisesIndexError.
_resolve_paragraphnow raisesParagraphIndexError(line 91), so this docstring is incorrect and the "breaking change" is under-documented at this raise site.📝 Proposed fix
Raises: HashMismatchError: If the paragraph hash doesn't match - IndexError: If paragraph index is out of range + ParagraphIndexError: If paragraph index is out of range🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docx_editor/track_changes.py` around lines 237 - 240, The docstring for rewrite_paragraph is stale: replace the raised IndexError with ParagraphIndexError and mention that rewrite_paragraph can propagate ParagraphIndexError from _resolve_paragraph (in addition to HashMismatchError). Update the "Raises:" section to list ParagraphIndexError (with a short note that it's raised when the paragraph index is out of range via _resolve_paragraph) and keep HashMismatchError unchanged.
1360-1368:⚠️ Potential issue | 🟠 Major
_insert_textfallback drops occurrence context on re-raise.When
_get_nth_matchraisesTextNotFoundErrorbecauseoccurrence >= len(matches), the exception carriesoccurrence/total_occurrences. This branch then runs the cross-boundary fallback; on still-not-found it constructs a freshTextNotFoundError(anchor) from None, discarding those structured fields — exactly the diagnostic signal the PR is introducing.The sibling paths in
replace_text(line 531) andsuggest_deletion(line 634) bare-raiseto preserve the original. Do the same here:🔒️ Proposed fix
# Find the text element containing the anchor text try: elem = self._get_nth_match(anchor, occurrence) except TextNotFoundError: # Fall back to cross-boundary search match = self._find_across_boundaries(anchor, occurrence) if match is None: - raise TextNotFoundError(anchor) from None + raise return self._insert_near_match(match, text, position)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docx_editor/track_changes.py` around lines 1360 - 1368, When `_get_nth_match` raises TextNotFoundError in `_insert_text`, the current fallback constructs a fresh TextNotFoundError and loses the original occurrence/total_occurrences fields; change the except block to capture the original exception (except TextNotFoundError as e:) and, if the cross-boundary fallback `_find_across_boundaries` returns None, re-raise the original exception (use bare `raise` or `raise e`) instead of creating a new TextNotFoundError so the occurrence context is preserved; keep the successful cross-boundary path returning `_insert_near_match(match, text, position)`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/opsx/apply.md:
- Around line 16-20: Update the apply-mode behavior to require explicit user
confirmation before selecting a change instead of auto-selecting when only one
active change exists: remove or modify the "Auto-select if only one active
change exists" branch and replace it with a step that runs `openspec list
--json` and invokes the AskUserQuestion tool to present the single available
change for explicit confirmation (mention apply mode and AskUserQuestion tool by
name so the text and any implementation logic use the explicit-confirmation flow
rather than implicit auto-selection).
- Around line 65-70: The current task-applier flow marks a task checkbox `- [ ]`
→ `- [x]` immediately after making edits; change the flow so that after making
code changes you run a mandatory validation step (e.g., execute the
validation/check routine, tests, or a lint/compile step) and only if that
validation returns success do you update the task line to `- [x]`; update the
logic that performs the checkbox replacement (search for the code that performs
the direct replacement of `- [ ]` with `- [x]`) to gate it behind the validation
result, and add unit/integration tests that simulate a passing and failing
validation to confirm the checkbox is only toggled on success.
In @.claude/commands/opsx/archive.md:
- Around line 58-63: The cancel option currently is ignored because the flow
always proceeds to archive; modify the control flow so that the "Cancel" choice
aborts the operation and does not invoke the Task tool or continue to archive.
Specifically, change the logic described in the prompt options and the "If user
chooses sync" block so the Task tool (subagent_type: "general-purpose", prompt:
"Use Skill tool to invoke openspec-sync-specs...") is only invoked when the user
explicitly picks a sync option, and ensure only the user-selected archive
options ("Archive now" or "Archive without syncing") trigger the archival step;
if "Cancel" is selected, return without performing sync or archive. Ensure the
prompt text and branching reflect this corrected behavior.
In @.claude/skills/openspec-apply-change/SKILL.md:
- Around line 69-74: The current guidance toggles task checkboxes (`- [ ]` → `-
[x]`) immediately after edits; change the flow so each task is only marked
complete after a validation step succeeds (e.g., run tests, lint, or other
required CI checks). Update the SKILL.md text around the "Mark task complete in
the tasks file: `- [ ]` → `- [x]`" line to require running and passing
unit/integration tests and linting for any code change, and add an explicit
bullet that says "Write/modify tests for the change and confirm test/lint pass
before marking - [x]". Ensure the instruction references running the repo's test
command and CI checks as the gating step.
- Around line 20-24: Update the SKILL behavior for implementation mode so it
never auto-selects a change: remove the "Auto-select if only one active change
exists" fallback and the "Infer from conversation context" behavior for
implementation mode and instead always run `openspec list --json` and invoke the
AskUserQuestion tool to get explicit user confirmation before proceeding; keep
the name-when-provided path unchanged but ensure the implementation-mode flow
(referenced as "implementation mode" and the AskUserQuestion tool) requires
explicit user selection even if only one change is available.
In @.claude/skills/openspec-archive-change/SKILL.md:
- Around line 62-67: The prompt flow in SKILL.md lists "Cancel" as an option but
then always proceeds to archive; update the flow so selecting the "Cancel"
option aborts the archive and does not invoke the Task tool or
"openspec-sync-specs" subagent. Concretely: detect the user's chosen option
string ("Cancel") and short-circuit the archive path (no Task tool call with
prompt "Use Skill tool to invoke openspec-sync-specs for change '<name>'..." and
no subsequent archive step); only run the Task tool and proceed to archive when
the choice is one of the sync/archive confirmations ("Sync now
(recommended)","Archive without syncing","Archive now","Sync anyway").
In `@docx_editor/exceptions.py`:
- Around line 164-168: Move the helper function _truncate_preview so it is
defined before the TextNotFoundError class (specifically before
TextNotFoundError.__init__ which calls it); this ensures the helper is available
at class-definition time and prevents future refactors (e.g., using it in
default args or class attributes) from breaking name resolution. Locate the
current _truncate_preview implementation and cut/paste it above the
TextNotFoundError class declaration, keeping the same signature and docstring;
no other code changes are required.
- Around line 100-112: The current message-building chain in the exception
constructor drops paragraph context when occurrence/total_occurrences is
present; change the logic in the exception initializer (the block that currently
checks occurrence, total_occurrences, paragraph_ref and sets msg before calling
super().__init__(msg)) to build msg incrementally: start with a base message
(e.g. "Text not found" or the occurrence summary when
occurrence/total_occurrences provided), then if paragraph_ref is not None append
the paragraph scope ("in paragraph {paragraph_ref}") and if
self.paragraph_preview is not None append the preview string; ensure the final
msg passed to super().__init__(msg) contains both occurrence info and
paragraph_ref/paragraph_preview when supplied.
- Around line 115-130: ParagraphIndexError builds a misleading hint "valid:
P1-P{total_paragraphs}" when total_paragraphs == 0; update the __init__ of
ParagraphIndexError to conditionally format the message: keep the existing
message for total_paragraphs > 0, but when total_paragraphs == 0 replace the
"valid: ..." fragment with something like "no valid paragraph indices (document
has 0 paragraphs)" so callers won't see a spurious range; modify the constructor
logic that composes the super().__init__ string in the ParagraphIndexError class
to branch on self.total_paragraphs and emit the appropriate text.
In `@docx_editor/track_changes.py`:
- Around line 107-109: _paragraph_preview currently returns the full visible
text which can be large; add a one-line clarifying comment above the
_paragraph_preview method stating that truncation to 80 chars is handled by
TextNotFoundError.__init__ -> _truncate_preview so callers should not truncate
here and to avoid duplicate/inconsistent truncation or premature optimization
that would duplicate behavior.
In `@openspec/changes/add-structured-error-types/specs/structured-errors/spec.md`:
- Around line 22-24: The occurrence branch in TextNotFoundError.__init__
(exceptions.py) currently omits paragraph_ref from the constructed message when
both occurrence and paragraph_ref are provided; update the constructor so the
message always includes search_text and, if paragraph_ref is present, also
includes paragraph_ref (and paragraph_preview when present) even on the
occurrence path, and ensure all raise sites (e.g., docx_editor/comments.py and
track_changes.py) continue using the new structured signature; alternatively, if
you prefer precedence, update the spec to state that occurrence short-circuits
paragraph_ref explicitly and keep the code as-is.
In `@skills/docx/SKILL.md`:
- Around line 445-460: The example only demonstrates HashMismatchError and
TextNotFoundError but the doc mentions ParagraphIndexError and
BatchOperationError; update the example around doc.replace to also show recovery
patterns for ParagraphIndexError (use e.paragraph_index to locate and retry the
replace on the corrected index) and BatchOperationError (inspect e.errors or
similar batch error payload to iterate failures and retry or log per-paragraph),
referencing the existing symbols ParagraphIndexError, BatchOperationError and
doc.replace so readers/agents can see programmatic in-loop recovery for these
new error classes.
In `@tests/test_batch_edit.py`:
- Around line 385-390: The test test_non_batch_value_error_unchanged should
assert the specific error message from ParagraphRef.parse to avoid masking
unrelated ValueError regressions; update the pytest.raises in that test to
include match="Invalid paragraph reference" (or the exact message used by
ParagraphRef.parse) so the assertion validates both the exception type and the
expected message.
In `@tests/test_error_quality.py`:
- Around line 70-75: The assertion is over-stripping the preview by using
err.paragraph_preview.rstrip(".") which will remove any trailing periods from
real content; in the test (variables err, paragraph_preview, msg) replace the
rstrip(".") usage with a removesuffix("...") call so only a trailing ellipsis is
removed (preserving legitimate trailing periods), and then assert that
err.paragraph_preview.removesuffix("...") appears in msg.
- Around line 137-156: Add an explicit return type annotation to the helper
function _build_batch_doc (e.g., def _build_batch_doc(...) -> Document) to
satisfy ruff ANN202; ensure the Document symbol is available in the module scope
(import or qualify it) so the annotation resolves in the test file and then run
the linter/tests to confirm the warning is resolved.
In `@tests/test_paragraph_hash.py`:
- Around line 459-461: The test currently uses current_preview.rstrip(".") which
over-strips trailing periods; change the logic that computes preview_prefix
(variable preview_prefix derived from current_preview in
tests/test_paragraph_hash.py) to remove only the truncation ellipsis by using
str.removesuffix("...") (or equivalent) before taking the first 10 chars, so
that ordinary sentence-ending periods are preserved; update the assertion that
checks preview_prefix in msg accordingly and keep references to current_preview
and preview_prefix (and the list_paragraphs behavior) to ensure the test still
validates truncated previews.
---
Outside diff comments:
In `@docx_editor/track_changes.py`:
- Around line 237-240: The docstring for rewrite_paragraph is stale: replace the
raised IndexError with ParagraphIndexError and mention that rewrite_paragraph
can propagate ParagraphIndexError from _resolve_paragraph (in addition to
HashMismatchError). Update the "Raises:" section to list ParagraphIndexError
(with a short note that it's raised when the paragraph index is out of range via
_resolve_paragraph) and keep HashMismatchError unchanged.
- Around line 1360-1368: When `_get_nth_match` raises TextNotFoundError in
`_insert_text`, the current fallback constructs a fresh TextNotFoundError and
loses the original occurrence/total_occurrences fields; change the except block
to capture the original exception (except TextNotFoundError as e:) and, if the
cross-boundary fallback `_find_across_boundaries` returns None, re-raise the
original exception (use bare `raise` or `raise e`) instead of creating a new
TextNotFoundError so the occurrence context is preserved; keep the successful
cross-boundary path returning `_insert_near_match(match, text, position)`.
🪄 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
Run ID: 4f1c8301-eac6-43b7-a349-ba1542a8cbb0
📒 Files selected for processing (26)
.claude/commands/opsx/apply.md.claude/commands/opsx/archive.md.claude/commands/opsx/explore.md.claude/commands/opsx/propose.md.claude/skills/openspec-apply-change/SKILL.md.claude/skills/openspec-archive-change/SKILL.md.claude/skills/openspec-explore/SKILL.md.claude/skills/openspec-propose/SKILL.mdAGENTS.mdCLAUDE.mddocx_editor/__init__.pydocx_editor/comments.pydocx_editor/exceptions.pydocx_editor/track_changes.pyopenspec/AGENTS.mdopenspec/changes/add-structured-error-types/.openspec.yamlopenspec/changes/add-structured-error-types/design.mdopenspec/changes/add-structured-error-types/proposal.mdopenspec/changes/add-structured-error-types/specs/structured-errors/spec.mdopenspec/changes/add-structured-error-types/tasks.mdopenspec/config.yamlskills/docx/SKILL.mdtests/test_batch_edit.pytests/test_error_quality.pytests/test_paragraph_hash.pytests/test_rewrite_paragraph.py
💤 Files with no reviewable changes (3)
- CLAUDE.md
- AGENTS.md
- openspec/AGENTS.md
…ases - TextNotFoundError: include paragraph context when both occurrence and paragraph_ref are passed; previously paragraph_ref was silently dropped. - ParagraphIndexError: when document has 0 paragraphs, say so instead of emitting 'valid: P1-P0'. - Tighten test_non_batch_value_error_unchanged with a match pattern so it won't mask unrelated ValueErrors. - Add coverage for TextNotFoundError(paragraph_ref=...) with no preview.
- hoist _truncate_preview above TextNotFoundError (exceptions.py)
- document single-source-of-truth for preview truncation (track_changes.py)
- extend recovery example with ParagraphIndexError + BatchOperationError (SKILL.md)
- use removesuffix("...") instead of rstrip(".") for preview assertions (tests)
- add type hints to _build_batch_doc test helper
Codecov patch gate flagged exceptions.py:113 and :137 as uncovered diff lines. Add direct unit tests for both branches: - nth-match miss with paragraph_ref appends 'Paragraph: <ref>.' - empty-document ParagraphIndexError uses distinct phrasing (no misleading 'valid: P1-P0')
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docx_editor/track_changes.py (2)
241-244:⚠️ Potential issue | 🟡 MinorStale docstring —
rewrite_paragraphnow raisesParagraphIndexError, notIndexError.
_resolve_paragraphwas updated to raiseParagraphIndexError(line 91), so theRaises:section here is out of date and will mislead callers trying to catch the right exception.📝 Proposed fix
Raises: HashMismatchError: If the paragraph hash doesn't match - IndexError: If paragraph index is out of range + ParagraphIndexError: If paragraph index is out of range🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docx_editor/track_changes.py` around lines 241 - 244, Update the docstring for rewrite_paragraph to reflect that it raises ParagraphIndexError (not IndexError) by replacing IndexError in the Raises: section with ParagraphIndexError; ensure the other listed exception (HashMismatchError) remains, and reference the existing ParagraphIndexError used by _resolve_paragraph so callers can catch the correct exception type.
137-158:⚠️ Potential issue | 🟠 MajorUnguarded
ParagraphRef.parse()in batch methods leaksValueErrorwithoutoperation_index.Both
batch_edit(line 141) andbatch_rewrite(line 214) callParagraphRef.parse()directly in their upfront validation loops without wrapping in try-except. If a batch receives a malformed paragraph reference like"not-a-ref",ParagraphRef.parse()raises a bareValueError— callers cannot determine which operation failed, violating the batch contract that all errors carryoperation_indexfor LLM recovery.Wrap both calls:
Fix for batch_edit
- ref = ParagraphRef.parse(op.paragraph) + try: + ref = ParagraphRef.parse(op.paragraph) + except ValueError as e: + raise BatchOperationError(i, str(e)) from eApply the equivalent guard around
ParagraphRef.parse(ref_str)inbatch_rewrite(line 214).Additionally,
rewrite_paragraphdocstring (line 243) claimsIndexErrorbut the method raisesParagraphIndexErrorvia_resolve_paragraph. Update the docstring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docx_editor/track_changes.py` around lines 137 - 158, The batch handlers (batch_edit and batch_rewrite) currently call ParagraphRef.parse(...) directly in their validation loops which allows a bare ValueError to escape without the required operation index; wrap the ParagraphRef.parse(op.paragraph) call in batch_edit (the loop building parsed list) and the ParagraphRef.parse(ref_str) call in batch_rewrite in a try/except that catches ValueError and raises BatchOperationError(operation_index, str(e)) from e so callers receive the operation index; also update the rewrite_paragraph docstring to mention ParagraphIndexError (the error raised by _resolve_paragraph) instead of IndexError.tests/test_batch_edit.py (1)
261-398:⚠️ Potential issue | 🟡 MinorTest updates correctly reflect the new
BatchOperationErrorcontract.Message-matching regexes align with the exceptions actually raised (
"paragraph reference is required for batch mode","replace requires...", etc.), and the newTestStructuredBatchOperationErrorexercises both the predispatch path and the inner_apply_single_editwrapping (try/except ValueError → BatchOperationError).test_non_batch_value_error_unchangednicely pins the scope of the breaking change.One coverage gap worth considering: there's no test for a malformed
ParagraphRefsyntax supplied tobatch_edit(e.g.,paragraph="not-a-ref"). In the current implementationParagraphRef.parse(op.paragraph)runs outside thetry/except ValueErrorblock inbatch_edit, so it would escape as a bareValueErrorwithout anoperation_index— see the related note ondocx_editor/track_changes.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_batch_edit.py` around lines 261 - 398, ParagraphRef.parse is called outside the try/except in batch_edit so a malformed paragraph ref (e.g. "not-a-ref") raises a plain ValueError without being wrapped with BatchOperationError.operation_index; move or wrap the ParagraphRef.parse call(s) inside the same try/except that wraps _apply_single_edit (or explicitly catch ValueError around parsing) and re-raise a BatchOperationError with the correct operation_index and reason (consistent with other cases), updating batch_edit and any helper that parses op.paragraph to ensure every parsing error for a batched op is converted to BatchOperationError referencing that op's index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docx_editor/exceptions.py`:
- Around line 70-159: Add explicit return type annotations "-> None" to the
three new initializer methods to satisfy ruff ANN204: update
TextNotFoundError.__init__, ParagraphIndexError.__init__, and
BatchOperationError.__init__ to declare their return type as None (e.g. def
__init__(...) -> None:) while keeping their current parameters and bodies
intact.
In `@openspec/changes/archive/2026-04-17-add-structured-error-types/design.md`:
- Around line 58-59: The batch path currently calls
ParagraphRef.parse(op.paragraph) outside the batch's try/except, so malformed
refs surface as bare ValueError; update batch_edit and batch_rewrite in
docx_editor/track_changes.py to either move the ParagraphRef.parse call inside
the existing try/except that converts ValueError to BatchOperationError or
explicitly wrap the parse call with try: ... except ValueError as e: raise
BatchOperationError(operation_index=i, cause=e) from e so malformed-paragraph
errors are reported with the operation_index like other validation failures.
In `@openspec/changes/archive/2026-04-17-add-structured-error-types/proposal.md`:
- Line 23: Several raise sites still call TextNotFoundError with the old
single-arg signature; update every raise to the structured form
TextNotFoundError(paragraph_ref=..., paragraph_preview=..., occurrence=...)
using the local paragraph context or anchor where available (e.g. in the
comments handler use the anchor and document to build paragraph_ref and a short
paragraph_preview; in track_changes functions/_resolve_paragraph/_resolve_text
use the local paragraph or p variable to construct paragraph_ref and
paragraph_preview and pass the matched occurrence index or default 0). Replace
bare raises like raise TextNotFoundError(anchor_text) / raise
TextNotFoundError(text) with the structured keyword arguments and ensure
consistency with ParagraphRef.parse/ParagraphRef factory usage to build
paragraph_ref.
In `@openspec/specs/structured-errors/spec.md`:
- Around line 3-4: Replace the placeholder "TBD" under the "Purpose" header with
a concise one-sentence purpose that describes the spec's intent; update the
Purpose section in openspec/specs/structured-errors/spec.md to a single sentence
such as "Defines the LLM-consumable structured exception contract for
docx-editor edit operations, guaranteeing in-loop recovery via typed fields on
DocxEditError subclasses." to make the document self-describing and actionable
(look for the "Purpose" header and the DocxEditError symbol to ensure the
sentence references the contract and intended consumers).
- Around line 78-103: The spec omits how malformed ParagraphRef syntax (from
ParagraphRef.parse) invoked during the batch upfront-validation loop in
track_changes.py should be classified; update the requirement text to state that
any ValueError raised by ParagraphRef.parse when called during batch_edit
validation must be wrapped and re-raised as BatchOperationError(i, reason) so
operation_index is preserved, and add an explicit scenario mirroring the
existing ones that demonstrates a malformed paragraph ref at index N yields
BatchOperationError with operation_index == N and a descriptive reason;
reference ParagraphRef.parse, batch_edit, _apply_single_edit, and
BatchOperationError in the new text so implementers know to treat ParagraphRef
parsing errors the same as other batch-path ValueError cases.
In `@skills/docx/SKILL.md`:
- Around line 460-464: The recovery snippet for ParagraphIndexError must
explicitly handle the empty-document case: check e.total_paragraphs (from the
caught ParagraphIndexError) and if it is 0, abort or handle gracefully (e.g.,
return/raise/log) instead of indexing doc.list_paragraphs(); otherwise compute
safe_idx as before (using max/min on e.index and e.total_paragraphs), get the
paragraph reference via doc.list_paragraphs()[safe_idx - 1].split("|")[0], and
call doc.replace("stale text", "new text", paragraph=ref). Ensure references to
ParagraphIndexError, e.total_paragraphs, safe_idx, doc.list_paragraphs(), and
doc.replace(...) are updated accordingly.
In `@tests/test_error_quality.py`:
- Around line 169-188: The helper _build_batch_doc reaches into private
internals (doc._document_editor, editor.dom, editor._parse_fragment); replace
this by either (A) using a public API to create the same document (e.g.
Document.open / Document.save with public editor methods) or (B) moving the
current implementation into a single shared fixture (conftest) so tests don't
manipulate privates directly; update references to _build_batch_doc to call the
new fixture or public authoring helper and remove direct uses of
_document_editor and _parse_fragment from test files.
- Around line 113-115: The three tight string-match assertions in
tests/test_error_quality.py (the assertions checking "Only 1 occurrence(s) of
'needle'", "occurrence=3 requested", and "Paragraph: P2#cafe") should be relaxed
to simple presence checks to avoid coupling to message wording; update the
assertions to check that "needle" is in msg, that "3" (or "occurrence 3") is
present in msg, and that "P2#cafe" (or "P2#cafe" substring) is in msg so the
test verifies the structured content rather than exact phrasing.
- Around line 35-42: The doc_path pytest fixture uses tempfile.mkdtemp and
manual cleanup; change it to use pytest's tmp_path fixture instead: accept
tmp_path as a parameter to the doc_path fixture, copy the test_data
(Path(__file__).parent / "test_data" / "simple.docx") into tmp_path /
"test.docx" and yield that Path; remove the tempfile.mkdtemp, shutil.copy/rmtree
manual logic so pytest handles temp dir creation and cleanup automatically in
the doc_path fixture.
---
Outside diff comments:
In `@docx_editor/track_changes.py`:
- Around line 241-244: Update the docstring for rewrite_paragraph to reflect
that it raises ParagraphIndexError (not IndexError) by replacing IndexError in
the Raises: section with ParagraphIndexError; ensure the other listed exception
(HashMismatchError) remains, and reference the existing ParagraphIndexError used
by _resolve_paragraph so callers can catch the correct exception type.
- Around line 137-158: The batch handlers (batch_edit and batch_rewrite)
currently call ParagraphRef.parse(...) directly in their validation loops which
allows a bare ValueError to escape without the required operation index; wrap
the ParagraphRef.parse(op.paragraph) call in batch_edit (the loop building
parsed list) and the ParagraphRef.parse(ref_str) call in batch_rewrite in a
try/except that catches ValueError and raises
BatchOperationError(operation_index, str(e)) from e so callers receive the
operation index; also update the rewrite_paragraph docstring to mention
ParagraphIndexError (the error raised by _resolve_paragraph) instead of
IndexError.
In `@tests/test_batch_edit.py`:
- Around line 261-398: ParagraphRef.parse is called outside the try/except in
batch_edit so a malformed paragraph ref (e.g. "not-a-ref") raises a plain
ValueError without being wrapped with BatchOperationError.operation_index; move
or wrap the ParagraphRef.parse call(s) inside the same try/except that wraps
_apply_single_edit (or explicitly catch ValueError around parsing) and re-raise
a BatchOperationError with the correct operation_index and reason (consistent
with other cases), updating batch_edit and any helper that parses op.paragraph
to ensure every parsing error for a batched op is converted to
BatchOperationError referencing that op's index.
🪄 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
Run ID: dab01c3a-8646-4162-8627-f88880bc8099
📒 Files selected for processing (12)
docx_editor/exceptions.pydocx_editor/track_changes.pyopenspec/changes/archive/2026-04-17-add-structured-error-types/.openspec.yamlopenspec/changes/archive/2026-04-17-add-structured-error-types/design.mdopenspec/changes/archive/2026-04-17-add-structured-error-types/proposal.mdopenspec/changes/archive/2026-04-17-add-structured-error-types/specs/structured-errors/spec.mdopenspec/changes/archive/2026-04-17-add-structured-error-types/tasks.mdopenspec/specs/structured-errors/spec.mdskills/docx/SKILL.mdtests/test_batch_edit.pytests/test_error_quality.pytests/test_paragraph_hash.py
…mple list_paragraphs()[0] would IndexError on an empty document because max(1, min(n, 0)) == 1. Raise to the caller instead — an LLM copying the snippet gets correct behaviour on documents with no paragraphs.
4 tasks done
pablospe
added a commit
that referenced
this pull request
Jul 13, 2026
* test: add foreign track-changes fixtures from docx-revisions (MIT) Two real-world fixtures copied from https://github.com/balalofernandez/docx-revisions: - OXML_TrackChanges_Test.docx: 8 revisions (4 w:ins + 4 w:del) from 6 authors, covering a del+ins replacement pair, an em-dash insertion next to smart-quote runs, and identical text inserted by one author and deleted by another in the same paragraph. - tricky-track-changes.docx: no revisions, but heavily fragmented runs (proofErr, bookmarks, lastRenderedPageBreak, formatting splits) that exercise cross-run search the way real Word output does. THIRD_PARTY_NOTICES records the source and reproduces the upstream MIT license. * test: promote replace_document_xml helper to conftest Move the word/document.xml swap helper from test_paragraph_location.py to conftest.py under a public name so other test modules can build synthetic .docx files without duplicating it. No behavior change. * test: cover foreign revisions, table lifecycle, fragmented-run search Closes ISSUES.md #13. New tests against the foreign fixtures plus synthetic documents: - parse all 8 foreign revisions with exact id/type/author/text/date, per-author filters, em-dash and smart-quote survival - accept_all/reject_all end-to-end, per-author accept/reject, and the conflicting insert/delete pair over identical text - partial accept survives save + reopen with the rest intact - rejecting a deletion keeps xml:space="preserve" on the restored w:t (Word-realistic delText) and attribute-less foreign delText still round-trips - tracked edits in table cells and nested tables accept/reject cleanly - cross-run search over real Word fragmentation (proofErr, bookmarks, superscript splits), plus find_in_text_map unit tests for many- fragment matches and matches ending mid-run Note: the fixture holds 4 w:ins + 4 w:del = 8 revisions total (the task description's "4 ins + 8 del" was wrong). * test: make fixture text assertions note-proof, compare Revision objects Review findings from the multi-reviewer pass: - The fixture's bracketed note runs quote expected outcomes verbatim, so several get_visible_text() substring assertions were satisfiable by a note instead of the body they meant to check. Route all such assertions through a _body_text() helper that strips the note runs; the reject-path and round-trip assertions are now genuinely discriminating and the accept-path assertions can no longer be satisfied by note text. - Replace the hand-ordered (id, type, author, text, date) tuples with Revision instances compared via the dataclass's own __eq__, removing the silent field reordering against the dataclass definition. - Simplify the reject_all em-dash check: with notes stripped, '— or is it?' must be absent outright. * test: address second review round in foreign-revision tests - Route the two remaining get_visible_text() assertions (smart-quote parsing check, tricky-fixture replace integration) through _body_text() so note runs can't satisfy them. - Assert revisions 1007/1008 actually leave list_revisions() after the conflicting-pair accepts. - Construct EXPECTED_REVISIONS with keyword arguments, matching the Revision construction convention in test_track_changes.py. - Fix inaccurate comments/docstrings: the pending-state body reads 'This sentence remains.' (single space); 'Quick Brown' has no note occurrence; the notes describe outcomes approximately, not verbatim. * test: apply final review-round nits - Guard against a doubled DRAFT after accept_all. - Verify Test Author's rejected insertion is gone from the body in the author-filtered reject test, not just the restored deletion. - Correct the 'Quick Brown' comment/docstring: it is a single-run match guarding against the note's 'ick Brown', not a fragmented one.
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
TextNotFoundError(search_text,paragraph_ref,paragraph_preview,occurrence,total_occurrences) so agents retry without re-reading the document.ParagraphIndexErrorandBatchOperationErrorclasses — bothDocxEditErrorsubclasses — replace stdlibIndexError/ValueErroron LLM-facing paths. BREAKING, butexcept DocxEditErrorstill catches them.batch_editwraps_apply_single_editso inner field-level failures carryoperation_index.skills/docx/SKILL.mdgains a recovery table for each structured error.See
openspec/changes/add-structured-error-types/for the proposal, design rationale, and spec deltas.Test plan
uv run pytest— 491 passed, 6 skippeduv run ruff check . && uv run ruff format --check .— cleanopenspec validate add-structured-error-types --strict --no-interactive— validtests/test_error_quality.pydocuments before/after for every structured fieldSummary by CodeRabbit
New Features
Explore,Propose,Apply,Archive) for managing experimental changes.Documentation