Skip to content

feat(agent-builder): log the silent generation/join pipeline failures#1161

Merged
yewreeka merged 1 commit into
devfrom
jarod/agent-builder-observability
Jul 10, 2026
Merged

feat(agent-builder): log the silent generation/join pipeline failures#1161
yewreeka merged 1 commit into
devfrom
jarod/agent-builder-observability

Conversation

@yewreeka

@yewreeka yewreeka commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Why

Diagnosing today's prod "creating a new agent fails" report (logs bundle convos-logs-BB7ACDAB, 17:56Z attempts) was nearly blind: both failing builds produced zero device-log lines between the built_agent event and the agent never appearing. Every terminal failure in the generation stage is a silent markFailed, the submit/poll retry loops swallow errors, and the generation + agents/join endpoints skip the response logging every other endpoint gets. The one same-day successful build (15:19Z) was only traceable because its direct-add happened to retry.

What

Logging (no behavior change):

  • AgentTemplateRepository: generation start (idempotencyKey + conversation), status transitions, submit/poll retry errors with attempt counts, backend-reported generation failures with the server's error message, invite start/success, and every markFailed with its message
  • Loud warning if the raw join fallback (no direct-add) ever runs outside tests — it provisions an agent that then never joins the conversation
  • ConvosAPIClient: non-2xx status + body from the generation endpoints and agents/join; join success logs instanceId and whether inboxId came back inline
  • SessionManager: the provision → addMembers handoff (instance, agent inbox, conversation)

Behavior fix:

  • 401/403 from the generation endpoints now map to a terminal .badRequest carrying the server's message (e.g. "Account required") instead of the retryable .server path, which silently retried 5x and then surfaced a generic "Couldn't reach the builder". A surfaced 401 has already exhausted the re-auth retry inside performAuthenticatedRequest, so plain retries can't heal it.

Tests

  • New test: generation 401/403 map to terminal badRequest with the parsed server message; 5xx stays retryable .server
  • Full ConvosCore suite: 1605 tests in 226 suites, all passing (Docker stack up)
  • SwiftLint strict on changed files: clean

🤖 Generated with Claude Code


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

Note

Log silent failures in agent generation and join pipeline

  • Adds structured logging throughout the agent builder pipeline in AgentTemplateRepository and ConvosAPIClient to surface previously silent failures during generation start, polling, and invite steps.
  • Maps 401/403 responses in decodeGenerationResponse to AgentGenerationError.badRequest with a parsed message instead of the generic AgentGenerationError.server, so callers can distinguish authorization failures.
  • Adds a test in AgentTemplateRepositoryTests asserting that 401 and 403 map to .badRequest and 500 maps to .server.
  • Behavioral Change: callers that previously received AgentGenerationError.server for 401/403 responses will now receive AgentGenerationError.badRequest.

Macroscope summarized d374bae.

Diagnosing a prod "agent never joins" report was nearly blind: every
terminal failure in the AgentTemplateRepository generation stage
(markFailed), the submit/poll retry loops, and the agents/join HTTP
outcomes left no trace in device logs. This adds logging across the
build -> generate -> provision -> direct-add pipeline so the next
repro shows exactly where it died:

- AgentTemplateRepository: generation start, status transitions,
  submit/poll retry errors, backend-reported generation failures,
  invite start/success, and every markFailed with its message
- warn loudly if the raw join fallback (no direct-add) runs outside
  tests, since it provisions an agent that never joins
- ConvosAPIClient: log non-2xx bodies from the generation endpoints
  and agents/join; log join success with instanceId/inboxId presence
- SessionManager: log the provision -> addMembers handoff
- behavior fix: 401/403 from generation endpoints are now terminal
  with the server's message (e.g. "Account required") instead of
  retrying through the .server path and surfacing a generic
  "Couldn't reach the builder"

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Claude finished @yewreeka's task —— View job


Code Review Complete ✅

  • Read and analyze changed files
  • Review logging implementation
  • Review error handling behavior change
  • Review test coverage
  • Check for potential issues
  • Provide comprehensive feedback

Summary

Solid observability improvement. The logging additions will dramatically improve debugging of production agent generation failures. The 401/403 behavior fix correctly treats auth failures as terminal instead of retryable. The new test coverage validates the critical behavior change. Minor recommendations below.

