fix: narrow evidence validation to ArtifactNotFoundError in propose_claim#50
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughpropose_claim() now imports ArtifactNotFoundError and only catches that exception during store.get_source/get_evidence; other I/O/parse errors are allowed to propagate. A regression test and three CHANGELOG entries were added. ChangesException Handling Tightening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Nice and minimal — this matches exactly what #48 asked for. Just one thing before merge: Needs changes
Nice to have
Otherwise this looks great. Happy to re-review once the test lands. Thanks! 🙏 |
…_claim
Asserts that a corrupt meta.yaml surfaces a real parse error rather
than being swallowed and replaced with a misleading
ProposalError('unknown source/evidence id').
Requested in review of vouchdev#50.
Fixes vouchdev#48
Added the regression test — it:
On point 2: All 102 tests pass. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_cli.py`:
- Around line 92-102: The test currently swallows the intended failure because
the "assert False" inside the try is caught by the broad "except Exception",
leading to false positives; replace the try/except with a proper assertion using
pytest.raises to ensure propose_claim raises ProposalError and then assert the
error message does not contain the misleading text. Concretely, wrap the call to
propose_claim(...) in "with pytest.raises(ProposalError) as excinfo:" (ensure
pytest is imported), call propose_claim inside that context, and then assert
"unknown source/evidence id" not in str(excinfo.value); alternatively, if you
prefer keeping try/except, remove the broad "except Exception" and let
unexpected exceptions propagate so the test fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Fix conflict |
…_claim
Asserts that a corrupt meta.yaml surfaces a real parse error rather
than being swallowed and replaced with a misleading
ProposalError('unknown source/evidence id').
Requested in review of vouchdev#50.
Fixes vouchdev#48
24119b0 to
1b75453
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_cli.py (1)
92-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix false-positive exception test flow and Ruff blocker.
Line 96 (
assert False) is both a lint failure (B011) and currently masked by the broadexcept Exceptionon Lines 101-102, so this test can pass when no exception is raised.Suggested fix
- try: - propose_claim( - store, text="some claim", evidence=[src.id], proposed_by="agent" - ) - assert False, "expected an exception" - except ProposalError as e: - assert "unknown source/evidence id" not in str(e), ( - f"real parse error was swallowed and replaced with misleading message: {e}" - ) - except Exception: - pass # any non-ProposalError is correct behaviour + with pytest.raises(Exception) as exc: + propose_claim( + store, text="some claim", evidence=[src.id], proposed_by="agent" + ) + if isinstance(exc.value, ProposalError): + assert "unknown source/evidence id" not in str(exc.value), ( + f"real parse error was swallowed and replaced with misleading message: {exc.value}" + )🤖 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/test_cli.py` around lines 92 - 102, The test currently uses assert False inside a try/except and a broad except Exception that can mask a missing exception (and triggers Ruff B011); replace the try/except with pytest.raises(ProposalError) as excinfo around the call to propose_claim(store, text="some claim", evidence=[src.id], proposed_by="agent") to ensure an exception is actually raised, then assert "unknown source/evidence id" not in str(excinfo.value) to preserve the original message check, and remove the broad except Exception branch; reference propose_claim and ProposalError to locate the code to change.
🤖 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.
Duplicate comments:
In `@tests/test_cli.py`:
- Around line 92-102: The test currently uses assert False inside a try/except
and a broad except Exception that can mask a missing exception (and triggers
Ruff B011); replace the try/except with pytest.raises(ProposalError) as excinfo
around the call to propose_claim(store, text="some claim", evidence=[src.id],
proposed_by="agent") to ensure an exception is actually raised, then assert
"unknown source/evidence id" not in str(excinfo.value) to preserve the original
message check, and remove the broad except Exception branch; reference
propose_claim and ProposalError to locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c439e4d2-0d68-4e00-8c0e-89c5c1442345
📒 Files selected for processing (3)
CHANGELOG.mdsrc/vouch/proposals.pytests/test_cli.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
The previous try/except pattern could pass silently if propose_claim unexpectedly succeeded, since assert False was caught by the broad except Exception. Replace with pytest.raises to ensure an exception is always raised. Suggested by CodeRabbit review of vouchdev#50.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_cli.py`:
- Around line 85-86: Tests currently trigger Ruff I001 due to function-local
imports; move these imports to the module scope in tests/test_cli.py. Ensure
pytest and propose_claim are imported at the top of the module (keep the
existing "import pytest" and "from vouch.proposals import propose_claim"), and
also add ProposalError to that same module-level import list (from
vouch.proposals import ProposalError, propose_claim) rather than importing
ProposalError inside any test function. This removes redundant local imports and
satisfies Ruff's import ordering/formatting rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| import pytest | ||
| from vouch.proposals import ProposalError, propose_claim |
There was a problem hiding this comment.
Move function-local imports to module scope to fix failing Ruff I001
These local imports are redundant and are currently breaking CI (ruff import formatting check). Keep pytest and propose_claim at module scope, and only add ProposalError there.
Suggested diff
import pytest
from click.testing import CliRunner
from vouch.cli import cli
-from vouch.proposals import propose_claim
+from vouch.proposals import ProposalError, propose_claim
from vouch.storage import KBStore
@@
def test_propose_claim_corrupt_source_surfaces_real_error(store: KBStore) -> None:
"""Corrupt meta.yaml must surface a parse error, not 'unknown source/evidence id'."""
- import pytest
- from vouch.proposals import ProposalError, propose_claim
-
src = store.put_source(b"evidence content")📝 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.
| import pytest | |
| from vouch.proposals import ProposalError, propose_claim | |
| def test_propose_claim_corrupt_source_surfaces_real_error(store: KBStore) -> None: | |
| """Corrupt meta.yaml must surface a parse error, not 'unknown source/evidence id'.""" | |
| src = store.put_source(b"evidence content") |
🧰 Tools
🪛 GitHub Actions: ci / 1_test (py3.12).txt
[error] 85-86: ruff (I001) Import block is un-sorted or un-formatted. Test file has imports inside the function; organize imports or run 'ruff check src tests --fix'.
🪛 GitHub Actions: ci / 2_test (py3.11).txt
[error] 85-86: ruff check failed (I001): Import block is un-sorted or un-formatted in tests/test_cli.py. Help: Organize imports.
🪛 GitHub Actions: ci / 3_test (py3.13).txt
[error] 85-86: ruff check failed (I001): Import block is un-sorted or un-formatted. Help: Organize imports.
🪛 GitHub Actions: ci / test (py3.11)
[error] 85-86: Ruff (python -m ruff check src tests): I001 Import block is un-sorted or un-formatted. Help: Organize imports.
🪛 GitHub Actions: ci / test (py3.12)
[error] 85-86: ruff check failed (I001). Import block is un-sorted or un-formatted. Organize imports. Found 1 error.
🪛 GitHub Actions: ci / test (py3.13)
[error] 85-86: Ruff check failed (I001): Import block is un-sorted or un-formatted. Help: Organize imports.
🤖 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/test_cli.py` around lines 85 - 86, Tests currently trigger Ruff I001
due to function-local imports; move these imports to the module scope in
tests/test_cli.py. Ensure pytest and propose_claim are imported at the top of
the module (keep the existing "import pytest" and "from vouch.proposals import
propose_claim"), and also add ProposalError to that same module-level import
list (from vouch.proposals import ProposalError, propose_claim) rather than
importing ProposalError inside any test function. This removes redundant local
imports and satisfies Ruff's import ordering/formatting rule.
Replace try/except pattern with pytest.raises(Exception) and assert
the raised exception is not a misleading ProposalError('unknown
source/evidence id'). Fixes Ruff B011 and ensures the test fails
if propose_claim unexpectedly succeeds.
Suggested by CodeRabbit review of vouchdev#50.
Updated — replaced the try/except pattern with pytest.raises(Exception)
125 tests pass. |
|
@plind-junior , ready to merge🫡 |
|
ASAP 😄.... As soon as those three are merged I should pull some more issues and requests, waiting at your approval on these first 💪 |
|
Fix MR conflict |
1 similar comment
|
Fix MR conflict |
|
One thing worth a second look before we merge: this branch quietly deletes |
All right |
…_claim
Asserts that a corrupt meta.yaml surfaces a real parse error rather
than being swallowed and replaced with a misleading
ProposalError('unknown source/evidence id').
Requested in review of vouchdev#50.
Fixes vouchdev#48
The previous try/except pattern could pass silently if propose_claim unexpectedly succeeded, since assert False was caught by the broad except Exception. Replace with pytest.raises to ensure an exception is always raised. Suggested by CodeRabbit review of vouchdev#50.
Replace try/except pattern with pytest.raises(Exception) and assert
the raised exception is not a misleading ProposalError('unknown
source/evidence id'). Fixes Ruff B011 and ensures the test fails
if propose_claim unexpectedly succeeds.
Suggested by CodeRabbit review of vouchdev#50.
9e8354a to
3057c3b
Compare
|
@plind-junior , fixed already..... Ready to get merged 🫡 |
|
PR should target test branch, not main |
…_claim
Asserts that a corrupt meta.yaml surfaces a real parse error rather
than being swallowed and replaced with a misleading
ProposalError('unknown source/evidence id').
Requested in review of vouchdev#50.
Fixes vouchdev#48
4dfdaef to
2074297
Compare
Replace try/except pattern with pytest.raises(Exception) and assert
the raised exception is not a misleading ProposalError('unknown
source/evidence id'). Fixes Ruff B011 and ensures the test fails
if propose_claim unexpectedly succeeds.
Suggested by CodeRabbit review of vouchdev#50.
Already fixed, but there's a blocker about the ci; |
|
Fix ci |
…laim
Bare except Exception on store.get_source() silently swallowed real
I/O and parse errors, masking them as a misleading
ProposalError('unknown source/evidence id'). Narrowing both except
clauses to ArtifactNotFoundError lets genuine errors propagate with
their original type and message intact.
Fixes vouchdev#48
Replace try/except pattern with pytest.raises(Exception) and assert
the raised exception is not a misleading ProposalError('unknown
source/evidence id'). Fixes Ruff B011 and ensures the test fails
if propose_claim unexpectedly succeeds.
Suggested by CodeRabbit review of vouchdev#50.
2074297 to
a0ee15c
Compare
Thanks for pointing that out. |
|
Excuse me, but why is the issue number 48 that was supposed to be closed by this pr still opened, just curious |
3 participants
Problem
The evidence validation loop in
propose_claim()caught bareExceptionon
store.get_source(). Any real error — YAML parse failure, permissionerror, disk I/O — was silently swallowed and fell through to
store.get_evidence(), which also failed, raising a misleadingProposalError("unknown source/evidence id: <eid>")even when the sourceexisted but was corrupt or unreadable. The real cause was completely
hidden from the caller.
Fix
Narrow both
exceptclauses fromExceptiontoArtifactNotFoundError.Only a genuinely missing artifact should produce the "unknown id" error.
All other exceptions now propagate with their original type and message
intact, making corruption and permission issues immediately visible.
Also adds
ArtifactNotFoundErrorto the imports inproposals.py.Tests
All 54 existing tests pass. No new failure paths were introduced —
the change only affects what gets caught, not the happy path.
Fixes #48
Summary by CodeRabbit
Bug Fixes
Tests