fix(intake): honor explicit ATIF per-result tool status

[mdcox]

Problem

Intake marks a successful ATIF tool call as failed when the tool's returned text contains the case-insensitive substring [error] anywhere in the payload.

_tool_result_is_error() ended with:

content = _result_text(result)
return content is not None and "[error]" in content.lower()

This treats arbitrary returned content as a status channel, which is unsafe for search, retrieval, log-reading, and ticket-reading tools — successful results routinely quote error messages from other systems.

Observed in a production qa-copilot trace (efdefb9d-94e2-4051-9839-c5b9caef03ea): a successful glean_search returned results containing the log line [ERROR] Failed to download package. The producer reported success (isError: false, "success": true), but Intake stored the tool span with status: "error" and copied the successful search response into error_message. Since repository/clickhouse/trace.py computes countIf(status = 'error') AS error_count, the span also inflated the trace's error count.

Reproduced on main before the fix; the mapper produced status=error with error_message set to the full successful payload.

Fix

Consume AtifObservationResult.extra.is_error as an authoritative per-result signal, in both directions, ahead of the existing markers. The field is already accepted by the schema but was never read.

Per-result rather than step-level is deliberate: _matched_tool_result_metadata() and _step_extra_bool() are both gated on _is_only_observation_result(), so neither can express status for parallel tool calls. That gap is why producers resorted to text sentinels. Note also that a step-level is_error: false would not have helped — the existing branches only short-circuit on exactly True, so False fell through to the substring check.

The three existing branches are unchanged. Source diff is 13 lines.

Why the text fallback is retained

It cannot be safely removed yet:

• Deleting it would flip real failures to success. Claude Code trajectories already send both step-level markers (see tests/integration/spans/test_atif_ingest.py), so they do not depend on it — but producers that signal secondary parallel-call failures only in text do, and the step-level markers structurally cannot express those.
• Anchoring it (e.g. to line starts) would fix this specific case, since [ERROR] here is mid-line inside JSON, but would still misfire on log-reading tools where [ERROR] legitimately begins lines. That trades one silent wrong answer for a narrower one.

The intended sequencing is: land this, migrate producers to emit extra.is_error, then remove the fallback. Sanitizing or escaping [ERROR] in tool content is explicitly not proposed — it would alter source evidence and only move the ambiguity to another string.

Producer contract

{
  "source_call_id": "call-1",
  "content": "...",
  "extra": { "is_error": false }
}

Backward compatible: trajectories without result.extra.is_error continue through the existing paths unchanged.

Tests

Six new tests in services/intake/tests/test_atif_v17.py:

• extra.is_error: false with content containing [ERROR] produces a successful span with no error_message
• extra.is_error: true produces an error span even when content has no marker
• two parallel results with differing explicit statuses map to differing span statuses
• a legacy result with no explicit status and [error] content remains an error
• the subagent-span path (_subagent_ref_to_span) honors explicit status — this is a second call site of _tool_result_is_error(), so the false positive was not limited to tool spans

Verification

• 307 intake unit tests pass, including the pre-existing test_atif_mapping_keeps_tool_error_on_tool_span
• pre-commit run --files clean on both changed files (ruff, ruff format, ty, copyright headers)
• Full services/intake suite green on this branch: 455 passed, 0 skipped (~12 min). The integration tests self-provision ClickHouse and were included in that run.

Validated against: https://docs.nvidia.com/nemo/relay/configure-plugins/observability/atif as a valid path

Summary by CodeRabbit

• Bug Fixes

  • Improved tool-result error detection by prioritizing explicit error status information.
  • Prevented successful results from being incorrectly marked as errors when their content only mentions error text.
  • Improved handling of parallel tool results and subagent results.
  • Preserved compatibility with existing metadata and legacy text-based error indicators.

• Tests

  • Added coverage for explicit error flags, quoted error text, parallel results, legacy markers, and subagent results.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

The tool-result mapper now prioritizes a boolean is_error value from observation-result extras. Existing fallback checks remain available when the explicit value is absent. Tests cover quoted error text, parallel results, legacy markers, and subagent results.

Suggested reviewers: shanaiabuggy, svvarom

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes honoring explicit per-result ATIF tool status, which is the main change.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-intake-atif-false-tool-error/morganc

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

services/intake/tests/test_atif_v17.py (1)

737-745: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test explicit False against both legacy boolean branches.

These tests only prove that extra.is_error=False overrides text. Add fixtures where tool_result_metadata.is_error=True and tool_result_is_error=True. Both must produce SpanStatus.SUCCESS. A later branch reordering can otherwise break the authoritative-false contract without failing this suite.

🤖 Prompt for 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.

In `@services/intake/tests/test_atif_v17.py` around lines 737 - 745, Extend
test_atif_mapping_keeps_successful_tool_result_that_quotes_an_error with
separate fixtures using tool_result_metadata.is_error=True and
tool_result_is_error=True alongside extra.is_error=False. Assert each mapping
produces SpanStatus.SUCCESS, no error_message, and successful statuses for all
spans, preserving the authoritative explicit-false behavior across both legacy
boolean branches.

🤖 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.

Nitpick comments:
In `@services/intake/tests/test_atif_v17.py`:
- Around line 737-745: Extend
test_atif_mapping_keeps_successful_tool_result_that_quotes_an_error with
separate fixtures using tool_result_metadata.is_error=True and
tool_result_is_error=True alongside extra.is_error=False. Assert each mapping
produces SpanStatus.SUCCESS, no error_message, and successful statuses for all
spans, preserving the authoritative explicit-false behavior across both legacy
boolean branches.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 15a30e5b-b548-4e68-9e48-8c9abc19803b

📥 Commits

Reviewing files that changed from the base of the PR and between 48cf7da and e88c1cb.

📒 Files selected for processing (2)

• services/intake/src/nmp/intake/spans/ingest/atif_mapping.py
• services/intake/tests/test_atif_v17.py

[github-actions]

Suite	Lines Covered	Line Rate	Branch Rate
Unit Tests	32233/40906	78.8%	63.7%
Integration Tests	18647/38832	48.0%	20.7%