fix: resolve post-integration review findings (closes #1656)#1657
Merged
curdriceaurora merged 6 commits intoJul 24, 2026
Conversation
Four findings from the cross-surface parity integration review, in
order of concreteness:
1. job_repo.py carry-forward: mirror lifecycle.py's behavior of
preserving prior error fields when transitioning to ROLLING_BACK,
ROLLED_BACK, or RECOVERY_REQUIRED without a new error argument.
Adds a DB-path regression test asserting error preservation across
those three transitions.
2. verify_claims_freshness.py anchor self-consistency: add
build_capability_anchor_map() that extracts the capability→anchor
mapping from the parsed matrix document. Drive the ID-anchor check
in _verify_matrix_links() from the map rather than re-deriving the
anchor via cap_id.replace('.', ''), so the checker validates against
the real document instead of an independent formula.
3. TUI evidence narrowed: remove OrganizationPreviewView from the
entry_points of organization.execute/tui and
organization.preview/tui in the registry. The conformance driver
exercises TUIOrganizationAdapter directly; the widget layer is not
reachable by the corpus. Regenerate capability-matrix.md to match.
4. Registry verified-flag guard: add test_verified_flags_are_backed_by_
corpus_evidence() with _CORPUS_COVERED as the authoritative set of
exercised (capability_id, surface) pairs. Any new verified flag
outside that set fails CI until corpus coverage is added or the set
is explicitly updated.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JVm2ys4rBjNALZCUNoyYrx
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe changes align TUI capability metadata with adapter entry points, validate README anchors against generated matrix anchors, guard verified capability coverage, preserve job errors during rollback transitions, and add TTL expiration to stored organization plans. ChangesCapability metadata and validation
Job rollback error retention
Organization plan expiration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ 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 |
_prune_plan_store now sweeps expired plans (older than ORGANIZE_PLAN_TTL_SECONDS = 3600s) before falling back to count-based eviction. _get_organize_plan returns None and eagerly evicts plans whose TTL has elapsed, preventing stale plan data from persisting across the full store-limit window. Covers the P2 code-review finding that the store lacked time-based expiration. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVm2ys4rBjNALZCUNoyYrx
Records injected directly into _ORGANIZE_PLAN_STORE without a created_at key (as in the existing regression test) now treat themselves as freshly created (effective age 0s), so they are never TTL-expired and count-based pruning still handles them. Fixes the KeyError: 'created_at' failure in test_web_organize_helper_validation_branches on py3.14. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVm2ys4rBjNALZCUNoyYrx
The integration coverage floor for organize_services.py dropped to 89.6% after the TTL addition because the new branches (expired-plan loop in _prune_plan_store, TTL guard in _get_organize_plan) weren't hit by any integration-marked test. Add test_plan_store_ttl_evicts_expired_records to the existing regression file to cover both paths under TTL=-1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVm2ys4rBjNALZCUNoyYrx
curdriceaurora
marked this pull request as ready for review
July 24, 2026 00:30
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/file_organizer/web/organize_services.py`:
- Around line 108-112: Update _get_organize_plan to retrieve created_at with the
same tolerant fallback used by _prune_plan_store, removing and returning None
for records with missing or invalid timestamps instead of raising KeyError.
Preserve the existing TTL expiration behavior, and add a regression test
covering lookup of a stored plan without created_at.
In `@tests/integration/test_job_repo_integration.py`:
- Around line 198-225: Extend test_rollback_states_preserve_prior_error_fields
to assert error, error_code, error_retryable, and error_details_json after
ROLLED_BACK, matching the existing ROLLING_BACK assertions. Add a
RECOVERY_REQUIRED transition and assert all four error fields there as well,
preserving the original values throughout each target state.
In `@tests/web/test_organize_services.py`:
- Around line 195-201: Update tests/web/test_organize_services.py lines 195-201
in test_plan_store_ttl_expiry_returns_none to store the record with a valid TTL,
modify its created_at timestamp to make it expired, then assert
_get_organize_plan returns None and clean up the ID. Update
tests/integration/test_executable_plan_review_regressions.py lines 354-371 to
separately age records for _prune_plan_store() and _get_organize_plan(),
preserving both records through storage and cleaning up both IDs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 842507c8-0ff7-40ac-bab6-d0b9c1fbf4c9
📒 Files selected for processing (9)
docs/developer/capability-matrix.mdscripts/verify_claims_freshness.pysrc/file_organizer/api/repositories/job_repo.pysrc/file_organizer/core/capability_registry.jsonsrc/file_organizer/web/organize_services.pytests/core/test_capabilities.pytests/integration/test_executable_plan_review_regressions.pytests/integration/test_job_repo_integration.pytests/web/test_organize_services.py
💤 Files with no reviewable changes (1)
- src/file_organizer/core/capability_registry.json
Comment on lines
+108
to
+112
| now = datetime.now(UTC) | ||
| if (now - plan["created_at"]).total_seconds() > ORGANIZE_PLAN_TTL_SECONDS: | ||
| _ORGANIZE_PLAN_STORE.pop(plan_id, None) | ||
| return None | ||
| plan["updated_at"] = now |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle missing created_at during retrieval.
_prune_plan_store() tolerates missing timestamps via .get(...), but _get_organize_plan() still evaluates plan["created_at"]. A legacy or malformed stored record can therefore raise KeyError during lookup. Reuse the same fallback/removal policy here and add a regression test.
🤖 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 `@src/file_organizer/web/organize_services.py` around lines 108 - 112, Update
_get_organize_plan to retrieve created_at with the same tolerant fallback used
by _prune_plan_store, removing and returning None for records with missing or
invalid timestamps instead of raising KeyError. Preserve the existing TTL
expiration behavior, and add a regression test covering lookup of a stored plan
without created_at.
- Use .get("created_at", now) fallback in _get_organize_plan to match
the defensive pattern already in _prune_plan_store; prevents KeyError
on any malformed record without a timestamp.
- Rewrite both TTL tests to store a plan normally then age its
created_at to a past datetime, so the tests exercise the actual
TTL-expiry code path rather than the already-proven "plan missing
from store" branch caused by immediate eviction during store.
- Add error_retryable and error_details_json assertions after
ROLLED_BACK in test_rollback_states_preserve_prior_error_fields and
add a full RECOVERY_REQUIRED transition path (running → failed →
rolling_back → recovery_required) asserting all four error fields.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JVm2ys4rBjNALZCUNoyYrx
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JVm2ys4rBjNALZCUNoyYrx
curdriceaurora
pushed a commit
that referenced
this pull request
Jul 24, 2026
- Use .get("created_at", now) fallback in _get_organize_plan to match
the defensive pattern already in _prune_plan_store; prevents KeyError
on any malformed record without a timestamp.
- Rewrite both TTL tests to store a plan normally then age its
created_at to a past datetime, so the tests exercise the actual
TTL-expiry code path rather than the already-proven "plan missing
from store" branch caused by immediate eviction during store.
- Add error_retryable and error_details_json assertions after
ROLLED_BACK in test_rollback_states_preserve_prior_error_fields and
add a full RECOVERY_REQUIRED transition path (running → failed →
rolling_back → recovery_required) asserting all four error fields.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JVm2ys4rBjNALZCUNoyYrx
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
2 participants
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.
Closes #1656.
Resolves the four findings captured after PR #1655 merged.
Changes
1. job_repo.py — error-field carry-forward for rollback states
JobRepository.update_statusnow mirrorslifecycle.py's behavior: when transitioning toROLLING_BACK,ROLLED_BACK, orRECOVERY_REQUIREDwithout a newerrorargument, the prior error fields are carried forward rather than nulled. Adds a DB-path regression test asserting preservation across those transitions.2. verify_claims_freshness.py — anchor self-consistency
Adds
build_capability_anchor_map()which extracts the capability→anchor mapping from the parsed matrix document._verify_matrix_links()now drives its ID-anchor check from that map instead of re-deriving the anchor viacap_id.replace(".", ""), so the checker validates against the real document rather than an independent formula.3. Capability registry — TUI evidence narrowed
Removes
OrganizationPreviewViewfromentry_pointsfororganization.execute/tuiandorganization.preview/tui. The conformance driver exercisesTUIOrganizationAdapterdirectly; the widget layer is not reachable by the corpus. Regeneratescapability-matrix.mdto match.4. test_capabilities.py — verified-flag guard
Adds
test_verified_flags_are_backed_by_corpus_evidence()with_CORPUS_COVEREDas the authoritative set of exercised(capability_id, surface)pairs. Any newverifiedflag outside that set now fails CI until corpus coverage is added or the set is explicitly updated.Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Quality Improvements