Repository navigation
fix(corrections): detect source fixes for missing values and align acceptance status - #346
Merged
Merged
Conversation
Replay skipped the current-value check when the recorded value was missing, so a correction of a blank cell silently overwrote a value later filled in at source. A missing recorded value is now checked like any other: the correction reapplies only while the cell is still missing, and fails with a reason otherwise. Applying such an entry checks the same. An accept row's Status was judged against the data at its position in the log, so a later correction of the same cell left it `Successful` while `get_active_acceptances` dropped it. Accept statuses are now judged against the final corrected data on replay and whenever log rows are appended. Closes #337 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Acceptance-status refresh can inspect the wrong record and incorrectly mark an active acceptance inactive.
1 open finding
What changed in this PR
Addresses #337, a v1.2.0 release blocker, by protecting source-fixed values during correction replay and aligning acceptance statuses with final corrected data.
Changes:
- Check missing recorded values before applying corrections.
- Refresh acceptance statuses after replay and log appends.
- Add regression tests and update documentation.
| File | Description |
|---|---|
| tests/processing/test_corrections.py | Adds missing-value and acceptance-status regressions. |
| src/datasure/processing/corrections.py | Updates correction validation and acceptance-status refresh. |
| docs/USER_GUIDE.md | Explains preservation of source-fixed values. |
| docs/ARCHITECTURE.md | Documents acceptance-status evaluation timing. |
| CHANGELOG.md | Records both fixes for #337. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Replay and acceptance-status refresh located a record by the first column holding its KEY value, so a value repeated in another column could point them at the wrong record and mark an active acceptance Inactive on save. Saves now pass through the key column their callers already have. Replay resolves it from the check config (survey_key, or the backcheck KEY for backcheck data), falling back to the old lookup only when none is set. Addresses Copilot review feedback on #346. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…g-value # Conflicts: # CHANGELOG.md
|
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
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.
Pull Request Summary 🚀
What does this PR do? 📝
Fixes both bugs in #337:
_current_value_mismatch: no longer skips a correction whose recorded value was missing. Missing still matches missing, so the correction applies only while the cell is still missing.refresh_corrected_data): accept rows are no longer judged mid-replay. After replay, their status is set against the final data, using the same rule asget_active_acceptances._append_log_rows): when the log has accept rows, their statuses are refreshed the same way.Closes #337
Why is this change needed? 🤔
Successfulwhile the check pages correctly treated it as inactive.Release blocker for v1.2.0 (#339).
How was this implemented? 🛠️
_values_matchalready treats missing as matching missing, so a correction of a missing value still applies while the cell is missing, and fails with "changed since this correction was recorded" once the source has a value._apply_entryapplies the same check when a correction is first saved, so an entry can't apply now and then fail on the next replay._with_acceptance_statusesjudges accept rows against the final corrected data. It runs after replay and on log saves that include accept rows.Points for review:
current_valuefrom the actual cell value, so a null in an existing log really means the cell was missing.Merge danger: two-way door. No log schema change. Blast radius: corrections. Replay results and Status values can change for existing logs, but only for the two cases above.
How to test or reproduce ? 🧪
Before / After: the reproductions from #337 are now tests in
tests/processing/test_corrections.py:Successful; nowInactive.Inactive.Successful.just test: 3912 passed, 6 skipped. On one earlier run,test_prep_regex.py::test_safe_regex_no_backtrackingfailed once and then passed, both alone and on the full rerun. It looks timing-dependent and unrelated.Screenshots (if applicable) 📷
N/A
Checklist ✅
status)Reviewer Emoji Legend
:code::smiley::+1::100:...and I want the author to know it! This is a way to highlight positive parts of a code review.
:star: :star: :star:And I am providing reasons why it needs to be addressed as well as suggested improvements.
:star: :star:And I am providing suggestions where it could be improved either in this PR or later.
:star:...and consider this a suggestion, not a requirement.
:question:This should be a fully formed question with sufficient information and context that requires a response.
:memo::pick:This does not require any changes and is often better left unsaid. This may include stylistic, formatting, or organization suggestions and should likely be prevented/enforced by linting if they really matter
:recycle:Should include enough context to be actionable and not be considered a nitpick.
🤖 Generated with Claude Code