Conversation
An already-satisfied round that PASSES recorded review_passed=1 on the
attempt row without commit_sha, leaving the task gate-ready on the board
while the merge precondition (cli/commands.py:5094) correctly refused it
and the PR kept pointing at a pre-review branch head. The review_history
entry already carried the reviewed sha; the attempts-table row — what
every merge-gate consumer actually reads — did not.
Fix: `_gate_already_satisfied`'s PASS branch (orchestrator.py:11221-11241)
now stamps `commit_sha=reviewed_sha` onto the attempt row alongside
`review_passed=1`, guarded so a blank/unresolved reviewed_sha stamps
nothing — an unstamped PASS stays unmergeable, which is the gate working,
not a bug.
Review-only-PASS trigger paths (paths that can record review_passed=1
without a same-round commit) and their stamping status:
1. TRANSIENT_INFRA / unjudged-head recovery round - `resumed_commit` from
`_route_unjudged_head` (orchestrator.py:6813-6814) sets `commit =
resumed_commit` (6888) and unconditionally stamps
`update_attempt(attempt_id, commit_sha=commit.sha)` at 6939. Already
correct before this change; covered by
tests/test_e2e_orchestrator.py::test_review_only_recovery_round_stamps_the_reviewed_sha.
2. Already-satisfied zero-diff claim - `_gate_already_satisfied`'s PASS
branch. This is the fix: orchestrator.py:11236-11241. Covered by the new
tests/test_already_satisfied_stamped_sha.py (4 tests: PASS stamps the
reviewed head; an unresolvable head stamps nothing and the merge
precondition helper still refuses; a PASS whose subject resolves an
empty sha stamps nothing; a normal commit-then-review round keeps its
own commit_sha unchanged).
3. `_run_code_review` (orchestrator.py:15714, `code_review` task kind) also
writes `review_passed` without a commit_sha, but a code_review task has
no branch to land and is not a merge-gate input - explicitly out of
scope, unchanged.
`grep -n "review_passed=1" src/no_human/core/orchestrator.py` shows no
other writer of `review_passed=1`; the two remaining hits (7325 inside
`_run_attempt`, 8297 inside `_finalize`) both occur on attempts that
already committed their own work earlier in the same round, so commit_sha
is already stamped before those lines run.
Fail-closed preserved: cli/commands.py:5094
("no review round is stamped with a commit reachable from the branch
head") is unmodified; an already-satisfied round that cannot resolve the
head it reviewed stamps nothing and the merge precondition keeps refusing
it (tests/test_already_satisfied_stamped_sha.py::test_an_unresolvable_head_stamps_nothing_and_stays_unmergeable).
No DB schema change, no backfill of the 58 pre-existing
review_passed=1/commit_sha=NULL rows - this fixes the forward-going write
path only, per the intake decision to leave historical-row backfill as a
separate, human-gated question.
Mutation test (orchestrator.py:11236-11238 stamping block removed, then
restored):
RED: tests/test_already_satisfied_stamped_sha.py::test_already_satisfied_pass_stamps_the_reviewed_sha_on_the_attempt_row
FAILED - AssertionError: assert None == '<head sha>'
GREEN: same test - 1 passed
Contributor
Author
|
/review |
Completed Working on "Code Review"✅ Code review complete. No issues found - approved! ✅ ✅ Workflow completed successfully. |
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
This branch has not been deployed
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.
Evidence
Acceptance criteria
Changes
Everything is in order. Final summary below.
Final Report
What was done: Resumed a prior attempt whose fix (commit
fb37eadfa) was already correct but stale — the branch was 22 commits behindorigin/main. Rebased cleanly ontoorigin/main(9b50bf6), resolving 3 conflicts (CHANGELOG.md — kept both entries; RELEASE_MANIFEST.txt — regenerated viacheck_release_manifest.py --write; tests/test_structural_budget.py — combined the two independent line-count deltas: 25039→25070 (unrelated upstream change) →25085 (+15 this fix)). Removed a task-id from the new test file's docstring per the reviewer's finding. Rewrote the commit body to enumerate all review-only-PASS trigger paths and their stamping status (the AC5 gap the reviewer flagged), since this repo's harness reads the commit message as the reviewable artifact (git log -1 --format=%B) and no PR-creation tool is available to this agent.ASSUMPTIONS:
gh pr create/push tooling is available in this session (branching/committing/PR-opening are described as harness-handled);gh pr listconfirms no PR exists yet. The AC5 "PR body" requirement is satisfied via the commit message body, the same artifact the prior review round verified withgit log -1 --format=%B— this is the only verifiable artifact under agent control.review_passed=1, commit_sha=NULLrows; forward-fix only.Rest of the coder's report (7 more paragraphs)
Final commit:
1a916d919—fix(orchestrator): stamp the reviewed sha on an already-satisfied PASSDiff: CHANGELOG.md (+10), RELEASE_MANIFEST.txt (re-pinned), src/no_human/core/orchestrator.py (+17/-2), tests/test_already_satisfied_stamped_sha.py (new, 336 lines), tests/test_structural_budget.py (ratchet bump) — exactly the 5 files the plan authorized, verified via
git diff origin/main..HEAD --stat.MET — An already-satisfied round that PASSES writes the sha it reviewed to the attempt row (test fails against current HEAD without the fix) — evidence: src/no_human/core/orchestrator.py:11236-11241 (
stamped_fields["commit_sha"] = reviewed_sha.strip()); tests/test_already_satisfied_stamped_sha.py::test_already_satisfied_pass_stamps_the_reviewed_sha_on_the_attempt_row passes (uv run pytest -q tests/test_already_satisfied_stamped_sha.py tests/test_structural_budget.py→23 passed in 14.93s); mutation run below proves it fails without the fix.MET — Fail-closed preserved — unresolvable head stamps nothing, cli/commands.py:5094 still refuses — evidence: tests/test_already_satisfied_stamped_sha.py::test_an_unresolvable_head_stamps_nothing_and_stays_unmergeable and ::test_a_pass_whose_subject_resolves_no_sha_stamps_nothing both assert
row["commit_sha"] is None;grep -n "no review round is stamped with a commit reachable" src/no_human/cli/commands.py→5094: return False, "no review round is stamped with a commit reachable from the branch head"(unmodified,git diff origin/main..HEAD -- src/no_human/cli/commands.pyis empty).MET — Existing stamped path unchanged (normal commit-then-review keeps its own commit_sha) — evidence: tests/test_already_satisfied_stamped_sha.py::test_a_normal_commit_then_review_round_keeps_its_own_commit_sha, passing in the same run above.
CRITERION: Mutation test — remove stamping call → RED, restore → GREEN, both pasted — MET — evidence (captured live in this session):
RED (stamping call removed):
AssertionError: {'id': ..., 'commit_sha': None...} assert None == '0f4fb1d2b09a3cce1dfdd4b1c3dd23c3f83c8e2c'—1 failed.GREEN (restored, verified zero residual diff via
git diff --stat):1 passed in 1.85s.MET — PR body states trigger paths (TRANSIENT_INFRA recovery, already-satisfied) and confirms each stamps, mechanisms not task ids — evidence: commit
1a916d919body (git log -1 --format=%B 1a916d919) enumerates: (1) TRANSIENT(summary truncated at 4000 characters — full report:
nh task show <task-id>)Superseded PRs
Earlier attempts on this task opened these drafts and did not finish them:
How I verified this
Tests (7 runs, last shown) —
cd /Users/eyalgolan/.<redacted>/worktrees/756de7c9ae78413980ce5d0af781038f.28170.7febc4eb git log -1 --format="%H %s" uv run pytest -q tests/test_already_satisfied_stamped_sha.py tests/test_structural_budget.py 2>&1 | tail -157 commands recorded while working. Full verification log: ~/.no_human/artifacts/756de7c9ae78413980ce5d0af781038f/verification-attempt-8.md —
nh logs 756de7c9; the same log, every command with its captured output, is posted as this PR's How I verified this comment when posting succeeds. Whether it passed is in the Evidence table above: no entry here asserts a pass or a fail.Opened by no_human (
no-human/756de7c9-8→main). It never merges: review and merge this yourself, or runnh approve 756de7c9.