Skip to content

feat(agent): declare workflow convergence criteria - #762

Open
ayushtr-aws wants to merge 2 commits into
aws-samples:mainfrom
ayushtr-aws:feat/517-explicit-convergence
Open

feat(agent): declare workflow convergence criteria#762
ayushtr-aws wants to merge 2 commits into
aws-samples:mainfrom
ayushtr-aws:feat/517-explicit-convergence

Conversation

@ayushtr-aws

@ayushtr-aws ayushtr-aws commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Adds an optional, validated convergence contract so workflows can explicitly describe the sensors and terminal signals that define completion. The v1 contract is declarative and preserves existing runtime success behavior.

Area

  • cdk — infrastructure, handlers, constructs
  • agent — Python runtime / Docker image
  • clibgagent client
  • docs — guides or design sources (docs/guides/, docs/design/)
  • tooling — root mise.toml, scripts, CI workflows

Related

Closes #517

Changes

  • Add the optional convergence schema and typed workflow models.
  • Validate convergence modes, sensors, outcomes, early exits, and matching verification steps at load time.
  • Declare convergence for new-task, PR-iteration, and PR-review workflows, with validation corpus coverage.
  • Document coding and repo-less convergence examples and synchronize generated documentation.

Validation

  • mise run build
  • uv run pytest --no-cov tests/test_workflow_loader.py tests/test_workflow_validation_corpus.py tests/test_workflow_tree_valid.py -q (64 passed)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

@ayushtr-aws
ayushtr-aws requested review from a team as code owners August 14, 2026 01:57
@ayushtr-aws

Copy link
Copy Markdown
Contributor Author

Self-review — principal-architect pass

Reviewed via the /review_pr workflow (pr-review-toolkit agents: code-reviewer, type-design-analyzer, pr-test-analyzer, comment-analyzer + principal-architect judgment). Posting for transparency since this is a self-authored PR.

Verdict: Approve with nits

Clean, backward-compatible, explicitly declarative-only. The convergence contract is validated at load (JSON schema → Pydantic) and correctly does not touch runtime success inference in v1. No blocking issues. Nits are test-coverage asymmetry and two small doc/comment refinements.

Vision alignment ✅

Serves reviewable outcomes — makes "what counts as done" a declarative, validated per-workflow contract while explicitly preserving the defensive _resolve_overall_task_status guard against false-positive completion. early_exit.allow_on_policy_deny: false is fail-closed. v1-descriptive scoping is the right incremental step.

Blocking issues

None.

Non-blocking suggestions / nits

  1. human_approved mode is entirely untested (most substantive gap). It's a first-class ConvergenceMode with its own schema contains rule, but no valid case, fixture, or invalid case exercises it — dropping it from the enum or breaking its outcome-coupling rule would pass all tests. Add a valid mode: human_approved + [human_approved] case and a negative mismatched-outcome case.
  2. Cross-field rules tested asymmetrically. Only review_submitted's mode↔outcome rule and only verify_lint's sensor↔step rule have negative tests. The parallel artifact_delivered contains rule and the verify_build→step rule are happy-path only — deleting either schema branch wouldn't fail a test. Add one negative case each.
  3. required_sensors default [] is looser than the schema (models.py vs schema minItems: 1). Harmless via the loader (schema runs first), but the model accepts required_sensors=[] while the contract rejects it. Either drop the default or document that [] is the model's spelling of "absent."
  4. Docs table cell over-promises runtime behavior. In WORKFLOWS.md (+ mirror), the required_sensors row says results "must be evaluated using their step's gate semantics" — reads as a runtime obligation, contradicting the v1-declarative claim below it. Reword to "results a conforming harness would evaluate … each must have a matching step (enforced at load)."
  5. early_exit.allow_on_policy_deny default (false) is never asserted — only true is covered. Add an early_exit: {} case asserting False.
  6. Naming trip-hazard. ConvergenceTerminalOutcome (pr_opened/review_published/…) is deliberately disjoint from the existing TerminalOutcome (pr_url/review_posted/…). Docs distinguish them well; a one-line comment on the alias — noting the distinction and that cross-field invariants are enforced by the loader's schema pass, not the model — would help future readers.

