Skip to content

fix(onboarding): readiness-driven UC-1 onboarding by nak (D35, part 1) - #241

Merged
oblinder merged 18 commits into
mainfrom
agent-onboarding-sync-bugs-fix
Oct 22, 2026
Merged

oblinder merged 18 commits into
mainfrom
agent-onboarding-sync-bugs-fix

Conversation

@oblinder

Copy link
Copy Markdown
Contributor

Summary

Readiness-driven onboarding for UC-1, the first part of D35 (docs/specs/PRD.md). The onboarding no longer waits inside the NATS handler for the rollout, the AgentCard and the tool manifest. It reads each readiness fact, and a fact that is not ready yet is nak'd and redelivered, so a slow workload does not reach the dead-letter queue.

The PRD narrows D35 to what this branch builds. Each layer that is not built is marked deferred.

Sync bugs fixed on this branch

  1. AgentCard race (5d21357): the card was accepted before the operator set Synced=True. A timing gap could give the agent a wrong role set, and OPA then denied every call. The card wait now waits for Synced=True, and a card that is not synced is nak'd.
  2. Rollout gate (91a6d0a): the pod wait could end mid-rollout and raise a permanent error for an old pod. The pod wait now reads the Deployment first and lists pods only when the rollout is complete. A pending rollout is nak'd.
  3. Not-ready errors share the real-failure budget (5d21357, 91a6d0a): not-ready errors were not nak'd, so a slow workload could reach the dead-letter queue with no retry. WorkloadNotReadyError and AgentCardNotReadyError are nak'd (_NAK_ERRORS).
  4. Dead-letter log filter (3d76caa): the filter searched for DLQ, but the subject is aiac.apply.dlq, so dead-letter moves never reached the test message.
  5. Harness gate on a stale pod (fd6998e): an agent gate could pass on a pod that had no AuthBridge yet, when the operator was late.
  6. Synced card selection and the paused-deployment message (a9a377a): a stale legacy card no longer hides the operator's synced card, and a paused Deployment is named as the cause.

Known limits and deferred layers

The PRD's D35 layers 5, 6, 9 and 10 are deferred. In particular:

  • The nak delay is constant (default 30 s), and MAX_DELIVER is 5, so a late workload has a fixed number of looks.
  • The start scan (D35 layer 9) is not built.
  • The in-handler waits (the client wait, the pod and rollout waits, the tool MCP wait) still run inside the handler. D35 layer 1 says so.
  • The bootstrap CR is dropped on any failure after the bootstrap, including a not-ready nak. It stays absent until the redelivery writes it again (docs/specs/components/aiac-agent.md, the Ack contract).

Harness gates in this PR

The system-test harness gains staged gates (test/system/uc1_onboard.py): the final template, the rollout, the operator log, the AgentCard, the CR content, and the probe hints. These are test-side work under the same decision, with no runtime change. Their pure deciders are pinned by test/unit/agent/uc/onboarding/test_uc1_staged_gates.py, spec'd in docs/testing/staged-gates-unit.md.

Review

Nine review rounds (mattpocock-skills:code-review, Standards and Spec axes). Round 1 raised 18 findings, and round 1 narrowed D35 to the built layers. Later rounds fixed the spec cites, the Ack contract, the REPO_ROOT index (which round 8 had broken and round 9 confirmed), the staged-gates spec, and the no-client log case.

Accepted exceptions (decided by the author):

  • test_uc1_staged_gates.py imports the system harness, which links to demo/assets/ at import time. Fourteen existing unit files do the same. docs/testing/staged-gates-unit.md records it.
  • Ten branch commits carry Co-Authored-By: Claude …. The repo's ai-assisted-by-trailer hook would rewrite it to Assisted-By. The author chose to keep the trailers and not rewrite history.
  • The harness gates stay in this PR while D35 is still marked proposed.

Declined, with reasons:

  • Duplicated _synced and duplicated DeploymentComplete rule, the harness file size, the repeated type switch in consumer.py, the divergent nodes.py, feature envy in _rollout_pending, and the data clump in _check_probes: out of scope for this change, reported only.
  • The comment-only "handoff" removals (a5c7fbc): out of scope, left in place.

