Skip to content

fix-bot-comment disposition hardening: idempotent retry, fail the run on failed target verification, behavioural test for the node-id guard (#1928 round-3) #1934

Open

Description

@rachel-petry

Summary

Follow-up hardening for the fix-bot-comment disposition flow added in #1919 (PR #1928). These are round-3 review findings on PR #1928. All three fail safe: the maintainer-comment gate stays blocked, so nothing is wrongly cleared. But each can leave a PR stuck with no clear signal. They were deferred to stop an unbounded review loop on #1928, which had already taken four rounds of fixes.

1. Retry after a partial run posts a second disposition (Codex P1, prompts/dev-lead/fix-bot-comment.md:132)

If a run posts the disposition reply but is cancelled or fails before resolve_dispositioned_comments minimizes the original, the retry finds the original still unresolved and posts a second marker. The resolver requires exactly one authorized disposition and skips otherwise, so the notice stays blocking.

Fix: before posting, look for an existing dev-lead:comment-disposition id=<node_id> reply by the agent and reuse it. Or make the resolver accept multiple identical authorized dispositions for one id.

2. A failed target check doesn't fail the run (Codex P1, prompts/dev-lead/fix-bot-comment.md:127)

When a valid-looking node id cannot be fetched, is the wrong node type, or fails the author check, the prompt says to post nothing and note it in the summary. A failed Bash call inside the agent does not necessarily make the writer engine exit nonzero, and resolve_dispositioned_comments succeeds after resolving zero comments. The run therefore finishes green with the notice still blocking.

Fix: have the harness verify the target itself (before or after the engine), or check afterwards that a disposition for COMMENT_NODE_ID exists and fail the run otherwise.

3. The fail-loud node-id guard is only tested by message text (cubic P3, tests/dev-lead/unit/test_agent_marker_convention.bats:128)

The parse-step guard in .github/workflows/dev-lead-reusable.yml is tested by grepping for its error text. A regression that keeps the message but drops exit 1, or inverts the INTENT_TYPE condition, stays green.

Fix: extract the guard into a small script or function, and add behavioural tests:

  • INTENT_TYPE=fix-bot-comment with a missing or malformed id → nonzero exit;
  • INTENT_TYPE=fix-bot-comment with a valid id → zero exit, id exported;
  • another intent with no id → zero exit.

Rollout

These are dev-lead changes. After merge, cut dev-lead/v139-next and let the canary rings promote.

Activity

  1. github-actions commented on Sep 25, 2026

    @github-actions
    Contributor

    Thanks for the thorough write-up and for deferring these cleanly out of #1928 — the round-by-round structure makes it easy to pick up. Items 1 and 2 read as latent bugs; before prioritising the fix for item 1, could you confirm whether the duplicate-disposition scenario has been observed in an actual cancelled/retried run, or is it currently a theoretical path identified during review? That'll help gauge how urgently it needs to land versus being bundled with item 3's test harness work.

  2. added a commit that references this issue on Sep 25, 2026
  3. don-petry commented on Oct 1, 2026

    @don-petry
    Collaborator

    Item 1 here (a retry after a partial run posts a second disposition) is now covered by #1992, together with the same bug in the fix-reviews path, which has deadlocked #1952 and is about to deadlock #1953. Items 2 and 3 stay on this issue.


    Generated by Claude Code

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

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions