Skip to content

fix(examples): repair the e2e gallery fake and floor the workflow concurrency clamp - #721

Draft
anilkmr-a2z wants to merge 2 commits into
awslabs:mainfrom
anilkmr-a2z:fix/examples-e2e-fake-and-concurrency-clamp
Draft

anilkmr-a2z wants to merge 2 commits into
awslabs:mainfrom
anilkmr-a2z:fix/examples-e2e-fake-and-concurrency-clamp

Conversation

@anilkmr-a2z

Copy link
Copy Markdown
Contributor

Two independent one-line fixes on the examples surface, plus the regression test the second one was missing.

On the two-fixes-one-PR shape: this is deliberate, not scope creep. Both defects are in the examples surface, both are one-line, and both were proven on current main; carrying them as one diff is cheaper to review than two PRs that touch adjacent files. Each is a separate commit, so either can be reverted alone.


Fix 1 - the e2e gallery fake omitted replayed

test/e2e/examples/test_examples_gallery_e2e.py's fake /terminals/run-step handler answered with terminal_id, last_message and status, but no replayed. The cao_workflow shim reads data["replayed"], so every test routed through the shim died with KeyError: 'replayed', surfacing as RunState.FAILED.

Before: 3 failed, 1 passed. After: 4 passed.

Failing tests now green:

  • test_loop_example_records_n_iterations
  • test_conditional_example_runs_only_the_taken_branch
  • test_fanout_example_records_distinct_step_ids_per_shard

test_loop_raw_http_example_works_without_the_shim passed all along, because it bypasses the shim by design.

A reviewer running the default pytest invocation will see nothing wrong

addopts carries -m 'not e2e and not integration', which silently deselects all four of these tests. Reproducing the failure requires an explicit -m e2e:

uv run pytest --no-cov -q -m e2e test/e2e/examples/test_examples_gallery_e2e.py

Please use that exact invocation to check this; the bare path collects zero tests and reports success.

Why the fix belongs on the fake and not on the shim

This is the non-obvious call in this PR, so stating it plainly. The tempting "fix" is .get("replayed", False) in the shim. That would be wrong, and there is a comment at src/cao_workflow/__init__.py forbidding it on BR-3/TD-7 grounds. Quoting it:

Direct indexing, matching the three reads above - NEVER .get("replayed", False) (BR-3/TD-7). The field is non-optional with a default on RunStepResponse, so the server always serialises it; a defaulting read would silently re-manufacture the exact false replayed=False this flag exists to eliminate, and would hand the author a DEAD terminal_id labelled live. A KeyError is the honest failure.

So the KeyError was doing its job: it was correctly reporting that the fake was not a faithful stand-in for the real route. The shim is untouched by this PR.

Corroborating evidence that the fake is the defect: the sibling fake at examples/workflow/tests/conftest.py already carries "replayed": False along with this same rationale in a comment. The gallery fake was simply missed. This change makes the two consistent, copying that comment across so the next person does not "simplify" it away.


Fix 2 - the workflow concurrency clamp crashed on 0

examples/workflow/workflow.py computed:

max_workers = min(inputs.get("concurrency", 2), INPUTS["concurrency"]["default"])

That bounds the input from above but not from below. INPUTS declares concurrency as a bare int with no minimum, so --input concurrency=0 clears the pre-run validation gate, resolves through min(0, 2) to max_workers=0, and ThreadPoolExecutor(max_workers=0) raises:

ValueError: max_workers must be greater than 0

as an opaque traceback mid-run - after the sequential plan step has already executed. A negative value behaves identically. Verified directly against the real _validate_inputs gate:

concurrency=0:  validation PASSED resolved=0  -> max_workers=0   executor ValueError: max_workers must be greater than 0
concurrency=-1: validation PASSED resolved=-1 -> max_workers=-1  executor ValueError: max_workers must be greater than 0
concurrency=1:  validation PASSED resolved=1  -> max_workers=1   executor OK
concurrency=3:  validation PASSED resolved=3  -> max_workers=2   executor OK

The fix is a max(1, ...) floor. One worker serialises the fan-out units rather than dropping any, so a nonsense input degrades to slow instead of to a crash.

Regression test added

The clamp was only ever asserted in the upward direction (3 -> 2, plus the 2 -> 2 no-op at the default). No test ever passed a value below the default, which is exactly why the unbounded lower half shipped. The new test in examples/workflow/tests/ asserts 0 -> 1 and 1 -> 1, and additionally asserts that the validation gate really does admit 0 - since that premise is the whole reason the floor has to live in the script rather than in the input declaration.

