Make contrib-screen fail gracefully, report partial index results - #23
Open
nishantkluhera wants to merge 2 commits into
Open
Make contrib-screen fail gracefully, report partial index results#23nishantkluhera wants to merge 2 commits into
nishantkluhera wants to merge 2 commits into
Conversation
Rebuild of the issue-2 work on top of origin/main's expanded plugin (record_outcome, org-graph MCP server, dashboard, build_env): - github.py: read-timeouts (bare TimeoutError), dropped connections (OSError), and mid-body truncations (http.client.IncompleteRead, an HTTPException not an OSError) all normalize to retryable GitHubError - handlers catch one transport type - org_index.sync_org: per-repo isolation for GitHubError AND local sqlite3.Error; partial sweeps keep prior successes searchable and report failed_repos - index handler: repos_indexed/failed_repos/note partial contract, preserving upstream's mcp_server keys; empty-org is its own error; all-failed carries per-repo detail; MCP registration only fires when something actually indexed - screen: API failure = distinct retryable tool_error, never a verdict; signed_orgs validated - search/_voice: limit parsed without the falsy-zero trap (0 must reach validation, same bug class as claim's old ttl_hours or 2); corrupt DB = recovery-step error - claim: unwritable dir/file/replace refuse to fake exclusivity Tests ported onto the merged 132-test suite's conventions (~20 new). Two upstream test adaptations documented inline: mcp-registration test now indexes one real repo (zero-repo indexing is now an error) and exposes its fake's latent repo-shape mismatch. The two test_find_dockerfiles_* failures are pre-existing on clean main (Windows path handling in build_env) - verified by stashing. Closes #2
…cion
- sync_org: store.db.rollback() in both failure branches. Without it,
a repo failing mid-sync_repo (rate limit landing after
list_repo_issues already wrote rows) left its pending writes on the
shared transaction, and the NEXT successful repo's commit() published
them - a 'failed' repo partially indexed and searchable, directly
contradicting the failed_repos note. Prior successes are durable
(each committed by its own sync_repo), so rollback discards exactly
the failing repo's partials. Regression test asserts bad's issue is
absent from the db while ok/fine persist.
- _coerce_positive_int: reject bools (int(True)==1 slipped through as
a valid limit) and catch OverflowError (int(float('inf')) escaped
the TypeError/ValueError net into the registry catch-all)
- comment corrections: empty repos:[] never reaches the no-results
branch (upstream coerces it to whole-org); test comment no longer
overstates the fake's prior exercise
Found by second-pass code review with a live reproduction.
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.
Audit of all 5 tools' failure modes, fixes included, rebuilt on top of the post-#11 plugin (record_outcome, org-graph MCP server, dashboard, build_env).
README gains a Failure behavior section. ~20 failure-path tests ported onto the merged suite's conventions. Two pre-existing test_find_dockerfiles_* failures are unrelated (Windows path handling in upstream build_env, verified on clean main).
Closes #2
Merge order: this PR first; #3->#10 in the chain retarget after each merge.