Open at exit (judgement only, no hard findings):

  • The Ack contract's failed-nak wording: the sequence is recorded before the nak runs, so a failed nak is not reflected in the text.
  • The PRD traceability list omits the new harness helpers probe_hint, signals_report, bundle_service_probe, controller_onboarding_lines and curl_pod.
  • The onboarding_lines keep-order wording should say "input order".
  • The probe-hint test asserts only the probe-pod wording, not the service wording.
  • Standards smells: an unused containers=None kwarg in kube_fakes.py, a misleading ready name in the staged-gates test, a name prefix in curl_pod, a repeated template-metadata chain in uc1_onboard.py, and two Deployment builders with the same counters.

Checks

  • Unit lane (-k uc1_staged_gates): 43 passed on ce4e4f7.
  • ruff check and ruff format --check (pinned v0.14.8): clean on the changed Python file.
  • markdownlint-cli2: 0 errors on the changed docs.
  • checks.sh --base origin/main: all gates pass on the branch.
  • System lane (pytest -m system): 87 passed, 0 failed, 0 errors, 14 skipped (the skips are in test/system/disabled/, intentional), on a9a377a. The branch has changed since then only in docs and in test/unit/, and in comments in test/system/uc1_onboard.py. src/ is unchanged since a9a377a. The system lane was not rerun on ce4e4f7, per the author's instruction.

🤖 Generated with Claude Code

oblinder and others added 18 commits October 21, 2026 23:09
… ready

analyze_agent accepted any AgentCard with skills, and fell back to the
default <workload>.access role and scope when the fixed card budget
(~28 s) ended. A card that the operator had not fetched yet looked the
same as a card with no skills, so a timing gap gave a permanently wrong
role set: the PRB made no agent -> tool edge, and OPA denied every call.

- _await_agent_skills: a card is ready only when its Synced condition is
  True (the operator writes status.card and Synced in one status update).
  Skills alone are not proof. No card, no Synced condition, or Synced not
  True after the budget raises the new AgentCardNotReadyError
  (HTTPException 502), whose detail names the last seen state.
- analyze_agent: the default-role fallback stays only for a Synced=True
  card with no skills. The "no AgentCard found" fallback is removed: an
  agent whose card never syncs is not onboarded (fail closed).
- NATS consumer: nak the not-ready errors (_NAK_ERRORS) with the
  not-visible delay; the redelivery runs the card wait again. Only
  ServiceNotVisibleError makes the redelivery skip the client wait. The
  nak log names the error type.
- Specs: the Synced gate, the new error and the narrowed fallback (UC1),
  the Ack contract and exception tables (aiac-agent, event-broker, PRD);
  fix the stale ONBOARD_LABEL_WAIT_ATTEMPTS default (30, not 15).

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…when not ready