Before: 8 passed, 1 skipped. After: 10 passed, 1 skipped.

uv run pytest --no-cov -p no:cacheprovider -q examples/workflow/tests/

The 1 skip is the live-provider test, correctly gated off by CAO_RUN_LIVE_PROVIDER_TESTS; this PR does not set it.

I confirmed the test guards the fix rather than merely passing alongside it: reverting only workflow.py and rerunning fails the new [0] case.


Verification summary

Check Result
-m e2e test/e2e/examples/test_examples_gallery_e2e.py 3 failed 1 passed -> 4 passed
examples/workflow/tests/ 8 passed 1 skipped -> 10 passed 1 skipped
New test fails without fix 2 Confirmed ([0] case)
black --check clean
isort --check-only clean
uv.lock in diff absent (no dependency change; all commands run --frozen)
src/cao_workflow/__init__.py in diff absent (the shim is correct as written)

Diff is 3 files, +39/-1.

Not fixed here

Refs #720 - a broad pytest examples/ fails to collect because examples/workflow/tests/__init__.py and examples/plugins/cao-discord/tests/__init__.py both claim the package name tests. CI is blind to it because CI passes the specific path. This PR does not address that; the candidate remedies both change import resolution for the suite and need a real verification run, so it is filed for a tracked decision instead. Referenced, deliberately not closed.

The examples-gallery fake /terminals/run-step handler answered with
terminal_id, last_message and status but no "replayed", so every test
that goes through the cao_workflow shim died with KeyError: 'replayed'
and surfaced as RunState.FAILED. Three of the four tests were failing.

The fix belongs on the fake, not on the shim. The shim reads
data["replayed"] by direct indexing on purpose: the field is
non-optional with a default on RunStepResponse, so a real server
always serialises it, and a defaulting .get("replayed", False) would
silently re-manufacture the exact false replayed=False the flag exists
to eliminate -- handing the workflow author a dead terminal_id
labelled live. A KeyError is the honest failure, and here it was
correctly reporting that the FAKE was not a faithful stand-in for the
route. The sibling fake in examples/workflow/tests/conftest.py already
carries the field with this same rationale; this one was just missed.

Note the default pytest invocation deselects all four tests --
addopts carries -m 'not e2e and not integration' -- so reproducing
this needs an explicit -m e2e:

  uv run pytest --no-cov -q -m e2e \
    test/e2e/examples/test_examples_gallery_e2e.py

Before: 3 failed, 1 passed. After: 4 passed. The one test that always
passed, test_loop_raw_http_example_works_without_the_shim, does so
because it bypasses the shim by design.

Assisted by AI
The clamp read min(concurrency, declared_default), which bounds the
input from above but not from below. INPUTS declares concurrency as a
bare int with no minimum, so --input concurrency=0 clears the pre-run
validation gate, resolves through min(0, 2) to max_workers=0, and
ThreadPoolExecutor(max_workers=0) raises

  ValueError: max_workers must be greater than 0

as an opaque traceback mid-run -- after the sequential plan step has
already executed. A negative value behaves the same way.

Floor it with max(1, ...). One worker serialises the fan-out units
rather than dropping any, so a nonsense input degrades to slow instead
of to a crash.

Also add the downward regression case. The clamp was only ever
asserted upward (3 -> 2, and the default 2 -> 2 no-op), which is
precisely why the unbounded lower half shipped: no test ever passed a
value below the default. The new test asserts 0 -> 1 and 1 -> 1, and
asserts that the validation gate really does admit 0, since that
premise is the whole reason the floor has to live in the script rather
than in the input declaration.

Verified: examples/workflow/tests/ went from 8 passed, 1 skipped to
10 passed, 1 skipped. Reverting only workflow.py fails the new [0]
case, confirming the test guards the fix.

Assisted by AI
@codecov-commenter

codecov-commenter commented Sep 2, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@c0c9b72). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #721   +/-   ##
=======================================
  Coverage        ?   91.63%           
=======================================
  Files           ?      203           
  Lines           ?    28444           
  Branches        ?        0           
=======================================
  Hits            ?    26065           
  Misses          ?     2379           
  Partials        ?        0           
Flag Coverage Δ
unittests 91.63% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

chaogebaba added a commit to chaogebaba/cli-agent-orchestrator that referenced this pull request Sep 9, 2026
…ck, A2.2 backfill entry point, AC-A2.9 audit

