Skip to content

Clarify terminal closeout target forms - #703

Closed
justin808 wants to merge 3 commits into
mainfrom
jg-codex/issue-503-terminal-release-fallback
Closed

Clarify terminal closeout target forms#703
justin808 wants to merge 3 commits into
mainfrom
jg-codex/issue-503-terminal-release-fallback

Conversation

@justin808

Copy link
Copy Markdown
Member

Fixes #503

Make the documented batch-manifest target string explicit, keep claim/release aligned to that exact string, and route the known terminal-closeout mismatch message straight to the ordinary claim-only recovery path.

Validation:

  • ruby skills/pr-batch/bin/coordination-telemetry-contract-test.rb

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Important

Approval pending

CodeRabbit has no unresolved comments, but it has not reviewed the latest commit.

Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@justin808

Copy link
Copy Markdown
Member Author

Evidence:

  • Head: 80b7ac459d8bf24c8521e3165db31f4f81ee39d6
  • Focused contract: ruby skills/pr-batch/bin/coordination-telemetry-contract-test.rb
  • Diff check: git diff --check
  • Lint: bin/lint reached an environment blocker (yamllint not installed)

Comment thread docs/coordination-backend.md
Comment thread docs/revert-runbook.md Outdated
@claude

claude Bot commented Sep 2, 2026

Copy link
Copy Markdown

Review summary

Small, focused doc/test change (fixes #503): aligns the documented batch-manifest targets[] example with the plain-numeric-string convention used elsewhere in the codebase ("issue:123""123"), documents that claim/release must use the exact same target string, and adds a recovery note plus contract-test coverage for a terminal closeout does not match exactly one lane mismatch.

Findings posted inline:

  • docs/coordination-backend.md: the edit left the JSON example's indentation inconsistent (some lines bumped +2 spaces, others — including the very targets line being changed — left at the old indent), so the "representative dry-run manifest" now looks malformed even though it still parses. Suggested a reindent.
  • docs/revert-runbook.md: the new recovery sentence directs agents to "the ordinary claim-only release fallback/recovery," but that's not a procedure defined anywhere else in the repo — the only existing "claim-only" concept (workflows/pr-processing.md) is a degraded-backend continuation mode for in-progress work, not a terminal-release recovery path. Worth spelling out the actual commands/steps, given this runbook is otherwise precise about exact CLI behavior and this section exists specifically to prevent dangling lanes.

Other notes (no action needed):

  • The "issue:123""123" change is consistent with how targets[] is used elsewhere in the repo (e.g. skills/post-merge-audit/bin/completed-batch-publication-preflight*), so this looks like a genuine bug fix rather than a stylistic change.
  • New Ruby test additions (test_registered_batch_manifest_dry_run_carries_pack_and_route_provenance assertions, test_terminal_closeout_mismatch_routes_to_claim_only_recovery) follow the file's existing doc-as-contract testing pattern and correctly assert the literal strings added to the docs. I wasn't able to execute coordination-telemetry-contract-test.rb in this sandbox (command execution was blocked), but the assertions read as consistent with the doc changes.
  • No security or performance concerns — this PR only touches documentation and a documentation-contract test.

@justin808 justin808 added the agent-claimed Active agent coordination claim; reconciled from private backend label Sep 2, 2026
@justin808

Copy link
Copy Markdown
Member Author

Address-review summary

Scan scope: full history from PR #703
Status: cutoff-safe summary. Detailed review outcomes are collapsed below.

Detailed review outcomes

Mattered

  • Fixed docs/revert-runbook.md recovery wording in d2d646db; it now says to re-register the affected lane under the exact manifest TARGET string and retry the same agent-coord release tuple.
  • Updated skills/pr-batch/bin/coordination-telemetry-contract-test.rb to match that concrete recovery wording.
  • Verified with ruby skills/pr-batch/bin/coordination-telemetry-contract-test.rb, git diff --check, and bin/lint on the final candidate.

Skipped

  • docs/coordination-backend.md indentation note declined as non-material: the JSON example still parses, and the contract test normalizes whitespace before checking the manifest rule, so the formatting does not change the documented contract.

Next default scan starts after this comment. Say check all reviews to rescan the full PR.

@justin808 justin808 removed the agent-claimed Active agent coordination claim; reconciled from private backend label Sep 2, 2026
Comment thread docs/revert-runbook.md
Comment on lines +1237 to +1240
immediately route to the ordinary claim-only release fallback/recovery:
re-register the affected lane under the exact manifest `TARGET` string, then
retry the same `agent-coord release` invocation from above with that exact
`--batch-id`, `--repo`, and `--target` tuple. Claim both lanes under their

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new recovery instruction tells the operator to "re-register the affected lane under the exact manifest TARGET string," but no such per-lane re-registration operation is documented anywhere in this repo (docs/coordination-backend.md's "Batch Provenance Manifest" section only describes registering an entire batch once, at batch start, not re-registering a single lane mid-batch). "re-register" doesn't appear anywhere else in docs/, workflows/, or skills/.

An operator who hits terminal closeout does not match exactly one lane in batch <id> and follows this new guidance has no concrete command to run — the recovery path this PR adds is effectively a dead end. Either this should point to whatever mechanism actually exists for correcting a mismatched lane target (e.g., re-claiming with the manifest's exact string, if that's what's meant), or the recovery command needs to be spelled out explicitly.

Comment on lines +126 to +129
{
"name": "implementation",
"owner": "batch-a-implementation",
"targets": ["123"],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation was introduced in this "representative" manifest example: name, owner, and worker_preference are now indented 8 spaces while targets and observed_host remain at 6 spaces (line 129 vs. 127-128/130). The JSON is still technically valid (whitespace-insensitive), but this doc is meant to be copy-pasted as a template, and the ragged indentation makes it look like a bad merge/edit rather than an intentional example. Worth re-flowing to a consistent indent.

@justin808 justin808 added agent-claimed Active agent coordination claim; reconciled from private backend and removed agent-claimed Active agent coordination claim; reconciled from private backend labels Sep 3, 2026
@justin808 justin808 added complexity:neutral Bounded repair, tests, docs or evidence with little net structural change. Not merge approval. P2 Useful follow-up: schedule after higher-impact work triage:drain-first Resolve the existing PR or concrete blocker before starting more work. Not merge approval. labels Sep 10, 2026
@justin808

Copy link
Copy Markdown
Member Author

🤖 Codex

Closing this stale implementation while retaining the small #503 documentation residual.

Agent Coordination #186 and current Agent Workflows already normalize canonical target identity and require exact target spelling. This branch’s proposed per-lane “re-register” recovery is not a documented operation: registration accepts the whole manifest, and the unresolved review correctly identifies that mismatch.

Issue #503 retains only the exact raw target-equality examples and, if supported by the backend, a documented whole-manifest update/re-registration path. The branch remains available as reference; no claim-only or invented per-lane recovery is being adopted.

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

Labels

complexity:neutral Bounded repair, tests, docs or evidence with little net structural change. Not merge approval. P2 Useful follow-up: schedule after higher-impact work triage:drain-first Resolve the existing PR or concrete blocker before starting more work. Not merge approval.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

agent-coord terminal closeout needs the manifest and claim target forms to match, but the documented manifest example does not

1 participant