Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
josjeon
force-pushed
the
sao-17523-upstream-diagnostics
branch
from
September 28, 2026 16:48
eb9dd54 to
e069e65
Compare
7 of 11 tasks
josjeon
added a commit
that referenced
this pull request
Sep 28, 2026
…SAO-17580] (#272) ## Summary `make typecheck` currently fails on `main` and on every open PR. This annotates the four call sites that SQLAlchemy 2.1.0 broke, unblocking merges. The failure belongs to no branch's changes. ## What happened SQLAlchemy 2.1.0 no longer lets mypy infer the element type through `result.scalars().all()`: ``` server/src/agent_control_server/services/control_bindings.py:282: error: Need type annotation for "rows" (hint: "rows: list[<type>] = ...") [var-annotated] Found 1 error in 1 file (checked 51 source files) make: *** [Makefile:138: typecheck] Error 1 ``` The repository lock pins 2.0.51, which still infers it, so the error appears only in CI, where dependencies resolve fresh. Main's own run at `bd7d91f` installs both and reports the error in the same log: ``` + mypy==2.3.1 + sqlalchemy==2.1.0 server/src/agent_control_server/services/control_bindings.py:282: error: Need type annotation for "rows" ``` ## Evidence this is not branch-specific | Branch | Date | CI | | --- | --- | --- | | `main` | 2026-09-22 | success | | `main` | 2026-09-24 | failure | | `fix/sao-17418-runtime-token-auth` | 2026-09-24 | success | | `fix/sao-17418-runtime-token-auth` | 2026-09-28 | failure | SQLAlchemy 2.1.0 was released between those dates, and the flagged file is not in the diff of either affected PR. Re-running the job does not clear it: CI resolves 2.1.0 again each time, so the failure is deterministic rather than flaky. ## What changed Four sites share the pattern. CI reports only the first, so fixing them one at a time would surface the next on the following run. | File | Annotation | | --- | --- | | `services/control_bindings.py:282` | `rows: list[ControlBinding]` | | `services/controls.py:306` | `versions: list[ControlVersion]` | | `services/controls.py:512` | `controls: list[Control]` | | `endpoints/agents.py:463` | `agents: Sequence[Agent]` | `agents.py` keeps `Sequence` rather than `list` because that call site does not wrap the result in `list()` and only reads and slices it. `Sequence` and `Agent` were already imported there. ## Scope - Type annotations only. No runtime behavior changes, no control flow, no queries touched. - Deliberately out of scope: raising the SQLAlchemy floor in `pyproject.toml` or regenerating the lock. That is a wider dependency decision, and these annotations are correct under either version. ## Risk and Rollout - Risk level: minimal. Annotations are erased at runtime. - No migration or configuration change. - Rollback plan: revert this PR. ## Testing - [x] `mypy server/src` clean under the locked versions: 51 source files, no issues. - [x] `ruff check server/src` clean. - [ ] Full server test suite: not run locally. The local Docker daemon is unresponsive, so the Postgres-backed suite cannot start. CI covers it here, and the change is annotations only. - [ ] Reproduced the failure locally against CI's resolved versions: not possible right now, the internal package index returns 401. The CI log for `main` is cited above instead. ## Checklist - [x] Linked issue: [SAO-17580](https://splunk.atlassian.net/browse/SAO-17580). - [x] Documentation/examples: no update required. - [x] Unblocks #268 (approved, blocked only by this) and #270. Both touch different files, so this merges independently of either. # AI Tool Assistance Usage Statement - [x] AI assistance was used to draft parts of the implementation, that was subsequently modified and extended. - [ ] AI assistance was used in generating tests/documentation/comments for this change. - [x] AI assistance was used for optimizing/troubleshooting/refactoring existing code in this change. - [ ] AI assistance was used to draft this entire change as is.
josjeon
enabled auto-merge (squash)
September 28, 2026 21:11
… [SAO-17523] When the upstream authorization service rejected a check, the log recorded only the operation and the status code. During the 2026-09-23 multitenant incident that left the cause unknown: the pods showed Orbit returning 422 and Agent Control translating it to 502, with nothing to say which field was rejected. Attach three things to that warning: the operation, the shape of the target context that was sent, and the field path plus error kind from the upstream validation body. Orbit answers with the standard FastAPI validation envelope, where `type` and `loc` alone separate a malformed target_id (`uuid_parsing` at body.context.target_id) from an unsupported target_type (`enum` at body.context.target_type) from an unknown operation (`enum` at body.operation). Caller data stays out. Only `type` and `loc` are taken from each entry; `input`, `ctx`, and `msg` echo the caller's values and are dropped. Target context is described by shape rather than value, so absent, null, empty, and wrongly typed stay distinguishable without logging an identifier. The logged list is capped, with the total reported separately so a truncated list never reads as complete. Nothing Orbit-specific is added. Target values remain opaque, with no checks for `log_stream` or UUID format, and the parser never raises: an unexpected rejection body degrades to an empty summary instead of turning a 502 into a 500.
josjeon
force-pushed
the
sao-17523-upstream-diagnostics
branch
from
September 29, 2026 18:00
e069e65 to
6396e25
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
During the September 23 multitenant incident, Orbit rejected two authorization checks with 422 responses, and Agent Control returned 502. Agent Control's warning named the operation and status, but gave us no way to tell which field Orbit rejected. We still do not have the original caller payload, so the incident's root cause is unconfirmed.
This PR makes the next rejection easier to diagnose from the logs. It is the scoped change for SAO-17523.
Before and after
What changed
When the upstream authorization service returns an unexpected 4xx, the warning now includes:
target_typeandtarget_id(for example,string:len=10rather than the value).parsed,oversized, orunusable; the total count isunknownwhen it cannot be computed.The details appear in the warning text and structured log fields, so they survive plain text, Agent Control JSON, and Orbit's host managed logging. The API response is unchanged: the same rejection still returns 502 with
AUTH_UPSTREAM_REJECTED.Illustrative outcomes (the original incident payload was not captured):
VALIDATION_ERROR; Orbit is not calledAUTH_UPSTREAM_REJECTEDoversizedorunusable; validation count isunknownKeeping caller data out of logs
The summary drops validation messages, inputs, and context values. It logs only known error kinds and field names; unknown entries become
otheror<other>. Each field path is limited to four parts, and the error list is limited to five entries. Bodies over 64 KiB are markedoversized; deeply nested, malformed, or non JSON bodies are markedunusable. These cases produce an empty summary with anunknowncount without changing the API response. A parsed empty list has a count of0.Verification and rollout
splunk-ao==0.4.0was not cached. The full server suite also needs PostgreSQL, which was unavailable locally.This change helps identify a future recurrence or equivalent reproduction. It does not establish or fix the original malformed payload. Local runtime token request validation is covered separately by #268.
Checklist
AI Tool Assistance Usage Statement