Builds on R1 (86f3070). Folds the three Opus DESIGN-delta corrections landed in
the blueprint at cli-subagents 41ca40cd. B4 rejection (terminal-token release
plane is wrong) was accepted; the other three become code/test here.

A2.5 — `cao identity release` interlock (CODE, fail-before/pass-after + 2 mutants):
  release_resume_claim_owned now enforces the AC4 TTL/liveness rule — a claim
  still WITHIN resume.claim_ttl_s is a live/uncertain claimant and release
  REFUSES `claimant_live` (cannot race a resume in flight); only a claim PAST the
  TTL (confirmed-dead) is quiesced (audited claim_reconciled) then released. It
  clears the claim ONLY — never rewrites owner_principal, never stamps
  resumed_by, and is audited as claim_released_by_owner. claim_ttl_s param
  (defaults to resume.claim_ttl_s=600) makes the interlock deterministically
  testable. Mutants: skip-the-liveness-check (killed by
  test_f865_r2_release_refuses_live_claimant); rewrite-owner-on-release (killed
  by test_f865_r2_release_never_rewrites_owner_or_stamps_resumed_by). The two R1
  release tests updated to model a leaked = dead claim (claim_ttl_s=0.0).

A2.2 — no owner-override on claim + explicit operator backfill entry point:
  Confirmed `cao identity claim` has NO owner-override mode (test:
  --owner on an already-owned root refuses already_owned, CLI included). Added
  the sanctioned recovery entry point run_owner_backfill_operator() (thin,
  idempotent re-run of the provenance-checked _migrate_f829_a2_owner_backfill;
  the version stamp gates only the automatic once-at-start run) + the
  `cao identity backfill-owners` CLI verb. Test recovers a root skipped at start
  for an active claim: release (dead) then re-run recovers the mailbox owner;
  a second run rewrites nothing (idempotent).

AC-A2.9 — audit-record separation (CODE): PrincipalResult gains an `audit` field.
  States (ii) schema-unavailable and (iii) lookup-failed keep the SAME coarse
  outcome (principal_unavailable/retryable true) and are separated ONLY in the
  audit record (schema_unavailable vs lookup_failed), asserted by
  test_f865_r2_principal_states_ii_iii_separated_in_audit.

Refs awslabs#721 (F865). Commit --no-verify pre-authorized (fx121 awslabs#708).
chaogebaba added a commit to chaogebaba/cli-agent-orchestrator that referenced this pull request Sep 9, 2026
…claim (no leaked claim)

`assign(resume_from=…, agent_profile=<position name>)` resolves the profile
(requested-or-root, prepare_resume) and a position name with no composed store
file fails CLOSED with E-PROFILE-MISSING deep in create_terminal
(terminal_service.py:2107) — AFTER the CAS resume claim was taken. The claim was
then held for the full resume.claim_ttl_s (600 s), wedging the conversation as
session_resume_in_progress (observed tonight on conv_ef975cd1…).

Fix (the brief's PREFERRED option — validate BEFORE the claim):
* api.main._f829_admit_resume validates the resolved agent_profile loads
  (_validate_resume_profile_loads — the SAME fail-closed load_agent_profile
  check create_terminal does at :2107) between authorize and claim. A missing
  profile is now a clean typed `resume_refused missing=profile reason=profile_missing`
  (retryable false) with ZERO claim ever taken. This makes the leak structurally
  impossible for this class, rather than relying on post-hoc compensation.
* Changes NOTHING about which handles/profiles resume ACCEPTS (awslabs#712 stays
  separate) — it only moves the SAME check ahead of the claim. A None resolved
  profile (bare native spawn) is left to create_terminal as before.
* The existing post-claim `except BaseException` compensation is retained as a
  belt-and-suspenders fallback for any OTHER post-claim failure (empirically it
  already covers a ProfileMissingError raised by create_terminal — see report).

Tests:
* Fail-before/pass-after (service seam, real sqlite): resume with a missing
  position profile → typed missing=profile refusal, claim NEVER taken, and the
  NEXT resume with a loadable profile succeeds and claims —
  test_f865_r3_missing_profile_refuses_before_claim_next_resume_succeeds
  (+ _valid_profile_resume_is_unaffected control).
* Route-level committed contract (zero spawn) —
  test_f874_resume_missing_profile_refused_zero_spawn (test_f829_a2_admission.py).
* Mutant skip-the-pre-claim-profile-validation → killed by the fail-before test.

Refs awslabs#730 (F874), awslabs#721 (F865). Commit --no-verify pre-authorized (fx121 awslabs#708).

This branch has not been deployed

No deployments
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.

2 participants