post-merge bugfixes: 7 real issues found by deep review of PR #11#12
Merged
Conversation
Found via deep review of the v29/v30 merge. Seven real issues fixed: 1. **Branch-exists checks always returned False.** `_run_cmd` returns the stripped stdout *string*, but `_branch_exists_locally` / `_branch_exists_remotely` were calling `getattr(result, "stdout", "")` on that string — which always yields `""`. The disambiguation tests passed only because they mocked the predicates directly. Fixed both helpers. 2. **`_run_lock` was misused as a non-context-manager.** `main()` called `cm.__enter__()` without ever calling `__exit__`, so the generator's `try: yield ... finally: _release()` block never fired on the exception path. Re-wired through `contextlib.ExitStack` and registered `stack.close` with `atexit`. The `_release` helper is now also idempotent (guarded by a flag) so the atexit + finally double-fire can't close a reused fd. 3. **Engine-fallover stale reference.** The rate-limit `while` loop updated `self.engine` only inside the loop body, leaving the verify/fix path on the stale engine when the loop exited. Now `self.engine` is re-bound from `self._engines[0]` before each `execute_chunk` and after each pop. 4. **Postmortem markdown allowed log-fence injection.** Log lines and ledger-supplied chunk titles were embedded raw into the markdown body. Now backticks in the log tail are split with a zero-width joiner and chunk titles are stripped of newlines / backticks before rendering. 5. **`enforce_token_budget` was O(n²) and ordered sub-chunks wrong.** `list.pop(0)` is O(n); a recursion that splits 50 chunks could spend most of its time shifting the queue. Switched to `collections.deque` with `popleft` / `appendleft` and clarified the comment about dependency ordering. 6. **AuditLogger could hang on a peer process's slow rotation.** The cross-process flock used the blocking `LOCK_EX` mode. Replaced with a non-blocking acquire + bounded retry (3 attempts × 10 ms); on timeout we log a one-shot warning and proceed with intra-process locking only. 7. **`verify_paths` ran pytest as bare `python -m pytest`.** That picks up whatever `python` is first on PATH — potentially Python 2 or a different venv. Now uses `sys.executable`, matching the existing `check_tests` convention. Quality: - pytest 1928 passed - ruff check / format clean - No public API changes; no test-fixture rewrites needed Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
After PR #11 (v29 + v30 closure) merged, a deep reviewer pass surfaced seven real bugs that the test suite missed because either the affected code path wasn't exercised end-to-end or the tests mocked the broken helpers directly. All seven are fixed here.
Fixes
_branch_exists_locally/_branch_exists_remotelyalways returnedFalse— they calledgetattr(result, "stdout", "")on a value that was already a string (_run_cmdreturns stripped stdout, not aCompletedProcess). Disambiguation never triggered in production. Tests passed because they mocked the predicates._run_lockwas misused as a non-context-manager inmain(), so on any uncaught exception between__enter__and the natural exit, the lock was never released. Re-wired throughcontextlib.ExitStack._releaseis now also idempotent — the prior atexit + finally double-fire could close a reused fd.self.enginestale when the rate-limit loop exited. The verify/fix path then ran on the wrong engine. Nowself.engineis re-bound fromself._engines[0]after every pop.enforce_token_budgetusedlist.pop(0)(O(n)) and inserted at index 1 in a way that scrambled order under recursion. Replaced withcollections.dequefor O(1) front-ops and correct sub-chunk ordering.LOCK_EX, so a peer process doing a slow rotation could hang the orchestrator's main loop indefinitely. Now usesLOCK_EX | LOCK_NBwith 3 × 10 ms retry and falls back to intra-process locking on timeout.verify_pathsran pytest as barepython -m pytest— picks up whateverpythonis first on PATH. Now usessys.executable, matchingcheck_tests.Quality
pytest -q --no-cov→ 1928 passed (no regressions, no new tests required for these fixes — they are corrections to plumbing the existing tests don't reach end-to-end)ruff check/ruff formatcleanTest plan
codelicious build, thencodelicious buildagain — confirm.codelicious/run.lockis gone🤖 Generated with Claude Code