Skip to content

Deferred low-priority polish from review-severity-binary (PRs #168–#173) #175

Description

@brent-hoover

Tracking issue for the Low / advisory findings deferred during the review-severity-binary stack (PRs #168#173). All correctness and feature-breaking issues from those PRs were fixed and verified across multiple review rounds; the items below are non-blocking polish surfaced by roborev and the GitHub review bot, consciously deferred so the stack could land. None affect correctness or CI.

Source attribution in each item (roborev job numbers, bot reviews). Knock these off individually or in a small cleanup PR — split this issue if you'd prefer separate tracking.

Observability

Stuck-watchdog refinement

  • An unrelated pending-proposed ticket can mask a genuine deadlock. _check_stuck's has_pending_proposed treats any non-review-notable PROPOSED ticket as "alive", so a front-door issue with no dependency relationship to a stuck cycle (x → y → x) suppresses project_stuck until that unrelated issue is approved/rejected. Tighten so a proposed ticket only counts as alive when it's actually a dependency of a stuck candidate (approving it could unblock something). (jig/orchestrator.py _check_stuck; roborev 586.)

Eval-runner robustness

  • analysis_dir discarded on the build-failure COMPLETED_WITH_FAILURES path. If analysis_complete arrived before the build ran, analysis_out_dir is set but the early-return manifest path returns analysis_dir=None, dropping the analysis artifact from the result. Carry it through. (jig/eval/runner.py; roborev 587.)
  • Tracer exit-nonzero return site untested with tickets_failed > 0. The manifest.tracer is None site is covered; the tracer-nonzero-exit site (same fail_outcome variable) isn't exercised on the COMPLETED_WITH_FAILURES path. Patch _check_tracer_outcome to return TRACER_FAIL on a _FailWS-style run and assert COMPLETED_WITH_FAILURES. (tests/test_eval_runner.py; feat(eval): classify ticket-failure and stuck outcomes distinctly #170 bot, minor.)

Cleanup nits

  • Stale docstring in _run_review_federation. Still reads "notable comments … RESOLVED but deferred via the Coordinator (DEFERRED queue)" — the pre-binary-severity wording. Actual behavior is "filed as proposed issues." (jig/orchestrator.py ~line 781; roborev 543/585.)
  • Unused coord locals in disposition tests. tests/test_reviewers_disposition.py still unpacks coord from _build_stores in several TestCriticalDisposition/TestImportantDisposition cases though apply_severity_disposition no longer takes a coordinator. Harmless today (CI lints only jig/, not tests/) but ruff F841 fires if tests/ is ever linted. (roborev 585.)
  • Private cross-module import from jig.worktree import _run_git. _worktree_head reaches into a private symbol; a thin public async def get_head_commit(path: Path) -> str in jig.worktree would be cleaner and avoid silent degradation (the except Exception → return None fallback) on a future rename. (jig/orchestrator.py; feat(review): multi-pass first review, delta-scoped re-review #171 bot, low-priority refactor.)
  • Misleading test comment. test_sweep_fails_late_ticket_with_failed_dep comments that it verifies blame attribution to the original failed dependency, but only asserts a ticket_failed event exists — the root_failure_id attribution is actually pinned by the separate test_sweep_blame_attributes_to_original_dep. Reword the comment. (tests/test_orchestrator_failure_cascade.py; roborev 586.)

🤖 Generated with Claude Code

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    code-qualitysuite:enforcementEnforcement suite — mechanical checks, reviewers, severity, gates

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions