Repository navigation
Eng 3688 v2 agent lifecycle functional tests - #1156
Closed
elsheikhams99 wants to merge 44 commits into
Closed
elsheikhams99 wants to merge 44 commits into
elsheikhams99 wants to merge 44 commits into
Conversation
`main.yaml` triggered only on push to `main`/`test` plus `workflow_dispatch`, so
the seven-leg functional matrix never ran before a merge: the earliest a
functional regression could surface was a push to `test`, after review. Several
env vars the suites read were also never set, which fails nothing -- the tests
skip and the leg reports green.
main.yaml
- `pull_request` trigger. `unit-coverage` and `package-integrity` are
credential-free and run on every PR. A new `functional-scope` job decides
whether the backend-hitting legs run: always on push/schedule/dispatch, never
on a fork PR (no secrets), and on a PR only when it touches `aixplain/**` or
`tests/functional/**` or carries the `functional-tests` label. `labeled` is in
`types` so the label starts a run by itself.
- The decision is an `if:`, not `on.pull_request.paths`: a path filter
suppresses the check itself, so a required context would never report on a
docs-only PR -- a merge deadlock rather than a merge gate. A job skipped by
`if:` still reports, and GitHub counts skipped as satisfying a required check.
- Nightly `schedule` against the test backend, with a `nightly-report` job that
posts to Slack when the run is not green. A backend change under an untouched
SDK is invisible to a path filter by construction.
- Env gaps in the functional legs: export `AIXPLAIN_API_KEY` (same value as
`TEAM_API_KEY`; config.py raises when the two differ), and
`TEST_COMPOSIO_INTEGRATION_ID` / `TEST_CONNECTION_ID`, without which
test_trigger.py's event-trigger classes skipped every CI run.
- `MODELS_RUN_URL` is built from a per-leg `models-api` matrix key, so the v2
leg gets a v2 URL directly instead of its conftest rewriting a v1 one.
- A `v*` tag now counts as production alongside `main`, so a run dispatched on a
release tag validates against the backend the wheel is published for.
- Secrets reach the env step through `env:` rather than `${{ }}` interpolation
into the script body.
release.yaml
- New `functional-gate` job, required by `build`: every `setup-and-test (...)`
leg must have succeeded on the released commit. Keys on `github.sha` (a tag
can be moved), reads the leg list from main.yaml at that commit, and unions
all completed runs so a re-run of failed legs counts. `contents: read` and
`actions: read` only.
pre-commit.yaml
- Drop `TEAM_API_KEY`. The `pytest-check` hook runs `tests/unit`, which is
credential-free by design, so the key was exposed to third-party hook code
and never used.
tests/functional/v2/conftest.py
- Default to the test backend instead of dev-platform-api, and derive the model
URL from whatever backend is in effect. The hardcoded ids in these tests exist
on one backend at a time, so a developer with no BACKEND_URL was validating
them against a different environment than the one that gates a merge.
- The conftest no longer rewrites the execution path it is handed. An explicit
MODELS_RUN_URL contributes its host; the path is always the v2 execution
endpoint, built from one constant. Nothing here knows about any other API
version, so the v1 removal does not have to come back through this file.
tests/unit/test_ci_pr_and_release_gates.py
- Static guards for all of the above, in the existing credential-free idiom:
each fix is one deleted line from being undone and none of them fails visibly
when removed.
docs/ci/required-checks.md
- Updated tiers and trigger tables, the per-event functional-scope matrix, the
release gate and its recovery path, and a table of the secrets the legs read
(the two `TEST_…_ID` rows being the ones whose absence is silent). Resolves
the "how should main.yaml run on PRs?" open question.
Manual follow-ups, unchanged by this commit: set
`TEST_COMPOSIO_INTEGRATION_ID(_PROD)`, `TEST_CONNECTION_ID(_PROD)` and
`SLACK_NIGHTLY_WEBHOOK_URL` in repository secrets, and flip
`required_status_checks.contexts`.
…ed test
The ENG-3544 guard fails a leg only when zero test bodies ran. tests/functional/v2
cleared that floor while executing 12 of its 195 collected tests: one
module-scoped fixture skip in test_actions_inputs.py took ~40 tests out on its
own, and 36 more inline skips, each conditional on backend state, took out the
rest. Every one of them turned a missing or broken platform fixture into a green
run.
Guard
- tests/ci_guards.py: ExecutionLedger now records collected vs executed per test
module and the id/reason of every skip. New AIXPLAIN_MIN_EXECUTED_RATIO
(default 0.8, shares the existing opt-in) with should_fail_for_low_execution_ratio
and a summary that prints the per-module table and the skip reasons -- the
reason is the actionable part, since it names the fixture that went missing.
An unparsable floor raises rather than falling back, so a typo cannot leave the
workflow claiming a threshold nobody enforces.
- tests/conftest.py: tallies the final item list in pytest_collection_finish, so
-k/-m deselection narrows the denominator too; enforces both floors in
pytest_sessionfinish, zero-executed first.
- Both workflows pin the floor at 0.8 explicitly.
Skips
tests/functional/asset_ids.py centralises the pinned backend ids and carries the
failure helpers (missing_fixture, require_env, resolve_multi_action_tool). Then:
- test_actions_inputs.py: the tool fixture resolves the pinned tool and fails
when it is absent; a new action_with_inputs fixture replaces ten per-test
searches that each skipped on their own.
- test_model.py: streaming and connection_type assertions run against the pinned
models instead of discovering one; the catalogue sweep in
test_model_validation_edge_cases is replaced by the pinned model.
- test_tool.py, test_snake_case_e2e.py, inspector_functional_test.py: sweeps and
empty-result skips become assertions against pinned assets.
- test_trigger.py: TEST_COMPOSIO_INTEGRATION_ID / TEST_CONNECTION_ID fail when
unset and are now exported by the workflow.
- test_arabic_agent.py: the empty-output skip becomes three attempts and then a
hard failure.
grep -c "pytest.skip(" tests/functional/v2/*.py drops from 37 to the 2
credential gates in conftest.py.
The ticket also named tests/functional/model/get_model_by_name_test.py, which
exercises the v1 factories. Left alone: v1 is being deprecated under its own PR,
so the skip there stays rather than creating a conflict for a file on its way
out. The ratio floor still covers that leg -- one conditional skip against its
collected count is well inside 0.8.
Tests: tests/unit/test_ci_guards.py covers the ratio predicate, the env parsing,
the per-module/skip bookkeeping and the message; test_ci_guard_wiring.py drives
the floor end-to-end over a throwaway tree, including the deselection and
misconfiguration paths; test_ci_matrix_coverage.py asserts the workflow pins it.
Two live-shaped credentials sat in tracked root-level scratch files. Both were outside `testpaths`, so no suite ever executed or reviewed them: - test_agent_eval.py passed a 64-hex platform key as a literal `api_key=` argument pointed at dev-platform-api. Deleted rather than reworked to read the environment: it was an ad-hoc driver for the v2 evaluator, and that behaviour is already covered by tests/unit/v2/test_agent_evaluator.py. - pipeline_test2.ipynb set TEAM_API_KEY to a second 64-hex key and committed the executed cell output, which replayed that key a further 16 times inside request logs, webhook JWTs and pipeline payloads. Redacting the assignment alone would not have been enough. It exercised the deprecated v1 PipelineFactory, so it is deleted outright. Gitleaks now runs in two places, because one alone is not enough: the pre-commit hook scans the staged diff, which is the last point a credential can be kept out of history, but it is `--staged`-only and therefore a no-op under `pre-commit run --all-files`. A separate CI step scans the whole tree so the job cannot report a clean repository while checking none of it. A unit test asserts the two pinned versions do not drift apart. detect-secrets was evaluated first and reported ~350 findings, almost all of them 24-hex MongoDB asset IDs matched by its entropy plugin. Gitleaks matches credential shapes instead and reported 8, all obvious placeholders, each now allowlisted by its literal value. The SSRF fixture addresses in the v2 URL-guard tests are exempted with `condition = "AND"`, so the exemption covers those IPs in those files and a real secret committed alongside them is still reported. tests/unit/test_secret_scanning.py re-states the specific defect and fails if the hook, the CI step or `useDefault = true` is dropped -- a scanner that has been quietly unhooked reports the same "no leaks found" as a clean tree. It is static and credential-free, so it runs in the unit-coverage job. Still open, both needing platform access and a team decision, documented in docs/ci/secret-scanning.md: revoking the two keys, and whether to rewrite history. Rotation is what ends the exposure; a rewrite only reduces the residue and cannot reach forks or existing clones. The Zapier MCP share URL in tests/functional/model/run_connect_model_test.py is deliberately untouched -- it is a v1 test covered by the separate deprecation PR, the endpoint is dead, and gitleaks does not flag it, so it will not block CI in the meantime.
Every v2 functional test that needed a platform asset carried the raw ObjectId inline: 24 hex characters, no name, no owner, no override. The default LLM appeared in four files, the Slack integration in seven, and Firecrawl/Tavily/Google Search were copy-pasted byte-for-byte between the agent and tool suites. Those literals are not portable. CI resolves them against test-platform-api, a push to main resolves them against production, and a laptop with no BACKEND_URL resolves them against dev-platform-api: three separate id spaces. A retired asset therefore failed one leg at a time, and each failure read as an unrelated backend error from somewhere deep inside a test body. tests/functional/_assets.py is now the single source: one `AssetIds` dataclass per environment (dev/test/prod, selected by BACKEND_URL), a named field per asset, and an AIXPLAIN_TEST_<NAME> override for a single run -- which is also how a backend this module has never heard of gets a working id without a commit. Retiring an id is one line. A session fixture in tests/functional/v2/conftest.py resolves every id before any test runs and reports all the gaps at once, naming the asset, the id, the call that failed and the override that unblocks the run. The v2 conftest now imports its default backend from the same module rather than repeating the literal, because the id space selected and the backend the client talks to drifting apart is the failure this ticket is about. test_arabic_agent.py parametrises on the asset *name* rather than the id, so collection stays backend-independent. Its `gpt-4o` label was the correct half of the pair the ticket flags: the id was onboarded 2024-05-16 per its ObjectId timestamp, four days after GPT-4o shipped and five weeks before Claude 3.5 Sonnet did, and skills/aixplain-builder gives it as the id form of openai/gpt-4o. The "Anthropic Claude 3.5 Sonnet v1" comment the v1 suite attached to it was the stale one; the dating is recorded next to the id. Guards: tests/unit/test_functional_hygiene.py fails on any bare 24-hex literal under tests/functional/ -- in a comment as readily as in code, since one of the removed ids had been documenting a tool that no longer existed -- plus two checks that stop it passing vacuously. The module's own logic (host matching, overrides, unknown backends) is covered in tests/unit/test_functional_assets.py, following the cleanup_guards/test_functional_cleanup split. Scope is the v2 suite. The v1 legs still carry their own literals and are deliberately untouched: PROD-2918 deletes all eight of those directories, so rewriting them here would buy a delete/modify conflict and nothing else. The exemption is a named tuple in the guard rather than a detector quietly pointed at tests/functional/v2, and it matches nothing once PROD-2918 lands.
test_no_plaintext_key_literals walked the working tree, but the defect it guards is a credential reaching history. The tree is full of ignored files that hold real keys by design -- .cache/model.json, which the SDK writes an api_key into on every run, and the .env that tests/conftest.py loads through load_dotenv(override=True) -- so the guard failed over files git will never accept. It was not only a developer-machine problem: the pre-commit job hit the same .cache/model.json line, because the cache is written during the very pytest session that then scans it. _tracked_files() lists the index with `git ls-files` and returns None when the root is not a checkout, so the synthetic tmp_path trees keep their filesystem walk. The check is on root/.git itself, so a temporary directory cannot inherit an enclosing repository's index. test_detector_ignores_an_untracked_key plants a key in an ignored .cache/model.json and expects no finding, then commits the same content and expects the finding, so narrowing the scope cannot pass by having quietly turned the detector off.
…e-committed-secrets-add-secret-scanner
…-and-nightly-functional-gates
…xecuted-ratio-guard
…d-functional-asset-ids
…nest Addresses the review findings on this branch. The ENG-3544 blanket-skip guard had gone blind to the v2 suite. Its detector treated any function whose AST dump contained "environ" as guarded, and the autouse verify_assets_resolve fixture contained that text only by accident (its docstring, an `environment` variable, `environment_for`). An unconditional pytest.skip there still passed, and with the API-key read moved behind _api_key(), the real-conftest check only matched slack_token. Now: - the `client` and `assets` fixtures read TEAM_API_KEY/AIXPLAIN_API_KEY with an inline os.getenv, where the detector can see it; - the detector needs a real getenv/environ attribute or name node, not the substring; - test_ci_matrix_coverage.py has a regression case: an autouse fixture that skips unconditionally while naming "the environment" is flagged; - the real-conftest check requires every skip in the v2 conftest to be guarded and one guard to read the API key. A new test deletes the env reads from the real conftest and expects every skip to be flagged. Verified by mutation in a scratch copy: an unconditional skip in client or assets, an autouse blanket skip, or the guard behind _api_key() each fail the suite. The session-wide, autouse verify_assets_resolve is replaced by per-name resolution. The `assets` fixture returns LazyAssets: the first read of `assets.X` resolves X through SPECS (client.<resource>.get(id)) and caches the outcome, success or failure, so @flaky reruns don't hit the backend again. A missing asset fails only the tests that read it, with the same message as before (asset, id, backend, call, override). A leg that reads no asset makes no resolution call and builds no client. The fixture checks credentials before it computes the id space, so no key plus an unrecognised BACKEND_URL skips instead of erroring. Host matching in tests/functional/_assets.py is now an exact lookup of urlparse(url).hostname, lower-cased. Before, a substring test read staging-platform-api as prod and a query string naming test-platform-api as test. AIXPLAIN_TEST_ENV is applied only to an unrecognised host. If it contradicts a recognised one, that raises ConflictingEnvironmentError. Override and pin values are stripped, and a whitespace-only value counts as unset. Unit tests cover the new host cases, the pin rules, whitespace overrides, and LazyAssets with a fake client. A static check makes sure no autouse fixture in the v2 conftest comes back. The module docstring and AGENTS.md now describe lazy resolution and no longer treat PROD-2918 as future work. This also corrects counts in 073af9d's message: Slack was inlined in six files, not seven; two ids (Firecrawl, Tavily) were shared by the agent and tool suites, not three; and the gpt-4o dating and "eight directories" no longer apply.
Addresses the review findings on test_functional_hygiene.py: - OBJECT_ID_PATTERN is case-insensitive, keeping its lookarounds, so an upper-case ObjectId is caught too. - The scan asserts it found at least one file, so pointing it at the wrong directory can't pass vacuously. - Relative paths are compared in POSIX form, so the _assets.py exemption also holds on Windows. - The V1_LEGS/_is_v1_leg exemption is removed. v1 is gone (PROD-2918), and the generic directory names it listed would have exempted a new tests/functional/model/ file with a bare id. - A new test checks that every asset name in test_arabic_agent.py's MODELS is a field of AssetIds. It parses the module rather than importing it, so a typo fails here instead of at run time on a backend. - Docstrings are added to the test functions this PR introduced.
Addresses review findings on the execution-ratio guard. - Count executed tests as a set of nodeids. pytest-rerunfailures reruns a test whose teardown failed, so a body that already passed reached `call` twice and was counted twice, inflating the ratio. - Print the ratio and the floor with one decimal (`:.1%`); 199 of 250 used to read "(80%), below the 80% floor". - Replace should_fail_for_low_execution_ratio with breached_execution_floor, which returns the floor it parsed, so pytest_sessionfinish reads AIXPLAIN_MIN_EXECUTED_RATIO once. low_execution_ratio_message takes the xdist denominator as an argument instead of the conftest mutating the ledger to pass it. - Drop dead code: SkippedTest becomes a NamedTuple (its hand-written __eq__ was unused) and the test-only ExecutionLedger.executed_ratio() is gone. - Add wiring tests that run the floor end-to-end under `-n 2`, which is how the agent and arabic-agent legs run. They fail if the `or session.testscollected` fallback is removed from tests/conftest.py, or if the per-module table loses its executed+skipped fallback; both mutations previously left every guard test passing. - Strip GIT_* from the wiring tests' subprocess environment. - Reword comments that cited per-run counts which no longer hold against the current base, and the claim that 0.8 leaves room for credential-gated skips: the per-file legs have no conditional skips of their own.
Addresses review findings on the functional fixtures. - resolve_multi_action_tool searches up to 5 pages of 20 tools instead of only the first page, and requires the tool to list two or more actions, as its docstring promised. That holds for an AIXPLAIN_TEST_MULTI_ACTION_TOOL_ID override too. A connected tool is tenant-specific, so no id is pinned; the failure message names the Slack integration filter, what was searched, and the override. It still fails rather than skips. - Correct the test_actions_inputs.py / test_tool.py fixture docstrings, which claimed the tool was pinned in asset_ids.py. - test_arabic_agent.py: keep one retry layer for empty model output. The flaky markers now pass `rerun_except` (pytest-rerunfailures >= 16, as pinned) matching the _run_until_output exhaustion failure, so an output that is empty on every attempt is no longer rerun, which had doubled the billed runs per item. The marker still reruns timeouts. The failure message says how many consecutive runs were made and that it is not rerun. - Move the remaining literal ids into tests/functional/asset_ids.py by value: the Arabic suite's GPT-5.4 id (TEXT_MODEL_ID), the Slack integration in test_agent.py and test_integration.py (SLACK_INTEGRATION_ID), and the Tavily tool in test_agent.py (TAVILY_TOOL_ID). - Fix the asset_ids.py module docstring, which cited skip counts that no longer hold. - ruff format the touched files that were formatted on development.
Addresses review findings on the trigger fixtures and CI docs.
- main.yaml exports TEST_COMPOSIO_INTEGRATION_ID / TEST_CONNECTION_ID from
the *_PROD secrets on `main` and the plain secrets otherwise. Both name
backend-resident objects, so one value cannot serve the test and
production backends. The values reach the script through the step's
`env:` rather than `${{ }}` interpolation, with the same variable names
ENG-3683 uses so the two branches converge.
- docs/ci/required-checks.md documents the secrets the functional legs read,
including both trigger pairs, which now fail the trigger leg when unset.
- Reword the workflow comment on AIXPLAIN_MIN_EXECUTED_RATIO, which cited
counts that no longer hold and claimed the floor leaves room for
credential-gated skips. A missing credential skips a whole leg, which the
zero-executed floor covers.
- test_ci_matrix_coverage.py: a rejected floor is reported as "CI integrity
guard misconfigured", not an INTERNALERROR.
…sts bite
Addresses the review findings on the secret-scanning change.
- The guard's git calls (`_tracked_files` and the throwaway repository in
the untracked-key test) now run with every GIT_* variable stripped. The
pytest-check hook runs the unit suite from inside `git commit`, which
exports GIT_INDEX_FILE (and GIT_DIR in a linked worktree), so the test
staged files into the developer's real index and, from a worktree, could
rewrite its config to `core.bare = true`. A regression test points
GIT_DIR/GIT_INDEX_FILE at a bystander repository and asserts its index
and config are untouched.
- .gitleaks.toml adds two repo rules. The curated rules miss a 64-hex key
in `os.environ["TEAM_API_KEY"] = ...`, in an `Authorization: Token ...`
header and in `Aixplain("...")`; the deleted notebook was reported only
because of its saved cell output. Both deleted files are reported by the
new rules, and unit tests check the rules against those shapes.
- The wiring tests parse the config instead of matching substrings. They
now fail if `useDefault` is commented out or false, if a repo rule is
removed, if the hook gets a `stages` key, or if the CI scan step gets an
`if`, `continue-on-error`, a `||` fallback or `--exit-code`, or loses
`--redact`. Each of those mutations was checked to fail a test.
- CI verifies the gitleaks tarball's sha256 before extracting it, and sets
`SKIP: gitleaks` on `pre-commit run --all-files`: the hook is staged-only
and would build Go from source to scan nothing.
- Allowlists: dropped the SSRF `condition = "AND"` block, which exempted no
finding but would hide a key on a line containing one of those IPs, and
three placeholder entries that matched nothing. The ECR login fixture is
exempted by its exact value instead of by path, and the planted fake key
in the unit test by its exact value. The tree still scans clean.
- Detector: skip rules match the path relative to the scanned root, so a
checkout under a directory named `docs` or `build` is still scanned; only
the generated docs/api-reference is skipped. `.env` files are matched by
name, and non-UTF-8 files are read with replacement instead of skipped.
Addresses the review findings on docs/ci/secret-scanning.md. - The repository is public, so the removed keys have been readable since they were committed. Rotation is marked urgent and independent of this PR, the "unless the repository is about to be made public" condition is gone, and a full-history audit is added as an open item. - The Zapier token's file was deleted with v1 in #1136; the token remains only in history and falls under the rotation and history decision. - Only `Eval().evaluate` from the deleted evaluator script is covered by tests/unit/v2/test_agent_evaluator.py; the other paths it touched are not. - The notebook count is stated precisely (2 occurrences in cell sources, 22 in saved output), and the allowlist and rule descriptions match the new config.
Production backend only where it is meant to be: - The nightly ran every leg against production with TEAM_API_KEY_PROD: scheduled runs execute on the default branch, `main`, and IS_PROD keyed on the ref alone. IS_PROD is now job-level and allow-lists events (push or workflow_dispatch) as well as refs (refs/heads/main, refs/tags/v*). - The _PROD secrets expand through that same IS_PROD and are the empty string otherwise, so a PR job (which pip-installs the PR's unreviewed dependency changes) never holds the production key. - release.yaml's functional gate counted runs from any event, so a test-backend pass on the tagged SHA (push to test, PR, nightly) could satisfy a production release. It now counts only pushes to main and a workflow_dispatch on the released tag by name (not any ref starting with "v": `version_2` is a branch), still taking the union across attempts. The recovery text no longer implies a push to test helps, and says to wait and re-run when the main run is still in progress. Scope decision: - SIGPIPE: `printf | grep -q` under pipefail skipped the matrix on PRs whose file list outgrew the pipe buffer; replaced with a here-string. - pyproject.toml, tests/conftest.py and main.yaml now trigger the legs. - An empty head repo (deleted fork) is treated as a fork: fail closed. - New non-matrix `functional-result` job (needs scope + matrix, always()) is the one context to require. A job-level `if:` is evaluated before matrix expansion, so a skipped matrix reports a single `functional` context and requiring per-leg contexts would deadlock docs-only and fork PRs. nightly-report now also needs functional-scope and functional-result, so a failed scope job alerts. - Workflow-level concurrency: one group per PR with cancel-in-progress; every non-PR run gets its own group (run id) so a pending push run on main is never dropped. Irrelevant labels still go through the path decision rather than short-circuiting to skip. Cleanup: - Dropped the `models-api` matrix key: the v2 conftest keeps only the host of MODELS_RUN_URL, so it changed nothing. The workflow exports the v2 endpoint the conftest would build. - tests/functional/v2/conftest.py derives the models host only for `<env>-platform-api.aixplain.com` and otherwise raises asking for MODELS_RUN_URL; an explicit MODELS_RUN_URL without scheme/host raises. Tests: test_ci_pr_and_release_gates.py now executes the real scope script (bash + git against throwaway repos, including a >64 KB diff), the functional-result verdict, the release gate's jq run filter (fixture JSON), and evaluates the IS_PROD, _PROD-secret and concurrency expressions, instead of checking substrings. test_ci_permissions.py asserts the doc lists every leg and the functional-result aggregate. docs/ci/required-checks.md updated to match.
Add functional tests for utility, file, debugger, eval, resource, issue, and agent-progress v2 resources, extend skill coverage with list/get/ download/as_tool, and add unit tests for code_utils and agent_progress. Register the new suites in the CI matrix and required-checks doc.
…NG-3689) Re-cover the v1 tests/functional/team_agent scenarios against the v2 Agent(..., agents=[...]) surface, all passing live on prod: - task definitions/dependencies, role llm overrides (persisted and in the run payload), run_response_generation, nested save chain, add/remove subagents, shared deployed agent, instructions, llm parameter preservation, expected output, draft update, end2end, bad-llm rejection, and the evolve run parameter (legacy evolve_async twin). - port the legacy build_team_agent unit test to tests/unit via from_dict. - claim a team-agent functional CI leg; unit test asserts role overrides reach modelParameters on the run payload.
…context (ENG-3689) test_ci_permissions.py requires docs/ci/required-checks.md to match the main.yaml matrix exactly.
…-ids' into ENG-3688-v2-agent-lifecycle-functional-tests # Conflicts: # AGENTS.md # tests/functional/v2/test_agent.py # tests/functional/v2/test_agent_llm_persistence.py # tests/functional/v2/test_arabic_agent.py # tests/functional/v2/test_snake_case_e2e.py # tests/functional/v2/test_tool.py
Restore the v1 agent-lifecycle scenarios deleted in PROD-2918 as tests/functional/v2/test_agent_lifecycle.py, using the shared asset ids from ENG-3685: - draft/onboard transitions (as_draft, implicit draft save, onboarded mutation guard, end-to-end draft -> onboard -> run) - subcomponent saving and the unsaved-subagent guard - tool kinds: custom code, Python interpreter, add/remove, model parameter override, llm parameter preservation, Slack action tool - update/get round-trips (name, inspector fields, JSON expected output, file references) - failure modes (unknown llm, delete-in-use) Add offline unit coverage for the draft state machine in tests/unit/v2/test_agent_draft_lifecycle.py, add a TRANSLATION_MODEL asset, move the Slack agent test out of test_agent.py, and register the new functional leg (main.yaml + required-checks.md).
- pytest.ini: register functional/slow/destructive/flaky markers, add --strict-markers, error on AixplainV1DeprecationWarning, set asyncio_mode - _compat: keep AixplainV1DeprecationWarning importable for that filter - extend tests/unit/v2/test_no_v1_imports.py to tests/functional/v2 - ruff: lint and format tests/, exempt test docstrings from pydocstyle - move flaky reasons to comments (pytest-rerunfailures ignores reason) - stub ipinfo.io in unit tests, drop stale commented Tool assertions - move pipeline_test2.ipynb to notebooks/, ignore local test/ scratch
The functional suite is v2-only after PROD-2918, so the story's v1 skip sites are already gone; what remains is the hygiene pass plus the two backend-bug skips that survive in v2. - pytest.ini: enable --strict-markers and register the `flaky` marker, so the functional conftest's rerun marker is not unknown when pytest-rerunfailures is absent. - tests/unit/test_skip_hygiene.py: a rule (with a self-test) that every @pytest.mark.skip/skipif reason under tests/ cites an ENG-/PROD-/BUG- key. The detector is a function of a directory so it can be proven. - Backend-bug skips now name their ticket and re-test cadence: Slack send-message payload (BUG-1098) and session reactions (BUG-1099). Both re-verified against dev on 2026-10-02; the sessions service still stores role='user', so both stay skipped. - Delete the 13 unreferenced JSON fixtures left behind by the v1 removal (the finetune fixtures, image-repo-tags, create-asset, and the tests/test_requests/ and tests/mock_responses/ trees). - PARKED_TARGETS and DIRS_WITHOUT_TESTS are already empty; the v2-only functional tree maps one-to-one onto CI legs. Bases on ENG-3685 (shared functional asset ids), which the story's "centralised asset module" refers to.
Addresses review findings on PR #1157: - Delete notebooks/pipeline_test2.ipynb instead of moving it; it held a hard-coded credential (ENG-3686 deletes it at its old path, so the two now agree instead of producing a rename/delete conflict). - pytest.ini: drop the filterwarnings entry for AixplainV1DeprecationWarning (nothing emits it, and naming the category imported all of aixplain before any conftest ran), drop asyncio_mode = auto (pytest-asyncio is not a dependency, so it only produced an unknown-config warning), and drop the unused functional, slow and destructive markers. flaky stays. - Keep the restored AixplainV1DeprecationWarning class so downstream filters keep resolving; reword its docstring and add a CHANGELOG note. - .gitignore: anchor the run-log scratch entry to /test/. - Lint scope: ruff selects only D and tests are exempt from D, so tests are format-checked only. Say so in ruff.toml, .pre-commit-config.yaml and AGENTS.md, and keep the ruff lint hook on aixplain/v2/. Enabling pyflakes for tests was evaluated (35 findings across 15 files) and left for a follow-up.
Addresses review findings on the v2 agent lifecycle port:
- Catch ResourceError (what @with_hooks raises) for the refused delete of an
in-use subagent and for the unknown-llm save; assert "cannot be deleted"
without the loose "team" fallback.
- Verify persisted state instead of local state: the implicit draft save is
re-fetched (instructions + DRAFT) and the run must answer "pong"; the llm
test now saves and compares the fetched model id and temperature; the
stored expected_output is compared to People.model_json_schema().
- The draft-update tests (rename, add/remove tool) save with as_draft=True
and assert the fetched status stays DRAFT.
- Drop the marketplace interpreter test (Tool(code=...) already exercises
the Python Sandbox) and document it with the other unported v1 scenarios
({{var}} substitution, two utility tools, MCP deploy) in the module
docstring.
- Register resources with resource_tracker only once they have an id,
including the unknown-llm agent if the backend ever accepts it.
- Send a Portuguese query to the translation tool pinned to
sourcelanguage="pt" and assert a non-empty output.
- Mark the LLM-output-dependent tests flaky(reruns=2).
- Inline BUG-1098 in the Slack skip reason (no f-string).
- Add the create_sql_tool row to MIGRATION.md's AgentFactory tool table.
- Unit: the implicit draft save is checked through the real PUT payload
instead of a mocked save().
ENG-3685 introduced tests/functional/_assets.py as the single home for the functional suite's backend asset ids, read through the `assets` fixture and overridable per run with AIXPLAIN_TEST_<NAME>. This branch had grown its own registry in tests/functional/asset_ids.py, with a second set of override names (AIXPLAIN_TEST_TEXT_MODEL_ID, ..._SLACK_INTEGRATION_ID, ...) for the same ids. Two registries for one id space is the drift ENG-3685 exists to prevent, and its hygiene test rejects the bare ids that module still carried. So the id constants are gone. Every caller now reads assets.DEFAULT_LLM, STREAMING_TOOL_CALL_MODEL, SYNC_ONLY_MODEL, ASYNC_ONLY_MODEL, SLACK_INTEGRATION or TAVILY from the fixture, and the re-pin hints in test_model.py name ENG-3685's override variables. What remains is the failure helpers -- missing_fixture, require_env and resolve_multi_action_tool -- so the module is renamed to tests/functional/_helpers.py, which no longer claims to hold ids. resolve_multi_action_tool takes the Slack integration id as a parameter (callers pass assets.SLACK_INTEGRATION) instead of reading a module constant. The multi-action tool itself stays an AIXPLAIN_TEST_MULTI_ACTION_TOOL_ID override with no default, now read when the helper runs: a connection is tenant-specific, so it has no portable id to register.
…on statically
Addresses review findings on the skip-reason guard and strict markers:
- test_skip_hygiene: read a positional skip reason; resolve f-strings, "+"
concatenation and module- and class-level string constants (an unresolvable
f-string part contributes nothing), so a reason like
f"... ({SLACK_BACKEND_BUG})" with SLACK_BACKEND_BUG = "BUG-1098" now passes.
- Cover the permanent-skip forms the guard missed: pytestmark (marker, list or
tuple, at module or class level), pytest.param(..., marks=...),
xfail(run=False), an unconditional pytest.skip() at the top level of a test
body or module, and module-level marker aliases (called or not).
- Require a ticket only for permanent skips: skip, xfail(run=False), and
skipif with no condition or a constant truthy one. Environmental
skipif(not os.getenv(...)) no longer has to cite a ticket. The module
docstring now states exactly what is enforced.
- Add a self-test per form, flagged and accepted; each new capability was
mutation-checked in a scratch copy (reverting it fails a self-test).
- pytest.ini: note that --strict-markers is a no-op on pytest 9.0.x and is
enforced by 8.x (CI's Python 3.9 leg) and 9.1+.
- New tests/unit/test_marker_registration.py: every pytest.mark.<name> and
add_marker/applymarker string under tests/ must be a built-in or registered
in pytest.ini, which holds regardless of the pytest version.
Addresses review findings on the v2 team-agent functional twins: - Expected output is set the supported way (output_format="json" plus a pydantic expected_output on the team before save) instead of an unknown run kwarg; the test parses the JSON output and validates its structure. - Team runs now inspect response.data.steps: the instructions, tasks, nested, shared-agent, expected-output and end-to-end tests assert the expected subagent took part, without matching LLM text. - Instructions-steering and tasks tests run the team again; the llm parameter test builds a real team and checks supervisor/planner parameters after fetch. - Drop the evolve test (backend behaviour and cost unverified) and say so in the module docstring; rename run_response_generation to *_accepted. - Save and track each subagent before saving and tracking the team; the nested-chain test registers whatever the recursive save created. - test_fail_non_existent_llm expects ResourceError with an escaped match and tracks the agent if it ever gets an id. - Add a strict xfail for the public team.agents edit path, which build_save_payload drops because it reads _original_agents. - Remove the locally computed modelParameters assertion (covered by the unit class), the unused DEFAULT_LLM constant and the stale "kept" skipped-tests claim; the draft test re-checks DRAFT after re-saving. - Move the builder unit test to tests/unit/v2/test_build_team_agent.py and make its dependency check exercise Task-object normalization. - Run the team-agent CI leg with -n 2, matching the agent leg.
…683-ci-pr-and-nightly-functional-gates
Addresses review findings on the functional coverage for untested v2 resources. - test_eval: Eval.evaluate records agent and metric errors as FAILED rows instead of raising, so every run is now checked row by row (status SUCCESS, no agent_run_failed, a non-null <prefix>__score, metric status not FAILED). The seeded benchmarking metric is replaced by a tracked numeric metric created per module with Agent.DEFAULT_LLM, so the module no longer depends on a dev-only asset and Experiment.diff gets numeric scores. The metric round-trip passes the LLM explicitly instead of llm_path=None. - test_issue: a report must return an issue_id; any 4xx now fails (no gating code is documented). Filed issues are tagged sdk-functional-test, prefixed "[sdk-functional-test]" and sent at SEV4, since the endpoint has no delete API. - test_utility: assert the returned values (5, 50 after the update, 42), assert the fetched tool is on the Python Sandbox integration, and stop claiming coverage of parse_code/_upload_code, which Tool never calls. - test_file: spy on the presigned PUT to assert the client timeout reaches it, expect APIError after delete, drop the unused import and helper, and rename the tests whose names implied a 5 MB threshold or a get(). - test_skill: search by the skill's name instead of scanning the first unsorted page; replace the as_tool() shape checks with one on a fetched skill's persisted description. - test_debugger / test_resource / test_agent_progress: replace always-true assertions (isinstance DebugResult, >= 0 metadata) with status and content checks, verify the custom prompt reaches the debugger query and that progress_format="none" reaches the tracker, and turn the backend-conditional skip into an assertion with an IndexError guard. - Offline validations move out of the live legs into unit tests: validate_file_for_upload (now also asserting a .wav is classified as audio, using a sparse 51 MB file), File() without a source, issue description/severity validation (400, no backend call), and the code syntax-error check. - tests/unit/v2/test_agent_progress.py: assert "✓ Step 1" rather than a bare "✓", which the completion summary also contains; deleting the per-step print now fails the test. - Fix the eval module docstring: the root test_agent_eval.py script is tracked, not untracked.
…ly-functional-gates
ENG-3685 (merged here through ENG-3684) made tests/functional/_assets.py the single home of the functional suite's default backend, and the v2 conftest imports DEFAULT_BACKEND_URL from it so the id space the asset registry selects and the backend the client talks to cannot drift apart. This branch had independently moved the conftest's own default from dev-platform-api to test-platform-api, because test is the backend CI's non-main runs (PRs, pushes to `test`, the nightly) use and test_the_v2_conftest_defaults_to_the_backend_ci_uses pins that. Keeping one constant means the move has to happen in _assets.py: - DEFAULT_BACKEND_URL is now https://test-platform-api.aixplain.com. An unset BACKEND_URL therefore selects the "test" id space. TEST is replace(DEV), so no asset id changes. - _assets.py's module docstring and the DEV/TEST comments no longer say dev is the default; the DEFAULT_BACKEND_URL comment carries the ENG-3683 rationale. - The conftest's _backend_url docstring carries the same rationale this branch's removed local constant had. - test_functional_assets.py pins the new default explicitly (test_an_unset_backend_selects_the_test_id_space); the hygiene test's docstring no longer says a laptop resolves ids against dev.
ENG-3686 (now merged in via ENG-3692) deleted the root-level test_agent_eval.py script for its committed key, so test_eval's module docstring no longer describes a file that exists. Say it was deleted and why, rather than pointing readers at a missing script. The rest of this PR's files already satisfy the guards now present: no bare 24-hex ids under tests/functional/ (the eval metric uses client.Agent.DEFAULT_LLM, and no other test needs a registry asset), no skips or flaky markers, cleanup through resource_tracker / module_resource_tracker, and pinned ruff 0.12.12 format is clean.
…am-agent-functional-tests
…lifecycle-functional-tests This branch's earlier merge of an old ENG-3685 commit (073af9d) is superseded. This merge takes ENG-3689's tree, which contains the current ENG-3685 and every earlier PR in the chain, and re-applies only this PR's own changes (git diff a25a68c 7fc308e) on top, so no history is rewritten. The agent-lifecycle leg joins ENG-3683's informational docs table, and the Slack skip reason follows ENG-3691's BUG-1098 wording.
test_team_agent.py (ENG-3689) already covers saving an agent with an unknown llm id and asserts the backend's error text ("model.Invalid id"). The copy here asserted different text ("not found"), so the two could not both pass against the same backend. Keep the one whose text has been checked against a live backend.
Collaborator
Author
|
Superseded by #1163, which combines this PR with the other ENG-3683–3692 PRs into one branch based on development (one commit per original PR, conflicts resolved), plus fixes from the first live functional CI run. |
aix-ahmet
pushed a commit
that referenced
this pull request
Oct 9, 2026
Combined from PR #1156, including its review fixes and the conflict resolutions against the PRs that come before it in this branch.
aix-ahmet
added a commit
that referenced
this pull request
Oct 9, 2026
…l (combined) (#1163) * ENG-3686: remove committed API keys and scan for secrets on every push Combined from PR #1139, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3685: name the functional suite's backend asset ids, once Combined from PR #1140, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3684: hold each CI leg to an execution ratio, not just one executed test Combined from PR #1138, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3683: gate PRs and releases on the functional matrix Combined from PR #1137, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3691: ticket-key every permanent skip, enforce registered markers, 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. * ENG-3692: align v2 test hygiene, markers and lint scope Combined from PR #1157, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3687: add functional coverage for untested v2 resources Combined from PR #1154, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3689: add v2 team-agent functional tests Combined from PR #1155, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3688: port agent lifecycle functional tests to v2 Combined from PR #1156, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3690: cover public agent run-time features functionally Combined from PR #1162, including its review fixes and the conflict resolutions against the PRs that come before it in this branch. * ENG-3688: send expected output as a string in run payloads 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. * ENG-3688: assert the backend's in-use error code 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. * ENG-3688: stop asserting max_inspectors survives a fetch 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. * ENG-3690: reword the prompt and variables run tests neutrally 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. * ENG-3690: verify only acceptance of run-time inspectors 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. * ENG-3687: page through a sorted search when checking pagination 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. * ENG-3687: give every functional test skill a unique name 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. * ENG-3684: fall back to any multi-action tool 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. * ENG-3683: cap functional matrix parallelism 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. * ENG-3688: send expected output as a string from session runs too 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. * ENG-3684: xfail the sync-only model when the asset lacks connection_type 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. * ENG-3688: use one expectedOutput encoder for save, run and session 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. * ENG-3688: assert inspector fields on the save payload, not after fetch 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. * ENG-3690: make the run-params leg report backend gaps as expected failures 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. * ENG-3689: xfail the team expected-output run until the engine resolves $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. * ENG-3689: drop the run_response_generation team test 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. * ENG-3687: never file issues against production 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. * ENG-3683: hand SLACK_TOKEN only to the legs that read it, and drop HF_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. * ENG-3683: count only runs that prove they used production in the release 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). * ENG-3683: let the leg timeout fire before the functional job timeout 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. * ENG-3683: correct stale CI docs and comments - 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. * ENG-3686: keep leaked-key details out of the public doc 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. * ENG-3686: drop the dead login_response.json allowlist entry .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". * ENG-3684: let a failing integration action listing fail the test 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. * ENG-3687: check that pages 0 and 1 of a sorted search are disjoint 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. * ENG-3684: create the recurring test triggers disabled 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. * ENG-3687: encode every non-string expected_output in one shared module 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. * ENG-3685: cache only permanent asset-resolution failures 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. * ENG-3683: tighten functional-scope paths, label cancellation and the 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. * ENG-3686: catch platform keys after per-environment credential names 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. * ENG-3684: resolve the multi-action tool once per module and stop masking 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. * ENG-3688: keep two run tests off backend gaps they do not test - 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. * ENG-3684: share unique_name and agent_ids from tests/functional/_helpers.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. * ENG-3684: add the Apache 2.0 header to the new functional modules 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. * ENG-3684: find the event-trigger integration and connection without secrets 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. --------- Co-authored-by: aix-ahmet <ahmet.gunduz@aixplain.com>
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.
Context
PROD-2918 removed SDK v1, taking
tests/functional/agent/with it. The generic agent CRUD and web-tool paths already had v2 coverage intests/functional/v2/test_agent.py, but the draft/onboard lifecycle, subcomponent, tool-kind, and round-trip scenarios had no v2 twin and were silently uncovered. This PR restores them against the v2aix.Agent/aix.ToolAPI.Depends on / includes ENG-3685 (shared functional asset ids). The merge commit is
a25a68c2; the ENG-3688 work isd35f17a4. Merge after ENG-3685, or review knowing the first commit is that branch.Changes
New functional suite —
tests/functional/v2/test_agent_lifecycle.py(19 tests)save(as_draft=True)→DRAFT, plainsave()→ONBOARDED, implicit draft save onrun(), onboarded-mutationValueError, end-to-end draft → onboard → fetch → run.save_subcomponents=Truepersists an unsaved subagent; plain save rejects it.Tool(code=...)), Python Interpreter, add/remove tool round-trip, model parameter override survives save → fetch → run, llm parameter preservation, Slack action tool.max_inspectors/inspector_targets, JSONexpected_output(Pydantic schema) both persisted and run,files=[...]reference.New unit suite —
tests/unit/v2/test_agent_draft_lifecycle.py(11 tests)Pins the
before_save/before_runstate machine offline (status transitions, implicit-save vs raise, unsaved-subagent guard).Supporting changes
TRANSLATION_MODELasset.agent-lifecycleCI leg inmain.yaml+docs/ci/required-checks.md.test_agent.py.MIGRATION.mdgaps.Verification
pytest tests/unit→ 2420 passed, 4 skipped.test_functional_hygiene,test_functional_assets,test_ci_matrix_coverage,test_ci_permissions.ruff check/ruff formatclean on new files.test_client.py::TestRateLimitAndJitteredRetryfailures are pre-existing (installed urllib3 lacksretry_after_max) and unrelated.Reviewer Notes
BUG-1098(backend rejects the send-message payload); rename if the filed ticket differs.Tool.searchand skips if absent — v1's client-side interpreter has no stable v2 asset id documented inMIGRATION.md._assets.pyregistry; no inline ObjectIds.Checklist
MIGRATION.mdgaps.