The operator rolls a new workload's Deployment more than once (the type
label, then the client-credentials annotation), and the CLIENT_CREATED
event that starts the onboarding comes before the last rollout. The pod
wait was time-only, so when its budget ended mid-rollout it could raise a
permanent EnforcementPreconditionError (#1) for an old pod that was about
to be replaced, or a plain 502 that the consumer did not nak (the message
then waited ACK_WAIT, 600 s).

- await_labelled_pods: each look reads the workload's Deployment first
  and lists the pods only when its rollout is complete (the controller's
  DeploymentComplete test). A 404 keeps the pod-list path (StatefulSet,
  Sandbox). Any other Deployment GET error is a plain 502 at once.
- A wait that ends not ready (rollout in progress, no pod, no label)
  raises the new WorkloadNotReadyError (HTTPException 502). Its detail
  names the rollout counters of the last look.
- The precondition checks run #6 on the Deployment's pod template when
  the rollout is never complete: an httpGet probe through the sidecar
  keeps the pod not Ready, and that stays a permanent D30 failure.
- NATS consumer: WorkloadNotReadyError joins _NAK_ERRORS; its redelivery
  waits for the client as usual.
- RBAC: get on apps/deployments for the aiac-agent ClusterRole.
- Specs: the rollout gate (UC1 classify_service and precondition checks),
  the Ack contract, exception and RBAC tables (aiac-agent, event-broker,
  PRD).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…e stuck step

The agent gate of onboarded_stack could pass before any onboarding. The
first ReplicaSet of a new workload has no rossoctl.io/type label, so the
operator's webhook does not inject AuthBridge, and its app answers the
inbound probe with HTTP 200 ("allow"). When the operator was late, the
gate passed on that pod, and the D31 check then said "the workload
converged, so its Provision ran ... a stale image". The failure messages
also probed again at raise time and dropped the raw answer, and the final
message always pointed at issue #139.

- Gates in order, per workload: registration; the final rollout (the
  template has the operator's type label and credentials annotation, the
  rollout is complete, every live pod has authbridge-proxy); onboarded
  (client.type, Provision's last call, and the SPM; a disabled client
  fails at once); D31; for the agent, the live inbound gate and the
  agent_scopes content. After the loop, the tool CR's calling-agent gate.
- Each gate failure ends with one state dump: the operator's identity
  change (pod, restarts, start, Lease holder; a note only), bundle-service
  /readyz and /bundles from the namespace, the template and rollout, the
  AgentCard Synced condition, the client and SPM, the CR content.
- Messages print the last polled (code, body) with a hint by answer, never
  a new probe. The D31 message names the cause from client.type.
- Save the Controller log before each Controller roll and at teardown.
- New knobs AIAC_ROLLOUT_TIMEOUT, AIAC_ENFORCED_TIMEOUT,
  AIAC_CR_CONTENT_TIMEOUT; BUNDLE_TIMEOUT is unchanged (rungs 5 and 6).
- Docs: the gate order, budgets and the 10-120 s OPA poll; the system lane.

Handoff 03.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
The filter of the Controller's lines for an onboarding that did not end
looked for "DLQ" in capitals, but the consumer writes the subject
"aiac.apply.dlq", so a dead-letter move never reached the message. It
also matched the full AgentCardNotReadyError traceback lines, so the
same error came four times.

Keep only the consumer's own lines for this client: each not-ready nak
("nak'd <subject> (<error>, delivery N)", which names the error) and the
dead-letter move ("moved <subject> to aiac.apply.dlq"). The pure
onboarding_lines does the match; controller_onboarding_lines reads the
log. Found by a fault injection that stopped the operator during an
agent onboarding.

Handoff 03.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
UC-1 starts on Keycloak CLIENT_CREATED, which comes before the workload
is ready (final rollout, AgentCard Synced=True, tool manifest and OPA
bundle). Today the Controller polls for each fact inside the NATS
handler, and the consumer handles one message at a time, so a slow
agent blocks every other message for about 2 min and a slow tool for
about 5 min. The not-ready naks share the 5 deliveries of real
failures, so a workload that is ready after about 4 min goes to the DLQ
and nothing retries it (measured: a 240 s operator outage during an
agent onboarding).

D35 (status: proposed) recommends "A-prime": keep the Keycloak trigger
and the NATS message as the durable record; one look per fact per
delivery and a growing nak; a time deadline with a minimum of looks,
separate from the count of real failures; a raised max_deliver backstop
and an age check before any work; keep the tool's bootstrap CR across
naks; the card source stays the AgentCard; a scan at start for enabled
clients with no SPM. It compares options A to E on seven points, names
the rejected ones, the known limits, the specs that change and the open
questions.

Docs only. Handoff 04.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
The handoff documents are not in the repo (docs/handoffs/ is
gitignored), so a reference to one points at nothing. Specs and source
code now name the durable fact instead: a decision ID (D20, D28, D33),
a spec section, or the component (the IdP configuration service).
References that only said where an idea came from are removed, with
the handoff-internal labels (Bug N, Group N, B1-B3).

Comments and docs only: no behaviour change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…lose the review gaps

From the two-axis review of handoffs 01-03.

- NATS consumer: the dead-letter line names the error class ("moved
  <subject> to aiac.apply.dlq (permanent EnforcementPreconditionError
  failure after 1 deliveries)"). The onboarded-gate message already
  keeps that line, so a permanent D30 failure and the failure at the
  last delivery are now named. Rung 5 matches the new text.
- Harness, operator: select the pods with the operator Deployment's own
  selector (AIAC_OPERATOR_DEPLOYMENT replaces AIAC_OPERATOR_SELECTOR).
  When the operator changed during the run, a gate failure saves its
  log, with --previous after a container restart in the same pod.
- Harness, state dump: print every rollout counter and the template
  marks (rollout_counters), not only the first reason.
- Harness, tool onboarded gate: read the SPM (a port-forward) only when
  the scopes and the CR hold.
- Cleanups: OperatorIdentity and Observation dataclasses replace dicts;
  one _exc_text helper and one _read_rollout reader replace repeated
  code; capture_pod_logs loses its unused root option; _check_probes
  takes (name, spec) pairs, so the pod-template check needs no fake
  pod; the nak-delay symbols lose "not_visible" (the env name stays).
- Remove the handoff references from these files too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
c4a0853 added the error class to the consumer's dead-letter line and
moved lines that D35 cites. D35 now quotes the full dead-letter line
("moved <subject> to aiac.apply.dlq (<permanent|retryable> <ErrorClass>
failure after N deliveries)"), which the harness filter still matches,
and cites every AIAC line at c4a0853.

Docs only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
- The card wait picks a Synced=True card first when more than one card
  targets the workload, so a stale legacy card does not hide the
  operator's synced card.
- WorkloadNotReadyError names a paused Deployment, a Progressing=False
  condition (ProgressDeadlineExceeded) and spec.replicas 0. These stay
  not ready (nak'd), but the DLQ cause is now clear.
- The docstring and the UC-1 spec no longer say that a complete rollout
  is always the operator's last template. They say why the checks give
  the same result on both.
- System harness: _agentcard_state matches cards as the Controller does
  (targetRef or the legacy name, a synced card first). A failed operator
  read is no longer reported as a restart, and saves no operator log.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
test(system): drop inert BLE001 noqa tags

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
…e timings

Add the staged-gates unit test module to the UC-1 unit-test list. Drop the SPIRE, lease and rollout durations that the operator source at 10dd076 does not show. Reword the D35 layer-3 built claim: the readiness checks are built but still polled inside the handler. Name the harness gates as test-side work under D35.

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
- PRD D35 layer 3: the Deployment 404 branch is nodes.py:352-356, not nodes.py:306 (the spec.paused detail).
- aiac-agent Ack contract: a tool's bootstrap CR is dropped on a Provision-side WorkloadNotReadyError nak (orchestrator.py:415-417) and stays absent until the redelivery (deferred, PRD layer 7).

Signed-off-by: Oleg Blinder <oblinder@gmail.com>
The Ack contract said a Provision-side WorkloadNotReadyError nak drops the tool's bootstrap CR. Provision does not wait for pods on this path (the Orchestrator passes the namespace, workload and type), so the only not-ready raise is in check_preconditions, which runs before the bootstrap. The line now states the generic except at orchestrator.py:415-417. The _probe cite in D35 layer 3 is nodes.py:554; line 556 is its call.

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
test_uc1_staged_gates.py has 42 tests and no spec of its own. The spec is one file under docs/testing/, listed in testing-strategy.md and in the PRD index, as the rule requires. It names each gate the tests pin and the one known exception: the file imports the system harness, which links to demo/assets/ at import time.

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
… drop

The staged-gates spec overstated three points against its tests: the CR deciders return lists, the default-access-scope agent CR is a problem, and onboarding_lines keeps each nak line rather than naming its error. D35 layer 6 now says the nak delay is set by AIAC_NOT_VISIBLE_NAK_DELAY_SECONDS. The Ack contract moves the bootstrap-drop sentence out of the nak list and cites both Orchestrator branches (the rollback quarantine and the generic except).

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
REPO_ROOT resolved to test/, not the repo root (parents[4] -> parents[5]). The staged-gates spec now states the last-keep rule of onboarding_lines, names the inbound hint only, and cites the PCE-failure drop of the bootstrap CR. D35 layer 6 states the ERROR log of every failure.

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>
1d9e3b7 changed REPO_ROOT from parents[4] to parents[5]. parents[5] is the directory above the repo, which has no test/. parents[4] is the repo root, as in test_uc1_cr_capture.py. The tests passed in both cases because pytest already puts the root on sys.path, so the wrong index went unnoticed.

The staged-gates spec now states that onboarding_lines keeps every client's lines when no client matches, and the unit test pins that case.

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Signed-off-by: Oleg Blinder <oblinder@gmail.com>

@anatolykoyfman anatolykoyfman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the runtime changes (consumer.py, preconditions.py, provision/nodes.py, provision/kube.py, the RBAC rule), their unit tests, and the D35 / UC-1 spec updates. The unit lane passes locally on ce4e4f7 (1952 passed), and CI is green.

The core logic looks right to me:

  • _rollout_pending matches the Deployment controller's DeploymentComplete test: observed generation, updated == spec, replicas == updated, available == spec. It also covers spec.replicas defaulting to 1 and zero counters that the API omits.
  • Reading the Deployment before listing pods closes the stale-pod window. A 404 (StatefulSet / Sandbox) falls back to the plain pod list, and any other read error is still a 502 at once.
  • When the rollout never completes, running #6 on the pod template is a good call. An httpGet probe through the sidecar would otherwise look like a race forever and only surface as a DLQ'd not-ready error.
  • An AgentCardNotReadyError is raised in analyze_agent before provision_service writes anything. The bootstrap drop applies only to tools. So a nak there leaves nothing behind to compensate.
  • Choosing the synced card over cards[0] fixes the case where a stale legacy card hides the real one.
  • _not_visible_seqs is still set only for ServiceNotVisibleError, so a workload or card nak redelivery still waits for the client. The test pins this.
  • The new apps/deployments get RBAC rule is the only manifest change needed. k8s/agent-deployment.yaml is the only manifest with the Controller's rules.

Non-blocking notes:

  1. Behavior change for card-less agents. The old fallback ("partial: no AgentCard found, default scope assigned") is gone. An agent workload with no AgentCard, for example one with no protocol.rossoctl.io/<name> label, is now nak'd until MAX_DELIVER and then DLQ'd, where before it was onboarded with a default scope. I think this is the right fail-closed choice, and the UC-1 spec documents it. Still, it's worth calling out in the release notes / demo docs so nobody is surprised by a DLQ entry for a misconfigured agent.
  2. The env name. AIAC_NOT_VISIBLE_NAK_DELAY_SECONDS now sets the delay for all three not-ready classes. The comment explains why, but a follow-up rename to something like AIAC_NOT_READY_NAK_DELAY_SECONDS, keeping the old name as a fallback, would make the k8s config easier to read.
  3. The shared budget. As the description says, the not-ready naks still use the same 5 deliveries as real failures, so a workload that is ready after ~4 min still lands in the DLQ with nothing to retry it. That is acknowledged as deferred D35 layers (5/9). Just flagging it as the main thing still open before D35 moves out of "proposed".
  4. The "open at exit" smells in the description (the unused containers=None kwarg in kube_fakes.py, the ready name in the staged-gates test, and the rest) are fine to leave for a follow-up.

Approving.

@oblinder

oblinder commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

System test lane: results

Result: 87 passed, 14 skipped, 0 failed, 0 errors (pytest -m system -v, exit 0, 1895 s).

Commit under test: ce4e4f7 (branch agent-onboarding-sync-bugs-fix). Cluster: Kind rossoctl, namespace aiac-system, workloads in team1.

Per file

File Passed Skipped Errors
test/system/test_uc1_onboard_agent_only.py 5 0 0
test/system/test_uc1_onboard_agent_then_tool.py 23 0 0
test/system/test_uc1_onboard_tool_then_agent.py 19 0 0
test/system/test_uc1_onboard_resync.py 6 0 0
test/system/test_uc1_onboard_failure_rollback.py 17 0 0
test/system/test_policy_pipeline.py 17 0 0
test/system/disabled/test_launcher_select_live_pod.py 0 5 0
test/system/disabled/test_uc1_onboard_cross_service_conflict.py 0 1 0
test/system/disabled/test_uc1_onboard_policy_agnostic.py 0 8 0

The 14 skips are all in test/system/disabled/. They are skipped by their own markers and did not run. They are not coverage.

Platform preconditions (checked before the run)

  • k8s/opa-kind-verify.sh: all checks passed.
  • k8s/keycloak-spi-verify.sh: all checks passed.

Redeploy before the run

  • Rebuilt and loaded localhost/aiac-pdp-config:local, localhost/aiac-pdp-policy-opa:local, localhost/aiac-policy-model-store:local, localhost/aiac-agent:local.
  • Ran the agent LLM reload block from k8s/aiac-deployment-guide.md. The .env key returned HTTP 200.
  • Re-ran keycloak-spi-install.sh with KEYCLOAK_IMAGE_BASE=quay.io/keycloak/keycloak:26.5.2, because the StatefulSet had no base-image annotation.
  • Rebuilt the demo images with demo/assets/kind-load.sh --rebuild. The demo deploy.sh was not run.
  • Health: agent, interface, PDP policy, and policy model store return {"status":"ok"}.

Not checked

  • Whether the demo team1 workloads were removed at teardown.
  • Whether the Direct Access Grants setting on the rossoctl login client was reset at session end.

@oblinder
oblinder merged commit 60f3815 into main Oct 22, 2026
19 checks passed
@oblinder
oblinder deleted the agent-onboarding-sync-bugs-fix branch October 22, 2026 17:32
@github-project-automation github-project-automation Bot moved this from New/ToDo to Done in Rossoctl Issue Prioritization Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants