Skip to content

fix(fossil): narrow generic exception handlers in forum adapter - #961

Open
HeaTTap wants to merge 1 commit into
apache:mainfrom
HeaTTap:fix/narrow-fossil-forum-exception-handlers
Open

fix(fossil): narrow generic exception handlers in forum adapter#961
HeaTTap wants to merge 1 commit into
apache:mainfrom
HeaTTap:fix/narrow-fossil-forum-exception-handlers

Conversation

@HeaTTap

@HeaTTap HeaTTap commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Fixes #954

Narrowed generic except Exception blocks in tools/fossil/src/magpie_fossil/forum.py to except FossilError, ensuring unexpected system exceptions are properly raised.

@justinmclean justinmclean left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

Blocking — narrowing to FossilError alone drops the "corrupt" half of what the handler promises (tools/fossil/src/magpie_fossil/forum.py:88, same at :137)

Issue #954 says explicitly: "Read what each try block can actually raise before narrowing." Working through the body:

  • run_fossil(...) raises FossilError for a missing binary and for a non-zero exit. Covered.
  • parse_forum_artifact(art_text) is pure string handling and always returns all five keys, so neither it nor the parsed[...] lookups can raise. Nothing to cover.
  • run_fossil also calls subprocess.run(..., text=True), and client.py:55 converts only FileNotFoundError. When the artifact blob is not valid UTF-8, the decode happens inside subprocess.run and raises UnicodeDecodeError, which passes straight through run_fossil uncaught. I confirmed the mechanism: subprocess.run(text=True) on non-UTF-8 stdout raises UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff.

That is precisely the case the handler exists for. Its own message reads "skipped corrupt or missing forum post artifact". Before this change, except Exception caught the corrupt case and skipped one row. After it, a single corrupt blob aborts list_forum_threads entirely and the caller gets a traceback instead of a list, so one bad artifact takes out the whole forum listing.

Smallest correct fix, at both sites:

except (FossilError, UnicodeDecodeError) as exc:

Major — the change ships with no test in either direction (tools/fossil/tests/test_fossil.py)

Issue #954's acceptance criteria include "An unexpected exception propagates". Nothing asserts that, and nothing asserted the old skip-and-warn behaviour either: test_list_forum_threads and test_read_forum_thread are both happy-path, with run_fossil mocked to return a well-formed manifest. So the entire behaviour this PR changes is currently unobserved by the suite, which is why pytest passing tells us nothing here.

Two small tests using the @patch("magpie_fossil.forum.run_fossil") decorator already in the file would cover it:

  1. run_fossil raises FossilError on the second of three rows: assert the warning reaches stderr and the other two threads are still returned.
  2. run_fossil raises something genuinely unexpected, say RuntimeError: assert it propagates.

Add the UnicodeDecodeError case to the first test once the handler names it.

This review was drafted by an AI-assisted tool and confirmed by a Magpie maintainer. After you've addressed the points above and pushed an update, a Magpie maintainer, a real person, will take the next look at the PR. The findings cite the project's review criteria; if you think one of them is mis-applied, please reply on the PR and a maintainer will weigh in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Narrow the empty exception handlers in the Fossil adapter

2 participants