Repository navigation
feat: implement issue #1404 — [Phase 2] Author a machine-readable interaction contract for every agentic role, per the standard - #1418
Conversation
…eraction contract for every agentic role, per the standard
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🤖 CodeAnt AI — Review Status
|
📝 WalkthroughWalkthroughThe PR adds machine-readable contracts for eight personas and three runtimes. It adds a standalone YAML validator, hermetic Bats coverage, and a pinned CI job for interaction-contract changes. ChangesInteraction contract definitions
Contract validation and CI
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LintWorkflow
participant Validator
participant Repository
LintWorkflow->>Validator: Run contract validation
Validator->>Repository: Read interaction-contract YAML files
Validator->>Validator: Check schema, timers, emits, guards, and workflow paths
Validator-->>LintWorkflow: Report success or validation error
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
codeant-ai
Bot
added
the
size:XL
PR Summary by QodoAdd per-role interaction contracts and hermetic CI validation gate
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces machine-readable interaction contracts for various agentic roles along with a Python validation script (validate-interaction-contracts.py) and a BATS test suite to verify their well-formedness. The review feedback suggests aligning the Python script with PEP 8 by moving the yaml import to the top level and removing redundant local imports/checks. Additionally, the BATS test suite should be updated to use $BATS_TEST_TMPDIR for automatic cleanup instead of manual directory creation and teardown.
Code Review by Qodo
Context used✅ Compliance rules (platform):
48 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: f5bb8dc87651a7b838270b13755198cada5a9d3d
Review mode: triage-approved (single reviewer)
Summary
Additive change implementing Story 2 / #1404: machine-readable interaction contracts for all 8 personas (personas/<id>/interaction.yml) and 3 non-persona runtimes (interaction-contracts/*.yml), plus a hermetic well-formedness validator, a 13-case bats suite, and a new validate-interaction-contracts job in lint.yml. The triage assessment holds: no deletions, no secrets, workflow-level permissions: contents: read, and the new job's actions/checkout pin 3d3c42e5aac5ba805825da76410c181273ba90b1 was independently verified via the GitHub API as the v7.0.1 tag commit (not guessed — per repo policy).
Risk is MEDIUM (not LOW) only because the PR modifies a CI workflow and adds validator logic; nothing security-sensitive was found.
Linked issue analysis
Closes #1404 — substantively addressed. All 5 acceptance criteria verified:
- Contracts exist for all 8 personas + dev-lead/pr-review/ci-failure-analyst runtimes ✓
- Every contract declares triggers.events[], triggers.timers[] (cron + role + justification + stop_condition + event_fast_path), emits[], idempotency_key, concurrency_lane, stop_markers[], budget ✓
- Independently cross-checked each contract's triggers against the deployed
on:blocks at the PR head SHA via the API: dev-lead.yml (6 events + 3 repository_dispatch types — exact match), dev-lead-retry.yml (cron15 */2 * * *— match, §6.3 leak correctly declared with null fast path), pr-review-trigger.yml (match, workflow_dispatch deliberately excluded per §3), pr-review-sweep.yml (cron2,17,32,47 * * * *+ workflow_run:[completed] fast path — match), ci-failure-analyst.lock.yml (check_run only — match), persona-mention.yml (issue_comment / pull_request_review_comment / discussion_comment — matches all 8 advisory persona contracts) ✓ - Standalone-file location per §8.2 option (b); validate-personas untouched and green ✓
- Hermetic validator (no network, yaml.safe_load) + dedicated bats coverage for valid/invalid/missing/malformed ✓
Findings
No blocking issues. Non-blocking notes (carry to a follow-up, ideally before Story 4 / #1406):
- Docs/validator emit-vocabulary divergence — the §8.1 normative example in docs/agentic-interaction-model.md uses
comment-marker:but the validator's EMIT_PREFIXES only acceptscomment:(all authored contracts usecomment:). Reconcile the doc example or the validator before #1406 consumes the doc as normative. (Also flagged by codeant-ai/qodo.) - Permissive prefix match — bare
commit/pushin EMIT_PREFIXES meansstartswithaccepts e.g.commitfoo; tighten when convenient. - Workflow-path check accepts traversal —
repo_root / wfwith../or absolute paths would passis_file(); low impact (contracts land via reviewed PRs) but an easy hardening. - Pin-version inconsistency — new job uses checkout v7.0.1 while the rest of lint.yml uses v6.0.3; pin is valid, just inconsistent.
- Unpinned
pip3 install pyyamlmatches the existing validate-personas job pattern — consistent with repo precedent, not a regression.
Unresolved review threads are all advisory comments from third-party bots (codeant-ai, gemini-code-assist, qodo); the dev-lead runtime already assessed them (review-changes: no-changes), and no human reviewer has raised questions. The substantive items are captured in the findings above.
MCP secret scan (run_secret_scanning) unavailable in this environment; gitleaks CI check is green.
CI status
All checks green at f5bb8dc: validate-interaction-contracts, bats, shellcheck, actionlint, Lint, unit-tests, CodeQL (actions + python), Agent Security Scan, agent-shield, gitleaks, SonarCloud quality gate, caller-stub-freeze, template-drift, validate-personas, verify-persona-teams/identity, holdout-guard — all SUCCESS. Cancelled entries are superseded duplicate runs.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
b37dd82
don-petry
left a comment
don-petry
left a comment
There was a problem hiding this comment.
Review — PR #1418 (#1404 interaction contracts)
Genuinely good work, and more thoughtful than the story asked for. Highlights worth naming: splitting runtime (interaction-contracts/*.yml, deployment lens) from role (personas/<id>/interaction.yml) is a distinction §8.1 didn't make but should have; requiring event_fast_path as a key so a null is meaningful (marking the §6.3 leak) is exactly right; binding each contract to a real on-disk workflow keeps it non-aspirational; and the budget == pr-automation-budget ⇒ stop_markers invariant is a nice inference. Validator runs clean — OK: 11 interaction contract(s) valid, invariants hold. — and the 229-line bats suite is real coverage.
One substantive finding.
(should fix) The #860 rule-1 check is bypassed for the one role that actually self-triggers
check_self_trigger is well-built and would fire here. dev-lead subscribes to repository_dispatch:dev-lead-ci-failure, and dev-lead-retry.yml fires exactly that dispatch. Declaring dispatch:dev-lead-ci-failure in emits would trip the collision rule — I confirmed it:
events include repository_dispatch:dev-lead-ci-failure : True
emits declared: ['commit', 'comment:<!-- … budget exhausted -->', 'label:needs-human-review']
→ if 'dispatch:dev-lead-ci-failure' were declared, check_self_trigger would FAIL
interaction-contracts/dev-lead.yml's header addresses this head-on — the re-dispatch is "the TIMER's stop-condition-gated mechanism … NOT a free emit; modeling it as the timer is what keeps the #860 rule-1 self-trigger check meaningful." The reasoning is sound and the gating is real. But the effect is that the check passes vacuously for the only role in the fleet with a live self-trigger loop — the rule is satisfied by a modeling decision rather than by a verified guard.
There's a second instance the check also can't see: emits: commit + a pull_request subscription. dev-lead pushes commits and pull_request:synchronize fires on push — textbook trigger-by-own-output. It's genuinely safe, but only because of the dev-lead-own-commit sender check (scripts/dev-lead-intent.sh:176-185). The docstring honestly flags this class and defers it to Story 4.
So both of dev-lead's real self-trigger paths are currently declared safe by prose in a YAML comment, and nothing mechanical will notice if either guard is later removed — which is precisely the #860 failure shape (a control that is documented rather than enforced).
Suggested shape — turn the exemption into a checkable invariant rather than deleting it:
emits:
- "dispatch:dev-lead-ci-failure" # declare it honestly
self_trigger_guards: # …and declare why it is safe
- emit: "dispatch:dev-lead-ci-failure"
guard: "dev-lead-retry stop_condition (open, not human-gated, budget not exhausted)"
- emit: "commit"
guard: "dev-lead-own-commit sender skip — scripts/dev-lead-intent.sh:176-185"Then check_self_trigger becomes: a collision is allowed only when a matching self_trigger_guards entry exists — and Story 4 (#1406), which already reads real code, can verify each named guard is still present. That keeps the honest modeling, makes the mitigation load-bearing, and means removing a guard breaks CI instead of passing silently.
Happy for this to land as a follow-up rather than blocking #1418 if you'd prefer to keep the story scoped — but it should be a tracked item, not a comment. Say which and I'll file it against the epic.
Minor
personas/pr-review/interaction.ymlexists but there is nopersonas/pr-review/persona.yml(pr-review is a runtime, not a persona inpersonas/). Worth confirming that placement is deliberate and thatvalidate-personas.pywon't later trip over a directory with aninteraction.ymland no manifest.
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Auto-dismissed (#617): coderabbitai[bot] CHANGES_REQUESTED on a superseded commit. The bot re-reviews the new head automatically — a valid concern will return as a fresh review.
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #1418 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-08-02T03:47:45Z. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 9c4ac5dfd9056730bd0ce638a7c02698e59a2da7
Review mode: triage-approved (single reviewer)
Summary
Re-review after the fix-requested cycle at b37dd82. Three commits since then (+202/−13 across 6 files) resolve the single blocking finding and all carried non-blocking notes. The blocking (should fix) thread from @don-petry on interaction-contracts/dev-lead.yml (the vacuous #860 rule-1 check) is implemented exactly as proposed: dev-lead now honestly declares its retry dispatches (dispatch:dev-lead-ci-failure, -reviews-retry, -issue-retry) and the commit emit collision, each legalized only by a matching self_trigger_guards: entry naming the in-code guard and its location — giving #1406 a verifiable hook. The validator enforces guard shape (emit must be declared in emits, non-empty guard + location) and only suppresses a collision when a matching guard exists. All 16 review threads are now resolved.
Linked issue analysis
Closes #1404 — substantive AC verification from prior cycles still holds (contracts for all 8 personas + 3 runtimes; required trigger/timer/emit/idempotency/concurrency/stop-marker/budget fields; triggers cross-checked against deployed on: blocks; hermetic validator + bats coverage). The delta strengthens AC #3 (contracts no longer aspirational — dev-lead's real self-trigger loop is declared, not omitted) and AC #5 (validator now covers guard semantics with 5 new bats cases: guarded self-dispatch allowed, mismatched guard rejected, missing location rejected, undeclared-emit guard rejected, whitespace-only payload rejected).
Findings
Prior blocking finding — resolved. self_trigger_guards: implemented in both interaction-contracts/dev-lead.yml and personas/dev-lead/interaction.yml (commit guard at scripts/dev-lead-intent.sh:176-185); validator's check_self_trigger now fails an unguarded collision and directs authors to declare a guard. Thread resolved by the reviewer.
Prior non-blocking findings — resolved. pip3 install now pinned (pyyaml==6.0.3, --only-binary); persist-credentials: false added to the new lint job's checkout. Also fixed from other reviewers: pr-review.yml declares the sweep re-dispatch (workflow_dispatch:pr-review-trigger, with the new workflow_dispatch: emit prefix); whitespace-only emit payloads rejected via .strip().
New issues: none found. The guard-check logic is sound (guard entries must reference declared emits; prefix-overlap ordering comment-marker: before comment: preserved; the .strip() payload check subsumes the old bare-prefix check). Remaining nit (checkout pinned at v7.0.1 while other jobs in lint.yml use v6.0.3) is cosmetic and consistent with prior-cycle precedent — non-blocking.
MCP secret scan (run_secret_scanning) unavailable in this environment; gitleaks CI check is green at head.
CI status
All checks green at 9c4ac5dfd9056730bd0ce638a7c02698e59a2da7: bats (incl. 5 new guard tests), unit-tests, validate-interaction-contracts, validate-personas, shellcheck/ShellCheck, Lint, actionlint, CodeQL (actions + python), Agent Security Scan, agent-shield, SonarCloud quality gate, gitleaks, caller-stub-freeze, template-drift, gh-aw-compile, verify-persona-teams/identity, holdout-guard, CodeRabbit, Graphite — SUCCESS. CANCELLED entries are superseded duplicate runs from earlier pushes; SKIPPED entries are conditional dependency-audit ecosystems. Branch is BEHIND main but MERGEABLE.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #1418 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
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 `@tests/test_validate_interaction_contracts.bats`:
- Around line 381-386: Update the test “validate-interaction-contracts rejects
an emit with a whitespace-only payload” to assert the specific
whitespace-payload diagnostic, such as “non-empty value,” instead of the generic
“emits” substring; keep the nonzero status assertion unchanged.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e783b3f4-1ed4-4c1a-81d3-093e5186ab27
📒 Files selected for processing (5)
.github/workflows/lint.ymlinteraction-contracts/pr-review.ymlinteraction-contracts/validate-interaction-contracts.pypersonas/dev-lead/interaction.ymltests/test_validate_interaction_contracts.bats
| @test "validate-interaction-contracts rejects an emit with a whitespace-only payload (label: )" { | ||
| sed -i 's# - "label:demo"# - "label: "#' "$TMP/personas/demo/interaction.yml" | ||
| run python3 "$VALIDATOR" "$TMP" | ||
| [ "$status" -ne 0 ] | ||
| [[ "$output" == *"emits"* ]] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the whitespace-payload assertion specific.
[[ "$output" == *"emits"* ]] can pass when another validation rule causes the failure. Match the whitespace-payload diagnostic, such as non-empty value.
Proposed assertion
- [[ "$output" == *"emits"* ]]
+ [[ "$output" == *"non-empty value"* ]]📝 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.
| @test "validate-interaction-contracts rejects an emit with a whitespace-only payload (label: )" { | |
| sed -i 's# - "label:demo"# - "label: "#' "$TMP/personas/demo/interaction.yml" | |
| run python3 "$VALIDATOR" "$TMP" | |
| [ "$status" -ne 0 ] | |
| [[ "$output" == *"emits"* ]] | |
| } | |
| `@test` "validate-interaction-contracts rejects an emit with a whitespace-only payload (label: )" { | |
| sed -i 's# - "label:demo"# - "label: "#' "$TMP/personas/demo/interaction.yml" | |
| run python3 "$VALIDATOR" "$TMP" | |
| [ "$status" -ne 0 ] | |
| [[ "$output" == *"non-empty value"* ]] | |
| } |
🤖 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/test_validate_interaction_contracts.bats` around lines 381 - 386,
Update the test “validate-interaction-contracts rejects an emit with a
whitespace-only payload” to assert the specific whitespace-payload diagnostic,
such as “non-empty value,” instead of the generic “emits” substring; keep the
nonzero status assertion unchanged.
Review — fix requested (cycle 1/3)The automated review identified the following issues. Please address each one: Findings to fixAutomated review — NEEDS HUMAN REVIEWRisk: MEDIUM SummaryAdds machine-readable interaction contracts for all agentic roles plus a hermetic Python validator wired into CI with bats coverage (1103 additions, 14 files). Code quality is high and the human reviewer's substantive finding (self_trigger_guards as a checkable invariant) was fully implemented. Escalating only because an active CHANGES_REQUESTED review from CodeRabbit with one unresolved minor thread blocks approval. Linked issue analysisCloses #1404 (Phase 2: machine-readable interaction contracts per the §8.1 standard). Substantively addressed: contracts exist for all 8 personas plus 3 runtime lenses, the validator enforces the §8.1 shape, timer invariants, workflow-path existence, and the #860 rule-1 self-trigger check. don-petry's review finding — that self-trigger exemptions were documented in prose rather than enforced — was resolved in code: check_self_trigger now permits a collision only when a matching self_trigger_guards entry names the in-code guard, with bats coverage for guarded/unguarded/malformed cases. FindingsBlocking (process): CodeRabbit's latest review (2026-08-02T02:53Z) is CHANGES_REQUESTED with 1 unresolved thread on tests/test_validate_interaction_contracts.bats: the whitespace-payload test asserts the broad substring "emits" where the validator emits the specific "non-empty value" diagnostic. Valid minor nit — a one-line fix ( Security: None. Validator uses yaml.safe_load, no subprocess/eval/network. New workflow job uses SHA-pinned checkout, persist-credentials: false, version-pinned PyYAML, 5-min timeout. Gitleaks passed on head SHA. MCP secret-scanning tool was not available in this session; the gitleaks CI check covers this PR. Note: The only change since the prior approved review at CI statusAll checks green on head SHA Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review. Additional tasks
The review cascade will automatically re-review after new commits are pushed. |
2 participants
User description
Closes #1404
Implemented by dev-lead agent. Please review.
CodeAnt-AI Description
Add machine-readable interaction contracts and validate them in CI
What Changed
Impact
✅ Fewer misconfigured agent workflows✅ Earlier detection of self-triggering automation✅ Clearer CI errors for invalid interaction contracts💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Tests
Chores