Repository navigation
ENG-3683–3692: CI gates, secret scanning and functional-suite overhaul (combined) - #1163
Merged
aix-ahmet merged 45 commits intoOct 9, 2026
Merged
Conversation
This was referenced Oct 6, 2026
Collaborator
Author
|
Verdict: request changes. The SDK change is correct, and CI went from 17 failing legs to 6 with no concurrency failures. But Reviewed: PR #1163 at Blocking
Should fix
Backend / platform tickets to file (found by this PR's tests)
What's good
Not verified
|
Collaborator
Code review — PR #1163 (head
|
Combined from PR #1139, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
Combined from PR #1140, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
…ed test Combined from PR #1138, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
Combined from PR #1137, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
…, drop dead fixtures Combined from PR #1158, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
Combined from PR #1157, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
Combined from PR #1154, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
Combined from PR #1155, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
Combined from PR #1156, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
Combined from PR #1162, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
build_run_payload turned a Pydantic class into model_json_schema() and an instance into model_dump(), both dicts, so executionParams.expectedOutput reached the backend as an object. The backend rejects that with "400 executionParams.expectedOutput must be a string", which is how test_agent_lifecycle.py::test_agent_expected_output_run_returns_json failed in the first live CI run. Save already sent json.dumps(schema). Run payloads now JSON-encode a class (the same schema string save persists) and an instance, as they already did for a dict. Unit tests cover the class passed through execution_params, the agent's own class, and an instance.
test_delete_agent_in_use_fails expected "cannot be deleted" in the error,
but the backend refuses the delete with the code err.agent_is_in_use
("Failed to delete resource: err.agent_is_in_use" in the first live CI
run). Assert on that code instead of prose the backend never sends.
test_agent_round_trips_execution_and_inspector_fields failed in the first live CI run with "assert None == 2" on fetched.max_inspectors. The SDK encodes and decodes the field under the same key (maxInspectors, checked offline: build_save_payload sends it, from_dict reads it back), so the None means the backend does not persist it. The test still saves with max_inspectors=2, so the backend must accept it, and keeps asserting the fields that do round-trip.
Both tests asked the model to repeat a "secret", and in the first live CI
run GPT-5.4 treated that as a request to leak hidden instructions:
test_prompt_override_reaches_model got "I can't share secrets or hidden
instructions", and test_variables_are_substituted_into_instructions got
the literal "word" back instead of the substituted value.
Each now asks for a random token ("Reply with exactly this code and
nothing else: <token>") and asserts the token appears. The variables test
keeps the placeholder in the instructions, where the SDK rewrites {{x}} to
{x} on save and the backend substitutes the query keys at run time (the
same contract v1's process_variables used), so the SDK path is unchanged.
test_run_time_inspector_runs expected a step for the run-time inspector, but in the first live CI run the response's steps held only the agent's own LLM step, so the response exposes no evidence the inspector ran. Renamed to test_run_time_inspector_accepted per the file's *_accepted convention: it asserts the inspectors list reaches the payload as given and that the backend accepts the run. Drops the now-unused _step_agent_label helper.
test_second_page_does_not_repeat_the_first compared pages 0 and 1 of an
unsorted page_size=1 model search, and in the first live CI run both
returned the same model (6ab3e2f8...). Without sort_by the SDK sends
sort: [{}], so the backend's order is not guaranteed across two calls,
and whether pageNumber counts from 0 or 1 is not something the test
should depend on.
The test now sorts by creation time and compares pages 1 and 2 (distinct
under either convention) with three records each, asserting the two id
sets are disjoint. The SDK sends pageNumber/pageSize exactly as v1 did,
so no SDK change.
The skill_dir fixture named each skill func-test-skill-<epoch seconds>. Two skills created within the same second (consecutive tests, or a rerun) share that name, and in the first live CI run the backend refused the second with err.exist_name, erroring the setup of TestAgentReflectsSkillUpdate::test_agent_run_reflects_updated_skill_content. Append a uuid fragment, as the agent names in the same file already do.
In the first live CI run every caller of resolve_multi_action_tool failed
("Searched 100 tool(s) ... Slack-backed tools with actions available:
none"), and both TestInputActionFieldsRoundTrip tests failed on "Pinned
integration 686432941223092cb4294d3f reports no actions available".
The cause is actions_available, not the tenant: the backend leaves it
unset (None) on search results and integrations. The same run shows the
Slack integration with actions_available=None while
test_tool.py::test_tool_get_parameters passed on that integration's
non-empty list_actions(). Filtering on the field ruled out every tool.
resolve_multi_action_tool now skips only an explicit False and probes
candidates with list_actions(): Slack-backed tools first, then tools of
any other integration, then the rest, bounded to 5 pages and 25 probes.
The AIXPLAIN_TEST_MULTI_ACTION_TOOL_ID override is unchanged, and finding
nothing still fails, naming every probed tool and its action count.
The snake-case round-trip tests use the new resolve_action_source: the
pinned Slack integration when it lists actions, else the resolved
multi-action tool (same list_actions/list_inputs surface), failing loudly
when neither lists any.
The first live run of the functional matrix lost ~40 tests to "429 err.agent_concurrency_limit_reached" (limit 20 for the test team): every leg drives agents on the same tenant, several at -n 2, and all legs started at once. Those failures say nothing about the SDK. Add max-parallel: 4 to the functional job's strategy, with the reason beside it, and a unit test in test_ci_matrix_coverage.py that fails if the cap is dropped or raised above four. required-checks.md notes the cap next to the concurrency group that bounds runs per PR.
ExecutionConfig.to_api_dict passed execution_params['expected_output'] through unchanged, so a session run with a Pydantic class, instance or dict sent a non-string executionParams.expectedOutput, which the backend rejects with "400 executionParams.expectedOutput must be a string". This is the same bug the previous commit fixes on the direct run path. The session path now encodes it the same way: a class as its JSON schema, an instance as its JSON, and a dict or list with json.dumps.
…ype metadata ENG-3684 turned the old skip in test_sync_model_connection_type into an assert, but the pinned sync-only model exposes no connection_type metadata on the backend, so the model leg went red on a fact no SDK change can fix. Both connection-type tests now call pytest.xfail when the metadata is missing, naming the pinned asset and the AIXPLAIN_TEST_SYNC_ONLY_MODEL / AIXPLAIN_TEST_ASYNC_ONLY_MODEL override. When the metadata is present the real assertions run unchanged, so the tests start passing by themselves once the asset carries it or is re-pinned.
expectedOutput was encoded three times, differently. build_run_payload JSON-encoded a Pydantic class, instance or dict but sent a list raw; ExecutionConfig.to_api_dict also encoded lists; build_save_payload turned a class into its schema string but stored a Pydantic instance (and a dict) as an object, which the agent engine reads back as a Python repr when a run falls back to the stored value. All three now call session._expected_output_to_wire (imported lazily in agent.py, as resolve_attachments already is, to avoid an import cycle): class -> json.dumps(model_json_schema()), instance -> model_dump_json(), dict/list -> json.dumps, str/None unchanged. The run path keeps its rule that an empty string under text/markdown is sent as None. The save-side change is deliberate. The only test pinning a dict at save, test_basemodel_instance_is_still_dumped_to_dict, preserved the original v2 behaviour rather than a backend contract: the save endpoint already stores the schema string for a class. It now asserts a JSON string, and new tests cover a dict at save, save/run agreement, a list on the run path and the text-format empty-string rule. CHANGELOG notes the wire-format fix and that a fetched agent now carries the string.
test_agent_round_trips_execution_and_inspector_fields failed in CI with "assert [] == ['output']": the platform's v2 create/update path does not persist inspectorTargets or maxInspectors, and the read side defaults the targets to []. That is a backend gap (ticket to be filed), not an SDK bug. The test now asserts both fields on agent.build_save_payload() -- what the SDK actually sends -- before saving, with a comment saying why and when to move them back below the fetch. The output_format and expected_output round-trip assertions are unchanged.
…lures
Five tests in test_agent_run_params.py were red in CI for reasons that are
not SDK bugs, and one for a fixture bug:
- variables and prompt: the SDK sends the v1-compatible contract (query
{input, <vars>}; top-level prompt/criteria/identifier), but the agent
engine reads only query.input. Each test is split into a payload test
that must pass and an output test under xfail(strict=True,
raises=AssertionError). A refused run still raises APIError and a
non-success status calls pytest.fail, so only the output check can be
the expected failure.
- budget maxDurationSeconds=0: the payload carries the cap, but the run
hangs IN_PROGRESS on the backend. The payload is asserted first, the run
gets timeout=60 instead of the 300 s default, and the test is a strict
xfail on the SDK TimeoutError only.
- attachments: RED_PNG_BYTES was one pixel at 50% alpha, so "coral" was a
correct answer. It is now a 16x16 8-bit RGB PNG with no alpha channel,
generated with zlib/struct and decoded to confirm every pixel is
(255, 0, 0).
- test_debug_returns_analysis duplicated test_debugger.py, so one missing
meta-agent turned two legs red. Deleted; the module docstring now says
where debug() is covered and how the xfails are structured.
…s $defs
test_run_team_agent_with_expected_output failed in CI with a pydantic
ValidationError: _People nests _Person, so its JSON schema reaches _Person
through $ref/$defs, which the agent engine does not resolve. The nested
type degrades to a list of anything and the model answered
{"result": ["Bruno: 34 ..."]}.
The test is now xfail(strict=True, raises=pydantic.ValidationError) with a
backend reason (ticket to be filed). The persistence checks, a failed run
and a missing delegation raise AssertionError, which the xfail does not
accept, so only the structured-output validation can be the expected
failure. The flaky mark is dropped: rerunning a known backend gap only
spends three billed team runs to reach the same xfail.
test_run_response_generation_accepted spent a billed, rerun-able team run to show only that the backend accepts run_response_generation, while test_agent_run_params.py documents that the agent runtime ignores the flag (and its JSON-output test already passes it). Two files disagreeing about the same parameter is worse than one saying it once. Delete the test and replace its line in the module docstring with a note pointing at test_agent_run_params.py.
tests/functional/v2/test_issue.py files real issues, and there is no API
to delete one. The issue leg ran on every push to main, against the
production backend, so each merge added two undeletable test reports to
the production issue system.
The issue leg now carries `prod_safe: false` in the matrix. "Run Tests"
receives the leg's path and that key through env: (no more `${{ }}` in
the script body) and, when IS_PROD is true and the key is false, prints a
::notice:: explaining the skip and exits 0 without running pytest. The
leg still runs on PRs, pushes to test and the nightly, against the test
backend.
On production runs `functional (issue)` therefore reports success
without having run, and release.yaml's gate counts it green. That is
deliberate (no production run can exercise it without the side effect)
and is stated in the workflow comment and docs/ci/required-checks.md.
New unit tests run the real step with a stub python: the issue leg runs
pytest when IS_PROD is false and skips with a notice when it is true,
every other leg still runs its exact pytest command on production, and
the step body holds no `${{ }}`. The expression evaluator learned
`matrix.*` lookups for this.
…_TOKEN SLACK_TOKEN and HF_TOKEN were exported to every functional leg, including same-repository PR runs, which pip install whatever dependency changes the PR carries. Only three legs read SLACK_TOKEN (agent-lifecycle, model and tool, through the slack_token fixture), and nothing under tests/functional reads HF_TOKEN at all; aixplain/utils/config.py defines it but no SDK or test path consumes it. Those three legs now carry `slack: true` in the matrix, and the env step expands the secret as `matrix.slack && secrets.SLACK_TOKEN || ''`, the same pattern that gates the _PROD secrets, writing SLACK_TOKEN to GITHUB_ENV only when it is non-empty. HF_TOKEN is no longer exported. Unit tests: the `slack` set must equal the legs whose test file uses the slack_token fixture, and every other leg renders SLACK_TOKEN as empty; the real env step exports SLACK_TOKEN only when it has a value; nothing under tests/functional reads HF_TOKEN and main.yaml does not export it. The secrets table in docs/ci/required-checks.md says the same.
…ase gate release.yaml's functional gate recognised a manual dispatch on the release tag with `.event == "workflow_dispatch" and .head_branch == $tag`. A run's head_branch is a bare name, so a dispatch on a *branch* named exactly like the tag (which IS_PROD treats as a test-backend run) passed the filter, and its green legs could satisfy a production release. main.yaml gains a trivial `production-backend` job whose `if:` is the functional job's IS_PROD expression (repeated, since a job-level `if:` cannot read another job's env). The gate now reads each candidate run's jobs through a jq `jobs_filter` that yields the run's successful job names only when `production-backend` is among them; a run without it is logged as a warning and not counted. Checking the real ref inside the run is robust where a "does refs/heads/$TAG exist now" lookup is not: a branch deleted after its dispatch would slip past that. Unit tests: the job's condition matches IS_PROD for every event/ref case, including a dispatch on a branch named `v0.3.1`; the jobs filter, run on paginated fixture JSON, drops a run whose production-backend job was skipped and keeps one where it succeeded on a later page; the gate script still wires the filter in; the required-checks doc names the new job, which is listed as not to be required (it is skipped on every PR).
Six legs have `timeout: 45`, which equalled the functional job's `timeout-minutes: 45`. When a leg hung, the job-level kill landed first, so the leg showed as cancelled and its teardown never ran -- the per-leg timeout could never do its job. Raise the job timeout to 55, above the longest leg timeout, with the reason beside it, and add a unit test in test_ci_matrix_coverage.py that fails if the job timeout is not above every leg's.
- AGENTS.md pointed at tests/mock_responses/, which ENG-3691 deleted, and said every leg has a 30-minute timeout; legs declare 30 or 45 minutes. - main.yaml's concurrency comment (and the matching unit-test docstring) said the matrix is 14 legs; it is one leg per functional test file, so say that instead of a count that rots. - docs/ci/required-checks.md left `benchmark` out of the deleted v1 legs. - release.yaml (and a unit-test message) justified `actions: read` by the repository being private; it is public. The scope is needed because the workflow's deny-all default grants nothing.
docs/ci/secret-scanning.md is published in a public repository, and it listed each removed credential's file and line, the backend it worked against, a partial fingerprint of each key and the path of a third token, alongside the statement that none had been rotated. That is a map for anyone looking for live credentials in history. The doc now says only what a reader of this repository needs: committed credentials were removed in ENG-3686, they remain in history and must be rotated, rotation is urgent and independent of any PR, and the specifics are tracked privately in the ENG-3686 ticket. The scanner design, the repo rules, the gitleaks-vs-detect-secrets rationale and the allowlist policy are unchanged; the open items are kept, without identifiers. The same details are removed from the test_secret_scanning.py module docstring, the .gitleaks.toml header comments, the test_eval.py docstring (which tied a deleted script to its key and backend) and a test_agent_lifecycle.py docstring that named the provider of a leaked MCP URL.
.gitleaks.toml exempted, by exact value, the recorded ECR password in tests/mock_responses/login_response.json. ENG-3691 deleted that file, so the entry matches nothing, and the allowlist policy is to remove a dead exemption: it can only hide something later. gitleaks dir . with the pinned config still reports no leaks; with the value allowlists removed it reports only the documentation placeholders and the hand-written fake keys in unit tests, which is what the updated docs/ci/secret-scanning.md describes in place of the old "8 findings".
resolve_action_source caught every exception from the pinned integration's list_actions() and treated it as "no actions", then fell back to a multi-action tool. A broken listing endpoint therefore went unnoticed: the snake-case round-trip tests passed on the fallback tool while the integration path they are meant to cover was down. Only an empty listing falls back now; an exception propagates and fails the test with the real error. The docstring says why.
test_second_page_does_not_repeat_the_first compared pages 1 and 2 only, so it could no longer see the bug it was written around: in the first live CI run pages 0 and 1 returned the same model. The SDK documents page_number as 0-based, and a backend that counts from 1 and clamps 0 up to 1 would alias exactly those two pages. Under the stable CREATED_AT sort the test now also fetches page 0 and asserts it is full and shares nothing with page 1, with a message naming the 1-indexing suspicion. The 1-vs-2 check stays first, so a failure tells the two problems apart.
test_trigger.py created an enabled trigger every two hours (plus daily, weekly and monthly ones) and relied on teardown to delete them. PR runs use cancel-in-progress, so a newer push can cancel a leg before teardown runs, leaving an active recurring trigger that keeps running its agent on the shared test backend. The recurring triggers are now created with enabled=False. What those tests check -- the schedule_type mapping and notifications -- does not depend on the trigger being active, and enable/disable is still covered by test_enable_disable_via_save on a far-future one-off trigger. The two-hourly test also asserts the fetched trigger is stored disabled, so a backend that ignored the flag on create would show up rather than quietly leak an active schedule.
Tuples, numbers and bools went out unencoded and still drew the 400; dicts were ASCII-escaped while Pydantic instances kept UTF-8. The encoder now lives in aixplain/v2/_expected_output.py so Agent no longer imports it lazily from session.py, and the read-back change is listed under Breaking.
One 503, timeout or 429 on the first read of an asset was cached for the whole session, so every later test in the worker, @flaky reruns included, replayed it. Successes and definite failures (a 4xx other than 408/409/425/429, an explicit retryable=False, an undeserializable response, a client that cannot be built) are still cached; anything else is asked again on the next read.
…Slack leg - The path filter now covers every module directly under tests/ and pytest.ini, so a PR touching only ci_guards.py, cleanup_guards.py or the marker registry no longer skips the matrix and passes functional-result green. - An unrelated label no longer cancels a running matrix: its run queues in the PR's group instead. A push or the functional-tests label still cancels. - agent-lifecycle no longer holds SLACK_TOKEN: its only Slack test is skipped (BUG-1098). The guard now ignores unconditionally skipped tests, so it fails when that skip is removed without restoring slack: true.
The separator between the credential name and the key could not contain a-f,
so TEAM_API_KEY_PROD="<key>", AIXPLAIN_API_KEY_DEV=<key>, SECRET_KEY = "<key>",
api_key_test: <key> and {"x-aixplain-key": "<key>"} all went unreported. The
rule now needs only a non-hex character right before the key, and aixplain-key
is a credential name. Checked with gitleaks v8.28.0: all five shapes are caught,
a bare sha256 digest is not, and the tree scan stays clean.
…ing listing errors multi_action_tool re-ran the bounded search (up to 5 Tool.search pages and 25 list_actions probes) for each of its consumers; the id is now resolved once per module and each test gets a fresh Tool.get. _action_count no longer turns every exception into 0: a pinned tool's listing error propagates, and a search candidate is passed over only when the backend refuses that tool with a 4xx.
- test_deprecated_files_alias_uploads_and_reads reads a .txt file, so it runs on the default LLM instead of VISION_LLM, whose outages turned the leg red. - test_agent_expected_output_run_returns_json uses a flat schema: the engine does not resolve $ref/$defs (the strict xfail in test_team_agent.py), so the nested People schema could fail the leg on all three flaky attempts.
…ers.py _unique, _name, _unique_name and _agent_ids were redefined in five test files. They are now unique_name (uuid-suffixed, safe under xdist and reruns) and agent_ids (id strings, dicts or Agent objects) in the shared helpers module.
AGENTS.md requires it on new source files; _assets.py, _helpers.py and the ten test modules this PR adds had none. Appended to each module docstring, as in aixplain/v2/file.py.
…ecrets TEST_COMPOSIO_INTEGRATION_ID / TEST_CONNECTION_ID (and their _PROD variants) were never created, so the trigger leg was red on every run and release.yaml could not release. Neither is a credential: - the integration is fetched by its marketplace path, composio/gmail, which resolves on every backend; - the connection comes from resolve_trigger_connection, a bounded search for a connected tool that lists event-trigger types (Gmail-backed tools first), pinnable with AIXPLAIN_TEST_TRIGGER_CONNECTION_ID. The workflow no longer reads the four secrets, and a unit guard keeps them out.
aix-ahmet
force-pushed
the
ENG-3683-3692-ci-and-functional-suite-overhaul
branch
from
October 9, 2026 12:45
fc6abf8 to
2eed62c
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
2 participants
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.
Summary
This PR combines ten open PRs into one branch based on
development, then fixes the test bugs that the first live functional CI run exposed.Each original PR is one commit, in dependency order. Its conflicts with the earlier ones are already resolved, so the commits can be reviewed one at a time:
tests/functional/_assets.py, one named registry of backend asset ids per environment. Theassetsfixture resolves each id lazily. A hygiene guard fails on any bare id anywhere else.tests/functional/_helpers.py.main.yamlon PRs, with afunctional-scopegate and a single requiredfunctional-resultcheck. Adds a nightly run on the test backend and a release gate that only counts production runs. Production secrets are expanded only for production runs.tests/. Tidies the pytest config. Deletes the leaked notebook.Fixes from the first live CI run
Until now, no CI job had ever run the new functional tests. The run on #1162 surfaced these real bugs:
expectedOutputsent as an object. Both run paths,Agent.build_run_payloadandExecutionConfig.to_api_dict, sent a Pydantic class, instance or dict as an object. The backend rejects that with400 executionParams.expectedOutput must be a string. Both paths now send a string, the same onesave()persists, and unit tests cover each case.resolve_multi_action_toolfound nothing. It filtered onactions_available, which the backend leaves empty, so every tool was ruled out. It now checks each tool's actual actions, prefers Slack-backed tools, and falls back to any tool that has two or more actions. The snake-case round-trip tests use the same lookup.err.agent_is_in_usecode.max_inspectorsis not persisted by the backend, so the test no longer asserts it.max-parallel: 4on the functional matrix. The test tenant allows 20 concurrent agent runs, and several PRs running their full matrices at once hiterr.agent_concurrency_limit_reached.Review follow-ups
These are the changes made after the review comment on this PR:
xfails, each naming the gap; the payload assertions still run first. The gaps:variablesandpromptare not applied;expected_outputschemas lose their types;developmentskip into an assert. They nowxfailwith re-pin instructions when the pinned model has noconnection_typemetadata. An earlier version of this description wrongly called that failure pre-existing; this PR caused it.issueleg is skipped on production runs.expectedOutputencoder is now shared by save, run and session runs. A Pydantic instance or dict is now saved as a JSON string. See CHANGELOG.SLACK_TOKENgoes only to the legs that use it, andHF_TOKENis no longer exported;Not fixed here (environment or platform)
TEST_COMPOSIO_INTEGRATION_IDandTEST_CONNECTION_ID, each with a_PRODvariant. Thetriggerleg fails without them.SLACK_NIGHTLY_WEBHOOK_URLis needed for the nightly alert.VISION_LLMis intermittently unavailable on the test backend.variables/prompt;$ref/$defsinexpectedOutputschemas;inspectorTargets/maxInspectors;team.agentsare dropped on save (strictxfail; still needs a ticket).Notes for reviewers