refactor(casework): one evidence normaliser, not two copies - #440
Open
gaurav-karki wants to merge 2 commits into
Open
refactor(casework): one evidence normaliser, not two copies#440gaurav-karki wants to merge 2 commits into
gaurav-karki wants to merge 2 commits into
Conversation
Review feedback on #429: the duplication was justified in a docstring with "these are standalone scripts", and that reason does not hold. A casework script may import from shared and app modules; only the reverse is forbidden. Both scripts already do it -- `enrich_news_articles` imports `materials.jsonld`, and both import half of `casework/common/`. `current_evidence` and `merge_evidence` now live in `casework/common/evidence.py` and both writers import them. The copies differed in one respect: the binder appended `additional_details: ""` while the news stage appended a real note. The shared `merge_evidence` takes `(material_iri, note)` pairs, so the binder passes `""` explicitly at its call site rather than the difference living in a second function. This matters because `PATCH /evidence` is a destructive whole-list replace: the body IS the new list, so a normaliser that drops a field drops evidence rows from a published case. The old arrangement guarded that with a comment asking the next person to keep two copies in sync. The test that checked the comment existed is replaced by one asserting the two modules reference the SAME function objects -- identity, which cannot drift, instead of similarity, which can. Trimmed the docstrings on the lines this touched, per the new rule in CLAUDE.md, including two more `file.py:NNN` references. 4,618 passed, 5 skipped; ruff and ty clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code review on the previous commit: `casework/README.md` teaches the canonical `/evidence` write, and its snippet still passed bare IRI strings. Against the shared `merge_evidence` that is `ValueError: too many values to unpack` on the first real IRI -- so the one page whose job is to stop people destroying an evidence list handed them code that does not run. Its import also still named `casework.bind_materials`, which kept working as a re-export and would have gone on pointing readers at the old home. The review also spotted the quiet half, which is the worse one: a 2-character string unpacks into `iri="a", note="b"` and binds silently. Real IRIs are long so this never fires by accident, but a function guarding a destructive whole-list replace should not have a shape of input it accepts and corrupts. `merge_evidence` now rejects a bare string with a message naming the fix. Verified both: the long IRI and the 2-character one now raise, pairs are unaffected. Dropped a `file.py:NNN` reference in the same README paragraph while there. 4,619 passed, 5 skipped; ruff and ty clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds shared evidence normalization and merge helpers. Material binding and news enrichment now use these helpers. Tests validate tuple-based evidence additions, duplicate handling, note preservation, invalid inputs, and shared helper usage. ChangesEvidence utility consolidation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
🛠️ Relevant configurations:
[config] enable_ai_metadata: False
custom_model_max_tokens: 200000
git_provider: github
output_relevant_configurations: True
model: openai/cx/gpt-5.5
ENABLE_AUTO_APPROVAL: True
custom_reasoning_model: False
fallback_models: ['openai/cx/gpt-5.4-mini']
is_auto_command: True
publish_output: True
publish_output_progress: True
progress_gif_url:
progress_gif_width: 48
verbosity_level: 0
use_extra_bad_extensions: False
log_level: DEBUG
use_wiki_settings_file: True
use_repo_settings_file: True
use_global_settings_file: True
extra_config_url:
disable_auto_feedback: False
ai_timeout: 120
response_language: en-US
repo_context_files: ['AGENTS.md']
repo_context_from_default_branch: True
repo_context_max_lines: 500
max_description_tokens: 500
max_commits_tokens: 500
max_model_tokens: 32000
model_token_count_estimate_factor: 0.3
patch_extension_skip_types: ['.md', '.txt']
allow_dynamic_context: True
max_extra_lines_before_dynamic_context: 10
patch_extra_lines_before: 5
patch_extra_lines_after: 1
cli_mode: False
large_patch_policy: clip
duplicate_prompt_examples: False
seed: -1
temperature: 0.2
ignore_pr_title: ['^\\[Auto\\]', '^Auto', '^Bump ', '^chore\\(deps\\)']
ignore_pr_target_branches: []
ignore_pr_source_branches: []
ignore_pr_labels: []
ignore_pr_authors: []
ignore_repositories: []
ignore_language_framework: []
restricted_mode: False
reasoning_effort: medium
enable_claude_extended_thinking: False
extended_thinking_budget_tokens: 2048
extended_thinking_max_output_tokens: 4096
claude_extended_thinking_models_override: []
extract_issue_from_branch: True
branch_issue_regex:
enable_custom_labels: False
[pr_reviewer] require_ticket_analysis_review: False
require_score_review: False
require_tests_review: True
require_estimate_effort_to_review: True
require_can_be_split_review: False
require_security_review: True
require_estimate_contribution_time_cost: False
require_todo_scan: False
publish_output_no_suggestions: True
persistent_comment: True
extra_instructions: Focus on: logic errors and edge cases; security/authz regressions; missing error handling;
Django/DRF correctness (migrations, N+1 queries, transaction/atomicity, serializer & permission gaps).
Do NOT comment on formatting, import order, or naming — ruff handles those in CI.
num_max_findings: 3
final_update_message: True
enable_review_labels_security: True
enable_review_labels_effort: True
require_all_thresholds_for_incremental_review: False
minimal_commits_for_incremental_review: 0
minimal_minutes_for_incremental_review: 0
enable_intro_text: True
enable_help_text: False
|
PR Code Suggestions ✨Explore these optional code suggestions:
🛠️ Relevant configurations:
[config] enable_ai_metadata: False
custom_model_max_tokens: 200000
git_provider: github
output_relevant_configurations: True
model: openai/cx/gpt-5.5
ENABLE_AUTO_APPROVAL: True
custom_reasoning_model: False
fallback_models: ['openai/cx/gpt-5.4-mini']
is_auto_command: True
publish_output: True
publish_output_progress: True
progress_gif_url:
progress_gif_width: 48
verbosity_level: 0
use_extra_bad_extensions: False
log_level: DEBUG
use_wiki_settings_file: True
use_repo_settings_file: True
use_global_settings_file: True
extra_config_url:
disable_auto_feedback: False
ai_timeout: 120
response_language: en-US
repo_context_files: ['AGENTS.md']
repo_context_from_default_branch: True
repo_context_max_lines: 500
max_description_tokens: 500
max_commits_tokens: 500
max_model_tokens: 32000
model_token_count_estimate_factor: 0.3
patch_extension_skip_types: ['.md', '.txt']
allow_dynamic_context: True
max_extra_lines_before_dynamic_context: 10
patch_extra_lines_before: 5
patch_extra_lines_after: 1
cli_mode: False
large_patch_policy: clip
duplicate_prompt_examples: False
seed: -1
temperature: 0.2
ignore_pr_title: ['^\\[Auto\\]', '^Auto', '^Bump ', '^chore\\(deps\\)']
ignore_pr_target_branches: []
ignore_pr_source_branches: []
ignore_pr_labels: []
ignore_pr_authors: []
ignore_repositories: []
ignore_language_framework: []
restricted_mode: False
reasoning_effort: medium
enable_claude_extended_thinking: False
extended_thinking_budget_tokens: 2048
extended_thinking_max_output_tokens: 4096
claude_extended_thinking_models_override: []
extract_issue_from_branch: True
branch_issue_regex:
enable_custom_labels: False
[pr_code_suggestions] commitable_code_suggestions: False
dual_publishing_score_threshold: -1
focus_only_on_problems: True
extra_instructions: Prefer a few high-impact, project-specific suggestions over many generic ones.
Skip style/formatting (ruff-enforced) and changes under cases/migrations/.
enable_help_text: False
enable_chat_text: False
persistent_comment: True
max_history_len: 4
publish_output_no_suggestions: True
suggestions_score_threshold: 0
new_score_mechanism: True
new_score_mechanism_th_high: 9
new_score_mechanism_th_medium: 7
auto_extended_mode: True
num_code_suggestions_per_chunk: 3
max_number_of_calls: 3
parallel_calls: True
final_clip_factor: 0.8
decouple_hunks: False
demand_code_suggestions_self_review: False
code_suggestions_self_review_text: **Author self-review**: I have reviewed the PR code suggestions, and addressed the relevant ones.
approve_pr_on_self_review: False
fold_suggestions_on_self_review: True
num_code_suggestions: 4
|
PR Agent Walkthrough 🤖Welcome to the PR Agent, an AI-powered tool for automated pull request analysis, feedback, suggestions and more. Here is a list of tools you can use to interact with the PR Agent:
(1) Note that each tool can be triggered automatically when a new PR is opened, or called manually by commenting on a PR. (2) Tools marked with [*] require additional parameters to be passed. For example, to invoke the |
|
Auto-approved PR |
![jawafdehi-pr-agent[bot]](https://avatars.githubusercontent.com/u/241969530?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
User description
Follow-up to @damo-da's review comment on #429:
current_evidenceandmerge_evidenceexisted twice — once inbind_materials.py,once in
enrich_news_articles.py— with a docstring justifying the copy because"these are standalone scripts with no shared sequencing".
That reason does not hold, and the files disprove it themselves. A casework script
may import from shared and app modules; only the reverse is forbidden.
enrich_news_articlesalready importsmaterials.jsonld, a Django app module, andboth scripts already import half of
casework/common/.What changed
Both helpers now live in
casework/common/evidence.pyand both writers import them.Net −24 lines.
The copies were not identical: the binder appended
additional_details: "", the newsstage appended a real Nepali note. The shared
merge_evidencetakes(material_iri, note)pairs, so the binder passes""at its own call site. Thedifference is now one visible argument instead of a second function.
Why it is worth doing
PATCH /evidenceis a destructive whole-list replace — the body is the new list,so a normaliser that drops a field drops evidence rows from a published case. The old
arrangement guarded that with a comment asking the next person to keep two copies in
sync.
The test that checked the comment existed is replaced by one that checks the thing
that matters:
Identity, not similarity. Two copies can drift; the same function object cannot.
Second commit: what code review caught
casework/README.mdteaches the canonical/evidencewrite, and its snippet stillpassed bare IRI strings —
ValueError: too many values to unpackagainst the newsignature. The page whose job is to stop people destroying an evidence list handed
them code that does not run. Its import also still named
casework.bind_materials,which kept working as a re-export and would have gone on pointing readers at the old
home.
The quieter half is the worse one. A 2-character string unpacked into
iri="a", note="b"and bound silently. Real IRIs are long so it never fires byaccident, but a function guarding a destructive write should not have a shape of input
it accepts and corrupts.
merge_evidencenow rejects a bare string with a messagenaming the fix.
Verification
One thing to confirm
The shared code went into
casework/common/, next to its only two callers and besidethe other shared casework helpers. Your comment mentions django modules, so if you
meant
jawafdehi_shared/instead, that is a one-file move — say so and I will redo it.🤖 Generated with Claude Code
PR Type
Enhancement, Tests, Documentation
Description
Share
/evidencemerge helpersPreserve note differences via arguments
Reject bare IRI additions
Update tests, README recipe
Diagram Walkthrough
File Walkthrough
3 files
Import shared evidence helpersAdd shared evidence utilitiesReuse shared evidence merging2 files
Clarify blank note rationaleUpdate evidence merge recipe2 files
Cover pair-based evidence mergeAssert shared helper identity🛠️ Relevant configurations:
These are the relevant configurations for this tool:
[config]
[pr_description]
Summary by CodeRabbit
Bug Fixes
Documentation
Tests