fix(intake): accept ATIF observation on all step types - #1189
Conversation
Relay emits an `observation` on any ATIF step, but Intake only permitted it on agent steps. In-process harnesses like DeepAgents put an observation on a nested system step, so Intake rejected the entire ATIF POST with HTTP 422 and stored nothing. Move `observation` from AtifStepAgent to the shared AtifStepBase; run the tool-call-reference and v1.7 subagent-ref validators on all step types; let the mapper read observations on any step. Adds a regression test reproducing a DeepAgents nested-system-step observation. Co-authored-by: Yuchen Zhang <134643420+yczhang-nv@users.noreply.github.com> Signed-off-by: Nathan Walston <nwalston@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughATIF observations are defined on the shared step base, exposed in the OpenAPI schemas, validated for non-agent steps, mapped for all step types, and covered by a nested system-step regression test. Execution-profile documentation now describes capability filtering and topology-specific results. ChangesATIF observation handling
Execution-profile listing documentation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/intake/tests/test_atif_v17.py`:
- Around line 1094-1100: Strengthen the assertions in the trajectory_to_spans
test around the non-agent observation case by locating the nested system-step
span and verifying its mapped output or raw observation attributes contain
“is_likely_phishing: true”. Keep the existing spans presence assertion, but
ensure the test fails when _step_observation returns None.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ec1b351f-05d2-4e02-926d-00736b7605c6
📒 Files selected for processing (3)
services/intake/src/nmp/intake/spans/ingest/atif_domain.pyservices/intake/src/nmp/intake/spans/ingest/atif_mapping.pyservices/intake/tests/test_atif_v17.py
|
Address review: the prior `assert spans` passed even if `_step_observation` regressed to agent-only, since trajectory/step spans emit independently. Locate the nested system-step span and assert `_step_observation` returns the observation value on the non-agent step, so the test fails if the mapper stops reading observations there. Signed-off-by: Nathan Walston <nwalston@nvidia.com>
…on-all-steps/nwalston Signed-off-by: Nathan Walston <nwalston@nvidia.com>
The intake fix moved `observation` from `AtifStepAgent` to `AtifStepBase`, which changes the ATIF ingest contract: `observation` now appears on the `system` and `user` step schemas as well. Regenerated via `script/generate-openapi-spec.sh`; no hand edits. Co-authored-by: Yuchen Zhang <134643420+yczhang-nv@users.noreply.github.com> Signed-off-by: Nathan Walston <nwalston@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Stainless sync (`sdk/stainless.sh sync`) for the ATIF ingest contract change: `observation` moved from `AtifStepAgent` to `AtifStepBase`, so the generated `AtifStepSystemParam` / `AtifStepUserParam` models now carry it, and the `.nmpcontext` snapshot matches the regenerated spec (clears `lint-python-sdk`). Also sweeps one unrelated docstring-only update in `resources/jobs/jobs.py` (execution-profiles description), which was pre-existing drift on main picked up by the clean regen — generated output, not hand-edited. Co-authored-by: Yuchen Zhang <134643420+yczhang-nv@users.noreply.github.com> Signed-off-by: Nathan Walston <nwalston@nvidia.com>
|
Pushed two follow-up commits to clear the CI blockers (no change to the fix itself):
One heads-up for review: the SDK regen also swept a docstring-only update in |
`lint-cli` runs `make update-cli` and fails if it dirties the tree. Running it regenerates the jobs execution-profiles docstring in the generated CLI command (and its vendored SDK copy + CLI reference docs). This is drift from #1082 (AIRCORE-971, merged to main today), which changed the jobs API docstring without regenerating the spec/SDK/CLI — swept up here by the clean regen, since the generators produce it deterministically and the branch cannot go green otherwise. Generated output, no hand edits. Co-authored-by: Yuchen Zhang <134643420+yczhang-nv@users.noreply.github.com> Signed-off-by: Nathan Walston <nwalston@nvidia.com>
Problem
Relay emits an
observationon any ATIF step, but Intake's schema only permittedobservationon agent steps. In-process harnesses like DeepAgents attach an observation to a nested system step, so Intake rejected the entire ATIF POST with HTTP 422 and stored nothing — DeepAgents traces never reached Intake/Studio.Diagnosed by @yczhang-nv (Yuchen Zhang), who also recommended the fix below.
Fix
observationfromAtifStepAgentto the sharedAtifStepBase, so every step type (system/user/agent) can carry one.validate_atif_tool_call_references,validate_atif_v17_subagent_ref_resolution_keys) on all step types (tool-call ids are still sourced only from agent steps)._step_observationno longer gates onAtifStepAgent).Verification
ruff+tyclean;services/intake/tests/test_atif_v17.py23/23 pass (incl. the newtest_atif_v17_observation_on_nested_system_step).system-*steps — confirmed both via the Intake spans API and in the Studio trace view.Notes
Independent Intake bug fix, split out from the Fabric email-phishing example work. Getting DeepAgents to emit ATIF additionally requires Fabric ≥
0207e06(0.2.0); that dependency bump is a separate, platform-wide change and is not part of this PR.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests