chore(backend): enforce ruff TRY400 so error logs carry tracebacks - #10220
chore(backend): enforce ruff TRY400 so error logs carry tracebacks#10220saltas888 wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 23 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
|
🤖 CI reconcile — two checks were red. One was mine and is fixed; one is pre-existing on 1.
|
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
6396a66 to
0f74c05
Compare
|
🤖 Reconcile pass 1 — CI was red. Rebased onto latest Triage of the previous run (31510405357)Six jobs failed. None failed on an assertion attributable to this diff — every one is a resource/timing signature:
Meanwhile every job this change could deterministically affect passed: On the E2E worker tracebackThe shard logs show Hypothesis I considered and did not dismiss
No code changed in this pass — rebase only. Nothing merged. |
CI triage — failures look environmental, failed jobs re-runAutomated reconcile pass. CI came back red on 7 jobs; I looked at each failure's actual cause before touching anything, and none of them is an assertion failure related to this diff.
Three independent signatures — OOM kill, a Neo4j transient deadlock, and a server read timeout — all consistent with a resource-constrained runner. A Corroborating: #10218 (an unrelated, human-authored ruff-ignores cleanup) shows the same Branch staleness was checked first and ruled out — Action taken: re-ran the failed jobs (run For reference, local verification on this branch was and remains green: full |
|
🤖 CI triage — the 4 red checks are infrastructure, not this change. Re-ran the failed jobs; no code or branch change.
An unhealthy RabbitMQ container and DNS resolution failures on the runner. Nothing in a logging change can prevent a container from passing its healthcheck.
All three are node-creation / list-query tests. This PR touches no code they exercise: the only changed file anywhere near that path is For reference, If the re-run comes back red on the same three tests with actual tracebacks, that changes the picture and I'll dig into the code rather than the runner. The checks that actually gate this change were green in the same run, and locally: ruff (0 TRY400, no new violations), ty, mypy (1610 files), and 2233 backend unit tests. |
CI status: 43/45 green — the 2 failures are a repo-wide infrastructure problem, not this PRFlagging this rather than pushing blind fixes at it. What fails: The stack never booted and compose returned empty stdout/stderr. Why it isn't this branch:
Ask: this needs either a fix to the e2e docker-compose boot on the runners (which is blocking Reminder: this PR is the TRY400 half only. TRY004 remains suppressed and needs its own card, so merging this shouldn't close INBOX-29. |
CI triage — root cause found: runner Docker address-pool exhaustion. Escalating.Automated reconcile pass 4. Down to 2 red jobs from 7, both The failure is
|
Response to
|
| tasks.md says | actually now |
|---|---|
process.py:204 |
:208 |
infrahub_async.py:194 |
:196 |
integrator.py noqa 456 / 638 |
458 / 644 |
integrator.py noqa 459 / 641 |
463 / 649 |
integrator.py:810 |
:818 (and no longer a log.error site) |
The numbers were measured pre-conversion and the conversions shifted them. As an archived record that's misleading — someone opening integrator.py:810 later finds nothing. The suggestion to use symbol anchors (get_check_definition, get_python_transforms, the except ValidationError handler in _get_jinja2_transforms) instead of absolute lines is the right shape.
Deferred rather than dismissed: it's a docs-only edit to dev/specs/, and pushing now would restart CI onto the same exhausted runners and add churn to an escalated PR. Worth doing in the same pass that applies the app.py fix above.
❌ REBUTTED — "should target stable, not develop"
Two independent reasons this is wrong here:
1. The premise is factually incorrect. The finding characterises this as "primarily repo tooling — lint config plus spec docs — which cannot affect a running product." It isn't. The diff converts 28 runtime logging call sites across 12 backend modules (git/, core/merge/, core/branch/, database/, graphql/, services/, webhook/, workers/). log.error → log.exception changes what production emits — that is exactly why the webhook site's record-dropping hazard mattered enough to document. Lint config is the enabler; the runtime logging change is the substance.
2. The closest precedent went to develop. #10002 — "Re-enable ruff BLE rule and fix all violations", the near-identical predecessor to this change — merged to develop. So did #10147 and the currently-open #10218 ("remove ruff ignore rules that no longer suppress anything"). The two ruff PRs that did target stable (#9199, #8669) were pure config/docs with no source changes — the distinction the guidance is actually drawing, and this change falls on the develop side of it.
Keeping develop. Happy to be overruled by a maintainer who owns the release-vehicle policy.
Net: one genuine code inconsistency to fix (app.py:195), one docs accuracy fix, both deferred to the human who unblocks the CI infra. Nothing pushed, nothing merged.
🤖 Re-ran the two stale E2E shards — the blocker is confirmed clear on today's developNo code changed, nothing pushed, nothing rebased, nothing merged. This card is escalated ( The two red checks are a 3-day-old result, and the shards pass on current developThe failing run on this PR is 31515711302, attempt 3, created 2026-08-11 17:04 UTC — the run that produced the "Docker address-pool exhaustion" diagnosis. Nothing has run here since. develop's newest run (31787850882, 2026-08-14 09:23 UTC) is itself a failure, which could look like the outage returning. It isn't — the failures are different jobs:
Both shards red here pass on current develop, so the compose fixture boots on the runners again. Action:
|
0f74c05 to
4af080e
Compare
|
🤖 Reconcile pass — CI was red, cause was a stale branch. Rebased onto The single failure was Unrelated to this PR's diff (Python logging calls only, zero frontend files). The actual history:
This branch was 7 commits behind What I did: rebased onto Re-verified on the new base:
CI is re-running. I'll reconcile again when it settles. |
CI status — the blocker changed, and it now points at this PRNothing pushed in this pass. This PR is escalated ( What's failing nowThe previously-escalated failure —
Why this is harder to write off as infrastructure
The honest counter-argumentEvery failure is a timeout, never an assertion; 1178 tests in the same job passed; and I could not identify a mechanism by which Cheapest decisive next stepRe-run just
Also still queued from the previous pass and unaffected by this: the 🤖 platform-health-agent, INBOX-29 |
…c sites by symbol Addresses two review findings on PR #10220. A Starlette ClientDisconnect is raised whenever a client aborts while its request body is being read - routine rather than exceptional - and its traceback only shows the body-read path. Converting it emitted a full ERROR-level stack trace per aborted request. That contradicted the change's own stated criterion (convert unless the traceback would be actively harmful or worthless), so it moves to log.error with a justified noqa alongside the other such sites. Now 27 conversions and 7 in-line suppressions. The spec's task list identified each site by absolute line number, measured before the conversions shifted them; several no longer pointed at a log call at all. Since the spec merges as the archived record, those anchors actively mislead, so sites are now named by enclosing function with a note on how to regenerate the set. Refs: INBOX-29 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs
The
|
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The post-review revert of the ClientDisconnect handler moved one site from converted to noqa, leaving stale 28/6 figures in plan.md, research.md, tasks.md, alignment-check.md and the implementation report. Verified against the tree: 7 `noqa: TRY400` comments, 2 sites deferred by per-file-ignore in auth/auth.py, 27 converted = 36. The dated critique keeps its original figure, marked as-of, rather than being rewritten after the fact. Addresses review threads on PR #10220. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs
CI triage: the one red check is infra flake, not this change
Why it is not this changeThe stack trace lands in The task-worker could not reach the server during stack startup, so it exited via the pre-existing Two further points:
An accidental demonstrationThe except SdkError as err:
self._logger.exception(f"Error in communication with Infrahub: {err.message}")
raise typer.Exit(1) from errBefore this change that handler logged the message with no stack, so a worker that failed to reach the server gave you one line and nothing to trace. The failing job is, incidentally, a live example of the debuggability the rule is meant to restore. Action taken
Note that pushing (2) cancelled the re-run from (1), so the flake retest is inconclusive; a full fresh run is now in flight on |
There was a problem hiding this comment.
0 issues found across 6 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Re-enables ruff TRY400, converting log.error→log.exception with documented noqas at ~36 sites, preserving log level, message, and keyword fields. Verified by clean lint and 2233 passing tests; no contract, auth, or operational behavior change requiring human tradeoff.
Re-trigger cubic
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs
Key finding: converting webhook/tasks/process.py:204 would make TracebackSuppressionFilter drop the whole record — kept as log.error + noqa. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs
…BOX-29) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs
Re-enable TRY400 (error-instead-of-exception) via extend-select, keeping the rest of the ignored TRY family - TRY004 included - suppressed. TRY004 is out of scope: its fix changes caller-visible exception types on core/schema/ and GraphQL mutation surfaces and needs human design review (INBOX-29). Of the 36 flagged sites, 28 become log.exception. Six keep log.error with a justified noqa, the important one being the webhook delivery failure report: WebhookDeliveryError is registered for traceback suppression and the filter drops the *whole* record for a registered type, so attaching the exception would have silently deleted that classified failure from the run logs. The two sites in auth/auth.py are suppressed by file rather than edited, since this change was produced by an automated pipeline that may not touch auth modules; the entry documents the deferral. Converting the broad handlers also made nine BLE001 suppressions redundant, so they are dropped. FakeLogger.exception now records alongside error events, matching the real logger's error-level emission. Refs: INBOX-29 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs
"suppressions" is not in the repo's Vale accept vocabulary and appears in no other changelog or release-notes file, so the fragment is reworded rather than growing the shared vocabulary for a single use. Refs: INBOX-29 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs
…c sites by symbol Addresses two review findings on PR #10220. A Starlette ClientDisconnect is raised whenever a client aborts while its request body is being read - routine rather than exceptional - and its traceback only shows the body-read path. Converting it emitted a full ERROR-level stack trace per aborted request. That contradicted the change's own stated criterion (convert unless the traceback would be actively harmful or worthless), so it moves to log.error with a justified noqa alongside the other such sites. Now 27 conversions and 7 in-line suppressions. The spec's task list identified each site by absolute line number, measured before the conversions shifted them; several no longer pointed at a log call at all. Since the spec merges as the archived record, those anchors actively mislead, so sites are now named by enclosing function with a note on how to regenerate the set. Refs: INBOX-29 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs
The post-review revert of the ClientDisconnect handler moved one site from converted to noqa, leaving stale 28/6 figures in plan.md, research.md, tasks.md, alignment-check.md and the implementation report. Verified against the tree: 7 `noqa: TRY400` comments, 2 sites deferred by per-file-ignore in auth/auth.py, 27 converted = 36. The dated critique keeps its original figure, marked as-of, rather than being rewritten after the fact. Addresses review threads on PR #10220. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs
d085964 to
2660959
Compare
|
🤖 platform-health-agent — automated reconcile pass (CI red). Action: rebased onto latest Why a rebase rather than a code fixI checked each of the 5 failures against this diff first. None of them implicate it —
The two UI failures are the informative ones: three of the six commits this branch was missing are frontend token refactors — "tokenize the sheet frame and surface colors", "tokenize primary and muted text colors", "add Card secondary and panel surface variants" — i.e. exactly the surfaces those locators target. Rebase outcomeClean — no conflicts, despite Re-verified locally on the rebased head:
One clarification for anyone re-running the numbers: Nothing in the working tree changed beyond the replay: the diff is byte-for-byte the same set of files as before the rebase. If CI comes back red on the same UI specs from the rebased head, that points at |
CI triage: red, but not from this PRReconcile pass on the red CI. None of the 6 failures is attributable to this change, and I've re-run the failed jobs. Evidence below. Branch is not stale
What each failure actually is
This diff is Python logging calls (
|
PR #10220 (INBOX-29) re-enabled ruff TRY400 across 36 backend logging sites. Its review was unusually dense - two cubic findings, a rebutted release-vehicle finding, and a long CI-triage thread - and investigation turned up five durable gaps. - cubic-dev-ai flagged graphql/app.py's ClientDisconnect site being converted to log.exception when it's a routine, expected condition (the traceback only shows the body-read path). The author's own research.md stated the criterion ("convert unless the traceback would be actively harmful or worthless") and misapplied it on this exact site - meaning the criterion existed only in a throwaway per-PR spec doc, never in the durable guideline. Added a log.exception vs log.error section to exceptions.md with both the worthless case (this one) and the harmful case (cross-linked to webhooks.md's TracebackSuppressionFilter, the mechanism the same PR's webhook site depends on). - cubic also flagged the same PR's tasks.md citing absolute line numbers that its own commits shifted mid-review. That rule already exists almost verbatim in documentation.md ("cite the module path and symbol only... a spec's own line-numbered citations routinely rot before the feature it describes even merges") - so this is "covered but still flagged," not missing. Neither speckit-plan nor speckit-tasks, the skills that actually generate research.md/plan.md/tasks.md, pointed at that rule while enumerating sites. Added a one-line pointer to each. - The same post-review fix (28/6 to 27/7 conversions) went stale in five sibling spec files (plan.md, research.md, tasks.md, alignment-check.md, the implementation report) before cubic finished flagging all five individually. Root AGENTS.md already tells authors to verify a claim against the diff before writing it down; extended that bullet to also require propagating a later correction across every file in dev/specs/<feature>/ that restates the same figure, not just the one a reviewer pointed at. - cubic asked whether this should target stable instead of develop ("repo tooling... cannot affect a running product"). The rebuttal was right (the diff converts 27 runtime logging call sites; the closest precedents targeted develop) but the underlying policy it invoked - "release-vehicle guidance" - doesn't exist anywhere in dev/ or AGENTS.md; grepped for it directly. Added the rule both sides were actually reasoning from into git-workflow.md's Branch Strategy: a tooling/lint/CI change with a runtime source diff targets develop like a product change; only a zero-source-diff change targets stable. - The PR's CI came back red repeatedly on self-hosted-runner exhaustion, and each reconcile pass proved out two techniques monitoring-pull- requests/SKILL.md didn't have: citing a specific run on the base branch as proof a failure predates the PR (rather than an inferred "probably pre-existing"), and re-running a failed job on the same SHA instead of pushing a fix-attempt commit first, since ci.yml sets concurrency.cancel-in-progress: true and a new push cancels the experiment. Added both to Phase 2's existing staleness callout. Not encoded: a Vale spelling-exceptions judgment call (reword a fragment vs. add a one-off word to the shared vocabulary) - no evidence this recurs, and the decision is a generic technical-writing tradeoff rather than an Infrahub-specific gotcha. The "rebase deliberately skipped" call and the runner-exhaustion diagnosis itself are the existing staleness rule working correctly and an infra incident, respectively - not new lessons. Swept the destination layer for citation rot per the skill's step 5; found none to prune. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QQtPSrTWwyKv5U4vpVTNZ
CI triage — both failures are external to this PRAutomated reconcile pass. Two jobs were red on run 31817672397; I've re-run just those two. Neither is caused by this change, and I've made no code changes in this pass.
|
|
🤖 CI note — the red check is a pre-existing flake, not a regression from this PR. Re-ran the failed job; no code changed. Exactly one job failed: The failing test was I did not wave this off as flake on sight, because it fails on a branch-merge call and this PR does touch
No rebase: this branch is 2 commits behind Two things a reviewer may want to pick up separately from this PR:
|
|
🤖 Reconcile pass — the re-run passed. This PR is fully green and unblocked. Closing the loop on my previous comment: the 45 success · 10 skipped · 1 neutral · 0 failed · 0 pending. First fully-green run on this branch. Nothing pushed this pass — no code change, no rebase, no force-push. State
On the 3 commits this branch is behindDeliberately not rebased. All three are frontend-only ( Ready for a human review decision. As always: I don't merge. |
Re-enables the ruff TRY400 (
error-instead-of-exception) rule so alog.errorreporting a caught exception carries its traceback. Jira: INBOX-29.Scope: TRY400 only — this does not close INBOX-29
The card asked for TRY400 and TRY004. Only TRY400 is here.
TRY004's fix changes the exception type a guard raises at 40 sites — 5 in
core/schema/schema_branch.pyand 14 acrossgraphql/mutations/*andgraphql/types/node.py. SwappingValueError/Exception→TypeErrorthere is caller-visible in GraphQL error responses, so it is an API-behaviour decision rather than a lint cleanup. TRY004 stays suppressed and needs a human decision — please split a follow-up card rather than letting this merge mark INBOX-29 Done.What changed
extend-select = ["TRY400"]in[tool.ruff.lint]enables just this rule; the broad"TRY"entry inignorestays, keeping the rest of the family (TRY004 included) suppressed — ruff resolves the more specific selector first. Enumerating the remaining TRY codes instead is not possible:TRY200is a removed rule and naming it breaks ruff.Of the 36 flagged sites:
log.exception— sameerrorlevel, same message, same keyword fields, no control-flow change; the traceback is the only addition.log.errorwith a justified# noqa: TRY400— includinggraphql/app.py→_handle_http_request, where a client aborting mid-request is a routine event whose traceback only shows the body-read path (converted in the first pass, reverted after review).backend/infrahub/auth/auth.py(see below).Bonus: 9 fewer suppressions. BLE001 doesn't flag a blind
except Exceptionwhose handler logs via.exception(), so converting those handlers made 9# noqa: BLE001directives redundant (services/scheduler.py×1,utilities/infrahub_load_tester.py×8). They're gone.The concrete debuggability win:
services/scheduler.py:91, the scheduler's keep-alive handler, previously logged onlystr(exc)— a failing recurring task was undiagnosable by construction.The one site that must not be converted 🔍
backend/infrahub/webhook/tasks/process.py:204deliberately keepslog.error.WebhookDeliveryErroris registered with@suppress_traceback_in_logs, andTracebackSuppressionFilter— installed on the Prefect run loggers this site logs through — drops the entire log record, not merely its traceback, for a registered exception type. Converting this call would have attached aWebhookDeliveryErrorto the record and thereby silently deleted the classified delivery-failure report (status class, message, remediation, attempt, elapsed) from the run logs — while leaving the lint gate green and the diff looking innocuous.ruff --fix --unsafe-fixesproduces exactly that bug. Every site here was read at its call site instead.Related hazard, no action needed now: two converted sites (
git/integrator.py:1568,:1608) catch bareExceptionbehind a run logger. They're safe only becauseWebhookDeliveryErroris the sole registered type and is unreachable from repository-integration code. Registering a second, widely-raised type would silently mute them.Two things to look at closely
backend/infrahub/graphql/app.pyis in the diff — 2 log lines in ASGI/websocket error handling, not the GraphQL contract surface. At line 535 the redundantexc_info=erroris dropped since the call sits insideexcept Exception as error:and.exception()attaches it implicitly. The similar-looking call in_log_error(line 392) is left alone: it runs outside anyexceptblock and must passexc_infoexplicitly. Ruff agrees — it never flagged that one.A new
per-file-ignoresentry forbackend/infrahub/auth/auth.py— added by a change whose whole purpose is removing a suppression, which deserves an explanation. Both of its TRY400 sites are pure logging insideexceptblocks and would convert cleanly; the file is untouched only because this change was produced by an automated pipeline that may not edit auth modules unattended. The merged BLE precedent (Re-enable ruff BLE (blind-except) rule and fix all violations #10002) did edit this same file, adding# noqa: BLE001at the very handlers holding these two sites — so if you'd rather have the 2-line inline fix, drop the entry and convert them. That call is deliberately yours.FakeLogger.exception(backend/tests/adapters/log.py) was a no-op stub, sotest_scheduler_task_with_errorfailed once the scheduler site converted. The fake was wrong, not the test —.exceptionemits at error level, so it now records alongside error events and the existing assertion passes unmodified.Testing
ruff check --select TRY400ruff check .(full, default config)ruff check --select TRY004invoke lintinvoke backend.test-unitvalidate-generated,docs.validate,schema.validate-*,uv lock --checkcore/schema/,core/migrations/,auth/,.github/, no generated fileEvery remaining
# noqa: TRY400carries a one-line reason. Frontend Betterer/codegen not run — no frontend file, GraphQL schema, or generated frontend type is in the diff.Spec, per-site decision table, critique, and full report:
dev/specs/005-ruff-try400-tracebacks/.🤖 Generated with Claude Code