Documentation ✅

docs/design/WORKFLOWS.md and the generated Starlight mirror are updated in lockstep and byte-identical (no stale-mirror risk). The terminal_outcomes vs convergence.terminal_outcomes distinction (with the pr_url/pr_opened example) pre-empts the likely misreading. Schema $id description appends #517 provenance without overwriting #248. Only refinement is nit 4.

Tests & CI

  • Confirm build (agentcore) goes green before merge (it was still pending at review time; other checks pass). Local mise run build + targeted pytest (57 passed) reported in the PR body.
  • Coverage is genuinely additive: happy paths, optionality (backward-compat guard), sensor↔step (lint), JSON-pointer error paths, and all three shipped workflows end-to-end. Gaps are the negative-case asymmetry in nits 1/2/5.
  • Bootstrap synth-coverage: N/A — agent-runtime + docs only; no CDK constructs/stacks/CFN resource types touched.

Review agents run

  • code-reviewer ✅ — confirmed (conf. 95) no runtime path bypasses schema validation (only parse_workflow calls Workflow.model_validate, after validate_shape); exact enum parity between models.py Literals and the JSON schema.
  • type-design-analyzer ✅ — schema-only cross-field validation is sound given the file's single-source-of-truth design; flagged nits 3 and 6.
  • pr-test-analyzer ✅ — flagged nits 1, 2, 5 and two loose match patterns.
  • comment-analyzer ✅ — docs accurate; flagged nits 4 and 6.
  • silent-failure-hunter — omitted: no error-handling/fallback code; validation raises WorkflowValidationError rather than swallowing.
  • /security-review — omitted: no IAM/Cedar/network/secrets/input-gateway change; workflow files are first-party trusted config and the change is additive, fail-closed validation.

Human heuristics

  • Proportionality ✅ — schema + two thin Pydantic models + 3 YAML declarations; no runtime machinery built ahead of need.
  • Coherence ⚠️ minor — the two "terminal outcome" vocabularies are disjoint by design and documented (nit 6).
  • Clarity ✅ — names/docstrings bound v1 behavior carefully; fails closed with clear JSON-pointer error paths. One doc over-promise (nit 4).
  • Appropriateness ⚠️ minor — tests assert intended behavior but with negative-case gaps (nits 1/2/5); a regression breaking human_approved or the artifact_delivered/verify_build rules would pass today.

Recommendation: Approve pending build (agentcore) green. Nits worth a quick follow-commit — especially human_approved coverage (nit 1) and the "must be evaluated" doc wording (nit 4) — but none block merge.

🤖 Generated with Claude Code

@codecov-commenter

codecov-commenter commented Aug 14, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@5d6da09). Learn more about missing BASE report.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #762   +/-   ##
=======================================
  Coverage        ?   92.06%           
=======================================
  Files           ?      318           
  Lines           ?    87611           
  Branches        ?     8763           
=======================================
  Hits            ?    80657           
  Misses          ?     6954           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ayushtr-aws

Copy link
Copy Markdown
Contributor Author

Addressed all self-review nits in d346c440:

  1. Added valid and mismatched-outcome coverage for human_approved.
  2. Added negative coverage for artifact_delivered outcome coupling and both verify_build/verify_lint sensor-to-step rules.
  3. Changed omitted required_sensors to None and added min_length=1, with direct model and loader tests rejecting explicit empty lists.
  4. Reworded the docs to say a conforming harness would evaluate sensors, preserving the v1 declarative-only scope.
  5. Added coverage asserting early_exit.allow_on_policy_deny defaults to False.
  6. Added a model comment distinguishing convergence signals from artifact-oriented terminal outcomes and locating cross-field enforcement in the schema pass.

Also tightened schema-path assertions in the convergence validation tests.

Validation:

  • mise run build
  • Focused workflow suite: 64 passed
  • Agent suite: 1601 passed, 82.84% coverage
  • CDK suite: 4072 passed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Agent: Workflow schema — explicit convergence and terminal_outcomes

2 participants