fix(dcode): classify managed non-interactive failures (#8121) - #8206
Conversation
Managed Deep Agents Code non-interactive runs reduced every failure that did not match one hard-coded provider-capacity string to `error_class=unknown category=unknown retryable=false`, so an operator could not tell a transport failure from an authorization rejection or a genuine upstream outage. Replace the single checkpoint pattern with an ordered classifier table covering capacity, rate limiting, authorization, model/route lookup, timeout, route reachability, and remote server errors, and add an exception-type fallback for runs whose cause never reaches the checkpoint database. Categories stay hop-neutral so an MCP, gateway, or policy failure is never reported as upstream provider capacity, and both the log line and the console line remain a closed vocabulary: no checkpoint text, exception message, or unlisted type name is echoed. Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughManaged non-interactive diagnostics classify persisted and active errors across ordered categories. Classifiers scan bounded error windows and exception chains, emit fixed sanitized metadata, and include category and retryability in console output. Tests cover classification, isolation, correlation, chain handling, quoted-prose fallback, and secret protection. ChangesManaged error classification
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The trailing group in the ResourceExhausted classifier was optional, so it could never change whether the pattern matched. Match the bare provider status name instead, and record why the longer NVIDIA#7415 wording must not be required. Signed-off-by: Tony Luo <xialuo@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@agents/langchain-deepagents-code/patch-managed-deepagents-code.py`:
- Around line 803-870: Update the persisted-error classification flow using
_NEMOCLAW_PERSISTED_ERROR_CLASSIFIERS so it extracts and classifies only a known
structured exception type or status field, rather than scanning the entire
__error__ prose; require the ResourceExhausted capacity qualifier where
applicable. When no structural indicator exists, return None and preserve the
active-exception fallback, and add negative fixtures covering quoted model/tool
text containing classifier terms.
In `@test/non-interactive-error-classification.test.ts`:
- Around line 52-54: Update the assertions covering the unlisted exception class
in the non-interactive error classification test to verify that the complete
class name, including its runtime_secret suffix, is absent from the combined
observable output. Keep the existing PLANTED_SECRETS checks, but add a full-name
assertion so separately logged class-name disclosure cannot pass.
🪄 Autofix (Beta)
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: 231d4e5e-0cb0-4db2-9e07-fd5b73385111
📒 Files selected for processing (2)
agents/langchain-deepagents-code/patch-managed-deepagents-code.pytest/non-interactive-error-classification.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
5 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. Since last review: 0 prior items resolved · 0 still apply · 0 new items found 4 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
…A#8121) A persisted `__error__` row embeds model output and tool results, so matching prose let quoted text decide the verdict: a row reading `APIError('tool output: timeout')` produced `Timeout retryable=true` for an unrelated failure. Classify only on structural evidence — an exception or gRPC status name, or a status code carried in a named status field. A row with no structural indicator now stays unclassified and falls through to the exception-type fallback, so an inconclusive row can no longer produce a confident transport, timeout, authorization, or rate-limit claim. Add negative fixtures covering quoted model and tool text, and assert that an unlisted exception class name never reaches the output. Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
Both CodeRabbit findings addressed in b641bf8. Major — "Bind persisted categories to structural error data". Valid, and the failure mode is worse than misclassification: a persisted
Every prose alternative is gone: New negative fixture The note about the optional capacity qualifier was already fixed in 59a712a, before this review landed — the pattern is now the bare Minor — "Assert that the unlisted class name is absent". Valid;
|
PRA-1 asked which source leaves a managed run without an `__error__` checkpoint row to classify, why that source cannot be fixed in this patch, and when the exception-type fallback can be removed. Document both sources (a failure raised before the graph writes any checkpoint, and the LangGraph server writing checkpoints from its own process so a row can land after the client-side exception), the read-only consumer boundary, the allow-list scope, the covering tests, and the removal condition. Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
PRA-1 addressed in 6835a8c. The warning asked which source leaves a managed run with no
Source-fix constraint: NemoClaw cannot make a third-party server write a row for a failure that never reached it, and blocking here to wait for a late row would delay the exit path of every failing run for a write that may never arrive. Removal condition: drop the fallback once the managed runtime can read a structured cause for every failure — either the server persists a row before the client observes the exception, or upstream exposes a classified error object on the non-interactive path. The block also records the read-only consumer boundary ( The two terminology decisions are kept as-is: the first-use explanation of "exception-type fallback" is retained and expanded, and "checkpoint row" stays the term for runtime-stored error records.
|
The advisor's PRA-1 blocker is correct: restricting classification to
exception and status names still searched the complete persisted row, so
a checkpoint reading `APIError('tool output: ResourceExhausted')`
produced a confident capacity verdict from quoted model or tool payload
text — the exact misattribution the surrounding contract claims to
prevent.
Require a name to sit where a serializer would have written it: at the
start of the row, or directly after an opening delimiter, quote, or
module dot with no separating whitespace, and followed by its own message
or argument list. `ResourceExhausted: ...`,
`APIError('ResourceExhausted: ...')`, and `openai.RateLimitError: ...`
still classify, while the same names quoted inside payload prose do not.
Apply the same position rule to the named status field.
Add the parameterized regression the advisor asked for: every supported
name placed in quoted payload content must leave the row unclassified,
plus the serialized-position counterpart.
Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
PRA-1 blocker addressed in 2ab013f — the finding is correct and the earlier fix was incomplete. Restricting classification to exception and status names still searched the complete persisted row, so Classification now requires a name to sit where a serializer would have written it:
Accepted: Regressions added as requested:
Note for the advisor lane: the Nemotron 3 Ultra job has failed on every run of this PR at |
|
Two PRs raced on a stale base:
At Evidence it is not PR-specific:
I have not touched it here, since absorbing another PR's fix into this one would mix concerns. The fix is to update the two literals in
|
|
Nemotron 3 Ultra lane reported 0 blockers on 2ab013f, with one warning about concurrent PR overlap on #7822 — no conflict. #7463 — conflicts, but not with this PR. The conflict is at base line ~546 in The So no sequencing is required for this PR: classification tables, exception fallback, and the redaction boundary are untouched by both overlapping diffs. On the terminology decisions: I have left Both advisor lanes are still failing on their own harness rather than on findings — this run stopped with |
The advisor reported two warnings: the documented late-checkpoint race and the two scanning limits had no regression coverage, leaving the constants I introduced unverified at their boundaries. Add three tests. The first writes an `__error__` row from inside the failing call, reproducing a row that lands after the exception but before classification, and asserts the persisted classification wins over the exception-type fallback. The second inserts one row beyond `_NEMOCLAW_PERSISTED_ERROR_ROW_LIMIT` and asserts the only classifiable row, sitting outside the window, is not reached. The third builds a chain one link deeper than `_NEMOCLAW_EXCEPTION_CHAIN_LIMIT` and asserts the transport cause beyond the limit is not matched and the walk terminates. Both boundary tests were mutation-checked: raising the limits to 21 and 9 makes each classify as `route_unreachable`, so they fail if the bound stops being enforced. Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
Both advisor warnings addressed in 17ac52f (Nemotron lane reported 0 blockers, 2 warnings; recovered from the partial artifact again — the job stopped with Late checkpoint row race. Boundary values. Two tests, one per constant:
Both were mutation-checked rather than assumed: raising the limits to 21 and 9 respectively makes each case classify as
On the five terminology decisions ( |
Decode only the root exception class from pinned MessagePack error rows. Treat nested payload data, unsupported shapes, and malformed values as unclassified. Signed-off-by: Carlos Villela <cvillela@nvidia.com>
`codebase-growth-guardrails` fails: the MessagePack string header helper added three `if` statements to a changed test file, and the policy requires test files not to add any. Replace the branch chain with a narrowest-first lookup over the fixstr, str8, str16, and str32 headers. The encoding is unchanged; the scanner in tools/growth-guardrails/test-conditionals.mts now reports zero `if` statements for this file. Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
@cvillela thanks for 2427d59 — decoding the root exception class from the pinned MessagePack row is the right boundary. My version matched the persisted value as text, which was wrong at the root: That commit left The three came from the
One note in case it matters for your revision: the three tests I added in 17ac52f covered the late-checkpoint-row race and the two scanning limits ( |
wscurran
added
bug-fix
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
Failure point: Why it is not attributable to this PR:
If attempt 2 reproduces it, I will stop treating it as a flake and dig into the target rather than rerun again. |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
This Root cause of that shard, from the job log: The test spawns a child, writes its PID to Not attributable to this PR: the changed files are Neither
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
@JulieYaunches the Terra advisor lane flagged one warning, and it is about your readiness retry rather than the classifier: I checked the two sides rather than guess, in case it saves you the lookup:
So the two are genuinely different owners, which is exactly what the warning asks you to state. A drop-in block if you want it: # Invalid state: OpenShell publishes the recreated sandbox as Ready before the
# in-sandbox inference route accepts probes, so the first status after a fresh
# re-onboard can report failureLabel=unreachable for a sandbox that becomes
# healthy moments later.
# Source boundary: readiness is published by OpenShell's sandbox lifecycle and
# only consumed here. The probe is NemoClaw's
# probeSandboxInferenceGatewayHealth in
# src/lib/actions/sandbox/inference-route-health.ts, which reports the route
# state at the instant it runs and must not wait.
# Source-fix constraint: NemoClaw cannot make OpenShell delay Ready until the
# route serves, and making status retry internally would turn a point-in-time
# report into a wait, hiding real outages from every other caller. The retry
# belongs to this check, the only consumer that knows a re-onboard just
# happened.
# Regression: test/e2e/support/platform-parity-cloud-experimental.test.ts covers
# eventual status success and retry exhaustion.
# Removal condition: delete this retry once OpenShell publishes Ready only after
# the in-sandbox inference route serves, or once NemoClaw exposes an explicit
# readiness-wait command this check can call instead.Say the word and I will push it; I did not want to edit your commit's comment unprompted, and we have already collided on this branch a few times. |
Both advisor lanes raised the same warning on the post-re-onboard status retry: the check does not name the component that publishes Ready before the in-sandbox inference route accepts probes, why that ordering cannot be corrected here, or when the retry can be removed. Record the boundary. Readiness is published by OpenShell's sandbox lifecycle and only consumed here, while the probe is NemoClaw's probeSandboxInferenceGatewayHealth, which reports the route state at the instant it runs and must not wait. NemoClaw cannot delay OpenShell's Ready, and making status retry internally would turn a point-in-time report into a wait that hides real outages from every caller, so the retry belongs to the one consumer that knows a re-onboard just happened. Comment-only; shellcheck and bash -n pass. Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
Pushed the readiness-retry boundary block in 09d4374. Both advisor lanes raised the same warning independently on the latest revision ( It records what I verified earlier rather than an assumption:
Comment-only change; On the advisor's stronger option ("identify and correct the component"): correcting the ordering means changing when OpenShell publishes Ready, which is outside this repo, so documenting the constraint is the available path here. If a maintainer wants the source-level readiness contract pursued, that is an OpenShell-side change and worth its own issue rather than this PR. Both advisor jobs died on |
6 participants
Summary
Managed Deep Agents Code non-interactive runs reported only
error_class=unknown category=unknown retryable=falsefor most failures. This change classifies exact exception class objects from the bounded active client exception chain. The classes come from the pinned LangGraph SDK, HTTP transport, TLS, and managed model-configuration packages. Persisted checkpoint exception text is not classified because it contains forgeablerepr(exception)output. Operator output uses a closed vocabulary and never emits exception messages, checkpoint content, or unlisted type names.Related Issue
Partially addresses #8121.
Changes
__cause__and__context__chain.Leakage boundary
Classification inputs are exact imported exception class objects in the active client process. Outputs are literals from the classifier table plus the existing correlation ID. Exception messages, checkpoint content, forged names, and unlisted type names never reach logs or the console.
Type of Change
Quality Gates
09d437439passed all nine repository categories. Classification uses exact imported exception classes, bounds the cause/context walk, ignores forgeable checkpoint representations, emits fixed labels without exception content, adds no dependency or authorization surface, and has adversarial regression coverage.Documentation Writer Review
docs-updateddocs/manage-sandboxes/run-deep-agents-code.mdxdocuments the exact-class diagnostic fields, retry semantics, unknown fallback, checkpoint-text trust boundary, and fixed-label output behavior. The final E2E comment records the bounded readiness-retry boundary and removal condition.Codex DesktopVerification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run checks:repository,npm run test:changed, andnpm run validate:prpassed.npm run docsbuilds without warnings (doc changes only)Scope note: the exit-code half of #8121
#8121 also reports that the run exits 0. This PR does not change exit propagation. The patched handler already returns 1 and upstream exits with that value. Sibling issues #8137 and #8138 track the runtime exec transport behavior that requires a Linux sandbox to reproduce.
Signed-off-by: Tony Luo xialuo@nvidia.com
Summary by CodeRabbit