Forward-merge release/2.2 into develop - #366
Merged
Merged
Conversation
…ct (#360) #### Overview The `aiq-research` skill helper (`skills/aiq-research/scripts/aiq.py`) had two contract mismatches with the 26.07 backend. Both were verified to still reproduce at `release/2.2` HEAD, and the fix was checked against the backend source that defines each contract. **1. `interrupted` was not a terminal job state.** `_DONE_JOB_STATES` and `_FAILED_JOB_STATES` omitted `interrupted`, but the backend sets a cancelled job to that state (`JobStatus.INTERRUPTED = "interrupted"` in `nat` `async_jobs/job_store.py`; the cancel route in `frontends/aiq_api/.../routes/jobs.py` returns `{"status": "interrupted"}`). Because the poll loop only exits on a member of `_DONE_JOB_STATES`, a cancelled job kept polling every 15s until the 3600s timeout. Adding `interrupted` to both sets makes polling stop immediately and report the job as failed rather than completed successfully. **2. Deep-research escalation was detected only in the legacy text format.** `_command_chat` matched only `Job ID: <uuid>`, so the backend's structured escalation payload was printed raw and never surfaced as `deep_research_running`, leaving the documented `SKILL.md` Step 2/3 flow stuck. The backend emits the escalation as a compact JSON object placed in the assistant message content (`_job_escalation_message` in `src/aiq_agent/agents/chat_researcher/agent.py`): ```json {"type": "job_escalation", "kind": "deep_research", "job_id": "<uuid>"} ``` A new `_detect_deep_research_escalation` helper recognizes this object both as the top-level `/chat` result and as JSON embedded in the message content, validates the `job_id` as a UUID, and emits the documented `{"status": "deep_research_running", "job_id": "<uuid>"}`. It is tried before the legacy `Job ID:` regex, which is retained for backward compatibility. Non-`deep_research` kinds (e.g. `report_edit`), malformed JSON, non-escalation JSON, and missing/invalid job IDs are deliberately ignored so they cannot produce a false `deep_research_running`. No `SKILL.md` prose change is required: the emitted shape is unchanged, so the existing Steps 2–4 flow now advances as documented. #### DCO sign-off for the squash commit Signed-off-by: Tanner Leach <tleach@nvidia.com> #### Validation Run from the repo root: ```bash uv run pytest tests/scripts/test_aiq_research_helper.py -q # 12 passed uv run ruff check skills/aiq-research/scripts/aiq.py tests/scripts/test_aiq_research_helper.py # All checks passed uv run pre-commit run --files skills/aiq-research/scripts/aiq.py tests/scripts/test_aiq_research_helper.py # ruff, ruff-format, detect-secrets, EOF/whitespace all pass ``` Contract verification against `release/2.2` source (no live 26.07 backend was required, and the fix was not validated against assumptions alone): - SK-1: `JobStatus.INTERRUPTED = "interrupted"` (nat `async_jobs/job_store.py`); cancel route returns `{"status": "interrupted"}` (`frontends/aiq_api/src/aiq_api/routes/jobs.py`). - SK-2: `_job_escalation_message(kind, job_id)` returns `{"type":"job_escalation","kind":..., "job_id":...}` and is delivered as `AIMessage(content=...)` (`src/aiq_agent/agents/chat_researcher/agent.py`), matching the "escalation embedded in message content" path the helper handles. New test matrix (`tests/scripts/test_aiq_research_helper.py`, loaded via `importlib` to match the standalone-script tests already in `tests/scripts/`): - `interrupted` membership in the terminal/failed state sets; - `poll_until_complete` returns on the first `interrupted` status (one status call, zero sleeps); - `research_poll` exits failure on `interrupted` without another poll cycle and without fetching a report; - JSON escalation detected as top-level result and as embedded content; - `_command_chat` emits `deep_research_running`; - legacy `Job ID: <uuid>` still detected; - guards: malformed JSON, non-escalation JSON, unsupported `kind`, missing/invalid/non-string `job_id`, and shallow-answer fall-through. - [x] I ran the relevant local checks or explained why they are not applicable. - [x] I added or updated tests for behavior changes. - [x] I updated documentation for user-facing or contributor-facing changes. - [x] I confirmed this PR does not include secrets, credentials, or internal-only data. - [x] I certify this contribution under the Developer Certificate of Origin (DCO) and signed my commits with `git commit -s` or an equivalent sign-off. - [x] I replaced the DCO sign-off placeholder with my GitHub commit identity and kept the required angle brackets around the email address. #### Where should reviewers start? - `skills/aiq-research/scripts/aiq.py`: the state-set change (`_DONE_JOB_STATES` / `_FAILED_JOB_STATES`) and the new `_escalation_job_id` / `_detect_deep_research_escalation` helpers plus the reworked `_command_chat` ordering (JSON escalation first, legacy regex retained). - `tests/scripts/test_aiq_research_helper.py`: the guard cases confirm no false `deep_research_running` on malformed / non-escalation / unsupported-kind / invalid-`job_id` payloads. #### Related Issues ## Summary by CodeRabbit * **Bug Fixes** * Deep-research escalations are now detected reliably from chat responses. * Valid job IDs are reported with a `deep_research_running` status. * Interrupted research jobs are treated as terminal failures and stop polling promptly. * Invalid or malformed job IDs now safely fall back to displaying the original response. * **Tests** * Added coverage for escalation formats, interrupted jobs, legacy job-ID parsing, and malformed responses. Authors: - Tanner Leach (https://github.com/tanleach) Approvers: - Ajay Thorve (https://github.com/AjayThorve) URL: #360
Author
|
SUCCESS - forward-merge complete. |
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.
Forward-merge triggered by push to release/2.2 that creates a PR to keep develop up-to-date. If this PR is unable to be immediately merged due to conflicts, it will remain open for the team to manually merge. See forward-merger docs for more info.