Emit coordination telemetry and provenance at workflow checkpoints - #290
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds batch provenance manifests, typed coordination telemetry, prompt-contract validation, ownership-recovery guidance, telemetry-completeness checks, and outcome-bound remediation authority for PR-batch workflows. ChangesPR-batch coordination contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BatchPlanner
participant PRBatchWorker
participant PrivateBackend
participant Coordinator
BatchPlanner->>PrivateBackend: register provenance manifest
PRBatchWorker->>PrivateBackend: emit typed coordination event
Coordinator->>PrivateBackend: run telemetry-completeness audit
PrivateBackend-->>Coordinator: return audit result
Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@workflows/pr-processing.md`:
- Line 1167: Define the canonical manifest lane provenance mapping at
workflows/pr-processing.md:1167-1167 as repeatable
lane-id-to-host-and-worker-route entries, then mirror it at
skills/plan-pr-batch/SKILL.md:585-585 and skills/pr-batch/SKILL.md:526-526.
Update skills/plan-pr-batch/scripts/check_goal_prompt_size.rb:18-20 to validate
the revised mapping, and update
skills/pr-batch/bin/coordination-telemetry-contract-test.rb:10-12 to assert it
across all prompt templates.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dfd35691-d4da-4bf3-9cd8-d7de17508dd9
📒 Files selected for processing (10)
bin/validatedocs/coordination-backend.mdskills/continue/SKILL.mdskills/pause/SKILL.mdskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/scripts/check_goal_prompt_size.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/coordination-telemetry-contract-test.rbskills/pr-monitoring/SKILL.mdworkflows/pr-processing.md
ReviewThis PR adds workflow-side telemetry/provenance guidance (typed coordination events, batch provenance manifest) across the 1.
|
![chatgpt-codex-connector[bot]](https://avatars.github.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: 1b70679104
ℹ️ 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".
ReviewOverviewThis PR adds workflow-side telemetry/provenance for PR-batch coordination: four typed coordination events ( The new contract test is solid: it round-trips the operational-signal table, the JSON manifest example, exact command-occurrence counts, and the Issue found: dropped execution-rule instructionThe character-budget compaction of the three synced Goal-mode prompt templates ( It was merged/collapsed with the adjacent "Run gates; merge only when…" bullet, but the actual "apply the Batch QA Lane / include QA Evidence" imperative is gone from all three copies — not just reworded. E.g. in -- Apply Batch QA Lane; include QA Evidence.
-- Run gates; merge only when `merge_authority` is `auto_merge_when_gates_pass` or explicit merge approval exists, release+gates pass; document confidence data in the PR description.
+merge only when `merge_authority` is `auto_merge_when_gates_pass`|explicit merge approval;release+gates pass;document confidence data in the PR descriptionThe Minor / style
Security/perfNo security or performance concerns beyond the above — this PR only touches documentation/prompt-template text and a Ruby contract test; no executable/runtime code paths change. The new literal |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 072b7feb65
ℹ️ 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".
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
skills/pr-batch/bin/coordination-telemetry-contract-test.rb (2)
218-230: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject altered
agent-coordexecutables.The negative lookbehind accepts
other-agent-coord batch-audit --batch-id <id> --jsonbecause the immediate prefix still ends withagent-coord. Scan everybatch-auditinvocation and require its executable token to equalagent-coord.🤖 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 `@skills/pr-batch/bin/coordination-telemetry-contract-test.rb` around lines 218 - 230, Update test_authoritative_closeout_surfaces_use_exact_batch_audit_command to reject any batch-audit invocation whose executable token is not exactly agent-coord, including prefixed names such as other-agent-coord. Scan each batch-audit command in the checked files and validate the complete executable token rather than relying on the current negative lookbehind.
154-160: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate manifest field values.
Lines 155-160 only validate key presence. A manifest can use
nullor an empty string forcoordinator_route,worker_route, orhostand still pass. Require each provenance value to be a non-empty string, withUNKNOWNas the explicit fallback.🤖 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 `@skills/pr-batch/bin/coordination-telemetry-contract-test.rb` around lines 154 - 160, Strengthen the manifest assertions around coordinator_route, each lane’s worker_route, and host so every provenance value is validated as a non-empty string, while permitting UNKNOWN as the explicit fallback. Update the checks in the existing manifest and lanes iteration without changing the required key sets.
🤖 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 `@skills/pr-batch/bin/coordination-telemetry-contract-test.rb`:
- Around line 232-243: Update
test_batch_audit_fails_closed_only_for_an_advertised_capability to extract each
closeout section from the workflow, coordination document, and PR batch skill
content before asserting. Within every extracted section, verify
AUDIT_CAPABILITY appears in the gating condition and AUDIT_UNAVAILABLE is
recorded at the expected count, rather than matching both independently across
the whole file.
In `@skills/pr-batch/SKILL.md`:
- Around line 793-800: Restrict the telemetry-completeness audit flow to
backends with a defined audit contract, specifically `agent-coord` compatible
backends. Update the surrounding `coordination_backend` capability logic so
unsupported or unknown backends record `telemetry audit: unavailable` and
continue, while only `agent-coord batch-audit --batch-id <id> --json` results
can trigger blocking closeout behavior.
---
Outside diff comments:
In `@skills/pr-batch/bin/coordination-telemetry-contract-test.rb`:
- Around line 218-230: Update
test_authoritative_closeout_surfaces_use_exact_batch_audit_command to reject any
batch-audit invocation whose executable token is not exactly agent-coord,
including prefixed names such as other-agent-coord. Scan each batch-audit
command in the checked files and validate the complete executable token rather
than relying on the current negative lookbehind.
- Around line 154-160: Strengthen the manifest assertions around
coordinator_route, each lane’s worker_route, and host so every provenance value
is validated as a non-empty string, while permitting UNKNOWN as the explicit
fallback. Update the checks in the existing manifest and lanes iteration without
changing the required key sets.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e290168-e602-4f22-96f9-039f61eb047b
📒 Files selected for processing (7)
docs/coordination-backend.mdskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/scripts/check_goal_prompt_size.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/coordination-telemetry-contract-test.rbskills/pr-batch/bin/goal-completion-contract-test.rbworkflows/pr-processing.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/coordination-backend.md
- workflows/pr-processing.md
|
Review: Emit coordination telemetry and provenance at workflow checkpoints (PR #290) Overview
Since this repo's actual logic is almost entirely markdown prompt specs enforced by string-matching Ruby tests, I focused on cross-file consistency of the templates/phrases rather than traditional runtime bugs. I verified (via grep) that the exact 'agent-coord batch-audit --batch-id --json' command occurs the number of times the new test expects (2 in workflows/pr-processing.md, 1 each in docs/coordination-backend.md and skills/pr-batch/SKILL.md), and that the new numbered closeout steps don't leave any stale cross-references to old step numbers. Findings
Nothing else stood out as a functional bug; the extensive contract-test additions look internally consistent with the doc changes (spot-checked several of the new assertions against the actual file contents). |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
skills/plan-pr-batch/SKILL.md (2)
379-379: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the hardcoded consumer-repository documentation path.
The shared skill hardcodes
docs/coordination-backend.md. Consumer repositories may not contain that path. Resolve repository-specific documentation through the correspondingAGENTS.mdseam key instead.As per coding guidelines, shared
skills/*/SKILL.mdfiles must not hardcode consumer-repository paths.🤖 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 `@skills/plan-pr-batch/SKILL.md` at line 379, Replace the hardcoded docs/coordination-backend.md reference in the Batch Provenance Manifest guidance with resolution through the corresponding AGENTS.md seam key. Keep the documentation lookup repository-specific and ensure the shared SKILL.md contains no consumer-repository path.Source: Coding guidelines
585-586: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve verified provenance in both goal templates.
Both templates use opaque placeholders that do not require a verified pack value or expose each lane's worker model, effort, host, and binding source. Update both templates to use explicit field grammar and
UNKNOWNfallbacks. The separate Lane Card does not replace manifest provenance.
skills/plan-pr-batch/SKILL.md#L585-L586: define explicitpack_shaand per-lane worker fields.skills/pr-batch/SKILL.md#L524-L527: apply the same manifest grammar.Based on the PR objective, goal prompts must preserve verified provenance without inventing unavailable values.
🤖 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 `@skills/plan-pr-batch/SKILL.md` around lines 585 - 586, Update the goal-template provenance grammar at skills/plan-pr-batch/SKILL.md:585-586 to use explicit pack_sha and per-lane worker model, effort, host, and binding fields with UNKNOWN fallbacks, while retaining the no-guesses rule. Apply the same manifest grammar at skills/pr-batch/SKILL.md:524-527; ensure both templates preserve verified provenance and do not rely on the separate Lane Card or invent unavailable values.
🤖 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 `@skills/plan-pr-batch/scripts/check_goal_prompt_size.rb`:
- Around line 875-884: Update the prompt-size fixtures in the first and second
ready-item definitions to use the canonical completion text from the triage
skill’s documented item shape, replacing the shortened Done when text with the
required merge_authority final-state wording and PR/no-PR evidence or no-fix
rationale.
---
Outside diff comments:
In `@skills/plan-pr-batch/SKILL.md`:
- Line 379: Replace the hardcoded docs/coordination-backend.md reference in the
Batch Provenance Manifest guidance with resolution through the corresponding
AGENTS.md seam key. Keep the documentation lookup repository-specific and ensure
the shared SKILL.md contains no consumer-repository path.
- Around line 585-586: Update the goal-template provenance grammar at
skills/plan-pr-batch/SKILL.md:585-586 to use explicit pack_sha and per-lane
worker model, effort, host, and binding fields with UNKNOWN fallbacks, while
retaining the no-guesses rule. Apply the same manifest grammar at
skills/pr-batch/SKILL.md:524-527; ensure both templates preserve verified
provenance and do not rely on the separate Lane Card or invent unavailable
values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 576263ab-d6e6-405d-b322-c039fb999d0a
📒 Files selected for processing (7)
docs/coordination-backend.mdskills/plan-pr-batch/SKILL.mdskills/plan-pr-batch/scripts/check_goal_prompt_size.rbskills/pr-batch/SKILL.mdskills/pr-batch/bin/coordination-telemetry-contract-test.rbskills/triage/SKILL.mdworkflows/pr-processing.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/coordination-backend.md
- workflows/pr-processing.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 543b27e739
ℹ️ 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".
* origin/main: Support direct merges without merge queues (#297)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43653cc084
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6644e102fd
ℹ️ 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".
ReviewThis PR wires four typed coordination events ( Code quality / correctness
Maintenance risk
Minor
SecurityNo concerns beyond what's noted above — the argv/no-shell-eval requirements, the signed-payload host binding, and the fail-closed treatment of |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 401750c269
ℹ️ 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".
Review SummaryThis PR adds workflow-side telemetry/provenance (typed What looks solid
Issues found (see inline comments)
General observations (not blocking, worth a second look)
Overall this is a large but internally consistent and well-tested change (the PR's own contract-test suite is quite aggressive about catching drift between the synchronized surfaces). The two findings above are the concrete gaps that suite didn't catch. |
Review: Emit coordination telemetry and provenance at workflow checkpointsOverviewThis PR extends the
The change is almost entirely additive prose (SKILL/workflow docs + Code quality / correctness
SecurityNo vulnerabilities found. The new signed-payload and manifest-provenance logic is fail-closed by design ( Test coverageCoverage is unusually thorough for this kind of change: the new Minor observations
Overall this is a well-tested, internally consistent change with sound fail-closed security properties in the one piece of real executable logic it touches. My main suggestion is the cross-file duplication noted inline — worth a follow-up if there's ever appetite to reduce the six-way sync surface. |
Address-review summaryScan scope: full history via Mattered
Optional
Skipped
Deferred-work tracking: none; no new issue or task was created. Next default scan starts after this comment. Say |
|
head_sha: 4f62608
|
…gned-launch-readiness * origin/main: Emit coordination telemetry and provenance at workflow checkpoints (#290) # Conflicts: # CONTEXT.md # docs/agent-workflows-model-routing.md # docs/pr-batch-skills.md # skills/pr-batch/SKILL.md # skills/pr-batch/bin/dispatcher-capability-preflight # skills/pr-batch/bin/model-routing-contract-test.rb # skills/triage/SKILL.md # workflows/pr-processing.md
…-policy * origin/main: Emit coordination telemetry and provenance at workflow checkpoints (#290)
…/pr291-redesign-implementation * commit '6239fd0afa5bb7d87cd3fb09cc22ae30bcf1e369': Fix locale-dependent test, surface policy-only CLAUDE.md follow-ups, add read-only seam-drift audit (#337) Add PR #377 changelog entry (#382) Make PR descriptions human-first (#377) Remove unsupported signed-launch enforcement (#374) Emit coordination telemetry and provenance at workflow checkpoints (#290) Add explicit multi-language lint CI (#313) Reconcile later-completed audit targets (#315) Report source lines in security preflight findings (#311) Gate completed-batch publication on terminal scope and QA (#308) Add guarded merge submission seam (#304) Fix exact-head readiness when status rows omit SHA (#307) fix: allow YAML timestamps in action scanner (#305) Pin workflow dependencies and define release trust boundary (#295) Support direct merges without merge queues (#297)
1 participant
Why
Coordinated batches need durable, backend-neutral telemetry and verified execution provenance. Without these contracts, closeout can silently infer missing backend state, lose route/host identity, or report incomplete assignment and audit evidence as if it were complete.
Outcome
This PR:
UNKNOWNwhen facts cannot be authenticated;actual_hostafterlaunch_token, and the exact legacy pre-host migration rule across all six authoritative surfaces;PR URLs/issue URLs) across the workflow and both batch prompts, with a non-vacuous contract guard.Scope
4f626084a03ae23b336aa18e1807e097b04ae64fd025594886b2a03b5d0037c97502a32b0a8bd8c7d34553551969402c3b622baf19374c125e9a1b66e2106ebcb657ca38b93656d3bfc5487a4da70a83+1993/-147+15/-3PR #290 was squash-merged by maintainer
justin808as282a87aaf4bf1dd602723efddc78252f7a138235from exact reviewed head4f626084a03ae23b336aa18e1807e097b04ae64f. The merge commit's sole parent is015970aa752827b501ec37be421612361fd5b9d6, and its treed2f46622f558a5a64e75133ef8a00466f3500da1exactly matches the independently constructed combined-tip tree.Verification
bin/validate: PASS on attempt 2, exit 0, 1,084 seconds; RuboCop 115 files/0 offenses; log SHA-2562ccfad48d66b66c17222eb6c3e2b8cb985249e9e000f0150ddb53f88afb19ea5.umask 077made the unchanged mode-comparison fixture non-discriminating. Attempt 2 changed only the validator child umask to022.SECURITY_PREFLIGHT_OK; no API coverage, untrusted/hidden participant, untrusted/metadata queue, or suspicious-text findings requiring action.validatePASS,claude-reviewPASS, and CodeRabbit PASS/approved.015970aa752827b501ec37be421612361fd5b9d6: PASS, exit 0, 1,644 seconds; RuboCop 117 files/0 offenses; log SHA-256f3e238954b249ca7772413f0f6b8c3ed9aceb63a8f737c42dbed671395710a6e.accepted-deferral; no changelog issue, PR, or scope expansion was created.Confidence note: exact-head validation, independent QA/checker review, dependency replay, security preflight, hosted CI, review convergence, combined-tip validation, readiness v2, and merge authentication are complete. Issues #205 and #206 are closed by the exact-head merge.
Codex Decision Log
origin/mainadvanced after the frozen-base candidate was validated and had two overlapping paths.015970aa, and authenticate the final squash-merge tree.282a87aaf4bf1dd602723efddc78252f7a138235has the identicald2f46622f558a5a64e75133ef8a00466f3500da1tree.QA Evidence
actual_host, documentation/code/test parity, and protected-helper parityCloses #205
Closes #206