Repository navigation
fix(test): system tests set up Direct Access Grants themselves, no demo/ dependency - #238
Conversation
The system lane skipped all 102 tests when the login client `rossoctl` had no Direct Access Grants, and the skip text pointed to the demo realm fix-up in demo.md. A test must not depend on a demo doc. - Add `ensure_direct_access_grants` and call it in the session fixture before `verify_login_subject`. It is idempotent and does not create the client. - Remove the `demo.md` pointers from `launcher.py`. - Update docs/agents/test.md, docs/testing and the PRD to match. - Add unit tests for the new function. - Record the dependency direction in CLAUDE.md: tests do not use `demo/`, except `demo/assets/`. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
Move ensure_direct_access_grants into provision_realm_and_users, so that
every setup path (onboarded_stack, pristine_stack and the disabled
cross-service test) enables the password grant on the login client.
- Send a partial update ({"directAccessGrantsEnabled": true}), not the
full client, so a concurrent IdP Configuration Service change is kept.
- Fail on a missing login client (as require_subject_scope does); log a
refused update, so verify_login_subject skips with the cause.
- Document that the grant stays after the run (leave-in-place).
- Move the grant unit tests to test_uc1_login_client.py.
- Add the manual grant step to k8s/opa-kind-runbook.md and point the PRD
and docs/agents/test.md to it; correct the demo teardown note.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
The redeploy section of the guide now has a flow for the aiac-agent image: rebuild, kind load, apply agent-deployment.yaml, patch LLM_BASE_URL and LLM_MODEL from .env, then restart last. It also notes that the apply resets AIAC_ENFORCEMENT_SIDE and LOG_LEVEL, and that the Secret is not changed. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…t, and restore it at teardown ensure_direct_access_grants now calls pytest.skip with the cause when the login client is missing or the admin API refuses the read or the update. Before, these cases raised an error or wrote only a log line. It returns True when it turned the grant on, and each fixture teardown then turns it off again (restore_direct_access_grants, best-effort). verify_login_subject moves into the fixture try block, so a skip there also restores the grant. The lookup reuses login_client (one copy). Docs: the Agent rebuild block in the deployment guide checks the LLM values in .env, keeps the live enforcement side and log level, builds the patch JSON with jq, and runs in a set -e subshell. The runbook snippet for Direct Access Grants reads the harness env and stops with a clear message when the admin login fails or the client is missing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
oblinder
left a comment
There was a problem hiding this comment.
Code review: 10 candidate findings, most severe first. These are candidates only: no separate verification pass ran on them.
The most serious problem: the system-test teardown can leave Direct Access Grants on, because the restore call comes after cleanup steps that can raise (findings 1–3). Finding 7 suggests one fix for all three: a context manager that restores the grant in its own finally.
🤖 Generated with Claude Code
| undeploy_workload(workload) | ||
| # Explicit scrub — do not trust an unverified operator cascade. Same reset the pre-run slate runs. | ||
| _scrub_to_pristine(admin) | ||
| if grant_enabled: # best-effort, before the pristine check (which can raise) |
There was a problem hiding this comment.
1. Teardown can leave Direct Access Grants on. restore_direct_access_grants runs only after _scrub_to_pristine, and that call can raise. The comment says the restore is "best-effort, before the pristine check (which can raise)", but the step before it can raise too. pristine_stack (line 2496) has the same order.
Failure scenario: The realm has DAG off, so the fixture turns it on (grant_enabled=True). At teardown, clear_policy_store gets a non-2xx response and raises AssertionError, or cleanup_provisioned/delete_workload_registrations raise an unguarded KeycloakError from admin.get_realm_roles()/get_clients() (for example, when the admin token is refused). The finally block exits before if grant_enabled:, so the login client keeps the password grant on. This breaks the PR's promise that a realm that had it off on purpose stays as it was.
There was a problem hiding this comment.
Fixed in 4f8043b. The new context manager direct_access_grants(admin, realm) in test/system/uc1_onboard.py turns the grant off in its own finally. onboarded_stack now runs its whole body inside with direct_access_grants(...), so the restore runs even if _scrub_to_pristine, clear_policy_store, cleanup_provisioned or the pristine poll raises. A new unit test raises AssertionError and KeycloakError from a teardown step and checks that the grant is still turned off and that the error passes through unchanged.
| for workload in reversed(list(workloads)): | ||
| undeploy_workload(workload) | ||
| _scrub_to_pristine(admin) | ||
| if grant_enabled: |
There was a problem hiding this comment.
1 (continued). pristine_stack has the same order as onboarded_stack (line 1677): the grant restore comes after cleanup steps that can raise. If one of them raises, the restore does not run and DAG stays on.
There was a problem hiding this comment.
Fixed in 4f8043b. pristine_stack uses the same with direct_access_grants(admin, TEST_REALM): block. Its teardown has no inline restore now, so no cleanup step that raises can skip the restore.
| finally: | ||
| uc1.delete_workload_crs() # after — drop any CR (there should be none on the raising path) | ||
| uc1.cleanup_provisioned(admin, TEST_REALM) # restore the pre-run Keycloak state | ||
| if grant_enabled: |
There was a problem hiding this comment.
2. Grant restore can be skipped. In the finally block, the grant restore comes after delete_workload_crs() and cleanup_provisioned(). Either can raise, and then the restore does not run.
Failure scenario: cleanup_provisioned raises KeycloakError from admin.get_realm_roles() (it is not guarded), or delete_workload_crs raises. The if grant_enabled: restore_direct_access_grants(...) line never runs, so DAG stays on. The policy store is also not cleared, and the next tests then depend on the order they run in.
There was a problem hiding this comment.
Fixed in 4f8043b. The disabled cross-service test now wraps its try/finally in with uc1.direct_access_grants(admin, TEST_REALM):. If delete_workload_crs or cleanup_provisioned raises, the grant is still restored. The order of the cleanup steps (a raise there skips clear_policy_store) was the same on main before this PR, so I did not change it here.
| # BEFORE deploying (PRB reads the role universe when the event fires). It also enables Direct Access | ||
| # Grants on the login client (the password grant of ``mint_token`` needs it; skip if it cannot); | ||
| # ``grant_enabled`` = it was off, so the teardown turns it off again. | ||
| grant_enabled = provision_realm_and_users(admin, TEST_REALM) |
There was a problem hiding this comment.
3. The grant is turned on outside the try/finally that restores it. The two statements between this call and try: can raise and leave the grant on.
Failure scenario: grant_enabled = provision_realm_and_users(...) returns True after the PUT. Then list(ready_signals) (a bad iterable passed in) or _default_ready_signals(tool_onboarded) raises before try:. The finally block never runs, so DAG stays on and nothing restores it.
There was a problem hiding this comment.
Fixed in 4f8043b. The grant is now enabled when the with block starts, and the with block covers everything after it. A raise from list(ready_signals) or _default_ready_signals(...) therefore restores the grant. verify_login_subject is back outside the inner try, as it is on main, because the context manager now covers it.
| return True | ||
|
|
||
|
|
||
| def restore_direct_access_grants(admin, realm: str) -> None: |
There was a problem hiding this comment.
4. Restore always writes directAccessGrantsEnabled=False on a shared realm. The realm is shared ("shared realm, leave-in-place"). One run's teardown can turn the grant off while another user of the realm still needs it.
Failure scenario: Run A starts with DAG off and turns it on (True). Run B, or a demo session on the same realm, sees it on (False) and depends on it. A ends and turns it off. B's next mint_token gets unauthorized_client, so B's fixture or tests skip or fail partway through the run.
There was a problem hiding this comment.
Declined. Two runs on the same cluster cannot share it in any case. Each fixture's pre-run slate undeploys both workloads, deletes their Keycloak clients and CRs, and clears the Policy Store, so a parallel run B or a demo session breaks no matter what the grant does. The restore runs only when this run turned the grant on, so a realm that had it on stays on. If we left the grant on permanently, we would break the PR's stated rule that a realm with the grant off goes back to off.
| teardown step, on the success and the failure path, best-effort (it never raises) | ||
| → teardown full-to-pristine (undeploy + delete all registrations + delete CRs, verified) | ||
| → teardown full-to-pristine (undeploy + delete all registrations + delete CRs, verified); the | ||
| realm users, roles and Direct Access Grants on the login client stay (leave-in-place) |
There was a problem hiding this comment.
6. The docstring does not match the new teardown. The module docstring says the realm users, roles "and Direct Access Grants on the login client stay (leave-in-place)". The PR now turns the grant off at teardown when the setup turned it on.
Failure scenario: A maintainer reads the module header and expects DAG to stay on after a run, for example for a manual probe. It is turned off. This goes against the documented teardown contract and against the onboarded_stack docstring.
There was a problem hiding this comment.
Fixed in 4f8043b. The module docstring now says that only the realm users and roles stay. It adds a separate step: the fixture turns Direct Access Grants on before the pre-run slate and turns it off at the end if it was off. The onboarded_stack and pristine_stack docstrings say the same.
|
|
||
|
|
||
| def provision_realm_and_users(admin, realm: str) -> None: | ||
| def provision_realm_and_users(admin, realm: str) -> bool: |
There was a problem hiding this comment.
7. Wrong level: the grant bookkeeping is copied into three callers. The on/off bookkeeping is copied into three callers and joined onto the return value of provision_realm_and_users. A context manager with its own finally would guarantee the restore.
Failure scenario: Each caller must pass grant_enabled through and put the restore in the correct place in its own finally. All three callers got that order wrong (findings 1–3). A with direct_access_grants(admin, realm): context manager would restore no matter which teardown step raises. It would also keep provision_realm_and_users focused on users and roles, not on a bool about one client field.
There was a problem hiding this comment.
Fixed in 4f8043b. I added the direct_access_grants context manager with its own finally, and all three callers use it. The grant_enabled bookkeeping is gone. provision_realm_and_users returns None again and does users and roles only. The other docs that described the old call chain now name the context manager (launcher.py, PRD.md, policy-pipeline.md, uc1-onboarding-pipeline.md, opa-kind-runbook.md).
|
|
||
|
|
||
| def login_client(admin) -> dict | None: | ||
| def login_client(admin, realm: str = TEST_REALM) -> dict | None: |
There was a problem hiding this comment.
8. Repeated full client listings. login_client lists every client in the realm (admin.get_clients()) to find one clientId. ensure and restore each list them again, and restore reads the UUID again when ensure already had it.
Suggestion: Use admin.get_client_id(KEYCLOAK_CLIENT_ID) or a query filtered by clientId, and give restore the UUID that ensure found. reenable_provisioned_clients (line 409) already has the same find-client plus partial update_client pattern, so both could share one helper.
There was a problem hiding this comment.
Fixed in 4f8043b. login_client now finds the client with admin.get_client_id(KEYCLOAK_CLIENT_ID), a query filtered by clientId, and then get_client(uuid), so it no longer lists all clients. ensure_direct_access_grants returns the UUID, and restore_direct_access_grants(admin, realm, client_uuid) uses it with no second read. I did not merge it with reenable_provisioned_clients: that function finds two workload clients by name and changes enabled, so a shared helper would save very little.
| # Read the live side and log level BEFORE the apply: the apply resets them to the manifest values. | ||
| SIDE=$(kubectl get configmap aiac-agent-config -n aiac-system -o jsonpath='{.data.AIAC_ENFORCEMENT_SIDE}') | ||
| LEVEL=$(kubectl get configmap aiac-agent-config -n aiac-system -o jsonpath='{.data.LOG_LEVEL}') | ||
| kubectl apply -f k8s/agent-deployment.yaml |
There was a problem hiding this comment.
9. The ConfigMap reload quietly resets tuned keys. The new "always reload the ConfigMap" flow runs kubectl apply -f k8s/agent-deployment.yaml on every agent rebuild. It keeps only AIAC_ENFORCEMENT_SIDE and LOG_LEVEL, so the apply quietly sets all other live keys back to the manifest values.
Failure scenario: An operator has tuned LLM_REQUEST_TIMEOUT, LLM_MAX_RETRIES or LLM_RETRY_BACKOFF_* on the cluster. The guide says not to skip the reload even after a code-only rebuild, and the reload puts those keys back to the defaults (120/3/1/30) with no warning. This change is also outside the scope of this PR (no demo dependency in the tests).
There was a problem hiding this comment.
Fixed in 4f8043b. The reload block in k8s/aiac-deployment-guide.md now reads all live keys before the apply. The patch then writes them back, with the .env LLM values on top ($live + {LLM_BASE_URL, LLM_MODEL}). Tuned keys such as LLM_REQUEST_TIMEOUT, LLM_MAX_RETRIES and LLM_RETRY_BACKOFF_*, the side and the log level keep their live values. Only a key that is new in the manifest gets the manifest value. I also added pipefail, so a failed read stops the block before the apply. The scope question is still open: this section came in with commit a589c4f, and that commit can move to its own PR.
| ( set -e # a subshell: a failed check stops the snippet, not your shell | ||
| set -a; . .env; set +a | ||
| KC=${KEYCLOAK_URL:?}; REALM=${AIAC_TEST_REALM:-rossoctl} | ||
| ADMIN=$(curl -s -X POST "$KC/realms/master/protocol/openid-connect/token" \ |
There was a problem hiding this comment.
10. The snippet hard-codes the admin realm. The manual DAG snippet says it "uses the same variables as the harness", but it hard-codes the admin realm master. The harness reads KEYCLOAK_ADMIN_REALM (default master).
Failure scenario: A .env sets KEYCLOAK_ADMIN_REALM to a value other than master, and the harness's admin login works with it. The runbook snippet logs in at realms/master, gets no access_token, and prints "admin login failed: check KEYCLOAK_ADMIN_*". That message points the operator at the wrong cause.
There was a problem hiding this comment.
Fixed in 4f8043b. The runbook snippet now sets ADMIN_REALM=${KEYCLOAK_ADMIN_REALM:-master} and logs in at realms/$ADMIN_REALM. The harness uses the same rule. The error message now shows the admin realm that it tried.
- Add the context manager direct_access_grants. It turns on Direct Access Grants on the login client and turns it off again in its own finally, so a teardown step that raises cannot leave the grant on. - onboarded_stack, pristine_stack and the disabled cross-service test enter it after the skip gates and before the first mutation, so a skip at entry changes nothing. - provision_realm_and_users does users and roles only again. - login_client uses a query filtered by clientId; the restore uses the UUID that the enable found. - Docs: module docstring, the ConfigMap reload keeps all live keys, and the runbook snippet reads KEYCLOAK_ADMIN_REALM. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
Review response
Fix commits: 4f8043b Checks ( The system lane did not run. It needs a live cluster. 🤖 Generated with Claude Code |
…n the client Address the second review on #238. - The grant is held once per session (hold_direct_access_grants) and turned off at session end (release_direct_access_grants, from the new test/system/conftest.py). Before, each module turned it on and off. - The enable writes the marker attribute aiac.test.direct-access-grants-by-harness in the same partial update. A run that is killed leaves the marker, so the next run takes the grant over and turns it off. - The release reads the client again. It turns the grant off only when the marker is still there. The runbook and demo snippets remove the marker, so a grant that an operator turns on is not turned off. - Any read or write failure is a skip with the cause, not only a KeycloakError. A write that fails is undone at once if Keycloak applied it. A 404 says that the realm does not exist. - onboarded_stack and pristine_stack share one _clean_slate context manager for the pre-run slate and the teardown. - One _update_client helper for the partial client writes, also used by reenable_provisioned_clients. The hold takes a client_id. - The disabled cross-service test holds the grant after provision_realm_and_users, which creates the realm. - Runbook snippet: a response with an unexpected shape prints the raw response and stops; the PUT fails on a status other than 204. Keycloak 26.5.2 was checked on a scratch client: a partial attributes map changes only the given keys, and "" removes a key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
A refused LLM API key (HTTP 401/403) was logged as
UnparseableLLMResponseError ("schema-invalid response"), which hid the
cause. raise_sanitized now maps 401/403 to the new LLMCredentialsError
(PRB subclass in graph.py). It stays permanent (DLQ on the first
delivery) and still triggers the UC1 rollback. It gets its own logger
(aiac.onboarding.llm_credentials) and a sanitized 502 route body.
upstream._status_code becomes the public http_status.
The failure_rollback system test now also requires the PRB request to
the injected URL and its 404 in the Controller log, so phase 1 proves
that the injection caused the failure.
Docs: deploy guide step 3b gets "Check or change the API key" (the
Controller reads the key only from aiac-agent-secret, not from .env).
The system-lane preconditions add a valid in-cluster LLM key. The specs
list the new error.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…mo dependency
- The clean redeploy in opa-kind-runbook.md no longer calls the demo's
teardown.sh / enable.sh or reads team1/openai-secret. It deletes and
installs AIAC with the AIAC manifests, as guide steps 3-5 say, and takes
all configuration from .env. It checks the LLM endpoint and key before
any change.
- The agent rebuild block in aiac-deployment-guide.md now reloads the LLM
endpoint, model and key from .env as one set (the key goes into
aiac-agent-secret), after it checks that the endpoint accepts the key.
Before, a swap of the LLM in .env moved only the endpoint, which gave
HTTP 401 on each onboarding.
- The OPA and Keycloak SPI platform preconditions move from
docs/agents/deploy.md to the guide ("Platform preconditions").
deploy.md and test.md point to that section instead of copying it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…t is off When the login client has the harness marker but Direct Access Grants is off (an operator turned it off, a realm re-import, or a partial release), hold_direct_access_grants now writes the grant on. It keeps the marker and records the hold, so the session release turns the grant off again. Before, the hold did no write, the password grant stayed off, and every stack skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
call_with_retry now retries HTTP 408 and 429 as transient, as it does a 5xx. When the retries are exhausted, raise_sanitized gives the access error (as for a 5xx), not UnparseableLLMResponseError. Before, one rate limit sent the onboarding to the DLQ. Also add LLMCredentialsError to the comments and docstrings that list the LLM errors (error_logging, the routes safety net, _digest_call). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
The LLM endpoint can accept a request and then send no answer. A retry on a new connection answers in seconds. With a 120 s or 300 s timeout, one such call used all of the 300 s that the UC-1 system fixture waits for the onboarding, and the fixture failed with "agent did not converge". With 60 s, the agent drops the call and the retry still has time to finish. The code default in aiac.agent.llm stays at 120 s. It applies only when the key is not set. The specs and the deployment guide now give both values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…o eval/scenarios Only the eval lane uses scenario_eval_agent_delegation.py and its policy file. No system test reads them. The old "one exception" location made the batch digest converter skip the policy file. - Move both files to eval/scenarios/ with the other seven scenarios. - Import the scenario from eval.scenarios in test_policy_pipeline_eval.py. - Remove the "exception" text from docstrings, the PRD, and the eval docs. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
System test resultsLane:
All 14 skips are in Not covered by this run
An earlier run failed because of the LLM model, not this changeA first full-lane run on
|
Commit 97dbbf8 added the agent-delegation import after the ambiguous-clause one. Ruff I001 (the CI lint job) needs agent before ambiguous. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Oleg Blinder <oblinder@gmail.com>
cb2164d to
8a0071a
Compare
Move the transient 408/429 rule from the LLM seam into the shared
aiac.shared.upstream.is_transient, so run_upstream also retries a rate
limit of the Kubernetes API (API Priority and Fairness), the IdP and MCP.
A 429 that names an exhausted quota (OpenAI insufficient_quota and
similar billing codes) stays permanent, as before the 429 rule.
The shared retry loop (upstream_retrying, used by run_upstream and by
call_with_retry) waits the Retry-After of the error (seconds or an HTTP
date) in place of the exponential backoff, capped at the backoff
maximum. The LLM-only _is_transient wrapper is removed.
raise_sanitized adds the HTTP status number to the access and
unparseable messages ("(HTTP 404)"), so AIAC's own ERROR line names the
cause; the endpoint and the key still cannot leak.
Update the stale comments: builder.py lists LLMCredentialsError, and the
ACK_WAIT sizing in stream.py uses LLM_MAX_RETRIES, the 60 s manifest
timeout and the capped backoff / Retry-After wait (600 s is still above
the 240 s worst case of one call).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…r any import name
The Direct Access Grants marker on the login client is now a JSON map of
holder (one per pytest session: the xdist run id and the worker id) to
expiry. A release removes its own holder and turns the grant off only
when no other live holder remains, so two runs at the same time do not
turn the grant off under each other. An expired entry, an empty map or
the legacy "true" value is stale and is taken over as before.
The session release in test/system/conftest.py finds the harness by its
file, whatever name it was imported under, and logs a warning when a
release cannot run.
The cross-service test holds the grant before its first mutation, as
_clean_slate does, so the skip cause is the same and a skip leaves no
new realm.
The rung-5 rollback test asserts on AIAC's own ERROR line with the
injected status ("UnparseableLLMResponseError: ... (HTTP 404)"), not on
the httpx INFO request line.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
| ) | ||
|
|
||
|
|
||
| def hold_direct_access_grants(admin, realm: str, client_id: str = KEYCLOAK_CLIENT_ID) -> None: |
There was a problem hiding this comment.
1. The Direct Access Grants marker could not tell two runs apart. Two pytest sessions (or xdist workers) both took over the grant, and the first one to finish turned it off while the other still minted tokens.
Fixed in 0a9e881. The marker on the login client is now a JSON map: holder id → expiry. The holder id is PYTEST_XDIST_TESTRUNUID (or one id for each process) plus the xdist worker id, because each worker runs its own session-end release. The expiry is the hold time plus AIAC_DAG_HOLD_TTL_SECONDS (default 6 h), so a run that crashed does not keep the grant on. A hold keeps the live holders and removes the expired ones. A release removes only its own entry, and turns the grant off only when no live holder stays. The session takes over an expired entry, an empty map or the old "true" value, as before. Limit: Keycloak has no compare-and-set, so two holds at the same instant can lose one entry. The code comment says this. Unit tests: test/unit/agent/uc/onboarding/test_uc1_login_client.py.
| LLM_REQUEST_TIMEOUT: "120" | ||
| # 60, not the code default 120: the endpoint can hold a request open with no answer, and a | ||
| # retry on a new connection answers at once. A short timeout leaves time for that retry. | ||
| LLM_REQUEST_TIMEOUT: "60" |
There was a problem hiding this comment.
2. LLM_REQUEST_TIMEOUT dropped from 120 s to 60 s. A slow but good completion (70–90 s) now times out on all attempts.
Not changed, by the PR author's decision: the 60 s timeout stays. The ACK_WAIT comment in src/aiac/agent/eventbus/stream.py now uses the 60 s value (see finding 8).
| return not _error_codes(exc).isdisjoint(_QUOTA_ERROR_CODES) | ||
|
|
||
|
|
||
| def retry_after(exc: BaseException | None) -> float | None: |
There was a problem hiding this comment.
3. A 429 was retried with the generic backoff. Retry-After was ignored, and a 429 that cannot clear (for example insufficient_quota) became a retryable LLMAccessError, so it went through up to 5 redeliveries before the DLQ.
Fixed in 5f4ce92. retry_after() reads the Retry-After header (seconds or an HTTP date). The wait is capped at the backoff maximum: LLM_RETRY_BACKOFF_MAX for the LLM, 30 s for run_upstream. A 429 that names an exhausted quota or billing limit (insufficient_quota, quota_exceeded, billing_hard_limit_reached, billing_not_active) is permanent again: it is not retried and it goes to the DLQ at once. Unit tests: test/unit/shared/test_upstream.py, test/unit/agent/test_llm.py.
|
|
||
|
|
||
| def _injected_error_line() -> str: | ||
| """AIAC's own ERROR line for the build failure that the injection causes (``log_by_type``: |
There was a problem hiding this comment.
4. The assertion read httpx's INFO log line. It failed with LOG_LEVEL=WARNING or after an httpx change of that line.
Fixed in 0a9e881 (with the message change in 5f4ce92). The test now asserts on AIAC's own ERROR line from log_by_type: UnparseableLLMResponseError: LLM returned an unparseable or schema-invalid response (HTTP 404). To make this possible, raise_sanitized adds the HTTP status number to the access and unparseable messages. It adds only the number, never the endpoint or the key.
| 5xx server response. Permanent failures (4xx, value/validation errors) fail identically on | ||
| every attempt, so they are surfaced immediately rather than retried. Duck-typed so it | ||
| covers ``requests`` and Kubernetes client errors without importing either here.""" | ||
| """Only transient upstream failures are worth retrying: connection resets, timeouts, a 5xx |
There was a problem hiding this comment.
5. Only the LLM code treated 408 and 429 as transient. The shared is_transient still treated 429 as permanent for the Kubernetes API, the IdP and MCP.
Fixed in 5f4ce92. The 408/429 rule and the quota exception are now in the shared is_transient. The new shared upstream_retrying(attempts, backoff_min, backoff_max) holds the Retry-After wait, and both run_upstream and the LLM call_with_retry use it. The LLM-only _is_transient is removed. Behaviour change: a 408/429 from Kubernetes, the IdP or MCP is now retried. The specs (aiac-agent.md, library-idp.md, policy-rules-builder.md) and the IdP docstrings say this.
| log = logging.getLogger(__name__) | ||
|
|
||
| # The harness that holds Direct Access Grants on the login client (``hold_direct_access_grants``). | ||
| HARNESS_FILE = Path(__file__).resolve().with_name("uc1_onboard.py") |
There was a problem hiding this comment.
6. The session release found the harness only by the exact sys.modules key. With another import name (for example --import-mode=importlib), the release did not run and logged nothing.
Fixed in 0a9e881. The conftest now finds each loaded copy of the harness by its file path (uc1_onboard.py), whatever its import name, and runs its release. If a copy has no release function, or the release raises, it logs a WARNING and the teardown does not fail.
| # Direct Access Grants on the login client, before the first mutation, as in ``uc1._clean_slate``: | ||
| # the same skip cause on the same cluster (a missing realm or login client), and a skip here leaves | ||
| # the cluster as it was (no new realm). Released at session end (``test/system/conftest.py``). | ||
| uc1.hold_direct_access_grants(admin, TEST_REALM) |
There was a problem hiding this comment.
7. The flows held the grant at different points. _clean_slate held it before provision_realm_and_users; the cross-service test held it after. So the skip causes were different, and the cross-service test could leave a new realm.
Fixed in 0a9e881. The cross-service test now holds the grant right after connect_admin, before its first change to the cluster. This is the same order as _clean_slate. The skip cause is now the same, and a skip leaves no new realm.
| # redeliver a message that's still being processed, so this is sized well above the worst case. | ||
| # Onboarding's LLM calls run synchronously inside the dispatch callback, across multiple structured | ||
| # calls (propose/audit). Each call is bounded by LLM_REQUEST_TIMEOUT (60 s in the aiac-agent-config | ||
| # manifest, 120 s code default) and makes up to LLM_MAX_RETRIES attempts (default 3; the LLM seam does |
There was a problem hiding this comment.
8. Stale comments here and in policy_builder/builder.py:84.
Fixed in 5f4ce92. The builder.py comment now lists LLMCredentialsError. This ACK_WAIT comment now has the current numbers: one LLM call takes at most LLM_MAX_RETRIES (3) × LLM_REQUEST_TIMEOUT, plus 2 waits of at most 30 s (backoff or Retry-After). That is 240 s with the 60 s manifest value, or 420 s with the 120 s code default. Both are less than 600 s, and the Retry-After cap keeps it so. The comment also says that 600 s does not cover an onboarding in which several calls each use their full retry budget.
Code-review findings: resultsA
Behaviour change (finding 5): a 408 or 429 from the Kubernetes API, the IdP or MCP is now retried, with a Fix commits: 5f4ce92 Checks
🤖 Generated with Claude Code |
Summary
System tests no longer depend on
demo/. The harness sets up Direct Access Grants on the login clientrossoctlitself, once per session, and turns it off again at session end if it turned it on.directAccessGrantsEnabled: trueand the client attributeaiac.test.direct-access-grants-by-harness: "true"in one partial update. A run that is killed (SIGKILL, job timeout,os._exit) leaves the marker. The next run takes the grant over and turns it off at its end._update_clienthelper (also used byreenable_provisioned_clients), so the write cannot overwrite the D31 scope link from the IdP Configuration Service.Not done: a dedicated login client that the harness owns. The tests must mint through
rossoctl, because it is the client inPLATFORM_SOURCE_CLIENTS: the IdP Configuration Service linksaiac-username-subto it (D31), and the source gate treats it as a platform client. A token from another client would test another path. The hold takes aclient_idfor callers that mint through another client.Evidence
demo.md; without itverify_login_subjectskipped.After: the harness enables the grant itself.
master(events off), then deleted:-m system -k test_uc1_onboard_agent_only, with the grant turned off first and turned on again after):The rung itself failed after the grant step:
agent did not converge. The cause is not in this PR. The in-cluster Controller's LLM key was refused (openai.AuthenticationError: 401 Invalid proxy server token), so the onboarding rolled back.rossoctlclient inmaster: no client -> clear message, exit 1; bad admin password -> raw response and a clear message, exit 1; happy path ->enable DAG HTTP 204, marker removed, other attributes kept.2101 passed, 406 deselected. New tests intest_uc1_login_client.pycover: one read and write per session, a marker left by a killed run, a lost response after an applied write, a grant that someone took over, a missing realm, a failed read of any type, and the_clean_slateorder (hold first, teardown on raise or skip).Code review
A
/code-reviewof the branch found 8 findings. I fixed 3 of them. The other 5 are open.Fixed
test/system/uc1_onboard.py(hold_direct_access_grants)verify_login_subjectthen failed, every stack skipped, and the marker stayed for later runs.47c184e: when the marker is there and the grant is off, writedirectAccessGrantsEnabled=True. The marker stays and the hold is recorded, so the release at session end turns the grant off again. New unit testtest_a_marker_with_the_grant_off_turns_the_grant_on_and_is_released.src/aiac/agent/llm.pyUnparseableLLMResponseError(permanent). One rate limit sent the onboarding to the DLQ and rolled the service back.6a82248: a local_is_transientadds 408 and 429 to the sharedis_transient.call_with_retryandraise_sanitizeduse it. After the last retry, the error isLLMAccessError, the same as a 5xx. The sharedis_transientis not changed, so the IdP, Kubernetes and MCP retries stay the same. New unit tests for 408 and 429: a retry that succeeds, and all retries fail.shared/error_logging.py,controller/routes.py,policy_digester/digest.pyLLMCredentialsError.6a82248: the three lists now nameLLMCredentialsError("five PRB errors" inerror_logging.py).Open (not done in this PR)
test/system/uc1_onboard.py(hold_direct_access_grants)test/system/uc1_onboard.py(hold_direct_access_grants)test/system/conftest.pysys.moduleskeytest.system.uc1_onboardand a module-global registry. If the module is imported under another name, the release does nothing and the grant stays on. A session-scoped fixture with a yield would remove this.test/system/uc1_onboard.py(_clean_slate)_clean_slateholds the grant before it creates the realm. The cross-service test does it in the opposite order, and its comment says that order gives the true skip cause.test/system/test_uc1_onboard_failure_rollback.pyAIAC_UNUSABLE_LLM_BASE_URL. An override that httpx normalizes (default port, uppercase host) makes the check fail although the injection worked.Checks after the fixes: all
checks.shgates pass (some are skipped as configured). Unit lane:2118 passed, 0 failed, 406 deselected.Merge Danger
Door: two-way
Blast Radius: test-harness
Tests and docs, plus error handling in the agent's LLM calls (
src/aiac/agent/): a refused LLM key isLLMCredentialsError, and HTTP 408 and 429 are retried. During a system session the harness writesdirectAccessGrantsEnabledand one marker attribute on the login client of the shared test realm, once at the start and once at the end. If a run is killed, the grant stays on until the next system run ends.🤖 Generated with Claude Code