feat(devtools): add bead-landing-check, empty-diff evidence verification - #3424
feat(devtools): add bead-landing-check, empty-diff evidence verification#3424Sinity wants to merge 4 commits into
Conversation
Problem: five open beads (o4j2, hiu, 0jf4, pbuh, cijx.4) each cost a dispatched agent a full investigation cycle (30-60+ min) on 2026-07-30/31 before turning out to describe work already completed on master. A one-off manual sweep of the 606-open-bead backlog is not a repeatable fix. Solution: devtools workspace bead-landing-check extracts commit hashes, PR numbers, and file paths cited in a bead's description/design/AC/notes/ close_reason/comments, then verifies them two ways: cherry-picks cited commits onto master in a single reused throwaway worktree (empty diff, or a per-file content-equivalence fallback when the raw cherry-pick conflicts, both prove landing regardless of squash-merge id rewriting -- more reliable than `git log --is-ancestor` or grepping for issue ids), and checks cited PR merge state via `gh pr view` (disk-cached, MERGED results reused permanently). Emits LIKELY-STALE / LIKELY-LIVE / UNDETERMINED per bead with a confidence tier and the evidence, never auto-closing anything. Beads with no cited evidence are reported UNDETERMINED, not guessed -- verified against o4j2 (closed, correctly flagged LIKELY-STALE from its cited merged PRs) and against hiu/0jf4/pbuh/cijx.4 (still open, correctly UNDETERMINED because the resolving evidence was never written back into the bead text itself -- exactly why they cost a fresh investigation cycle). Registered as a CommandSpec in devtools/command_catalog.py per the documented devtools-command pattern; docs/devtools.md regenerated via `devtools render devtools-reference`. Verification: ruff check/format, mypy --strict (clean), devtools render all --check (no drift). Manual runs against the five motivating beads confirm correct verdicts including a real does-not-apply/content-divergence case (polylogue-0jf4's cited commits landed but were further edited afterward -- tool correctly reports UNDETERMINED rather than a false empty-diff match). Co-Authored-By: Claude <noreply@anthropic.com>
…tree safety Covers: PR/commit/file evidence extraction (including the pull-URL false-positive fix), the LIKELY-STALE/LIKELY-LIVE/UNDETERMINED verdict matrix (empty-diff, already-on-master, content-equivalent, non-empty-diff, merged/open PR, unresolvable/conflicted evidence -- each must not be guessed into a confident verdict), CommitChecker against a real throwaway git repo (unknown-revision, already-on-master, non-empty-diff, and an empty-diff case built from independently-converging history to model a squash-merge equivalent), worktree reuse across checks, remove_worktree's live-process guard (spawns a real subprocess with cwd inside the worktree and confirms removal is blocked and the directory survives), and PrChecker disk-cache/offline/refresh semantics with `_run` mocked out (no live gh calls in tests). Verification: devtools test tests/unit/devtools/test_bead_landing_check.py -- 30 passed. ruff check/format and mypy --strict clean on the new file. Co-Authored-By: Claude <noreply@anthropic.com>
Problem: the first full sweep over the live backlog (629 open/in_progress
beads) flagged 174 "strong" LIKELY-STALE verdicts, but spot-checking them
found systematic false positives. This repo's prework-packet convention
writes "Generated from master @ <hash>" (185+ occurrences) and
verification-pass notes write "re-checked ... against current master
(<hash>)" -- both record what master looked like when a note was written,
not that the cited hash is the bead's own resolving commit.
Confirmed false positive: polylogue-lkrc cites its own verification-pass
master snapshot this way in a note that explicitly says the named gaps are
STILL open -- the tool nonetheless flagged it LIKELY-STALE (strong) because
the snapshot hash is, trivially, an ancestor of current master.
Fix: extract_evidence() now excludes a hex token when the ~24 characters
immediately preceding it match "master @", "master (", "master@", or bare
"master " (case-insensitive) -- this repo's snapshot-anchor idiom. Re-run
over the same 629 beads: strong LIKELY-STALE dropped from 174 to 71,
UNDETERMINED rose from 351 to 435 (favoring "not verifiable" over a false
positive, per the anti-vacuity requirement that a wrong LIKELY-STALE risks
dropping real work). Also added a standing CAVEAT reason line on every
LIKELY-STALE verdict: proving a cited commit/PR exists on master is not
proof every acceptance criterion is satisfied.
Verification: devtools test tests/unit/devtools/test_bead_landing_check.py
-- 33 passed (3 new regression tests reproducing the lkrc/2qx false
positives and a still-genuine self-citation case). ruff check/format and
mypy --strict clean.
Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds ChangesBead landing verification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BeadLandingCheck
participant CommitChecker
participant GitWorktree
participant PrChecker
participant GitHubCLI
BeadLandingCheck->>CommitChecker: verify cited commits
CommitChecker->>GitWorktree: check ancestry or content equivalence
GitWorktree-->>CommitChecker: commit result
BeadLandingCheck->>PrChecker: inspect cited PRs
PrChecker->>GitHubCLI: query merge state
GitHubCLI-->>PrChecker: cached or refreshed PR result
BeadLandingCheck-->>BeadLandingCheck: compute and render verdict
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7cd95d52a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._ensure_worktree() | ||
| self._reset_worktree() | ||
| result = _run(["git", "cherry-pick", "--no-commit", "--allow-empty", commit], cwd=self.worktree_dir) |
There was a problem hiding this comment.
When two agents run the default full-backlog check concurrently, both processes reuse .cache/bead-landing-check/worktree, but the reset, cherry-pick, diff, and cleanup operations are not protected by a lock. One process can reset the index after the other's cherry-pick but before its diff, causing that commit to be falsely classified as empty-diff, or it can replace the staged changes with another commit. Serialize the complete worktree transaction or allocate a per-process worktree.
Useful? React with 👍 / 👎.
| if not self._is_real_commit(commit): | ||
| return "unknown-revision" | ||
| if self._is_ancestor(commit): | ||
| return "already-on-master" |
There was a problem hiding this comment.
In a checkout whose remote-tracking refs have not been fetched recently, a valid cited commit that has landed on origin/master but is absent from the local object database is immediately returned as unknown-revision. The fetch only happens later inside _is_ancestor, which this early return never reaches, so the advertised check against current master becomes UNDETERMINED until the user manually fetches. Fetch first, or retry cat-file after fetching before rejecting the hash.
Useful? React with 👍 / 👎.
| def check(self, number: int) -> PrResult: | ||
| key = str(number) | ||
| cached = self._cache.get(key) |
There was a problem hiding this comment.
When --repo-slug is used to check a fork or another repository, this cache key contains only the PR number. A result previously cached for Sinity/polylogue—especially a permanently cached MERGED result—is therefore reused for the same PR number in the other repository without calling gh, potentially producing a false LIKELY-STALE verdict. Include repo_slug in the cache key or use a repository-specific cache file.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@devtools/bead_landing_check.py`:
- Around line 164-165: Update _run to catch subprocess.TimeoutExpired and
FileNotFoundError from subprocess.run, returning a synthetic CompletedProcess
with a non-zero returncode and suitable error text in its captured output.
Preserve normal CompletedProcess results so existing returncode != 0 handling
classifies timeouts and missing binaries as unverified without aborting the
sweep.
- Around line 245-252: Update the blob comparison loop in _commit_touched_files
to compare the base and commit versions using Git blob object IDs or hashes
rather than _run’s decoded stdout. Preserve the existing return-code handling
for paths present on only one side, and ensure binary or large files are
compared without text decoding or loading their contents into memory.
- Around line 480-488: Update the LIKELY-STALE reporting branch around landed,
live, and inapplicable so commits in inapplicable are explicitly included in
reasons alongside the existing landed and live details. Ensure unresolved
commits receive the same treatment, preserving the ambiguous evidence in the
summary and avoiding a strong stale conclusion when conflicted commits are
present.
- Around line 279-291: Remove the unused check_file_paths function and
_MAX_FILE_CHECKS_PER_BEAD constant from the bead landing check module. Keep
main’s existing behavior of copying raw evidence.file_paths into the payload,
without adding path verification.
- Line 636: Propagate the parsed offline flag from main to CommitChecker, then
update CommitChecker._ensure_fetch to return without running git fetch origin
when offline is enabled. Preserve the existing fetch behavior for normal online
runs and ensure _is_ancestor uses the offline-aware checker.
- Around line 643-648: Update CommitChecker._ensure_worktree to validate
ownership before reusing an existing directory containing a .git marker. Compare
the worktree’s resolved common Git directory with repo_root, and only permit
reuse when the resolved path is under the tool’s cache prefix or matches an
explicitly acknowledged custom path; otherwise reject it before _reset_worktree
or --remove-worktree can operate.
In `@tests/unit/devtools/test_bead_landing_check.py`:
- Around line 104-113: Add a parametrized test alongside
test_extract_evidence_reads_comments_too that iterates over title, description,
design, acceptance_criteria, notes, and close_reason, constructs a bead with
“Landed in `#3390`.” in the selected field, and asserts extract_evidence returns
pr_numbers [3390].
- Around line 428-447: Add a TTL-expiry test alongside
test_pr_checker_refresh_ignores_cache that uses the existing frozen_clock
fixture to control time, advances beyond the configured TTL, and verifies
PrChecker.check refetches the cached non-MERGED entry by asserting the mocked
_run call count increases. Cover the expiry boundary explicitly without relying
on the host wall clock.
- Around line 193-199: Update the tests for both strong-commit and weak-PR
LIKELY-STALE cases to assert that the verdict’s reasons include CAVEAT. In
test_verdict_merged_pr_only_is_likely_stale_weak and the corresponding
strong-commit test, preserve the existing verdict and confidence assertions
while validating the appended caveat reason.
- Around line 35-43: Update _make_repo to configure the throwaway repository
with commit signing disabled and hooks isolated from host configuration before
creating the initial commit. Add the repository-local Git settings needed to
override commit.gpgsign and core.hooksPath, while preserving the existing user
identity and commit setup.
- Around line 326-337: Update the second-commit setup in the test around
feature_sha and third_sha so the commit is created on a branch not reachable
from master, matching the earlier feature-branch setup. Ensure
checker.check(third_sha) must perform a second worktree operation and
cherry-pick, allowing the existing worktree directory and mtime assertions to
verify reuse.
- Around line 356-380: The worktree tests relying on /proc must run only on
Linux. Add an explicit Linux platform guard to
test_remove_worktree_blocks_on_live_process and
test_remove_worktree_removes_clean_unoccupied_worktree, and replace the host
time.sleep call in the live-process test with the repository-approved test-clock
sleep mechanism.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: ef55644f-a098-4926-9000-b5642ef41186
📒 Files selected for processing (4)
devtools/bead_landing_check.pydevtools/command_catalog.pydocs/devtools.mdtests/unit/devtools/test_bead_landing_check.py
| def _run(cmd: list[str], *, cwd: Path | None = None, timeout: float | None = 30) -> subprocess.CompletedProcess[str]: | ||
| return subprocess.run(cmd, cwd=cwd, capture_output=True, text=True, timeout=timeout) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Convert subprocess timeouts and a missing binary into a failed result.
_run lets subprocess.TimeoutExpired and FileNotFoundError escape. Every caller assumes a CompletedProcess. One slow git fetch, one slow gh pr view, or a host without gh installed aborts the whole sweep with a traceback. The PR cache is then never flushed, so every result fetched during that run is lost. This matters for the documented 600+ bead sweep.
Return a synthetic non-zero result instead, so the existing returncode != 0 paths classify the evidence as unverified.
🛡️ Proposed fix
def _run(cmd: list[str], *, cwd: Path | None = None, timeout: float | None = 30) -> subprocess.CompletedProcess[str]:
- return subprocess.run(cmd, cwd=cwd, capture_output=True, text=True, timeout=timeout)
+ try:
+ return subprocess.run(cmd, cwd=cwd, capture_output=True, text=True, timeout=timeout)
+ except subprocess.TimeoutExpired:
+ return subprocess.CompletedProcess(cmd, 124, stdout="", stderr=f"timeout after {timeout}s: {' '.join(cmd)}")
+ except (FileNotFoundError, OSError) as exc:
+ return subprocess.CompletedProcess(cmd, 127, stdout="", stderr=f"failed to run {cmd[0]}: {exc}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _run(cmd: list[str], *, cwd: Path | None = None, timeout: float | None = 30) -> subprocess.CompletedProcess[str]: | |
| return subprocess.run(cmd, cwd=cwd, capture_output=True, text=True, timeout=timeout) | |
| def _run(cmd: list[str], *, cwd: Path | None = None, timeout: float | None = 30) -> subprocess.CompletedProcess[str]: | |
| try: | |
| return subprocess.run(cmd, cwd=cwd, capture_output=True, text=True, timeout=timeout) | |
| except subprocess.TimeoutExpired: | |
| return subprocess.CompletedProcess(cmd, 124, stdout="", stderr=f"timeout after {timeout}s: {' '.join(cmd)}") | |
| except (FileNotFoundError, OSError) as exc: | |
| return subprocess.CompletedProcess(cmd, 127, stdout="", stderr=f"failed to run {cmd[0]}: {exc}") |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 164-164: Use of unsanitized data to create processes
Context: subprocess.run(cmd, cwd=cwd, capture_output=True, text=True, timeout=timeout)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[error] 164-164: Command coming from incoming request
Context: subprocess.run(cmd, cwd=cwd, capture_output=True, text=True, timeout=timeout)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 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 `@devtools/bead_landing_check.py` around lines 164 - 165, Update _run to catch
subprocess.TimeoutExpired and FileNotFoundError from subprocess.run, returning a
synthetic CompletedProcess with a non-zero returncode and suitable error text in
its captured output. Preserve normal CompletedProcess results so existing
returncode != 0 handling classifies timeouts and missing binaries as unverified
without aborting the sweep.
| for path in files: | ||
| base = _run(["git", "show", f"{self.base_ref}:{path}"], cwd=self.repo_root) | ||
| theirs = _run(["git", "show", f"{commit}:{path}"], cwd=self.repo_root) | ||
| if (base.returncode == 0) != (theirs.returncode == 0): | ||
| return False | ||
| if base.returncode == 0 and base.stdout != theirs.stdout: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the blob comparison against binary content.
_run uses text=True, so git show decodes the blob with the locale codec. _commit_touched_files returns every path the commit touched, including binary fixtures such as PNG or GIF files. A binary blob raises UnicodeDecodeError inside subprocess.run, and the whole sweep aborts.
Compare blob hashes instead of decoded content. This removes the decoding risk and avoids holding large blobs in memory.
🛡️ Proposed fix
for path in files:
- base = _run(["git", "show", f"{self.base_ref}:{path}"], cwd=self.repo_root)
- theirs = _run(["git", "show", f"{commit}:{path}"], cwd=self.repo_root)
+ base = _run(["git", "rev-parse", f"{self.base_ref}:{path}"], cwd=self.repo_root)
+ theirs = _run(["git", "rev-parse", f"{commit}:{path}"], cwd=self.repo_root)
if (base.returncode == 0) != (theirs.returncode == 0):
return False
- if base.returncode == 0 and base.stdout != theirs.stdout:
+ if base.returncode == 0 and base.stdout.strip() != theirs.stdout.strip():
return False
return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for path in files: | |
| base = _run(["git", "show", f"{self.base_ref}:{path}"], cwd=self.repo_root) | |
| theirs = _run(["git", "show", f"{commit}:{path}"], cwd=self.repo_root) | |
| if (base.returncode == 0) != (theirs.returncode == 0): | |
| return False | |
| if base.returncode == 0 and base.stdout != theirs.stdout: | |
| return False | |
| return True | |
| for path in files: | |
| base = _run(["git", "rev-parse", f"{self.base_ref}:{path}"], cwd=self.repo_root) | |
| theirs = _run(["git", "rev-parse", f"{commit}:{path}"], cwd=self.repo_root) | |
| if (base.returncode == 0) != (theirs.returncode == 0): | |
| return False | |
| if base.returncode == 0 and base.stdout.strip() != theirs.stdout.strip(): | |
| return False | |
| return True |
🤖 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 `@devtools/bead_landing_check.py` around lines 245 - 252, Update the blob
comparison loop in _commit_touched_files to compare the base and commit versions
using Git blob object IDs or hashes rather than _run’s decoded stdout. Preserve
the existing return-code handling for paths present on only one side, and ensure
binary or large files are compared without text decoding or loading their
contents into memory.
| def check_file_paths(repo_root: Path, paths: list[str], base_ref: str) -> list[dict[str, Any]]: | ||
| out: list[dict[str, Any]] = [] | ||
| for p in paths[:_MAX_FILE_CHECKS_PER_BEAD]: | ||
| exists = _run(["git", "cat-file", "-e", f"{base_ref}:{p}"], cwd=repo_root).returncode == 0 | ||
| entry: dict[str, Any] = {"path": p, "exists_on_master": exists} | ||
| if exists: | ||
| log = _run(["git", "log", "-1", "--format=%H|%cI", base_ref, "--", p], cwd=repo_root) | ||
| if log.stdout.strip(): | ||
| sha, _, date = log.stdout.strip().partition("|") | ||
| entry["last_commit"] = sha | ||
| entry["last_commit_date"] = date | ||
| out.append(entry) | ||
| return out |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify check_file_paths and _MAX_FILE_CHECKS_PER_BEAD have no callers outside their definition.
rg -nP --type=py -C3 '\bcheck_file_paths\b|_MAX_FILE_CHECKS_PER_BEAD'Repository: Sinity/polylogue
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -u
echo "files:"
git ls-files | rg '(^|/)devtools/bead_landing_check\.py$|(^|/)test.*bead.*\.py$|test.*bead|bead.*test|command_catalog\.py' || true
echo
echo "tracked python files under devtools:"
git ls-files 'devtools/*.py' | sort
echo
echo "search identifiers without rg maybe due tooling:"
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('*.py'):
if '.git' in p.parts or 'node_modules' in p.parts:
continue
try:
txt = p.read_text(errors='ignore')
except Exception:
continue
if 'check_file_paths' in txt or '_MAX_FILE_CHECKS_PER_BEAD' in txt:
print(f"\n--- {p}")
for i,line in enumerate(txt.splitlines(),1):
if 'check_file_paths' in line or '_MAX_FILE_CHECKS_PER_BEAD' in line:
print(f"{i}: {line}")
PYRepository: Sinity/polylogue
Length of output: 6982
Remove the unused file-check helpers.
check_file_paths and _MAX_FILE_CHECKS_PER_BEAD are defined in devtools/bead_landing_check.py, but main only copies raw evidence.file_paths into the payload and no caller resolves whether those paths exist. Remove them, or wire up verification and test the result.
🤖 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 `@devtools/bead_landing_check.py` around lines 279 - 291, Remove the unused
check_file_paths function and _MAX_FILE_CHECKS_PER_BEAD constant from the bead
landing check module. Keep main’s existing behavior of copying raw
evidence.file_paths into the payload, without adding path verification.
| if landed: | ||
| verdict, confidence = "LIKELY-STALE", "strong" | ||
| detail = ", ".join(f"{c} ({commit_results[c]})" for c in landed) | ||
| reasons.append(f"cited commit(s) already on master: {detail}") | ||
| if live: | ||
| reasons.append( | ||
| f"NOTE: commit(s) {', '.join(live)} still produce a non-empty diff -- " | ||
| "possibly a partial landing, verify remaining scope before closing" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report conflicted commits in the LIKELY-STALE branch.
The landed branch adds a NOTE for commits in live, but it never mentions commits in inapplicable. A bead that cites one landed commit and one commit that fails to cherry-pick is reported as LIKELY-STALE with strong confidence, and the conflicted commit disappears from reasons. A reader who follows the summary line loses the ambiguous evidence. unresolved has the same gap.
🐛 Proposed fix
if landed:
verdict, confidence = "LIKELY-STALE", "strong"
detail = ", ".join(f"{c} ({commit_results[c]})" for c in landed)
reasons.append(f"cited commit(s) already on master: {detail}")
if live:
reasons.append(
f"NOTE: commit(s) {', '.join(live)} still produce a non-empty diff -- "
"possibly a partial landing, verify remaining scope before closing"
)
+ if inapplicable:
+ reasons.append(
+ f"NOTE: commit(s) {', '.join(inapplicable)} could not be cherry-picked cleanly onto master "
+ "(conflict) -- landing state of that evidence is unknown"
+ )
+ if unresolved:
+ reasons.append(
+ f"NOTE: cited hex token(s) {', '.join(unresolved)} are not real commits in this repo"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if landed: | |
| verdict, confidence = "LIKELY-STALE", "strong" | |
| detail = ", ".join(f"{c} ({commit_results[c]})" for c in landed) | |
| reasons.append(f"cited commit(s) already on master: {detail}") | |
| if live: | |
| reasons.append( | |
| f"NOTE: commit(s) {', '.join(live)} still produce a non-empty diff -- " | |
| "possibly a partial landing, verify remaining scope before closing" | |
| ) | |
| if landed: | |
| verdict, confidence = "LIKELY-STALE", "strong" | |
| detail = ", ".join(f"{c} ({commit_results[c]})" for c in landed) | |
| reasons.append(f"cited commit(s) already on master: {detail}") | |
| if live: | |
| reasons.append( | |
| f"NOTE: commit(s) {', '.join(live)} still produce a non-empty diff -- " | |
| "possibly a partial landing, verify remaining scope before closing" | |
| ) | |
| if inapplicable: | |
| reasons.append( | |
| f"NOTE: commit(s) {', '.join(inapplicable)} could not be cherry-picked cleanly onto master " | |
| "(conflict) -- landing state of that evidence is unknown" | |
| ) | |
| if unresolved: | |
| reasons.append( | |
| f"NOTE: cited hex token(s) {', '.join(unresolved)} are not real commits in this repo" | |
| ) |
🤖 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 `@devtools/bead_landing_check.py` around lines 480 - 488, Update the
LIKELY-STALE reporting branch around landed, live, and inapplicable so commits
in inapplicable are explicitly included in reasons alongside the existing landed
and live details. Ensure unresolved commits receive the same treatment,
preserving the ambiguous evidence in the summary and avoiding a strong stale
conclusion when conflicted commits are present.
| def test_extract_evidence_reads_comments_too() -> None: | ||
| bead = _bead(comments=[{"text": "Landed in #3390."}]) | ||
| ev = extract_evidence(bead) | ||
| assert ev.pr_numbers == [3390] | ||
|
|
||
|
|
||
| def test_extract_evidence_finds_file_paths() -> None: | ||
| bead = _bead(description="See polylogue/sources/parsers/claude/code_parser.py:87 for the skip list.") | ||
| ev = extract_evidence(bead) | ||
| assert "polylogue/sources/parsers/claude/code_parser.py" in ev.file_paths |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover every field _bead_text reads.
_bead_text concatenates title, description, design, acceptance_criteria, notes, close_reason, and comment text. These tests exercise only description and comments. A change that drops design, acceptance_criteria, notes, or close_reason from _bead_text keeps this suite green, and the sweep then silently misses evidence.
Add one parametrized test over the field names.
💚 Proposed test
`@pytest.mark.parametrize`(
"field",
["title", "description", "design", "acceptance_criteria", "notes", "close_reason"],
)
def test_extract_evidence_reads_every_text_field(field: str) -> None:
ev = extract_evidence(_bead(**{field: "Landed in `#3390`."}))
assert ev.pr_numbers == [3390]🤖 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 `@tests/unit/devtools/test_bead_landing_check.py` around lines 104 - 113, Add a
parametrized test alongside test_extract_evidence_reads_comments_too that
iterates over title, description, design, acceptance_criteria, notes, and
close_reason, constructs a bead with “Landed in `#3390`.” in the selected field,
and asserts extract_evidence returns pr_numbers [3390].
| def test_verdict_merged_pr_only_is_likely_stale_weak() -> None: | ||
| bead = _bead("polylogue-a") | ||
| ev = Evidence(pr_numbers=[3390]) | ||
| pr_results = {3390: PrResult(number=3390, found=True, state="MERGED")} | ||
| v = verdict_for_bead(bead, ev, {}, pr_results) | ||
| assert v.verdict == "LIKELY-STALE" | ||
| assert v.confidence == "weak" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the CAVEAT reason on LIKELY-STALE verdicts.
verdict_for_bead appends a CAVEAT reason for every LIKELY-STALE verdict. The PR describes that caveat as a deliberate honesty guarantee. No test asserts it, so a change that drops the append stays green.
Add the assertion to the strong-commit case and to this weak-PR case.
💚 Proposed test change
v = verdict_for_bead(bead, ev, {}, pr_results)
assert v.verdict == "LIKELY-STALE"
assert v.confidence == "weak"
+ assert any(r.startswith("CAVEAT:") for r in v.reasons)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_verdict_merged_pr_only_is_likely_stale_weak() -> None: | |
| bead = _bead("polylogue-a") | |
| ev = Evidence(pr_numbers=[3390]) | |
| pr_results = {3390: PrResult(number=3390, found=True, state="MERGED")} | |
| v = verdict_for_bead(bead, ev, {}, pr_results) | |
| assert v.verdict == "LIKELY-STALE" | |
| assert v.confidence == "weak" | |
| def test_verdict_merged_pr_only_is_likely_stale_weak() -> None: | |
| bead = _bead("polylogue-a") | |
| ev = Evidence(pr_numbers=[3390]) | |
| pr_results = {3390: PrResult(number=3390, found=True, state="MERGED")} | |
| v = verdict_for_bead(bead, ev, {}, pr_results) | |
| assert v.verdict == "LIKELY-STALE" | |
| assert v.confidence == "weak" | |
| assert any(r.startswith("CAVEAT:") for r in v.reasons) |
🤖 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 `@tests/unit/devtools/test_bead_landing_check.py` around lines 193 - 199,
Update the tests for both strong-commit and weak-PR LIKELY-STALE cases to assert
that the verdict’s reasons include CAVEAT. In
test_verdict_merged_pr_only_is_likely_stale_weak and the corresponding
strong-commit test, preserve the existing verdict and confidence assertions
while validating the appended caveat reason.
| # A second, distinct commit must reuse the same worktree directory rather | ||
| # than creating a fresh one. | ||
| (repo / "third.txt").write_text("y\n") | ||
| _run_git(["add", "third.txt"], cwd=repo) | ||
| _run_git(["commit", "-m", "add third"], cwd=repo) | ||
| third_sha = _run_git(["rev-parse", "HEAD"], cwd=repo).stdout.strip() | ||
| _run_git(["checkout", "master~1"], cwd=repo) # detach so third_sha isn't already master's tip | ||
| _run_git(["checkout", "master"], cwd=repo) | ||
|
|
||
| checker.check(third_sha) | ||
| assert wt_dir.exists() | ||
| assert (wt_dir / ".git").stat().st_mtime == marker_mtime |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The reuse assertion does not exercise a second worktree operation.
Trace the repository state. Line 320 checks out master, so the commit created on Lines 328-330 lands on master. third_sha on Line 331 is therefore master's tip. Lines 332-333 detach and re-attach but leave the tip unchanged.
checker.check(third_sha) on Line 335 then hits the _is_ancestor short-circuit and returns already-on-master. It never calls _ensure_worktree and never runs a cherry-pick. The .git mtime assertion on Line 337 passes because no worktree work happens at all, so the test does not prove worktree reuse.
Create the second commit on a branch that is not reachable from master, as the earlier part of the test does for feature_sha.
💚 Proposed fix
# A second, distinct commit must reuse the same worktree directory rather
# than creating a fresh one.
+ _run_git(["checkout", "feature"], cwd=repo)
(repo / "third.txt").write_text("y\n")
_run_git(["add", "third.txt"], cwd=repo)
_run_git(["commit", "-m", "add third"], cwd=repo)
third_sha = _run_git(["rev-parse", "HEAD"], cwd=repo).stdout.strip()
- _run_git(["checkout", "master~1"], cwd=repo) # detach so third_sha isn't already master's tip
_run_git(["checkout", "master"], cwd=repo)
- checker.check(third_sha)
+ assert checker.check(third_sha) == "non-empty-diff"
assert wt_dir.exists()
assert (wt_dir / ".git").stat().st_mtime == marker_mtime📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # A second, distinct commit must reuse the same worktree directory rather | |
| # than creating a fresh one. | |
| (repo / "third.txt").write_text("y\n") | |
| _run_git(["add", "third.txt"], cwd=repo) | |
| _run_git(["commit", "-m", "add third"], cwd=repo) | |
| third_sha = _run_git(["rev-parse", "HEAD"], cwd=repo).stdout.strip() | |
| _run_git(["checkout", "master~1"], cwd=repo) # detach so third_sha isn't already master's tip | |
| _run_git(["checkout", "master"], cwd=repo) | |
| checker.check(third_sha) | |
| assert wt_dir.exists() | |
| assert (wt_dir / ".git").stat().st_mtime == marker_mtime | |
| # A second, distinct commit must reuse the same worktree directory rather | |
| # than creating a fresh one. | |
| _run_git(["checkout", "feature"], cwd=repo) | |
| (repo / "third.txt").write_text("y\n") | |
| _run_git(["add", "third.txt"], cwd=repo) | |
| _run_git(["commit", "-m", "add third"], cwd=repo) | |
| third_sha = _run_git(["rev-parse", "HEAD"], cwd=repo).stdout.strip() | |
| _run_git(["checkout", "master"], cwd=repo) | |
| assert checker.check(third_sha) == "non-empty-diff" | |
| assert wt_dir.exists() | |
| assert (wt_dir / ".git").stat().st_mtime == marker_mtime |
🤖 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 `@tests/unit/devtools/test_bead_landing_check.py` around lines 326 - 337,
Update the second-commit setup in the test around feature_sha and third_sha so
the commit is created on a branch not reachable from master, matching the
earlier feature-branch setup. Ensure checker.check(third_sha) must perform a
second worktree operation and cherry-pick, allowing the existing worktree
directory and mtime assertions to verify reuse.
| def test_remove_worktree_blocks_on_live_process(tmp_path: Path) -> None: | ||
| repo = _make_repo(tmp_path / "repo") | ||
| wt = tmp_path / "wt" | ||
| _run_git(["worktree", "add", "--detach", str(wt), "master"], cwd=repo) | ||
|
|
||
| proc = subprocess.Popen(["sleep", "5"], cwd=wt) | ||
| try: | ||
| # Give the OS a moment to publish /proc/<pid>/cwd. | ||
| for _ in range(50): | ||
| if Path(f"/proc/{proc.pid}/cwd").exists(): | ||
| break | ||
| time.sleep(0.05) | ||
| assert remove_worktree(repo, wt) == "blocked-live-process" | ||
| assert wt.exists() # never removed while occupied | ||
| finally: | ||
| proc.kill() | ||
| proc.wait() | ||
|
|
||
|
|
||
| def test_remove_worktree_removes_clean_unoccupied_worktree(tmp_path: Path) -> None: | ||
| repo = _make_repo(tmp_path / "repo") | ||
| wt = tmp_path / "wt" | ||
| _run_git(["worktree", "add", "--detach", str(wt), "master"], cwd=repo) | ||
| assert remove_worktree(repo, wt) == "removed" | ||
| assert not wt.exists() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which clock APIs devtools verify test-clock-hygiene blocks and what the allowlist contains.
fd -t f 'verify_test_clock_hygiene.py' devtools --exec rg -n 'time\.|datetime\.|BANNED|FORBIDDEN|allowlist'
fd -t f 'test-clock-allowlist.yaml' docs --exec catRepository: Sinity/polylogue
Length of output: 10797
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate files =="
fd -t f 'test_bead_landing_check.py|*.py' tests/unit/devtools | sed -n '1,80p'
echo
echo "== Target test file outline =="
ast-grep outline tests/unit/devtools/test_bead_landing_check.py --view compact 2>/dev/null | sed -n '1,160p' || true
echo
echo "== Source around remove_worktree tests =="
sed -n '320,390p' tests/unit/devtools/test_bead_landing_check.py
echo
echo "== Search for _worktree_has_live_process and remove_worktree =="
rg -n "_worktree_has_live_process|def remove_worktree|remove_worktree\(" tests/unit tests -g '*.py' | sed -n '1,200p'Repository: Sinity/polylogue
Length of output: 514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate target and related search terms =="
git ls-files | rg '(^|/)tests/unit/devtools/test_bead_landing_check\.py$|(_worktree_has_live_process|remove_worktree)\.py' || true
echo
echo "== Target source around remove_worktree tests =="
sed -n '320,390p' tests/unit/devtools/test_bead_landing_check.py
echo
echo "== Search for _worktree_has_live_process and remove_worktree =="
rg -n "def _worktree_has_live_process|def remove_worktree|remove_worktree\(|_worktree_has_live_process" tests -g '*.py' | sed -n '1,200p'
echo
echo "== Top-level import/setup of this test file =="
sed -n '1,80p' tests/unit/devtools/test_bead_landing_check.pyRepository: Sinity/polylogue
Length of output: 5601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate devtools.bead_landing_check =="
git ls-files | rg '(^|/)devtools/.*bead.*landing.*check.*\.py$|(^|/)devtools/b.*\.py$|(^|/)devtools/__init__\.py$' | sed -n '1,120p'
echo
echo "== Search worktree/live process implementation =="
rg -n "def remove_worktree|def _worktree_has_live_process|worktree_has_live_process|blocked-live-process|remove-worktree|worktree remove" . -g '*.py' | sed -n '1,240p'
echo
echo "== If devtools.bead_landing_check.py exists, inspect workspace handling =="
if [ -f devtools/bead_landing_check.py ]; then
wc -l devtools/bead_landing_check.py
rg -n "def remove_worktree|def _worktree_has_live_process|from pathlib import Path|Path\("/proc|cwd" or 'cwd' in|subprocess" devtools/bead_landing_check.py
sed -n '1,260p' devtools/bead_landing_check.py
fiRepository: Sinity/polylogue
Length of output: 1855
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== devtools/bead_landing_check.py relevant implementation =="
sed -n '260,340p' devtools/bead_landing_check.py
echo
echo "== module-level imports =="
sed -n '1,40p' devtools/bead_landing_check.pyRepository: Sinity/polylogue
Length of output: 5402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check whether time.sleep is allowlisted =="
python3 - <<'PY'
from pathlib import Path
import re
yaml = Path("docs/plans/test-clock-allowlist.yaml").read_text()
m = re.search(r'(^files:\n(?:^ - path: .*?\n(?:^ .*?\n)?)*?- path: .*?\n(?:^ .*?\n)?)', yaml, re.M | re.S)
if m:
print("allowlist:")
print(m.group(1))
else:
print("no allowfile found")
print("contains target_path_entry:", str(Path("tests/unit/devtools/test_bead_landing_check.py")) in yaml)
PY
echo
echo "== Read-only AST/source probe of time.sleep and /proc use =="
python3 - <<'PY'
import ast
from pathlib import Path
paths = [
Path("devtools/bead_landing_check.py"),
Path("tests/unit/devtools/test_bead_landing_check.py"),
]
for path in paths:
print(f"\n--- {path} ---")
source = path.read_text()
tree = ast.parse(source)
print("time.sleep Calls:")
found = False
for node in ast.walk(tree):
if isinstance(node, ast.Call):
if isinstance(node.func, ast.Attribute):
if isinstance(node.func.value, ast.Name) and node.func.value.id == "time" and node.func.attr == "sleep":
print(f" line {node.lineno}")
found = True
print("time.sleep" if found else "no time.sleep Calls")
print("absolute Path('/proc' ...), Path(\"/proc\" ...), or '/proc' found:")
for i, line in enumerate(source.splitlines(), 1):
if "/proc" in line or "Path(\"/proc\"" in line or "Path('/proc'" in line:
print(f" {i}: {line}")
PYRepository: Sinity/polylogue
Length of output: 783
Make the /proc-dependent worktree tests Linux-only.
_worktree_has_live_process returns True when /proc is absent, so on macOS these tests do not call the normal removal paths:
test_remove_worktree_removes_clean_unoccupied_worktreereturnsblocked-live-processinstead ofremoved.test_remove_worktree_blocks_on_live_processonly checks whetherremove_worktreereports the conservative unavailable state.
Add an explicit platform guard, and ensure the host time.sleep call does not violate the test clock allowlist.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 360-360: Command coming from incoming request
Context: subprocess.Popen(["sleep", "5"], cwd=wt)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 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 `@tests/unit/devtools/test_bead_landing_check.py` around lines 356 - 380, The
worktree tests relying on /proc must run only on Linux. Add an explicit Linux
platform guard to test_remove_worktree_blocks_on_live_process and
test_remove_worktree_removes_clean_unoccupied_worktree, and replace the host
time.sleep call in the live-process test with the repository-approved test-clock
sleep mechanism.
Source: Coding guidelines
| def test_pr_checker_refresh_ignores_cache(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: | ||
| calls = {"n": 0} | ||
|
|
||
| def fake_run(cmd: list[str], **kwargs: Any) -> subprocess.CompletedProcess[str]: | ||
| calls["n"] += 1 | ||
| return subprocess.CompletedProcess( | ||
| cmd, 0, stdout=json.dumps({"state": "OPEN", "mergedAt": None, "mergeCommit": None, "title": "x"}), stderr="" | ||
| ) | ||
|
|
||
| monkeypatch.setattr("devtools.bead_landing_check._run", fake_run) | ||
| cache_path = tmp_path / "cache.json" | ||
| checker = PrChecker("Sinity/polylogue", cache_path, ttl_days=7) | ||
| checker.check(300) | ||
| assert calls["n"] == 1 | ||
| checker.check(300) | ||
| assert calls["n"] == 1 # OPEN is TTL-cached, reused within TTL | ||
|
|
||
| checker_refresh = PrChecker("Sinity/polylogue", cache_path, ttl_days=7, refresh=True) | ||
| checker_refresh.check(300) | ||
| assert calls["n"] == 2 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover the TTL-expiry branch with a controlled clock.
PrChecker.check reuses a non-MERGED cache entry only while now - checked_at < ttl_seconds. This test proves the reuse side and the refresh=True side. No test proves the expiry side, so a change to the TTL comparison keeps this suite green while the tool serves stale OPEN PR state and produces a wrong LIKELY-LIVE verdict.
Add a case that ages the cached entry. Use frozen_clock so the assertion does not depend on the host wall clock.
💚 Proposed test
def test_pr_checker_refetches_after_ttl_expiry(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
calls = {"n": 0}
def fake_run(cmd: list[str], **kwargs: Any) -> subprocess.CompletedProcess[str]:
calls["n"] += 1
return subprocess.CompletedProcess(
cmd, 0, stdout=json.dumps({"state": "OPEN", "mergedAt": None, "mergeCommit": None, "title": "x"}), stderr=""
)
monkeypatch.setattr("devtools.bead_landing_check._run", fake_run)
cache_path = tmp_path / "cache.json"
PrChecker("Sinity/polylogue", cache_path, ttl_days=7).check(300)
assert calls["n"] == 1
# ttl_days=0 makes every non-MERGED entry immediately stale.
PrChecker("Sinity/polylogue", cache_path, ttl_days=0).check(300)
assert calls["n"] == 2As per coding guidelines, "Use frozen_clock for timestamp-sensitive tests".
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 433-433: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"state": "OPEN", "mergedAt": None, "mergeCommit": None, "title": "x"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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 `@tests/unit/devtools/test_bead_landing_check.py` around lines 428 - 447, Add a
TTL-expiry test alongside test_pr_checker_refresh_ignores_cache that uses the
existing frozen_clock fixture to control time, advances beyond the configured
TTL, and verifies PrChecker.check refetches the cached non-MERGED entry by
asserting the mocked _run call count increases. Cover the expiry boundary
explicitly without relying on the host wall clock.
Source: Coding guidelines
…s LIKELY-STALE Problem: five independent human reviewers checked all 190 LIKELY-STALE verdicts from the first full sweep against 114 real beads. Measured precision was catastrophic -- roughly 6 of 114 (~5%) were genuinely safe to close; ~95% were false positives. The root cause, found independently by two reviewers: the heuristic keyed off "a cited commit/PR exists on master" without checking whether the shipped code has any live production consumer -- exactly the "code shipped, nothing reads it" defect class this repo has spent the night finding elsewhere. Concrete confirmed false positives: polylogue-rxdo.9.6 (blind_items() has zero callers outside its own test), polylogue-rxdo.6 (ReferenceQueryPipeline has zero CLI/MCP/daemon references and still hard-errors), polylogue-rxdo.9.7 (ClaimWithControls has zero callers outside its own test), polylogue-hg97 (cost_outlook absent from polylogue/mcp/, its contract test still xfail), polylogue-dcz5 (3.14t live in prod, but daemon_parse_stage_split is still False), polylogue-yp0 (EventBus core landed, notes explicitly say "NOT wired to a live producer/consumer"). A second failure mode: epic/parent beads flagged because their own commit landed while most dotted-id/parent-child dependents remain open (polylogue-2qx: 9 open dependents; polylogue-3tl: 17; polylogue-rxdo.9: 7 of 9 children). Solution -- three corrective checks, all downgrade-only (never upgrade a verdict, per the standing rule that a wrong LIKELY-STALE is worse than an honest UNDETERMINED): 1. LIVE-CONSUMER CHECK (CommitChecker.added_symbols/has_live_consumer/ consumer_check): a landed commit only counts as strong evidence if at least one top-level symbol it added has a `git grep` hit outside the commit's own touched files and outside tests/. Landed-but-unconsumed or landed-with-inconclusive-symbols both downgrade to UNDETERMINED. 2. OPEN-DEPENDENTS CHECK (build_open_parent_child_dependents_index): a bead with unresolved `parent-child` dependents (open/in_progress) is never LIKELY-STALE regardless of its own evidence. 3. SUPPRESSION-PHRASE CHECK (find_suppression_signal): if the bead's own text already says "deferred"/"xfail"/"not wired"/"is NOT satisfied"/etc, that overrides commit/PR evidence and forces UNDETERMINED. Re-run over the same 629 open+in_progress beads: LIKELY-STALE dropped from 190 to 84 (UNDETERMINED rose from 435 to 541), runtime 14s warm (was 8s -- consumer checks add `git show`/`git grep` per landed commit, still far below the cold-sweep baseline). Known residual gap: polylogue-a7xr.16 remains a weak-confidence LIKELY-STALE from a merged-PR citation alone -- its own note claims "WORK COMPLETE" while only the INSERT half of the column-spec refactor actually landed (SELECT's 14 methods/503 accessors untouched); no cited commit and no suppression phrase exists in its text for this tool to catch, which is exactly why the CAVEAT reason on every LIKELY-STALE verdict says to read the AC list, not just the tool's verdict. Verification: devtools test tests/unit/devtools/test_bead_landing_check.py -- 54 passed, including a new labelled-evaluation regression block that runs the real pipeline (real git history, real .beads/issues.jsonl, no network) against the six named false positives and asserts none regress back to a strong-confidence LIKELY-STALE verdict. ruff check/format and mypy --strict clean. devtools verify --quick exit 0. devtools render all --check: no drift. Co-Authored-By: Claude <noreply@anthropic.com>
|
Closing unmerged. Measured against the 190-bead human-verified ground truth (5 independent review groups, complete VERDICT set in bd notes — see PR body "Measured against ground truth"):
This matches the coordinator's stated decision framework for "precision is still poor" / "recall collapsed on the fixes' own mechanism": close unmerged rather than merge a tool that produces a headline count nobody should trust. The 190 labelled verdicts are the durable asset from this investigation. They already survive independent of this PR, as bd notes on the reviewed beads themselves ( If this direction is revisited, the promising kernel per the coordinator's read (which the data supports) is note-staleness detection — has a bead's own most-recent note been contradicted by newer master state — rather than commit-graph archaeology. The current suppression-phrase check is a first, not-yet-safe attempt at that; its one demonstrated false negative (7mtf) shows a lexical keyword match isn't sufficient on its own. Co-Authored-By: Claude noreply@anthropic.com |
1 participant
Summary
Adds
devtools workspace bead-landing-check, a tool intended to verify whether a bead's cited implementation evidence already landed on master, before a human or dispatched agent burns a full investigation cycle on it.FINAL VERDICT (2026-07-31): do not merge. Recommend closing this PR unmerged. Five independent human reviewers labelled all 190 beads flagged by the first sweep (STALE/PARTIAL/LIVE) — a complete ground-truth set, not a sample. The corrected tool (commits
d7cd95d52,c94edc037) was measured against it below. Precision is still ~6% and the three downgrade-only fixes provably removed real STALE beads alongside false positives, so recall is also poor. The full reasoning is in Measured against ground truth below; read it before the rest of this PR.Problem (why this was attempted)
Five separate open beads cost a dispatched agent a full 30-60+ minute investigation cycle before discovering the described work was already done on master (
polylogue-o4j2,polylogue-hiu,polylogue-0jf4,polylogue-pbuh,polylogue-cijx.4). That is a recurring failure mode of a 1,260-bead backlog, not a one-off. The technique that worked twice — cherry-picking a cited commit onto master and checking for an empty diff — is real and survives squash-merge id rewriting, unlikegit log --is-ancestoror grepping for issue ids. The rest of this PR is the record of why that one true technique was not, in the end, enough to make the sweep trustworthy.Measured against ground truth (read this first)
The ground truth. All five review groups have now reported. Every one of the 190 beads flagged by the first (pre-fix) sweep carries a
bd notefrom its reviewer with an explicitVERDICT: STALE|PARTIAL|LIVEplus evidence — these are durable bd notes, already committed to the shared bd database, independent of whether this PR merges (queryable today viabd show <id>orbd sql "SELECT id, notes FROM issues WHERE notes LIKE '%VERDICT%'"). Reported group tallies:I parsed the actual verdict text out of the live bd notes (
VERDICT:/VERIFICATION (groupN...):followed by STALE/PARTIAL/LIVE) and matched 185 of the 190 unambiguously (7 STALE / 49 PARTIAL / 129 LIVE) — 5 beads' notes use phrasing my regex didn't catch; I did not force a match rather than guess. The STALE count I can attribute to specific bead ids is 7, not 9 — noted honestly below; both numbers point to the same conclusion.The corrected tool's current output (re-run today, commit
c94edc037): 66 LIKELY-STALE (10 strong-confidence, 56 weak-confidence) out of 629 open+in_progress beads.PRECISION — of the 66 now flagged, how many carry a STALE ground-truth label:
Strong confidence (the live-consumer-verified commit evidence) is meaningfully better than weak (PR-only), but 8 of every 10 strong verdicts are still wrong. This is a real improvement over the pre-fix ~5% but not a usable one.
RECALL — of the labelled STALE beads, how many does the corrected tool still flag:
4 of 7 found (57.1%), or 4 of the coordinator's reported 9 (44.4%) if the 2 I couldn't locate were also missed. The three fixes are downgrade-only, so every miss is a fix removing a genuine STALE it could not distinguish from a false positive:
polylogue-4fm3: the live-consumer check returned "inconclusive" (non-Python change) for a commit the reviewer confirmed both ACs landed on. Inconclusive was deliberately treated as non-strong evidence — correct in general, wrong here.polylogue-6pii: the live-consumer check found no externalgit grephit for the changed symbol on a bead the reviewer confirmed was a closable no-AC chore. The consumer check's blind spot: some genuinely-done changes don't leave a grep-visible caller (config/data-only diffs, or a caller added in the same squash-merged commit that the touched-files exclusion also removes).polylogue-7mtf: an own-goal. The suppression-phrase check'sxfailkeyword — added specifically to catchpolylogue-hg97's "Marked xfail" admission of incomplete work — fired on this bead's own text, which merely describes aMarked xfail(condition=py>=3.14, strict=True)regression guard the fix itself added. Same word, opposite meaning. This is exactly the risk of a lexical suppression check: it cannot tell "this admits incompleteness" from "this describes a completed change that happens to mention an xfail marker."2x2 over the 185 beads with a ground-truth label:
Conclusion, per the coordinator's decision framework: this is the "precision is still poor" outcome (6.1%, not materially different from the pre-fix ~5% at the level that matters for trusting a headline count) compounded by the "recall collapsed on the fixes' own mechanism" finding — the downgrade-only fixes got the tool quieter without getting it reliable. Recommendation: close this PR unmerged. The operator's framing is right: "is this work done?" is a question about whether acceptance criteria are semantically satisfied, and a git/text query can only check whether artifacts exist or specific phrases are absent.
polylogue-aggzremains the clearest illustration — two directly-matching MERGED PRs (#3401, #3405), and the PR bodies themselves state 2 of the bead's 3 declared invariants are untouched. No commit-graph query reaches that; only reading the PR body against the bead's own AC list does, which is a human (or a much more expensive, semantic-reading agent) task, not a git query.Worth preserving if any of this is revisited: the suppression-phrase check reads the bead's own most recent note, not the commit graph — that is the signal the human reviewers actually used, and several flagged beads said "no code written" or "not attempted" in their own latest note. The
polylogue-7mtffalse-negative shows the current lexical version isn't safe as-is, but a future tool aimed at note staleness (has this bead's own most-recent-note-implied status been contradicted by newer master state?) rather than commit archaeology (does a cited commit exist?) is the more promising direction the coordinator identified, not this one.What shipped in this branch (for the record)
devtools/bead_landing_check.py, registered viaCommandSpecindevtools/command_catalog.py:<hash>" snapshot-anchor idiom, and dropping agithub.com/.../pull/NURL pattern that matched unrelated PR numbers quoted inside sample payloads.CommitChecker: a single reused throwaway worktree classifies cited commits asunknown-revision/already-on-master/empty-diff/content-equivalent/non-empty-diff/does-not-apply, plus a live-consumer check (added_symbols/has_live_consumer/consumer_check) thatgit greps for a reference to a landed commit's new symbols outside its own touched files and outside tests.build_open_parent_child_dependents_indexandfind_suppression_signal: the open-dependents and suppression-phrase downgrade checks described above.PrChecker: disk-cachedgh pr viewmerge-state lookups.tests/unit/devtools/test_bead_landing_check.py, 54 tests including a labelled-evaluation regression block that runs the real pipeline against 6 named confirmed false positives using real git history and real.beads/issues.jsonl(no network).All of this is real, tested, and does what it claims to do at the mechanism level — extraction is accurate, the worktree/cache/safety machinery works, the git plumbing is correct. It simply does not clear the bar for a trustworthy sweep, which is the thing that was asked for.
Verification
ruff check/ruff format --check,mypy --strict— clean.devtools render all --check— no drift.devtools test tests/unit/devtools/test_bead_landing_check.py— 54 passed.devtools verify --quick— exit 0.devtools workspace bead-landing-check --jsonfor the current flagged set,bd sql "SELECT id, notes FROM issues WHERE notes LIKE '%VERDICT%'"(orbd show <id> --json) for the ground-truth labels.devtools verify(testmon not seeded) anddevtools lab policy bead-graph(pre-existing, unrelated failures).Ref #4960 (beads upstream, for context only — not filed against this repo).
Co-Authored-By: Claude noreply@anthropic.com