Strengths

  1. Comprehensive logging coverage - Logs all critical lifecycle events: generation start, status transitions, retry attempts with counts, backend errors, and invite outcomes
  2. Appropriate log levels - Uses .info for normal flow, .warning for recoverable issues, .error for terminal failures
  3. Context-rich messages - Includes identifiers (idempotencyKey, generationId, conversationId, templateId) and attempt counts
  4. Critical behavior fix - 401/403 now correctly terminal instead of retryable, with clear inline comment explaining why
  5. Excellent test coverage - New test validates all three status codes (401, 403, 500) and verifies error message parsing
  6. Production motivation - PR description clearly explains the debugging pain this solves

Issues Found

1. Logging PII Risk (Minor - Security)

Location: ConvosCore/Sources/ConvosCore/API/ConvosAPIClient.swift:906-907, ConvosAPIClient.swift:1274-1276

The error logging includes the full HTTP response body:

Log.error("agents/join failed [\(httpResponse.statusCode)]: \(String(data: data, encoding: .utf8) ?? "nil data")")

Concern: If the backend ever includes PII (user prompts, email addresses, names) in error responses, this will log it to device logs, which are uploaded to Sentry and can be pulled from devices. While backend error messages are typically safe, logging raw response bodies is risky.

Recommendation: Either:

  • Use parseErrorMessage(from: data) instead of raw body (already exists as a helper)
  • Add a prefix like "first 200 chars" limit: String(data: data, encoding: .utf8)?.prefix(200)
  • Verify backend never includes PII in these error responses and document that assumption

2. Missing Logging Context (Very Minor - Code Quality)

Location: ConvosCore/Sources/ConvosCore/AgentBuilder/AgentTemplateRepository.swift:310-311

case .server(let message):
    Log.error("AgentTemplateRepository: submit server error (attempt \(attempt + 1)/\(Constant.maxSubmitAttempts)): \(message ?? "no message")")

Missing the idempotencyKey or conversationId that other logs include, making it harder to correlate with other logs for the same generation.

Recommendation:

Log.error("AgentTemplateRepository: generation \(row.idempotencyKey) submit server error (attempt \(attempt + 1)/\(Constant.maxSubmitAttempts)): \(message ?? "no message")")

Same issue at line 330 (general submit error catch).

3. Test Gap (Minor - Test Coverage)

Location: ConvosCore/Tests/ConvosCoreTests/AgentTemplateRepositoryTests.swift:118-157

The new test validates decodeGenerationResponse directly, but doesn't test the retry behavior difference between 401/403 (terminal) vs 5xx (retryable). A future refactor could break the retry logic without this test catching it.

Recommendation: Add an integration test that verifies:

  • 500/503 actually retry up to maxSubmitAttempts
  • 401/403 fail immediately without retry attempts

(Not blocking - the behavioral change is correct and the unit test validates the error mapping)

Performance Considerations

No performance concerns. String interpolation for logging is lazy-evaluated in Swift, and the log volume is reasonable (one log per major state transition, bounded retry attempts).

Best Practices

✅ Follows project logging conventions (Log.info/warning/error)
✅ Inline comments explain non-obvious behavior (401/403 auth exhaustion)
✅ Test uses #require for clarity on preconditions
✅ Commit message is detailed and explains the "why"

Recommendation

Approve with minor improvements. The PII risk is low but worth addressing. The missing context in error logs is very minor. The test gap is optional. All issues are safe to fix in a follow-up if you want to ship this immediately.


@yewreeka yewreeka marked this pull request as ready for review July 10, 2026 18:32
@macroscopeapp

macroscopeapp Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved

This PR adds logging throughout the agent generation/join pipeline for debugging, plus a targeted bug fix that makes 401/403 errors fail terminally instead of silently retrying. The behavioral change is a straightforward fix with clear intent, limited scope, and test coverage.

You can customize Macroscope's approvability policy. Learn more.

@yewreeka yewreeka merged commit fca7eb3 into dev Jul 10, 2026
11 checks passed
@yewreeka yewreeka deleted the jarod/agent-builder-observability branch July 10, 2026 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant