From 184057ad637720b5576ddbcd1827a029947c25e5 Mon Sep 17 00:00:00 2001 From: Saltaferis Dimitrios Date: Tue, 11 Aug 2026 15:08:57 +0000 Subject: [PATCH 1/9] [Spec Kit] Specify TRY400 re-enable, TRY004 scoped out (INBOX-29) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs --- .../checklists/requirements.md | 46 +++++ dev/specs/005-ruff-try400-tracebacks/spec.md | 175 ++++++++++++++++++ 2 files changed, 221 insertions(+) create mode 100644 dev/specs/005-ruff-try400-tracebacks/checklists/requirements.md create mode 100644 dev/specs/005-ruff-try400-tracebacks/spec.md diff --git a/dev/specs/005-ruff-try400-tracebacks/checklists/requirements.md b/dev/specs/005-ruff-try400-tracebacks/checklists/requirements.md new file mode 100644 index 00000000000..f79c0954baf --- /dev/null +++ b/dev/specs/005-ruff-try400-tracebacks/checklists/requirements.md @@ -0,0 +1,46 @@ +# Specification Quality Checklist: Re-enable ruff TRY400 so error logs carry tracebacks + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-08-11 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +**Note on the first two items**: this is a lint/tooling feature, so its "user" is an Infrahub +developer and its subject matter is inherently the lint configuration. Naming ruff, TRY400 and +`pyproject.toml` is describing *what* the change is, not leaking a chosen implementation. The +spec still avoids prescribing per-site edits — those belong to plan/tasks. + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- Scope was narrowed from the source card (INBOX-29 named TRY004 **and** TRY400) to TRY400 only. + The spec documents this in "Out of Scope — TRY004" with the reason: TRY004's fix changes + caller-visible exception types on schema and GraphQL surfaces, which needs human design review. + This is an intentional, recorded scope reduction, not drift. +- Violation counts in the spec were measured on this branch rather than taken from the card + (card said ~56; actual is 76 = 36 TRY400 + 40 TRY004). +- FR-006's auth.py carve-out is a pipeline-permission boundary. The spec's Assumptions section + records that the merged BLE precedent edited the same file, so a reviewer can overrule it. diff --git a/dev/specs/005-ruff-try400-tracebacks/spec.md b/dev/specs/005-ruff-try400-tracebacks/spec.md new file mode 100644 index 00000000000..55cfc5459c7 --- /dev/null +++ b/dev/specs/005-ruff-try400-tracebacks/spec.md @@ -0,0 +1,175 @@ +# Feature Specification: Re-enable ruff TRY400 so error logs carry tracebacks + +**Feature Branch**: `pha/INBOX-29` + +**Created**: 2026-08-11 + +**Status**: Draft + +**Input**: Engineering Inbox card INBOX-29 — "Re-enable ruff TRY004 + TRY400 (~56 violations) to restore tracebacks". Scoped down to TRY400 only; see Out of Scope. + +## Context + +`pyproject.toml` selects every ruff rule (`select = ["ALL"]`) and then suppresses the whole +`TRY` (tryceratops) family in the global `ignore` list, under the "Rules below needs to be +Investigated" banner. The team's suppression-analysis thread ranked re-enabling two of those +rules as priority #4: + +- **TRY400** (`error-instead-of-exception`) — `log.error(...)` called inside an `except` block. + The handler reports that something failed but discards the traceback, so a production error + log names the symptom with no stack to locate the cause. +- **TRY004** (`type-check-without-type-error`) — an `isinstance`-style guard raising something + other than `TypeError`. + +Ground truth measured on this branch (2026-08-11, ruff 0.15.0, `origin/develop` @ `7d3f48635`): +**76 violations — 36 TRY400 + 40 TRY004**. The card's "~56" estimate is stale. + +TRY400 distribution (36 sites): + +| File | Sites | +|------|-------| +| `backend/infrahub/git/integrator.py` | 13 | +| `utilities/infrahub_load_tester.py` | 8 | +| `backend/infrahub/workers/infrahub_async.py` | 2 | +| `backend/infrahub/graphql/app.py` | 2 | +| `backend/infrahub/git/base.py` | 2 | +| `backend/infrahub/auth/auth.py` | 2 | +| `backend/infrahub/webhook/tasks/process.py` | 1 | +| `backend/infrahub/services/scheduler.py` | 1 | +| `backend/infrahub/git/tasks.py` | 1 | +| `backend/infrahub/git/repository.py` | 1 | +| `backend/infrahub/database/__init__.py` | 1 | +| `backend/infrahub/core/merge/orchestrator.py` | 1 | +| `backend/infrahub/core/branch/tasks.py` | 1 | + +This mirrors the already-merged BLE re-enable (card INBOX-19, PR #10002, spec dir +`dev/specs/002-ruff-ble-reenable/`), which used the same shape: remove the suppression, fix +every site, keep a small number of justified `# noqa` escapes. + +## Out of Scope — TRY004 + +TRY004 is deliberately **not** part of this change. Its fix changes the *exception type* a guard +raises (`ValueError`/`Exception` → `TypeError`) at 40 sites, and those sites sit on surfaces this +automated pipeline may not alter without human design review: + +- `backend/infrahub/core/schema/schema_branch.py` — 5 sites (schema surface) +- `backend/infrahub/graphql/mutations/*` and `backend/infrahub/graphql/types/node.py` — 14 sites + (the exception type is caller-visible in GraphQL error responses) + +Changing what a caller catches is a behavioural change, not a lint cleanup. TRY004 stays +suppressed and is escalated to a human on INBOX-29. Splitting it out keeps this change +reviewable as pure logging enrichment. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - An error log names the failure *and* locates it (Priority: P1) + +As an engineer debugging a production incident, when a caught exception is logged by any of the +converted handlers, the log record carries the exception and its traceback, so I can see where +the failure originated instead of only that it happened. + +**Why this priority**: This is the card's stated value — restoring debuggability of production +errors. It is delivered by the site conversions alone, independent of the rule being enforced. + +**Independent Test**: Trigger any converted handler (e.g. force a failure in the git +integrator), inspect the emitted log record, and confirm it contains exception/traceback +information that the pre-change `log.error` call did not emit. + +**Acceptance Scenarios**: + +1. **Given** a handler converted to `log.exception`, **When** it runs with an active exception, + **Then** the emitted record carries the exception info (traceback) at the same `error` level + and with the same message and keyword fields as before. +2. **Given** any converted site, **When** the surrounding code path is exercised, **Then** + control flow is unchanged — nothing is newly raised, suppressed, or re-ordered. + +--- + +### User Story 2 - TRY400 enforcement is active for future code (Priority: P2) + +As an Infrahub developer, when I add a new `log.error(...)` inside an `except` block, the lint +gate rejects it, so this class of traceback-losing handler cannot re-accumulate. + +**Why this priority**: Without turning the rule on, today's 36 fixes silently regress. The +durable value is the gate, but it depends on Story 1 being complete first. + +**Independent Test**: With the rule enabled, add a temporary `log.error` inside an `except` +block and run the lint gate — it must fail with TRY400; remove it — it must pass. + +**Acceptance Scenarios**: + +1. **Given** TRY400 is selected in `pyproject.toml`, **When** the repo lint gate runs, **Then** + it reports zero TRY400 violations. +2. **Given** the rule is active, **When** a developer adds an unjustified `log.error` inside an + `except` block, **Then** the lint gate fails pointing at that line. +3. **Given** TRY400 is selected, **When** lint runs, **Then** the other `TRY` rules — TRY004 + included — remain suppressed and report nothing. + +### Edge Cases + +- **A `log.error` inside an `except` block that is not reporting the active exception.** A + validation branch can sit lexically inside a handler while describing a different condition; + attaching a traceback there would be misleading. Such a site keeps `log.error` with a + targeted `# noqa: TRY400` and a one-line reason rather than taking a wrong conversion. +- **A site in a module this pipeline may not edit.** `backend/infrahub/auth/auth.py` is an auth + module and off-limits to the automated pipeline, so its 2 sites cannot be converted here; + they are suppressed by file and handed to a human (see Assumptions). +- **Tests asserting on captured log records.** A test pinning a site's log level or absence of + exception info could fail after conversion; such assertions are updated to match. +- **`ruff --fix` for this rule is unsafe-only.** Autofix is not trusted blind; every conversion + is reviewed at its call site. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: TRY400 MUST be enforced repo-wide by the lint configuration, while every other + `TRY` rule — including TRY004 — remains suppressed. +- **FR-002**: The configuration change MUST NOT touch dependency declarations in + `pyproject.toml`. +- **FR-003**: Every TRY400 site outside `backend/infrahub/auth/auth.py` MUST either be converted + to `log.exception` or carry a justified `# noqa: TRY400`. +- **FR-004**: A conversion MUST preserve the call's message text and all keyword fields exactly; + these are structlog-style calls, not `%`-formatting. +- **FR-005**: A conversion MUST NOT alter control flow — no change to what is raised, caught, + re-raised, or returned. +- **FR-006**: `backend/infrahub/auth/auth.py` MUST NOT be modified. Its 2 TRY400 sites MUST be + suppressed via a commented `per-file-ignores` entry that names INBOX-29 and the reason. +- **FR-007**: The change MUST NOT touch DB schema or migrations, GraphQL/REST contract surfaces, + auth behaviour, CI workflows, or any generated file. +- **FR-008**: A changelog fragment MUST be added if and only if the repo's towncrier conventions + call for one for an internal lint/logging change. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: The repo lint gate reports **zero** TRY400 violations, down from 36. +- **SC-002**: The lint gate reports no new violations of any other rule — the only rule whose + enforced set changes is TRY400. +- **SC-003**: TRY004 remains suppressed: its 40 pre-existing violations are unchanged, and no + file it flags is touched by this change. +- **SC-004**: `uv run invoke format` and `uv run invoke lint` are both clean. +- **SC-005**: The changed-file list contains no path under `backend/infrahub/core/schema/`, + `backend/infrahub/core/migrations/`, `backend/infrahub/auth/`, `.github/`, and no generated + file listed in `AGENTS.md`. +- **SC-006**: Backend unit tests covering the touched modules pass. +- **SC-007**: Every remaining `# noqa: TRY400` in the tree carries a one-line justification. + +## Assumptions + +- **`extend-select` is the mechanism.** Adding `extend-select = ["TRY400"]` under + `[tool.ruff.lint]` re-enables only TRY400 while the broad `"TRY"` entry in `ignore` keeps the + rest suppressed — ruff resolves the more specific selector first. Verified empirically on this + branch. Enumerating the individual TRY codes in `ignore` instead is **not** viable: `TRY200` + is a removed rule and naming it breaks ruff. +- **`log.exception` is level-equivalent.** It emits at `error` level and attaches exception + info; it does not change severity, so log-level-based alerting is unaffected. +- **The auth.py carve-out is a pipeline boundary, not a technical one.** Both sites are pure + logging inside `except` blocks and would convert cleanly. The merged BLE precedent (PR #10002) + *did* edit this same file, adding `# noqa: BLE001` at the very handlers holding these two + sites — so a reviewer may reasonably prefer the 2-line inline fix and drop the per-file + ignore. That call is left to a human because the automated pipeline is not permitted to edit + auth modules unattended. +- **Only TRY400's enforced set changes.** No other suppression is added or removed, so the + review surface stays proportionate to a logging cleanup. From c7b6f0e54197e20b6668a3f8af3aab9e663b4537 Mon Sep 17 00:00:00 2001 From: Saltaferis Dimitrios Date: Tue, 11 Aug 2026 15:12:37 +0000 Subject: [PATCH 2/9] [Spec Kit] Plan + Phase 0 research: per-site TRY400 decisions (INBOX-29) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs --- dev/specs/005-ruff-try400-tracebacks/plan.md | 179 ++++++++++++++++++ .../005-ruff-try400-tracebacks/research.md | 129 +++++++++++++ 2 files changed, 308 insertions(+) create mode 100644 dev/specs/005-ruff-try400-tracebacks/plan.md create mode 100644 dev/specs/005-ruff-try400-tracebacks/research.md diff --git a/dev/specs/005-ruff-try400-tracebacks/plan.md b/dev/specs/005-ruff-try400-tracebacks/plan.md new file mode 100644 index 00000000000..d8cf24073e7 --- /dev/null +++ b/dev/specs/005-ruff-try400-tracebacks/plan.md @@ -0,0 +1,179 @@ +# Implementation Plan: Re-enable ruff TRY400 so error logs carry tracebacks + +**Branch**: `pha/INBOX-29` | **Date**: 2026-08-11 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `dev/specs/005-ruff-try400-tracebacks/spec.md` + +## Summary + +Enable ruff's TRY400 (`error-instead-of-exception`) repo-wide via `extend-select`, leaving the +rest of the suppressed `TRY` family — TRY004 included — untouched, then resolve all 36 flagged +sites: **28 converted** to `log.exception`, **6 kept as `log.error` with a justified +`# noqa: TRY400`**, and **2 suppressed by file** because they live in +`backend/infrahub/auth/auth.py`, which this pipeline may not edit. Per-site decisions and their +reasoning are in [research.md](./research.md) §R4. + +## Technical Context + +**Language/Version**: Python 3.14 + +**Primary Dependencies**: ruff 0.15.0 (lint gate), structlog (`structlog.stdlib.BoundLogger` via +`infrahub.log.get_logger`), Prefect run loggers, stdlib `logging` + +**Storage**: N/A + +**Testing**: pytest 9.0 — `uv run invoke backend.test-unit` for the touched modules + +**Target Platform**: Linux server (backend) + repo tooling under `utilities/` + +**Project Type**: Backend service + repo tooling; lint-configuration change + +**Performance Goals**: N/A — no hot path touched. `log.exception` formats a traceback only when a +handler actually emits the record. + +**Constraints**: No behavioural change. Same log level, same message, same keyword fields, same +control flow. No DB schema/migration, GraphQL/REST contract, auth, dependency, CI, or generated-file +changes. + +**Scale/Scope**: 1 config file + 12 source files; 36 TRY400 sites (28 conversions, 6 in-line +suppressions, 2 file-level suppressions). + +## Constitution Check + +*GATE: evaluated before Phase 0 research and re-checked after design.* + +| Principle | Assessment | +|-----------|-----------| +| I. Schema-Driven Integrity | **PASS** — no schema, no generated files. `core/schema/` is explicitly excluded (it only holds TRY004 sites). | +| II. Branch-Safe by Default | **PASS** — no queries, no branch/temporal logic touched. | +| III. Type Safety & Explicit Contracts | **PASS** — no signatures or types change. Strengthens observability of the existing contracts. | +| IV. Test Discipline | **PASS with note** — a logging-call substitution has no new behaviour to test; the guard is the lint gate itself (SC-001/002) plus the existing unit suite proving no regression. Adding tests that assert on log internals at 28 sites would be test-for-test's-sake. Recorded in Complexity Tracking. | +| V. Query Performance & Efficiency | **PASS** — no query changes. | +| VI. Security & Input Boundaries | **PASS** — `auth/` is untouched by construction (FR-006). No new data enters a log record beyond the traceback of an already-caught exception. | +| VII. Simplicity & Maintainability | **PASS** — net simplification: removes a category-wide suppression for one rule and replaces implicit traceback loss with either a traceback or an explicit justified exception. | + +No violations requiring justification beyond the Test Discipline note below. + +## Project Structure + +### Documentation (this feature) + +```text +dev/specs/005-ruff-try400-tracebacks/ +├── spec.md # Phase -1 output +├── plan.md # This file +├── research.md # Phase 0 output — per-site decision table +├── tasks.md # Phase 2 output (/speckit-tasks) +└── checklists/ + └── requirements.md # Spec quality checklist +``` + +`data-model.md`, `contracts/`, and `quickstart.md` are **N/A** for this feature — there is no +data model, no API surface, and no user-facing flow to walk through. See research.md +"Not applicable". They are deliberately not created rather than filled with invented content. + +### Source Code (repository root) + +```text +pyproject.toml # [tool.ruff.lint] extend-select + per-file-ignores + +backend/infrahub/ +├── core/ +│ ├── branch/tasks.py # 1 convert +│ └── merge/orchestrator.py # 1 convert +├── database/__init__.py # 1 convert +├── git/ +│ ├── base.py # 2 convert +│ ├── integrator.py # 9 convert + 4 noqa +│ ├── repository.py # 1 convert +│ └── tasks.py # 1 convert +├── graphql/app.py # 2 convert (ASGI error handling only) +├── services/scheduler.py # 1 convert +├── webhook/tasks/process.py # 1 noqa (see research.md §R3) +├── workers/infrahub_async.py # 1 convert + 1 noqa +└── auth/auth.py # NOT MODIFIED — 2 sites suppressed by file + +utilities/infrahub_load_tester.py # 8 convert + +changelog/ # towncrier fragment +``` + +**Structure Decision**: no structural change. The edit set is the union of TRY400's current +violation sites plus the lint config, and nothing else. + +## Implementation Approach + +### Step 1 — Turn the rule on first + +Add to `[tool.ruff.lint]` in `pyproject.toml`: + +```toml +extend-select = [ + "TRY400", # error-instead-of-exception — enabled ahead of the rest of TRY (INBOX-29) +] +``` + +and annotate the existing `"TRY"` ignore entry so the two are not read as contradictory. Turning +the rule on *before* fixing sites makes ruff the worklist: the remaining violation count is the +progress meter, and reaching zero is the completion signal. + +### Step 2 — Fix sites file by file, following research.md §R4 + +Each file is independent, so files can be done in any order. For every site: read the handler, +apply the §R4 decision, preserve message and keyword arguments exactly. `ruff --fix` for TRY400 +is **unsafe-fix-only** and is not used — the 6 noqa sites and the `graphql/app.py:535` +`exc_info` removal are exactly the judgements an autofix gets wrong (see §R3, §R5). + +Each `# noqa: TRY400` carries a one-line reason on the same line or immediately above, so SC-007 +holds and the next reader does not have to re-derive the call. + +### Step 3 — Suppress the auth carve-out by file + +Add to `[tool.ruff.lint.per-file-ignores]`: + +```toml +"backend/infrahub/auth/auth.py" = [ + # TRY400 deferred to a human (INBOX-29): both sites are pure logging inside except blocks + # and would convert cleanly, but the automated pipeline may not edit auth modules. + "TRY400", +] +``` + +**This is the one part of the plan a reviewer is most likely to want changed**, and that is +deliberate: the merged BLE precedent (PR #10002) edited this same file, adding `# noqa: BLE001` +at the very handlers holding these two sites. Dropping this entry and converting the two calls +inline is a ~2-line follow-up. The plan defers rather than decides because an unattended agent +should not be the one to touch auth code. + +### Step 4 — Changelog fragment + +Add a towncrier fragment following whatever `changelog/` and the towncrier config establish for +an internal lint/logging change, matching what the BLE precedent did (research.md §R6). + +### Step 5 — Verify against the success criteria + +```bash +.venv/bin/ruff check --no-cache . # SC-001, SC-002: zero TRY400, nothing new +.venv/bin/ruff check --select TRY004 --no-cache . # SC-003: still 40, unchanged +uv run invoke format && uv run invoke lint # SC-004 +git diff --name-only origin/develop...HEAD # SC-005: no gated path +uv run invoke backend.test-unit # SC-006 (touched modules) +grep -rn 'noqa: TRY400' # SC-007: every one justified +``` + +## Risks + +| Risk | Mitigation | +|------|-----------| +| A conversion silently drops a log record via `TracebackSuppressionFilter` | Identified at `webhook/tasks/process.py:204` and excluded (research.md §R3). The registered-type set was enumerated — `WebhookDeliveryError` is the only member, and that site is its only catch site. | +| A blind autofix mangles a call | Autofix not used; all 36 sites read individually. | +| A test asserts on log level or exception info | Level is unchanged (`.exception` emits at `error`). Unit suite for touched modules is run; any assertion on exception info is updated (research.md §R7). | +| `extend-select` does not override the prefix `ignore` | Verified empirically before planning (research.md §R1). | +| Scope creep into TRY004 | The `ignore` entry for `TRY` stays; SC-003 asserts TRY004's 40 violations are untouched. | + +## Complexity Tracking + +| Violation | Why Needed | Simpler Alternative Rejected Because | +|-----------|------------|-------------------------------------| +| No new tests for 28 changed call sites (Principle IV) | The change is a level-preserving logging substitution with no new behaviour. The lint gate (SC-001/002) is the durable regression guard, and the existing unit suite proves nothing broke. | Asserting on captured log records at each site would pin implementation detail of logging calls, be brittle to message edits, and test structlog rather than Infrahub. | +| A new `per-file-ignores` entry added by a change whose purpose is *removing* a suppression | `backend/infrahub/auth/auth.py` is off-limits to the automated pipeline, but the rule must still be enforceable repo-wide. | Editing the 2 auth sites inline is the better end state and is what the reviewer will likely ask for — but it requires a human to own the auth-module change. Leaving the rule fully off instead would forfeit the other 34 sites. | diff --git a/dev/specs/005-ruff-try400-tracebacks/research.md b/dev/specs/005-ruff-try400-tracebacks/research.md new file mode 100644 index 00000000000..4b1adcb812d --- /dev/null +++ b/dev/specs/005-ruff-try400-tracebacks/research.md @@ -0,0 +1,129 @@ +# Phase 0 Research: TRY400 re-enable + +**Feature**: `dev/specs/005-ruff-try400-tracebacks/` | **Date**: 2026-08-11 | ruff 0.15.0 + +## R1 — How to enable TRY400 alone while the rest of `TRY` stays suppressed + +**Decision**: add `extend-select = ["TRY400"]` under `[tool.ruff.lint]`, leaving the broad +`"TRY"` entry in `ignore` untouched. + +**Why**: ruff resolves rule selection by specificity — a selector naming an exact code beats a +prefix-level `ignore`. Verified empirically on this branch: with `extend-select = ["TRY400"]` in +place, `ruff check backend/infrahub/git/integrator.py` reported its 13 TRY400 violations while +TRY004 (and every other `TRY` rule) stayed silent. + +**Alternative rejected — enumerate the remaining `TRY` codes in `ignore`**: this reads more +explicitly but is not viable. ruff 0.15 lists `TRY200` as a removed rule; naming a removed rule +in the config is an error, so the enumeration would break the lint run. `extend-select` also +keeps the diff to a single added block. + +## R2 — Is `log.exception` a safe substitution for `log.error` here? + +`get_logger()` (`backend/infrahub/log.py:66`) returns a `structlog.stdlib.BoundLogger`; some +sites instead use a stdlib `logging.Logger` (`utilities/infrahub_load_tester.py`) or a Prefect +run logger (`get_run_logger()`). All three expose `.exception(...)`, which emits at **`error`** +level with `exc_info` attached. So the substitution: + +- does not change severity — level-based alerting and log filters are unaffected; +- does not change control flow; +- adds the traceback of the currently-handled exception to the record. + +**Keyword arguments are preserved verbatim.** These are structlog-style calls carrying +`repository=`, `error=`, `extra={...}` etc., not `%`-style formatting, so no format-string +rewriting is involved. + +## R3 — The traceback-suppression filter makes one conversion actively harmful + +`backend/infrahub/log.py` defines `suppress_traceback_in_logs` and +`TracebackSuppressionFilter`. Per the filter's own contract: + +> Per the logging filter contract, returning `False` discards the whole record, not only its +> traceback. + +The filter is installed on the Prefect run loggers (`log.py:93-95`, +`PREFECT_RUN_LOGGERS = ("prefect.flow_runs", "prefect.task_runs")`), and +`WebhookDeliveryError` is registered via `@suppress_traceback_in_logs` +(`backend/infrahub/webhook/classifier.py:65`). + +`backend/infrahub/webhook/tasks/process.py:204` logs a curated, operator-facing +delivery-failure message (status class, message, remediation, attempt, elapsed) through +`log = get_run_logger()` inside `except WebhookDeliveryError`. Converting that call to +`log.exception` would attach a `WebhookDeliveryError` to the record, the filter would match it, +and **the whole record — the curated message included — would be dropped**. The failure would +silently stop being reported. + +**Decision**: this site keeps `log.error` with `# noqa: TRY400`. This is precisely the failure +mode a blind `--fix --unsafe-fixes` run would have introduced, and it is invisible in a diff +review. + +## R4 — Per-site decision: convert (28) vs. justified `# noqa` (6) + +All 34 in-scope sites were read at their call site. The rule applied: **convert unless the +traceback would be actively harmful or worthless.** + +### Keep `log.error` + `# noqa: TRY400` (6) + +| Site | Reason | +|------|--------| +| `webhook/tasks/process.py:204` | Traceback attachment makes `TracebackSuppressionFilter` drop the entire record — see R3. | +| `git/integrator.py:456` | Inside `for error in exc.errors():` — one line per Pydantic validation error in a user's `.infrahub.yml`. The traceback would repeat identically for every error and adds nothing to actionable user-facing validation feedback. | +| `git/integrator.py:638` | Same per-error validation loop, artifact-definition variant. | +| `git/integrator.py:459` | `log.error(exc.message)` for the SDK `ValidationError` paired with 456's handler; user-config validation feedback, then `continue`. | +| `git/integrator.py:641` | Same as 459, artifact-definition variant. | +| `workers/infrahub_async.py:194` | "missing configuration for internal_address" then a clean `typer.Exit(1)`. A pure configuration error — there is nothing in the traceback to diagnose. | + +### Convert to `log.exception` (28) + +| Site | Handled exception | Note | +|------|-------------------|------| +| `core/branch/tasks.py:119` | `MigrationFailureError` | re-raises; traceback locates the failing migration | +| `core/merge/orchestrator.py:152` | `BaseException` | rollback path — highest-value traceback in the set | +| `database/__init__.py:446` | `ServiceUnavailable` | wraps into `DatabaseError` | +| `git/base.py:619` | `GitCommandError` (unexpected status) | the expected status-1 case already returns earlier | +| `git/base.py:902` | `GitCommandError` | proceeds after reporting | +| `git/integrator.py:810` | `yaml.YAMLError` | traceback carries the parse position | +| `git/integrator.py:825` | `PydanticValidationError` | single report, then raises | +| `git/integrator.py:947` | `InfrahubSdkError` | re-raises | +| `git/integrator.py:1568` | `Exception` | re-raises; broad catch — traceback essential | +| `git/integrator.py:1608` | `Exception` | as above | +| `git/integrator.py:1896`, `:1903` | `ModuleNotFoundError`, `AttributeError` | loading a user check module → `CheckError` | +| `git/integrator.py:1968`, `:1975` | `ModuleNotFoundError`, `AttributeError` | loading a user transform → `TransformError` | +| `git/repository.py:416` | `GitCommandError` | nested ref lookup, then raises | +| `git/tasks.py:1217` | `CheckError` | check failed to run | +| `graphql/app.py:195` | `ClientDisconnect` | reports the active exception | +| `graphql/app.py:535` | `Exception` (non-`GraphQLError`) | see R5 | +| `services/scheduler.py:91` | `Exception` (keep-alive) | currently logs only `str(exc)`; a failing recurring task was undiagnosable | +| `workers/infrahub_async.py:202` | `SdkError` | a communication failure — traceback distinguishes refused / timeout / TLS | +| `utilities/infrahub_load_tester.py:49, 72, 88, 113, 119, 145, 156, 174` (8) | `Exception` (best-effort loops) | stdlib logger; load-test tooling | + +## R5 — `graphql/app.py:535` already passes `exc_info` + +The call is `self.logger.error("An exception occurred in resolvers", exc_info=error)` — it +already preserves the traceback, so TRY400 is flagging the idiom rather than a real loss. Inside +`except Exception as error`, `error` **is** the active exception, so +`self.logger.exception("An exception occurred in resolvers")` is equivalent and drops a now +redundant keyword. + +**Decision**: convert and remove the redundant `exc_info=error`. This is the one site where an +argument is deliberately not preserved verbatim; the emitted record is unchanged. + +## R6 — Changelog convention + +`changelog/` holds towncrier fragments named `..md`. The merged BLE precedent +(INBOX-19, PR #10002) added a fragment for the equivalent internal lint change, so this change +follows suit for consistency. Confirmed against `changelog/` contents and towncrier config at +implementation time. + +## R7 — Test exposure + +`log.exception` vs `log.error` can matter to a test asserting on captured log records (level, +or the absence of `exc_info`). Both remain `error` level, so a level assertion still holds; a +test asserting no exception info at a converted site would need updating. Checked during +implementation via the backend unit suite for the touched modules. + +## Not applicable + +- **`data-model.md`** — N/A. No entities, no persisted state, no schema. +- **`contracts/`** — N/A. No API surface changes; that is the explicit reason TRY004 was scoped out. +- **`quickstart.md`** — N/A. Verification is `uv run invoke lint` plus the unit suite; there is + no user-facing flow to walk through. From e7e684d9a69877a2eac7ba8c5decaf4173132857 Mon Sep 17 00:00:00 2001 From: Saltaferis Dimitrios Date: Tue, 11 Aug 2026 15:14:52 +0000 Subject: [PATCH 3/9] [Spec Kit] Critique + tasks: 15 tasks, TRY004 handoff obligations (INBOX-29) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs --- .../critiques/critique-2026-08-11.md | 120 +++++++++++++ dev/specs/005-ruff-try400-tracebacks/tasks.md | 162 ++++++++++++++++++ 2 files changed, 282 insertions(+) create mode 100644 dev/specs/005-ruff-try400-tracebacks/critiques/critique-2026-08-11.md create mode 100644 dev/specs/005-ruff-try400-tracebacks/tasks.md diff --git a/dev/specs/005-ruff-try400-tracebacks/critiques/critique-2026-08-11.md b/dev/specs/005-ruff-try400-tracebacks/critiques/critique-2026-08-11.md new file mode 100644 index 00000000000..b0276cc76d9 --- /dev/null +++ b/dev/specs/005-ruff-try400-tracebacks/critiques/critique-2026-08-11.md @@ -0,0 +1,120 @@ +# Dual-Lens Critique: TRY400 re-enable (INBOX-29) + +**Date**: 2026-08-11 | **Against**: spec.md, plan.md, research.md | **Verdict**: ✅ PROCEED + +--- + +## Lens 1 — Product Strategy + +### 🎯 Must-Address + +**P1. Merging this PR must not close INBOX-29.** The card asks for TRY004 **and** TRY400. This +change delivers TRY400 only, for a documented reason. The drain pipeline's normal behaviour is to +transition a card to **Done** when its PR merges — which would silently retire an unaddressed +half (40 TRY004 violations, the ones with real design questions attached). + +**Applied**: spec.md already records the split in "Out of Scope — TRY004". Added obligation on +the delivery step: the Jira comment accompanying the PR must state prominently that this closes +only the TRY400 half and that TRY004 needs a human decision plus its own card, so the human +merging can choose whether to keep INBOX-29 open or split it. Tracked as task T012. + +**P2. State the partial coverage honestly.** The card's headline value is "restores +debuggability of production errors". 8 of 36 sites (6 in-line noqa + 2 auth) do **not** gain a +traceback — 22%. The PR body must say 28/36 rather than implying a clean sweep. + +**Applied**: added to the delivery task (T011) as an explicit PR-body requirement. + +### 💡 Recommendations + +**P3. 8 of the 28 conversions are in `utilities/infrahub_load_tester.py`** — dev tooling, not +production. Their debuggability value is near zero; they are converted because the gate is +repo-wide. Not a problem, but the PR body should not count them as production wins. +**Accepted** — folded into P2's phrasing requirement. + +**P4. `services/scheduler.py:91` is the sleeper win.** A failing recurring task previously logged +only `str(exc)` inside a keep-alive handler — undiagnosable by construction. Worth calling out in +the PR body as the concrete example of the value. **Accepted.** + +### 🤔 Questions + +**P5. Should the auth carve-out simply be done inline?** Resolved by judgement, recorded in +plan.md Step 3: no — an unattended agent should not edit auth modules, even non-behaviourally. +The plan makes the reversal a ~2-line reviewer action and names the precedent (PR #10002) that +argues for it. The human decides. + +--- + +## Lens 2 — Engineering Risk + +### 🎯 Must-Address + +**E1. Broad-catch sites logging through a Prefect run logger are structurally exposed to +`TracebackSuppressionFilter`.** Beyond the confirmed `webhook/tasks/process.py:204` case +(research.md §R3), two *converted* sites catch bare `Exception` **and** log via +`get_run_logger()`: + +- `git/integrator.py:1568` (`get_check_definition`) +- `git/integrator.py:1608` (`get_python_transforms`) + +For these, `log.exception` attaches whatever was caught. If that exception is ever of a type +registered via `@suppress_traceback_in_logs`, the filter drops the **entire record** and the +failure stops being reported. + +**Assessed**: not reachable today. The registered set has exactly one member, +`WebhookDeliveryError`, raised only inside `webhook_post` during webhook delivery — it cannot +arise while loading a check definition or python transform from a repository worktree. Matching +is by exact type, so subclasses do not widen it either. + +**Applied**: accepted as a documented residual risk rather than a blocker (converting these +sites is where the traceback matters most — they are the broadest catches in the set). Added to +plan.md Risks in substance via research.md §R3, and recorded here as the durable hazard: +**`log.exception` at a broad-catch site behind a run logger is only safe as long as the +suppressed-type set stays narrow.** This is a codebase-wide trap worth codifying, not a defect +in this change — flagged for the conventions/harvest pass. + +**E2. `graphql/app.py:535` — verify `error` is still the active exception at log time.** +Dropping `exc_info=error` in favour of `.exception()` is only equivalent if `sys.exc_info()` is +that same object when the call runs. + +**Verified**: the call sits at line 535, inside `except Exception as error:` (533), guarded by +`if not isinstance(error, GraphQLError):` (534). The reassignment `error = GraphQLError(...)` +happens at 536 — **after** the log call — and no `await` intervenes between the `except` and the +log. Equivalent. Recorded in research.md §R5. + +### 💡 Recommendations + +**E3. Turn the rule on before fixing sites**, so ruff's remaining count is the worklist and +"zero" is the completion signal, rather than hand-tracking 36 line numbers. **Applied** — +plan.md Step 1, tasks ordered accordingly (T001 first). + +**E4. Do not use `ruff --fix --unsafe-fixes`.** The 6 noqa judgements, the `exc_info` removal, +and the dropped-record hazard are exactly what an autofix flattens. **Applied** — plan.md Step 2 +states it; T002–T010 are manual per-file edits. + +**E5. `extend-select` + a prefix `ignore` is subtle enough to be misread as contradictory.** +A future reader may "fix" the apparent conflict by deleting one of them. **Applied** — plan.md +Step 1 requires a comment on both the `extend-select` entry and the `"TRY"` ignore entry. + +**E6. Assert TRY004 is untouched, don't assume it.** A mistake in the selector could quietly +enable 40 more violations or disable the family. **Applied** — SC-003 is a verification command +in plan.md Step 5 and its own task (T013). + +### 🤔 Questions + +**E7. Could `.exception` change log volume or cost on a hot path?** No — no converted site is on +a per-request hot path except `graphql/app.py:195` (`ClientDisconnect`), which fires once per +disconnect and already logged at error level. Traceback formatting happens only when a record is +actually emitted. + +**E8. Any test pinning log level or exception info at a converted site?** Unknown until the +suite runs; both remain `error` level so a level assertion still passes. Covered by T013's unit +run (research.md §R7). + +--- + +## Outcome + +No 🛑 RETHINK findings. Two Must-Address items on the product side (P1, P2) are process/ +communication obligations folded into the delivery tasks; the two on the engineering side (E1, +E2) were investigated and resolved as verified-safe with E1 recorded as a documented residual +risk and a harvest candidate. Spec and plan needed no structural revision. diff --git a/dev/specs/005-ruff-try400-tracebacks/tasks.md b/dev/specs/005-ruff-try400-tracebacks/tasks.md new file mode 100644 index 00000000000..e0a12a7ff2e --- /dev/null +++ b/dev/specs/005-ruff-try400-tracebacks/tasks.md @@ -0,0 +1,162 @@ +# Tasks: Re-enable ruff TRY400 so error logs carry tracebacks + +**Feature**: `dev/specs/005-ruff-try400-tracebacks/` | **Branch**: `pha/INBOX-29` + +**Input**: [spec.md](./spec.md), [plan.md](./plan.md), [research.md](./research.md), +[critiques/critique-2026-08-11.md](./critiques/critique-2026-08-11.md) + +Per-site decisions are **research.md §R4** — that table is the authority for every edit below. +Do not re-derive it, and do not use `ruff --fix` (§R4, critique E4). + +--- + +## Phase 1 — Turn the rule on (worklist generator) + +### T001 — Enable TRY400 in `pyproject.toml` + +- Add `extend-select = ["TRY400"]` under `[tool.ruff.lint]` with the comment + `# error-instead-of-exception — enabled ahead of the rest of TRY (INBOX-29)`. +- Annotate the existing `"TRY"` entry in `ignore` so the pair does not read as contradictory + (critique E5). +- Do **not** touch dependency lists (FR-002). +- **Verify**: `.venv/bin/ruff check --select TRY400 --no-cache .` reports 36 violations, and + `--select TRY004` still reports 40 (FR-001, SC-003). + +From here, `ruff check --select TRY400` is the worklist; Phase 2 is done when it reports 0. + +--- + +## Phase 2 — Fix the sites (T002–T010 are independent; any order, parallelizable) + +Each task: apply research.md §R4, preserve message text and all keyword arguments exactly +(FR-004), change no control flow (FR-005), and give every `# noqa: TRY400` a one-line reason +(SC-007, FR-003). + +### T002 [P] — `backend/infrahub/git/integrator.py` — 9 convert + 4 noqa + +- **Convert** lines 810, 825, 947, 1568, 1608, 1896, 1903, 1968, 1975. +- **noqa** lines 456, 638 (per-error Pydantic validation loop) and 459, 641 (paired SDK + `ValidationError` on user config). +- Largest single file (13 of 36 sites); do it first if serializing. + +### T003 [P] — `utilities/infrahub_load_tester.py` — 8 convert + +- Lines 49, 72, 88, 113, 119, 145, 156, 174. Stdlib `logging.Logger`; `.exception` exists. + +### T004 [P] — `backend/infrahub/graphql/app.py` — 2 convert + +- Line 195 (`ClientDisconnect`) — straight conversion. +- Line 535 — convert **and remove the now-redundant `exc_info=error`** (research.md §R5). + This is the only site where an argument is intentionally not preserved; verify the reassignment + at line 536 still happens after the log call (critique E2). +- ASGI error handling only — **not** the GraphQL contract surface. Called out in the PR body + (T011). + +### T005 [P] — `backend/infrahub/git/base.py` — 2 convert + +- Lines 619, 902 (both `GitCommandError`). + +### T006 [P] — `backend/infrahub/workers/infrahub_async.py` — 1 convert + 1 noqa + +- **noqa** line 194 — pure configuration error ("missing configuration for internal_address") + followed by a clean `typer.Exit(1)`; nothing in a traceback to diagnose. +- **Convert** line 202 — `SdkError` communication failure; the traceback distinguishes refused / + timeout / TLS. + +### T007 [P] — `backend/infrahub/webhook/tasks/process.py` — 1 noqa + +- Line 204: keep `log.error`, add `# noqa: TRY400` whose reason states that attaching the + exception makes `TracebackSuppressionFilter` drop the whole record, because + `WebhookDeliveryError` is registered via `@suppress_traceback_in_logs` and `log` is a Prefect + run logger. **Do not convert this one** (research.md §R3 — the highest-consequence decision in + the change). + +### T008 [P] — `backend/infrahub/core/` — 2 convert + +- `branch/tasks.py:119` (`MigrationFailureError`), `merge/orchestrator.py:152` (`BaseException`, + rollback path). + +### T009 [P] — `backend/infrahub/git/` remainder — 2 convert + +- `repository.py:416`, `tasks.py:1217`. + +### T010 [P] — remaining single sites — 2 convert + +- `database/__init__.py:446` (`ServiceUnavailable`), `services/scheduler.py:91` (keep-alive loop + — the change's clearest win, critique P4). + +### T010b — auth carve-out (config only; `auth/auth.py` MUST NOT be edited) + +- Add a `[tool.ruff.lint.per-file-ignores]` entry for `"backend/infrahub/auth/auth.py"` listing + `"TRY400"`, commented with the INBOX-29 deferral reason (FR-006, plan.md Step 3). +- **Verify**: `git diff --name-only` never lists `backend/infrahub/auth/auth.py` (SC-005). + +--- + +## Phase 3 — Changelog + +### T011a — Towncrier fragment + +- Inspect `changelog/` and the towncrier config; add a fragment matching the convention the BLE + precedent (PR #10002) used for an equivalent internal lint change. Skip only if the convention + genuinely excludes internal-only changes (FR-008, research.md §R6). + +--- + +## Phase 4 — Verify (gates; all must pass before delivery) + +### T012 — Lint and format gates + +- `.venv/bin/ruff check --no-cache .` → **zero** TRY400, no new violations of any other rule + (SC-001, SC-002). +- `.venv/bin/ruff check --select TRY004 --no-cache .` → still **40**, unchanged (SC-003, + critique E6). +- `uv run invoke format` and `uv run invoke lint` → clean (SC-004). +- `grep -rn 'noqa: TRY400'` → every occurrence carries a justification (SC-007). + +### T013 — Tests and governance diff check + +- `uv run invoke backend.test-unit` for the touched modules; update any assertion pinning log + level or exception info at a converted site (SC-006, research.md §R7, critique E8). +- `git diff --name-only origin/develop...HEAD` → contains no path under + `backend/infrahub/core/schema/`, `backend/infrahub/core/migrations/`, `backend/infrahub/auth/`, + `.github/`, and no generated file listed in `AGENTS.md` (SC-005, FR-007). + +--- + +## Phase 5 — Deliver + +### T014 — Open the PR (honest framing required) + +Per critique P2/P3/P4, the PR body MUST: + +- state **28 of 36** sites converted, not a clean sweep — 6 justified in-line `# noqa` + 2 + deferred in `auth/auth.py`; +- lead with the `webhook/tasks/process.py:204` finding (converting it would have silently + deleted the delivery-failure log record) — it is the reason this was not an autofix; +- name `services/scheduler.py:91` as the concrete debuggability win; +- explicitly flag the `backend/infrahub/graphql/app.py` touch (2 log lines in ASGI error + handling, not the GraphQL contract) and the new `auth/auth.py` per-file ignore, with the + PR #10002 precedent that lets a reviewer ask for the 2-line inline fix instead; +- link `INBOX-29`. + +### T015 — Jira comment (critique P1 — do not let the card close silently) + +Comment on INBOX-29 stating that this PR addresses **only the TRY400 half**, that the 40 TRY004 +violations remain suppressed and need a human decision (caller-visible exception-type changes on +`core/schema/` and GraphQL mutation surfaces), and that a follow-up card should be split off +rather than letting a merge mark INBOX-29 Done. Also note the 2 deferred `auth/auth.py` sites. + +--- + +## Dependencies + +```text +T001 ──▶ T002..T010, T010b (all [P], independent of each other) + │ + ▼ + T011a ──▶ T012, T013 ──▶ T014 ──▶ T015 +``` + +**Total**: 15 tasks. T002–T010 are parallelizable across files; the config tasks (T001, T010b) +both touch `pyproject.toml` and must not run concurrently with each other. From 2d339e1c5fc8b5454574ae6c2df7e249d454f5d9 Mon Sep 17 00:00:00 2001 From: Saltaferis Dimitrios Date: Tue, 11 Aug 2026 15:15:21 +0000 Subject: [PATCH 4/9] [Spec Kit] Alignment check: ALIGNED, 0 remediation passes (INBOX-29) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs --- .../alignment-check.md | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 dev/specs/005-ruff-try400-tracebacks/alignment-check.md diff --git a/dev/specs/005-ruff-try400-tracebacks/alignment-check.md b/dev/specs/005-ruff-try400-tracebacks/alignment-check.md new file mode 100644 index 00000000000..c65737299cb --- /dev/null +++ b/dev/specs/005-ruff-try400-tracebacks/alignment-check.md @@ -0,0 +1,48 @@ +# Spec/Ask Alignment Check + +**Date**: 2026-08-11 | **Feature**: `dev/specs/005-ruff-try400-tracebacks/` + +## Source + +The source-of-truth ask is the **inline PRD** passed to this run, composed from Jira card +**INBOX-29** (Engineering Inbox, Tech Debt) by the platform-health drain pipeline. It carries the +card's Overview, Suggested solution, the measured ground truth, the TRY400-only scope decision, a +4-item "WHAT TO BUILD", an acceptance list, and a hard-constraints block. + +Two referenced URLs were **not** fetched: + +- the card's provenance link (`opsmillworkspace.slack.com/...`) — an authenticated Slack + permalink, not reachable; its substance is already quoted in the card and carried into the ask. +- the card itself was read directly via the Jira API before this run, not re-fetched here. + +Neither is requirement-bearing beyond what the inline ask already states, so the check runs +against the inline ask. + +## Verdict + +**✅ ALIGNED** — 0 remediation passes used. + +## Findings + +| Severity | Category | Ask reference | Spec reference | Description | +|----------|----------|---------------|----------------|-------------| +| info | expansion | WHAT TO BUILD #2 ("if `exception` would be wrong there, use a targeted `# noqa: TRY400`") | research.md §R4, FR-003 | The ask authorised per-site noqa in the abstract; the spec/research resolve it concretely into 28 conversions + 6 justified suppressions. Elaboration of an explicit instruction, not drift. | +| info | added | — | SC-007 | "Every remaining `# noqa: TRY400` carries a one-line justification" is a criterion the ask implied ("with a one-line reason") but did not list under ACCEPTANCE. Added as a verifiable gate. | +| info | added | — | research.md §R3 | The `TracebackSuppressionFilter` interaction was discovered during Phase 0, not present in the ask. It *narrows* scope at one site for a correctness reason and is documented. | +| info | changed | ask: "the 34 TRY400 violations ... EXCEPT auth/auth.py" | spec.md Context | The ask's own arithmetic (36 total, 2 in auth ⇒ 34 in scope) is preserved exactly; the spec additionally publishes the full 36-site distribution table. Presentation only. | + +**No** missing requirements, **no** off-scope additions, **no** softened or dropped acceptance +criteria, **no** contradicted constraints. Specifically confirmed present in the spec: + +- TRY004 out of scope, with the reason (spec "Out of Scope — TRY004", SC-003) +- `extend-select` mechanism and the TRY200-removed-rule warning (Assumptions, research.md §R1) +- no dependency-list edits (FR-002) +- `auth/auth.py` untouched, suppressed by file with a commented reason (FR-006) +- changelog fragment conditional on repo convention (FR-008) +- all four hard-constraint categories (FR-007, SC-005) +- structlog keyword-argument preservation (FR-004) +- test-assertion exposure for log records (research.md §R7) + +## Action + +Proceed to implementation. No phases re-run. From b3165cb2b76aa7f91ec584544cb1fa6fe228bad8 Mon Sep 17 00:00:00 2001 From: Saltaferis Dimitrios Date: Tue, 11 Aug 2026 15:28:19 +0000 Subject: [PATCH 5/9] chore(backend): enforce ruff TRY400 so error logs carry tracebacks 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 Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs --- backend/infrahub/core/branch/tasks.py | 2 +- backend/infrahub/core/merge/orchestrator.py | 2 +- backend/infrahub/database/__init__.py | 2 +- backend/infrahub/git/base.py | 4 +-- backend/infrahub/git/integrator.py | 34 ++++++++++++------- backend/infrahub/git/repository.py | 2 +- backend/infrahub/git/tasks.py | 2 +- backend/infrahub/graphql/app.py | 6 ++-- backend/infrahub/services/scheduler.py | 4 +-- backend/infrahub/webhook/tasks/process.py | 6 +++- backend/infrahub/workers/infrahub_async.py | 6 ++-- backend/tests/adapters/log.py | 7 +++- ...error-instead-of-exception.housekeeping.md | 1 + pyproject.toml | 15 +++++++- utilities/infrahub_load_tester.py | 32 ++++++++--------- 15 files changed, 80 insertions(+), 45 deletions(-) create mode 100644 changelog/+ruff-try400-error-instead-of-exception.housekeeping.md diff --git a/backend/infrahub/core/branch/tasks.py b/backend/infrahub/core/branch/tasks.py index a006c51f48b..4ca6c6e06e1 100644 --- a/backend/infrahub/core/branch/tasks.py +++ b/backend/infrahub/core/branch/tasks.py @@ -116,7 +116,7 @@ async def migrate_branch(branch: str, context: InfrahubContext, send_events: boo log.info(f"Running migrations for branch '{obj.name}'") await migration_runner.run(db=db, at=Timestamp()) except MigrationFailureError as exc: - log.error(f"Failed to run migrations for branch '{obj.name}': {exc.errors}") + log.exception(f"Failed to run migrations for branch '{obj.name}': {exc.errors}") raise if obj.status == BranchStatus.NEED_UPGRADE_REBASE: diff --git a/backend/infrahub/core/merge/orchestrator.py b/backend/infrahub/core/merge/orchestrator.py index 7a7aa5b2b95..0ccd8a71fc9 100644 --- a/backend/infrahub/core/merge/orchestrator.py +++ b/backend/infrahub/core/merge/orchestrator.py @@ -149,7 +149,7 @@ async def merge(self, *, context: InfrahubContext, proposed_change_id: str | Non target_branch_name=self.destination_branch.name, ) except BaseException as exc: - self.log.error("Merge failed, beginning rollback", extra={"error": str(exc)}) + self.log.exception("Merge failed, beginning rollback", extra={"error": str(exc)}) await self.rollback_handler.rollback( merge_started_at=merge_at, pre_merge_state=pre_merge_state, diff --git a/backend/infrahub/database/__init__.py b/backend/infrahub/database/__init__.py index 6a7296ce22f..02f1a62bf69 100644 --- a/backend/infrahub/database/__init__.py +++ b/backend/infrahub/database/__init__.py @@ -443,7 +443,7 @@ async def run_query( try: response = await execution_method.run(query=_query, parameters=params) except ServiceUnavailable as exc: - log.error("Database Service unavailable", error=str(exc)) + log.exception("Database Service unavailable", error=str(exc)) raise DatabaseError(message="Unable to connect to the database") from exc return response diff --git a/backend/infrahub/git/base.py b/backend/infrahub/git/base.py index 313a2950f73..95ee2853b84 100644 --- a/backend/infrahub/git/base.py +++ b/backend/infrahub/git/base.py @@ -616,7 +616,7 @@ def has_conflicting_changes(self, target_branch: str, source_branch: str) -> boo target=target_branch, ) return True - log.error( + log.exception( f"Unexpected error running git merge-tree for {source_branch} into {target_branch}", repository=self.name, source=source_branch, @@ -899,7 +899,7 @@ def validate_remote_branch(self, branch_name: str) -> bool: try: has_conflicts = self.has_conflicting_changes(target_branch=self.default_branch, source_branch=branch_name) except GitCommandError as exc: - log.error( + log.exception( "Unable to determine merge conflicts for branch", branch=branch_name, repository=self.name, diff --git a/backend/infrahub/git/integrator.py b/backend/infrahub/git/integrator.py index 103133c77fc..2725beb88e6 100644 --- a/backend/infrahub/git/integrator.py +++ b/backend/infrahub/git/integrator.py @@ -453,10 +453,14 @@ async def _build_jinja2_transform_definitions( except PydanticValidationError as exc: for error in exc.errors(): locations = [str(error_location) for error_location in error["loc"]] - log.error(f" {'/'.join(locations)} | {error['msg']} ({error['type']})") + # Validation feedback for the user's repository config, reported one line per + # error: a traceback would repeat identically for each and adds nothing. + log.error(f" {'/'.join(locations)} | {error['msg']} ({error['type']})") # noqa: TRY400 continue except ValidationError as exc: - log.error(exc.message) + # Same user-facing config validation feedback as above: the message is the whole + # actionable content, so no traceback. + log.error(exc.message) # noqa: TRY400 continue closure = closure_builder.build( @@ -635,10 +639,14 @@ async def _build_artifact_definitions( except PydanticValidationError as exc: for error in exc.errors(): locations = [str(error_location) for error_location in error["loc"]] - log.error(f" {'/'.join(locations)} | {error['msg']} ({error['type']})") + # Validation feedback for the user's repository config, reported one line per + # error: a traceback would repeat identically for each and adds nothing. + log.error(f" {'/'.join(locations)} | {error['msg']} ({error['type']})") # noqa: TRY400 continue except ValidationError as exc: - log.error(exc.message) + # Same user-facing config validation feedback as above: the message is the whole + # actionable content, so no traceback. + log.error(exc.message) # noqa: TRY400 continue local_artifact_defs[artdef.name] = artdef @@ -807,7 +815,7 @@ async def get_repository_config(self, branch_name: str, commit: str) -> Infrahub try: data = yaml.safe_load(config_file_content) except yaml.YAMLError as exc: - log.error(f"Unable to load the configuration file in YAML format {config_file_name}: {exc}") + log.exception(f"Unable to load the configuration file in YAML format {config_file_name}: {exc}") raise RepositoryConfigurationError( identifier=self.name, message=f"Repository '{self.name}' has an invalid configuration file '{config_file_name}'. " @@ -822,7 +830,7 @@ async def get_repository_config(self, branch_name: str, commit: str) -> Infrahub log.info(f"Successfully parsed {config_file_name}") return configuration except PydanticValidationError as exc: - log.error(f"Unable to load the configuration file {config_file_name}, the format is not valid: {exc}") + log.exception(f"Unable to load the configuration file {config_file_name}, the format is not valid: {exc}") raise RepositoryConfigurationError( identifier=self.name, message=f"Repository '{self.name}' has an invalid configuration file '{config_file_name}'. " @@ -944,7 +952,7 @@ async def _build_graphql_query_definitions( relative_path=str(commit_wt.directory), ) except InfrahubSdkError as exc: - log.error(f"Query '{query_config.name}': {exc}") + log.exception(f"Query '{query_config.name}': {exc}") raise return local_queries @@ -1565,7 +1573,7 @@ async def get_check_definition( ) except Exception as exc: - log.error( + log.exception( f"An error occurred while processing the CheckDefinition {check_class.__name__} from {file_path} : {exc} " ) raise @@ -1605,7 +1613,7 @@ async def get_python_transforms( ) except Exception as exc: - log.error( + log.exception( f"An error occurred while processing the PythonTransform {transform.name} from {file_path} : {exc} " ) raise @@ -1893,14 +1901,14 @@ async def execute_python_check( except ModuleNotFoundError as exc: error_msg = "Unable to load the check file" - log.error(error_msg) + log.exception(error_msg) raise CheckError( repository_name=self.name, class_name=class_name, commit=commit, location=location, message=error_msg ) from exc except AttributeError as exc: error_msg = f"Unable to find the class {class_name}" - log.error(error_msg) + log.exception(error_msg) raise CheckError( repository_name=self.name, class_name=class_name, commit=commit, location=location, message=error_msg ) from exc @@ -1965,14 +1973,14 @@ async def execute_python_transform( return await transform.run(data=data) except ModuleNotFoundError as exc: error_msg = f"Unable to load the transform file {location}" - log.error(error_msg) + log.exception(error_msg) raise TransformError( repository_name=self.name, commit=commit, location=location, message=error_msg ) from exc except AttributeError as exc: error_msg = f"Unable to find the class {class_name} in {location}" - log.error(error_msg) + log.exception(error_msg) raise TransformError( repository_name=self.name, commit=commit, location=location, message=error_msg ) from exc diff --git a/backend/infrahub/git/repository.py b/backend/infrahub/git/repository.py index 34da295967b..986995e16ef 100644 --- a/backend/infrahub/git/repository.py +++ b/backend/infrahub/git/repository.py @@ -413,7 +413,7 @@ async def update_latest_commit(self) -> None: try: latest_commit = git_repo.git.rev_parse(self.ref) except GitCommandError as err: - log.error(f"No object found for ref {self.ref} on repository {self.name}") + log.exception(f"No object found for ref {self.ref} on repository {self.name}") raise ValueError(f"Ref {self.ref} not found.") from err latest_commit = str(git_repo.commit(latest_commit)) synced_from_remote = await self.sync_from_remote(commit=latest_commit) diff --git a/backend/infrahub/git/tasks.py b/backend/infrahub/git/tasks.py index 44f1fe5ce5f..a13142d6335 100644 --- a/backend/infrahub/git/tasks.py +++ b/backend/infrahub/git/tasks.py @@ -1214,7 +1214,7 @@ async def run_user_check(model: UserCheckData) -> ValidatorConclusion: log_entries = check_run.log_entries except CheckError as exc: log.warning("The check failed to run") - log.error(exc.message) + log.exception(exc.message) log_entries = f"FATAL Error/n:{exc.message}" check = None diff --git a/backend/infrahub/graphql/app.py b/backend/infrahub/graphql/app.py index 4cd8634737d..69676b9a0b6 100644 --- a/backend/infrahub/graphql/app.py +++ b/backend/infrahub/graphql/app.py @@ -192,7 +192,7 @@ async def _handle_http_request( except ValueError as exc: return JSONResponse({"errors": [exc.args[0]]}, status_code=400) except ClientDisconnect as exc: - self.logger.error("Exception ClientDisconnect in _handle_http_request") + self.logger.exception("Exception ClientDisconnect in _handle_http_request") return JSONResponse({"errors": [str(exc)]}, status_code=400) if isinstance(operations, list): @@ -532,7 +532,9 @@ async def _observe_subscription( await websocket.send_json({"type": GQL_DATA, "id": operation_id, "payload": payload}) except Exception as error: if not isinstance(error, GraphQLError): - self.logger.error("An exception occurred in resolvers", exc_info=error) + # Inside the handler, so the active exception is attached implicitly; the helper + # below runs outside any except block and must pass it explicitly instead. + self.logger.exception("An exception occurred in resolvers") error = GraphQLError(str(error), original_error=error) await websocket.send_json( { diff --git a/backend/infrahub/services/scheduler.py b/backend/infrahub/services/scheduler.py index 5a530aed32f..a16b5cad510 100644 --- a/backend/infrahub/services/scheduler.py +++ b/backend/infrahub/services/scheduler.py @@ -87,8 +87,8 @@ async def run_schedule(self, schedule: Schedule) -> None: try: await schedule.function(self.service) # Keep-alive: a failing recurring task must not kill the scheduler loop - except Exception as exc: # noqa: BLE001 - self.service.log.error(str(exc)) + except Exception as exc: + self.service.log.exception(str(exc)) for _ in range(schedule.interval): if not self.running: return diff --git a/backend/infrahub/webhook/tasks/process.py b/backend/infrahub/webhook/tasks/process.py index 8c0b8b59fb7..5b037acedf2 100644 --- a/backend/infrahub/webhook/tasks/process.py +++ b/backend/infrahub/webhook/tasks/process.py @@ -201,7 +201,11 @@ async def webhook_send( except WebhookDeliveryError as error: elapsed_ms = (time.monotonic() - started) * 1_000 failure = error.failure - log.error( + # Deliberately not `exception`: WebhookDeliveryError is registered for traceback + # suppression, and the filter drops the *whole* record for a registered type - attaching + # the exception here would delete this classified failure report from the run logs. The + # traceback still reaches the caller via the re-raise below. + log.error( # noqa: TRY400 get_webhook_log_formatter().delivery_failed( status_class=failure.status_class, message=failure.message, diff --git a/backend/infrahub/workers/infrahub_async.py b/backend/infrahub/workers/infrahub_async.py index 9e7dd57f702..e1009a3f072 100644 --- a/backend/infrahub/workers/infrahub_async.py +++ b/backend/infrahub/workers/infrahub_async.py @@ -191,7 +191,9 @@ async def _init_infrahub_client(self, client: InfrahubClient | None = None) -> I ) ) except InitializationError as err: - self._logger.error( + # A missing configuration value, reported before a clean exit: there is nothing + # in a traceback to diagnose. + self._logger.error( # noqa: TRY400 "Infrahub client initialization failed due to missing configuration for internal_address." ) raise typer.Exit(1) from err @@ -199,7 +201,7 @@ async def _init_infrahub_client(self, client: InfrahubClient | None = None) -> I try: await client.branch.all() except SdkError as err: - self._logger.error(f"Error in communication with Infrahub: {err.message}") + self._logger.exception(f"Error in communication with Infrahub: {err.message}") raise typer.Exit(1) from err return client diff --git a/backend/tests/adapters/log.py b/backend/tests/adapters/log.py index 249026c74a4..bb4555901c3 100644 --- a/backend/tests/adapters/log.py +++ b/backend/tests/adapters/log.py @@ -23,4 +23,9 @@ def critical(self, event: str | None = None, *args: Any, **kw: Any) -> Any: """Send a critical event.""" def exception(self, event: str | None = None, *args: Any, **kw: Any) -> Any: - """Send an exception event.""" + """Send an exception event. + + Recorded alongside the error events because an exception event is emitted at error level - + it only adds the active exception's traceback to the record. + """ + self.error_logs.append(event) diff --git a/changelog/+ruff-try400-error-instead-of-exception.housekeeping.md b/changelog/+ruff-try400-error-instead-of-exception.housekeeping.md new file mode 100644 index 00000000000..2758e6df663 --- /dev/null +++ b/changelog/+ruff-try400-error-instead-of-exception.housekeeping.md @@ -0,0 +1 @@ +The TRY400 (error-instead-of-exception) ruff rule is now enforced — a `log.error` reporting a caught exception is either `log.exception`, so the traceback reaches the logs, or carries an explicit justified `# noqa: TRY400`. Nine now-redundant `# noqa: BLE001` suppressions were dropped as a result. diff --git a/pyproject.toml b/pyproject.toml index 8856289ef92..fa1eff0e815 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -460,6 +460,12 @@ task-tags = ["FIXME", "TODO", "XXX"] select = ["ALL"] +# Rules re-enabled individually out of a category that is still ignored below. A specific code +# here wins over the broader prefix in `ignore`, so these stay enforced. +extend-select = [ + "TRY400", # error-instead-of-exception - enabled ahead of the rest of TRY (INBOX-29) +] + ignore = [ "CPY", # flake8-copyright "T201", # use of `print` @@ -472,7 +478,7 @@ ignore = [ # Rules below needs to be Investigated # ################################################################################################## "EM", # flake8-errmsg - "TRY", # tryceratops + "TRY", # tryceratops (except TRY400, re-enabled in extend-select above) "FBT", # flake8-boolean-trap "G", # flake8-logging-format "RSE", # flake8-raise @@ -550,6 +556,13 @@ allow-dunder-method-names = [ [tool.ruff.lint.per-file-ignores] +"backend/infrahub/auth/auth.py" = [ + # Both remaining TRY400 sites here are pure logging inside `except` blocks and would convert + # cleanly, but INBOX-29 was delivered by an automated pipeline that may not edit auth modules. + # Deferred to a human: drop this entry and switch the two calls to `log.exception`. + "TRY400", # error-instead-of-exception +] + "backend/infrahub/**.py" = [ ################################################################################################## # Refactor code and remove the ignore rule diff --git a/utilities/infrahub_load_tester.py b/utilities/infrahub_load_tester.py index 0bf3d3e510d..924027b8275 100644 --- a/utilities/infrahub_load_tester.py +++ b/utilities/infrahub_load_tester.py @@ -45,8 +45,8 @@ async def _create_one(idx: int, client: InfrahubClient, prefix: str, log: loggin await proposed_change.save() log.info(f"✅ Created proposed change for branch {branch_name}") # Load test: absorb any request failure and continue - except Exception as e: # noqa: BLE001 - log.error(f"❌ Error creating proposed change for branch {branch_name}: {e}") + except Exception as e: + log.exception(f"❌ Error creating proposed change for branch {branch_name}: {e}") log.info(f"✅ User {uname} created with branch {branch_name}") return uname, branch_name @@ -68,8 +68,8 @@ async def _delete_branches(client: InfrahubClient, prefix: str, usernames: Itera try: all_branches = await client.branch.all() # Load test: absorb any request failure and continue - except Exception as e: # noqa: BLE001 - log.error(f"Error retrieving branches: {e}") + except Exception as e: + log.exception(f"Error retrieving branches: {e}") for uname in usernames: br = f"{prefix}/{uname}" @@ -84,8 +84,8 @@ async def _delete_branches(client: InfrahubClient, prefix: str, usernames: Itera else: log.warning(f"Branch {br} not found") # Load test: absorb any request failure and continue with remaining branches - except Exception as exc: # noqa: BLE001 - log.error(f"Error deleting branch {br}: {exc}") + except Exception as exc: + log.exception(f"Error deleting branch {br}: {exc}") async def _delete_users(client: InfrahubClient, usernames: Iterable[str], log: logging.Logger) -> None: @@ -109,14 +109,14 @@ async def _delete_users(client: InfrahubClient, usernames: Iterable[str], log: l log.info(f"🗑️ User {uname} Deleted") break # Load test: absorb any request failure and continue with remaining users - except Exception as e: # noqa: BLE001 - log.error(f"Error while deleting: {str(e)}") + except Exception as e: + log.exception(f"Error while deleting: {str(e)}") else: log.warning(f"User {uname} not found in the list") # Load test: absorb any request failure and continue with remaining users - except Exception as exc: # noqa: BLE001 - log.error(f"❌ General: {exc}") + except Exception as exc: + log.exception(f"❌ General: {exc}") async def cleanup(client: InfrahubClient, log: logging.Logger, *, prefix: str, n_users: int) -> None: @@ -141,8 +141,8 @@ async def create_admin_branches(client: InfrahubClient, log: logging.Logger, *, log.info(f"✅ Branch created: {branch_name} with test tag") # Load test: absorb any request failure and continue with remaining branches - except Exception as exc: # noqa: BLE001 - log.error(f"❌ Error creating branch {branch_name}: {exc}") + except Exception as exc: + log.exception(f"❌ Error creating branch {branch_name}: {exc}") async def delete_admin_branches(client: InfrahubClient, log: logging.Logger, *, n_branches: int, prefix: str) -> None: @@ -152,8 +152,8 @@ async def delete_admin_branches(client: InfrahubClient, log: logging.Logger, *, try: all_branches = await client.branch.all() # Load test: abort branch cleanup gracefully on any request failure - except Exception as e: # noqa: BLE001 - log.error(f"Error retrieving branches: {e}") + except Exception as e: + log.exception(f"Error retrieving branches: {e}") return for i in range(1, n_branches + 1): @@ -170,8 +170,8 @@ async def delete_admin_branches(client: InfrahubClient, log: logging.Logger, *, else: log.warning(f"Branch {branch_name} not found") # Load test: absorb any request failure and continue with remaining branches - except Exception as exc: # noqa: BLE001 - log.error(f"Error deleting branch {branch_name}: {exc}") + except Exception as exc: + log.exception(f"Error deleting branch {branch_name}: {exc}") async def run( From 7a860c5b836f2c0d334a08c3140d9085d28a36a9 Mon Sep 17 00:00:00 2001 From: Saltaferis Dimitrios Date: Tue, 11 Aug 2026 15:31:36 +0000 Subject: [PATCH 6/9] =?UTF-8?q?[Spec=20Kit]=20Implementation=20report=20?= =?UTF-8?q?=E2=80=94=20all=20gates=20green=20(INBOX-29)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs --- .../opsmill-implement-report.md | 126 ++++++++++++++++++ 1 file changed, 126 insertions(+) create mode 100644 dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md diff --git a/dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md b/dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md new file mode 100644 index 00000000000..9bb09a223a6 --- /dev/null +++ b/dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md @@ -0,0 +1,126 @@ +# Implementation Report: Re-enable ruff TRY400 (INBOX-29) + +**Branch**: `pha/INBOX-29` | **Date**: 2026-08-11 | **Status**: DONE + +## Outcome + +TRY400 is enforced repo-wide. All 36 flagged sites resolved: **28 converted** to `log.exception`, +**6** kept as `log.error` with a justified `# noqa: TRY400`, **2** suppressed by file in +`backend/infrahub/auth/auth.py` (not edited). TRY004 remains suppressed and is handed back to a +human. + +15 files changed, +80 / −45. + +## Tasks + +| Task | Status | Note | +|------|--------|------| +| T001 Enable TRY400 in `pyproject.toml` | ✅ | `extend-select = ["TRY400"]`; `"TRY"` ignore annotated | +| T002 `git/integrator.py` (9 convert + 4 noqa) | ✅ | | +| T003 `utilities/infrahub_load_tester.py` (8 convert) | ✅ | | +| T004 `graphql/app.py` (2 convert) | ✅ | incl. redundant `exc_info` removal | +| T005 `git/base.py` (2 convert) | ✅ | | +| T006 `workers/infrahub_async.py` (1 convert + 1 noqa) | ✅ | | +| T007 `webhook/tasks/process.py` (1 noqa) | ✅ | the dropped-record case | +| T008 `core/` (2 convert) | ✅ | | +| T009 `git/repository.py`, `git/tasks.py` (2 convert) | ✅ | | +| T010 `database/__init__.py`, `services/scheduler.py` (2 convert) | ✅ | | +| T010b auth carve-out (config only) | ✅ | `auth/auth.py` never opened for edit | +| T011a Changelog fragment | ✅ | `+ruff-try400-error-instead-of-exception.housekeeping.md` | +| T012 Lint + format gates | ✅ | see Verification | +| T013 Tests + governance diff | ✅ | 2233 unit tests pass | +| T014/T015 Deliver | ✅ | PR opened, Jira comment posted | + +## Verification (all success criteria) + +| Criterion | Result | +|-----------|--------| +| SC-001 zero TRY400 (was 36) | ✅ `ruff check --select TRY400` → All checks passed | +| SC-002 no new violations of any other rule | ✅ `ruff check --no-cache .` → clean, 0 errors | +| SC-003 TRY004 untouched | ✅ still exactly **40**; no file it flags was modified | +| SC-004 format + lint clean | ✅ `invoke format` clean; `invoke lint` → ruff + **ty** + **mypy** all pass (mypy: 1610 files, no issues) | +| SC-005 no gated path in the diff | ✅ no `core/schema/`, `core/migrations/`, `auth/`, `.github/`, no generated file | +| SC-006 unit tests pass | ✅ `invoke backend.test-unit` → **2233 passed** | +| SC-007 every `noqa: TRY400` justified | ✅ all 6 carry a reason comment | + +Also run green: `invoke main.lint`, `uv lock --check`, `invoke backend.validate-generated`, +`invoke docs.validate`, `invoke schema.validate-graphqlschema`, +`invoke schema.validate-jsonschema` — **no generated-file drift**. + +Not run: frontend Betterer / GraphQL codegen (no frontend file, GraphQL schema, or generated +frontend type is in the diff) and `invoke docs.format` (`markdownlint-cli2` is not installed in +this environment — the single added Markdown file is a one-line changelog fragment matching the +existing fragments' shape). + +## What changed relative to the plan + +1. **Nine redundant `# noqa: BLE001` suppressions dropped** (unplanned, correct). BLE001 does not + flag a blind `except Exception` whose handler logs via `.exception()`, so converting those + handlers made their suppressions unused — `RUF100` (unused-noqa) then failed the lint gate + until they were removed. Net effect: this change **retires 9 suppressions beyond the one rule + it enables**. Sites: `services/scheduler.py` (1), `utilities/infrahub_load_tester.py` (8). + +2. **`FakeLogger.exception` now records** (`backend/tests/adapters/log.py`). It was a no-op stub, + so `test_scheduler_task_with_error` — which asserts an error is reported — failed once the + scheduler site converted. The fake was wrong, not the test: `.exception` emits at **error** + level, so it now appends to `error_logs` and the existing assertion passes unmodified. This is + the test exposure research.md §R7 predicted. Verified no other test is affected: the only + other `error_logs` consumer (`test_rabbitmq.py`, including an `== []` assertion) exercises + code with no `.exception()` calls. + +3. **`integrator.py:825` line-wrapping reverted by the formatter** — it fits in 120 chars on one + line. Cosmetic. + +## The finding worth a reviewer's attention + +`backend/infrahub/webhook/tasks/process.py:204` was **deliberately not converted**. +`WebhookDeliveryError` is registered via `@suppress_traceback_in_logs`, and +`TracebackSuppressionFilter` — installed on the Prefect run loggers that this site logs through — +**drops the entire record**, not just its traceback, for a registered exception type. Converting +it would have 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-fixes` would have made exactly this change. + +**Durable hazard for the conventions pass**: `log.exception` at a broad-catch site behind a +Prefect run logger is only safe while the suppressed-type set stays narrow. Two *converted* sites +(`git/integrator.py:1568`, `:1608`) catch bare `Exception` through a run logger; they are safe +today only because `WebhookDeliveryError` is the sole registered type and is unreachable from +repository-integration code. Registering a second, more widely-raised type would silently mute +them. + +## Deliberately left for a human + +1. **TRY004 — 40 violations, still suppressed.** The fix changes caller-visible exception types on + `core/schema/schema_branch.py` (5 sites) and `graphql/mutations/*` + `graphql/types/node.py` + (14 sites). That is an API-behaviour decision, not a lint cleanup. **INBOX-29 asked for both + rules, so merging this PR does not complete the card** — a follow-up card should be split off + rather than letting the merge mark it Done. +2. **The 2 `auth/auth.py` sites.** Both are pure logging inside `except` blocks and would convert + cleanly. They are suppressed by file because this pipeline may not edit auth modules. The + merged BLE precedent (PR #10002) *did* edit this same file — adding `# noqa: BLE001` at the + very handlers holding these two sites — so a reviewer may reasonably prefer the 2-line inline + fix and drop the `per-file-ignores` entry. + +## Notable autonomous decisions + +- **Scoped the card down to TRY400 before planning**, on measuring that TRY004 lands on + governance-gated surfaces. Recorded in spec.md "Out of Scope" and escalated on the card rather + than silently half-delivering. +- **Per-site convert-vs-noqa rule**: convert unless the traceback is actively harmful (1 site) or + worthless (5 sites: per-error user-config validation feedback ×4, a pure missing-configuration + startup error ×1). Full table in research.md §R4. +- **`extend-select` over enumerating the TRY codes** in `ignore` — enumeration is impossible + because `TRY200` is a removed rule and naming it breaks ruff. +- **Kept ticket refs out of Python comments** per `.agents/rules/code-doc-style.md`; the two + `INBOX-29` references live in `pyproject.toml`, which has existing precedent (`INFP-566`). + +## Incident during implementation + +A first pass ran `ruff check --select RUF100 --fix .` to clear the 9 unused-noqa. Because +`--select` *replaces* the rule set, every noqa directive for every other rule became "unused" and +ruff stripped **870 directives across 388 files**. Caught immediately on the diff stat, reverted +with `git checkout -- .` (nothing had been committed), and the whole edit set was re-applied from +a scripted, assertion-checked list. Final state verified clean by full lint + mypy + ty + 2233 +tests. Lesson: never `--select --fix` on a whole repo; scope the fix to the specific lines. + +STATUS: DONE | SPEC_DIR: /home/ubuntu/projects/infrahub/dev/specs/005-ruff-try400-tracebacks | REASON: n/a From d24f9b0b2a81441aa21c3709a0e77cea1e74ed0d Mon Sep 17 00:00:00 2001 From: Saltaferis Dimitrios Date: Tue, 11 Aug 2026 16:03:38 +0000 Subject: [PATCH 7/9] docs(changelog): reword TRY400 fragment to satisfy Vale spelling "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 Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs --- .../+ruff-try400-error-instead-of-exception.housekeeping.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/+ruff-try400-error-instead-of-exception.housekeeping.md b/changelog/+ruff-try400-error-instead-of-exception.housekeeping.md index 2758e6df663..bd03e9c434b 100644 --- a/changelog/+ruff-try400-error-instead-of-exception.housekeeping.md +++ b/changelog/+ruff-try400-error-instead-of-exception.housekeeping.md @@ -1 +1 @@ -The TRY400 (error-instead-of-exception) ruff rule is now enforced — a `log.error` reporting a caught exception is either `log.exception`, so the traceback reaches the logs, or carries an explicit justified `# noqa: TRY400`. Nine now-redundant `# noqa: BLE001` suppressions were dropped as a result. +The TRY400 (error-instead-of-exception) ruff rule is now enforced — a `log.error` reporting a caught exception is either `log.exception`, so the traceback reaches the logs, or carries an explicit justified `# noqa: TRY400`. Nine now-redundant `# noqa: BLE001` comments were removed as a result. From d9893d2768d474d40cfa7e789fc4f7a80a301525 Mon Sep 17 00:00:00 2001 From: Saltaferis Dimitrios Date: Fri, 14 Aug 2026 14:10:30 +0000 Subject: [PATCH 8/9] fix(backend): keep log.error for routine ClientDisconnect, anchor spec 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 Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs --- backend/infrahub/graphql/app.py | 4 +- .../opsmill-implement-report.md | 33 ++++++++- dev/specs/005-ruff-try400-tracebacks/plan.md | 4 +- .../005-ruff-try400-tracebacks/research.md | 10 ++- dev/specs/005-ruff-try400-tracebacks/tasks.md | 67 ++++++++++++------- 5 files changed, 85 insertions(+), 33 deletions(-) diff --git a/backend/infrahub/graphql/app.py b/backend/infrahub/graphql/app.py index 69676b9a0b6..1dd68a2a1c2 100644 --- a/backend/infrahub/graphql/app.py +++ b/backend/infrahub/graphql/app.py @@ -192,7 +192,9 @@ async def _handle_http_request( except ValueError as exc: return JSONResponse({"errors": [exc.args[0]]}, status_code=400) except ClientDisconnect as exc: - self.logger.exception("Exception ClientDisconnect in _handle_http_request") + # A client aborting mid-request is routine, and the traceback only shows the body-read + # path, so it would be non-actionable noise on a normal operating event. + self.logger.error("Exception ClientDisconnect in _handle_http_request") # noqa: TRY400 return JSONResponse({"errors": [str(exc)]}, status_code=400) if isinstance(operations, list): diff --git a/dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md b/dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md index 9bb09a223a6..21d9b851ea1 100644 --- a/dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md +++ b/dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md @@ -4,11 +4,14 @@ ## Outcome -TRY400 is enforced repo-wide. All 36 flagged sites resolved: **28 converted** to `log.exception`, -**6** kept as `log.error` with a justified `# noqa: TRY400`, **2** suppressed by file in +TRY400 is enforced repo-wide. All 36 flagged sites resolved: **27 converted** to `log.exception`, +**7** kept as `log.error` with a justified `# noqa: TRY400`, **2** suppressed by file in `backend/infrahub/auth/auth.py` (not edited). TRY004 remains suppressed and is handed back to a human. +> Counts updated after review: `graphql/app.py` → `_handle_http_request` (`ClientDisconnect`) +> moved from converted to noqa. See "Post-review changes". + 15 files changed, +80 / −45. ## Tasks @@ -123,4 +126,30 @@ with `git checkout -- .` (nothing had been committed), and the whole edit set wa a scripted, assertion-checked list. Final state verified clean by full lint + mypy + ty + 2233 tests. Lesson: never `--select --fix` on a whole repo; scope the fix to the specific lines. +## Post-review changes + +Applied after the automated review on PR #10220: + +1. **`graphql/app.py` → `_handle_http_request` reverted to `log.error` + `# noqa: TRY400`.** + The finding was valid and it contradicted research.md §R4's own criterion rather than being a + defensible judgement call. Starlette raises `ClientDisconnect` whenever a client aborts while + its request body is being read — routine, not exceptional — and the traceback only shows the + body-read path. §R4's rule is "convert unless the traceback would be actively harmful or + worthless"; this one is worthless, so it belonged with the noqa sites all along. research.md + §R4 and plan.md counts updated to 27 convert / 7 noqa. + +2. **`tasks.md` site references switched from absolute line numbers to enclosing-function + anchors.** The numbers were measured pre-conversion, and the conversions shifted them — e.g. + `process.py:204` → `:208`, `infrahub_async.py:194` → `:196`, and `integrator.py:810` no longer + pointed at a log site at all. Since the doc merges as the archived record, absolute anchors + actively mislead. Change 1 above shifted them again, which is the point. The task list now + names functions and tells the reader how to regenerate the set + (`ruff check --select TRY400 .` / `grep -n 'noqa: TRY400'`). + +3. **Release-vehicle finding (target `stable` rather than `develop`) rebutted and resolved.** The + premise — that this is "primarily repo tooling ... which cannot affect a running product" — is + factually wrong: the diff changes 27 runtime logging call sites across 12 backend modules, and + what production emits is exactly what the webhook record-dropping hazard turned on. Left for a + maintainer who owns that policy to overrule if they disagree. + STATUS: DONE | SPEC_DIR: /home/ubuntu/projects/infrahub/dev/specs/005-ruff-try400-tracebacks | REASON: n/a diff --git a/dev/specs/005-ruff-try400-tracebacks/plan.md b/dev/specs/005-ruff-try400-tracebacks/plan.md index d8cf24073e7..5da55d64948 100644 --- a/dev/specs/005-ruff-try400-tracebacks/plan.md +++ b/dev/specs/005-ruff-try400-tracebacks/plan.md @@ -8,7 +8,7 @@ Enable ruff's TRY400 (`error-instead-of-exception`) repo-wide via `extend-select`, leaving the rest of the suppressed `TRY` family — TRY004 included — untouched, then resolve all 36 flagged -sites: **28 converted** to `log.exception`, **6 kept as `log.error` with a justified +sites: **27 converted** to `log.exception`, **7 kept as `log.error` with a justified `# noqa: TRY400`**, and **2 suppressed by file** because they live in `backend/infrahub/auth/auth.py`, which this pipeline may not edit. Per-site decisions and their reasoning are in [research.md](./research.md) §R4. @@ -35,7 +35,7 @@ handler actually emits the record. control flow. No DB schema/migration, GraphQL/REST contract, auth, dependency, CI, or generated-file changes. -**Scale/Scope**: 1 config file + 12 source files; 36 TRY400 sites (28 conversions, 6 in-line +**Scale/Scope**: 1 config file + 12 source files; 36 TRY400 sites (27 conversions, 7 in-line suppressions, 2 file-level suppressions). ## Constitution Check diff --git a/dev/specs/005-ruff-try400-tracebacks/research.md b/dev/specs/005-ruff-try400-tracebacks/research.md index 4b1adcb812d..5aca05ed0ac 100644 --- a/dev/specs/005-ruff-try400-tracebacks/research.md +++ b/dev/specs/005-ruff-try400-tracebacks/research.md @@ -61,7 +61,11 @@ review. All 34 in-scope sites were read at their call site. The rule applied: **convert unless the traceback would be actively harmful or worthless.** -### Keep `log.error` + `# noqa: TRY400` (6) +### Keep `log.error` + `# noqa: TRY400` (7) + +> Originally 6. `graphql/app.py` → `_handle_http_request` was added after review: it was converted +> in the first pass, and that was a misapplication of this section's own criterion. See the last +> row. | Site | Reason | |------|--------| @@ -71,8 +75,9 @@ traceback would be actively harmful or worthless.** | `git/integrator.py:459` | `log.error(exc.message)` for the SDK `ValidationError` paired with 456's handler; user-config validation feedback, then `continue`. | | `git/integrator.py:641` | Same as 459, artifact-definition variant. | | `workers/infrahub_async.py:194` | "missing configuration for internal_address" then a clean `typer.Exit(1)`. A pure configuration error — there is nothing in the traceback to diagnose. | +| `graphql/app.py` → `_handle_http_request` | `ClientDisconnect` is raised whenever a client aborts while its request body is being read — a routine operating event, not an exceptional one. The traceback only shows the body-read path and is non-actionable, so an ERROR-level stack trace per aborted request is pure log noise. **Added after review**; the original decision to convert it contradicted this section's stated criterion. | -### Convert to `log.exception` (28) +### Convert to `log.exception` (27) | Site | Handled exception | Note | |------|-------------------|------| @@ -90,7 +95,6 @@ traceback would be actively harmful or worthless.** | `git/integrator.py:1968`, `:1975` | `ModuleNotFoundError`, `AttributeError` | loading a user transform → `TransformError` | | `git/repository.py:416` | `GitCommandError` | nested ref lookup, then raises | | `git/tasks.py:1217` | `CheckError` | check failed to run | -| `graphql/app.py:195` | `ClientDisconnect` | reports the active exception | | `graphql/app.py:535` | `Exception` (non-`GraphQLError`) | see R5 | | `services/scheduler.py:91` | `Exception` (keep-alive) | currently logs only `str(exc)`; a failing recurring task was undiagnosable | | `workers/infrahub_async.py:202` | `SdkError` | a communication failure — traceback distinguishes refused / timeout / TLS | diff --git a/dev/specs/005-ruff-try400-tracebacks/tasks.md b/dev/specs/005-ruff-try400-tracebacks/tasks.md index e0a12a7ff2e..379e18b1a6d 100644 --- a/dev/specs/005-ruff-try400-tracebacks/tasks.md +++ b/dev/specs/005-ruff-try400-tracebacks/tasks.md @@ -32,58 +32,75 @@ Each task: apply research.md §R4, preserve message text and all keyword argumen (FR-004), change no control flow (FR-005), and give every `# noqa: TRY400` a one-line reason (SC-007, FR-003). +> Sites are identified by **enclosing function**, not line number. The conversions shift line +> numbers as they are applied, so absolute anchors written at plan time do not survive into the +> merged tree. To locate the current set at any time: +> `ruff check --select TRY400 .` before the change, and `grep -n 'noqa: TRY400'` after it. + ### T002 [P] — `backend/infrahub/git/integrator.py` — 9 convert + 4 noqa -- **Convert** lines 810, 825, 947, 1568, 1608, 1896, 1903, 1968, 1975. -- **noqa** lines 456, 638 (per-error Pydantic validation loop) and 459, 641 (paired SDK - `ValidationError` on user config). +- **Convert**: `get_repository_config` (2 — YAML parse, Pydantic parse), + `_build_graphql_query_definitions` (1), `get_check_definition` (1), `get_python_transforms` (1), + `execute_python_check` (2), `execute_python_transform` (2). +- **noqa**: `_build_jinja2_transform_definitions` (2) and `_build_artifact_definitions` (2) — each + is a per-error Pydantic validation loop plus the paired SDK `ValidationError` on user config. - Largest single file (13 of 36 sites); do it first if serializing. ### T003 [P] — `utilities/infrahub_load_tester.py` — 8 convert -- Lines 49, 72, 88, 113, 119, 145, 156, 174. Stdlib `logging.Logger`; `.exception` exists. +- `_create_one` (1), `_delete_branches` (2), `_delete_users` (2), `create_admin_branches` (1), + `delete_admin_branches` (2). Stdlib `logging.Logger`; `.exception` exists. -### T004 [P] — `backend/infrahub/graphql/app.py` — 2 convert +### T004 [P] — `backend/infrahub/graphql/app.py` — 1 convert + 1 noqa -- Line 195 (`ClientDisconnect`) — straight conversion. -- Line 535 — convert **and remove the now-redundant `exc_info=error`** (research.md §R5). - This is the only site where an argument is intentionally not preserved; verify the reassignment - at line 536 still happens after the log call (critique E2). +- **noqa** in `_handle_http_request` (`ClientDisconnect`) — a client aborting mid-request is a + routine operating event and the traceback only shows the body-read path, so it is + non-actionable noise. *(Converted in the first pass, then reverted after review — it failed + research.md §R4's own "worthless traceback" test.)* +- **Convert** in `_observe_subscription` — **and remove the now-redundant `exc_info=error`** + (research.md §R5). This is the only site where an argument is intentionally not preserved; + verify the `error` reassignment still happens after the log call (critique E2). +- Leave the `_log_error` helper alone: it runs outside any `except` block and must keep passing + `exc_info` explicitly. Ruff does not flag it. - ASGI error handling only — **not** the GraphQL contract surface. Called out in the PR body - (T011). + (T014). ### T005 [P] — `backend/infrahub/git/base.py` — 2 convert -- Lines 619, 902 (both `GitCommandError`). +- `has_conflicting_changes`, `validate_remote_branch` (both `GitCommandError`). ### T006 [P] — `backend/infrahub/workers/infrahub_async.py` — 1 convert + 1 noqa -- **noqa** line 194 — pure configuration error ("missing configuration for internal_address") - followed by a clean `typer.Exit(1)`; nothing in a traceback to diagnose. -- **Convert** line 202 — `SdkError` communication failure; the traceback distinguishes refused / - timeout / TLS. +Both sites are in `_init_infrahub_client`: + +- **noqa** the `InitializationError` handler — a pure configuration error ("missing configuration + for internal_address") followed by a clean `typer.Exit(1)`; nothing in a traceback to diagnose. +- **Convert** the `SdkError` handler — a communication failure; the traceback distinguishes + refused / timeout / TLS. ### T007 [P] — `backend/infrahub/webhook/tasks/process.py` — 1 noqa -- Line 204: keep `log.error`, add `# noqa: TRY400` whose reason states that attaching the - exception makes `TracebackSuppressionFilter` drop the whole record, because - `WebhookDeliveryError` is registered via `@suppress_traceback_in_logs` and `log` is a Prefect - run logger. **Do not convert this one** (research.md §R3 — the highest-consequence decision in - the change). +- In `webhook_send`, the `except WebhookDeliveryError` handler: keep `log.error`, add + `# noqa: TRY400` whose reason states that attaching the exception makes + `TracebackSuppressionFilter` drop the whole record, because `WebhookDeliveryError` is registered + via `@suppress_traceback_in_logs` and `log` is a Prefect run logger. **Do not convert this one** + (research.md §R3 — the highest-consequence decision in the change). ### T008 [P] — `backend/infrahub/core/` — 2 convert -- `branch/tasks.py:119` (`MigrationFailureError`), `merge/orchestrator.py:152` (`BaseException`, - rollback path). +- `branch/tasks.py` → `migrate_branch` (`MigrationFailureError`); + `merge/orchestrator.py` → `merge` (`BaseException`, rollback path — note this module has other, + pre-existing `log.exception` calls that are not part of this change). ### T009 [P] — `backend/infrahub/git/` remainder — 2 convert -- `repository.py:416`, `tasks.py:1217`. +- `repository.py` → `update_latest_commit`; `tasks.py` → `run_user_check` (again, other + `log.exception` calls in `tasks.py` are pre-existing). ### T010 [P] — remaining single sites — 2 convert -- `database/__init__.py:446` (`ServiceUnavailable`), `services/scheduler.py:91` (keep-alive loop - — the change's clearest win, critique P4). +- `database/__init__.py` → `run_query` (`ServiceUnavailable`); `services/scheduler.py` → + `run_schedule` (keep-alive loop — the change's clearest win, critique P4). ### T010b — auth carve-out (config only; `auth/auth.py` MUST NOT be edited) From 2660959d980c1a66a251437ec390fb26e45f6bd6 Mon Sep 17 00:00:00 2001 From: Saltaferis Dimitrios Date: Fri, 14 Aug 2026 15:06:47 +0000 Subject: [PATCH 9/9] docs(spec): reconcile TRY400 site counts to 27 convert / 7 noqa 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 Claude-Session: https://claude.ai/code/session_01Kkm5ozQ26yojjuzUT65txs --- dev/specs/005-ruff-try400-tracebacks/alignment-check.md | 2 +- .../critiques/critique-2026-08-11.md | 2 +- .../opsmill-implement-report.md | 4 ++-- dev/specs/005-ruff-try400-tracebacks/plan.md | 8 ++++---- dev/specs/005-ruff-try400-tracebacks/research.md | 2 +- dev/specs/005-ruff-try400-tracebacks/tasks.md | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/dev/specs/005-ruff-try400-tracebacks/alignment-check.md b/dev/specs/005-ruff-try400-tracebacks/alignment-check.md index c65737299cb..4438c33bc94 100644 --- a/dev/specs/005-ruff-try400-tracebacks/alignment-check.md +++ b/dev/specs/005-ruff-try400-tracebacks/alignment-check.md @@ -26,7 +26,7 @@ against the inline ask. | Severity | Category | Ask reference | Spec reference | Description | |----------|----------|---------------|----------------|-------------| -| info | expansion | WHAT TO BUILD #2 ("if `exception` would be wrong there, use a targeted `# noqa: TRY400`") | research.md §R4, FR-003 | The ask authorised per-site noqa in the abstract; the spec/research resolve it concretely into 28 conversions + 6 justified suppressions. Elaboration of an explicit instruction, not drift. | +| info | expansion | WHAT TO BUILD #2 ("if `exception` would be wrong there, use a targeted `# noqa: TRY400`") | research.md §R4, FR-003 | The ask authorised per-site noqa in the abstract; the spec/research resolve it concretely into 27 conversions + 7 justified suppressions. Elaboration of an explicit instruction, not drift. | | info | added | — | SC-007 | "Every remaining `# noqa: TRY400` carries a one-line justification" is a criterion the ask implied ("with a one-line reason") but did not list under ACCEPTANCE. Added as a verifiable gate. | | info | added | — | research.md §R3 | The `TracebackSuppressionFilter` interaction was discovered during Phase 0, not present in the ask. It *narrows* scope at one site for a correctness reason and is documented. | | info | changed | ask: "the 34 TRY400 violations ... EXCEPT auth/auth.py" | spec.md Context | The ask's own arithmetic (36 total, 2 in auth ⇒ 34 in scope) is preserved exactly; the spec additionally publishes the full 36-site distribution table. Presentation only. | diff --git a/dev/specs/005-ruff-try400-tracebacks/critiques/critique-2026-08-11.md b/dev/specs/005-ruff-try400-tracebacks/critiques/critique-2026-08-11.md index b0276cc76d9..608bd9814ce 100644 --- a/dev/specs/005-ruff-try400-tracebacks/critiques/critique-2026-08-11.md +++ b/dev/specs/005-ruff-try400-tracebacks/critiques/critique-2026-08-11.md @@ -26,7 +26,7 @@ traceback — 22%. The PR body must say 28/36 rather than implying a clean sweep ### 💡 Recommendations -**P3. 8 of the 28 conversions are in `utilities/infrahub_load_tester.py`** — dev tooling, not +**P3. 8 of the then-28 conversions are in `utilities/infrahub_load_tester.py`** — dev tooling, not production. Their debuggability value is near zero; they are converted because the gate is repo-wide. Not a problem, but the PR body should not count them as production wins. **Accepted** — folded into P2's phrasing requirement. diff --git a/dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md b/dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md index 21d9b851ea1..29b98a648ea 100644 --- a/dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md +++ b/dev/specs/005-ruff-try400-tracebacks/opsmill-implement-report.md @@ -21,7 +21,7 @@ human. | T001 Enable TRY400 in `pyproject.toml` | ✅ | `extend-select = ["TRY400"]`; `"TRY"` ignore annotated | | T002 `git/integrator.py` (9 convert + 4 noqa) | ✅ | | | T003 `utilities/infrahub_load_tester.py` (8 convert) | ✅ | | -| T004 `graphql/app.py` (2 convert) | ✅ | incl. redundant `exc_info` removal | +| T004 `graphql/app.py` (1 convert + 1 noqa) | ✅ | `_observe_subscription` converted incl. redundant `exc_info` removal; `_handle_http_request` kept as `log.error` + noqa | | T005 `git/base.py` (2 convert) | ✅ | | | T006 `workers/infrahub_async.py` (1 convert + 1 noqa) | ✅ | | | T007 `webhook/tasks/process.py` (1 noqa) | ✅ | the dropped-record case | @@ -44,7 +44,7 @@ human. | SC-004 format + lint clean | ✅ `invoke format` clean; `invoke lint` → ruff + **ty** + **mypy** all pass (mypy: 1610 files, no issues) | | SC-005 no gated path in the diff | ✅ no `core/schema/`, `core/migrations/`, `auth/`, `.github/`, no generated file | | SC-006 unit tests pass | ✅ `invoke backend.test-unit` → **2233 passed** | -| SC-007 every `noqa: TRY400` justified | ✅ all 6 carry a reason comment | +| SC-007 every `noqa: TRY400` justified | ✅ all 7 carry a reason comment | Also run green: `invoke main.lint`, `uv lock --check`, `invoke backend.validate-generated`, `invoke docs.validate`, `invoke schema.validate-graphqlschema`, diff --git a/dev/specs/005-ruff-try400-tracebacks/plan.md b/dev/specs/005-ruff-try400-tracebacks/plan.md index 5da55d64948..a1eee4d23cc 100644 --- a/dev/specs/005-ruff-try400-tracebacks/plan.md +++ b/dev/specs/005-ruff-try400-tracebacks/plan.md @@ -47,7 +47,7 @@ suppressions, 2 file-level suppressions). | I. Schema-Driven Integrity | **PASS** — no schema, no generated files. `core/schema/` is explicitly excluded (it only holds TRY004 sites). | | II. Branch-Safe by Default | **PASS** — no queries, no branch/temporal logic touched. | | III. Type Safety & Explicit Contracts | **PASS** — no signatures or types change. Strengthens observability of the existing contracts. | -| IV. Test Discipline | **PASS with note** — a logging-call substitution has no new behaviour to test; the guard is the lint gate itself (SC-001/002) plus the existing unit suite proving no regression. Adding tests that assert on log internals at 28 sites would be test-for-test's-sake. Recorded in Complexity Tracking. | +| IV. Test Discipline | **PASS with note** — a logging-call substitution has no new behaviour to test; the guard is the lint gate itself (SC-001/002) plus the existing unit suite proving no regression. Adding tests that assert on log internals at 27 sites would be test-for-test's-sake. Recorded in Complexity Tracking. | | V. Query Performance & Efficiency | **PASS** — no query changes. | | VI. Security & Input Boundaries | **PASS** — `auth/` is untouched by construction (FR-006). No new data enters a log record beyond the traceback of an already-caught exception. | | VII. Simplicity & Maintainability | **PASS** — net simplification: removes a category-wide suppression for one rule and replaces implicit traceback loss with either a traceback or an explicit justified exception. | @@ -87,7 +87,7 @@ backend/infrahub/ │ ├── integrator.py # 9 convert + 4 noqa │ ├── repository.py # 1 convert │ └── tasks.py # 1 convert -├── graphql/app.py # 2 convert (ASGI error handling only) +├── graphql/app.py # 1 convert + 1 noqa (ASGI error handling only) ├── services/scheduler.py # 1 convert ├── webhook/tasks/process.py # 1 noqa (see research.md §R3) ├── workers/infrahub_async.py # 1 convert + 1 noqa @@ -121,7 +121,7 @@ progress meter, and reaching zero is the completion signal. Each file is independent, so files can be done in any order. For every site: read the handler, apply the §R4 decision, preserve message and keyword arguments exactly. `ruff --fix` for TRY400 -is **unsafe-fix-only** and is not used — the 6 noqa sites and the `graphql/app.py:535` +is **unsafe-fix-only** and is not used — the 7 noqa sites and the `_observe_subscription` `exc_info` removal are exactly the judgements an autofix gets wrong (see §R3, §R5). Each `# noqa: TRY400` carries a one-line reason on the same line or immediately above, so SC-007 @@ -175,5 +175,5 @@ grep -rn 'noqa: TRY400' # SC-007: every one just | Violation | Why Needed | Simpler Alternative Rejected Because | |-----------|------------|-------------------------------------| -| No new tests for 28 changed call sites (Principle IV) | The change is a level-preserving logging substitution with no new behaviour. The lint gate (SC-001/002) is the durable regression guard, and the existing unit suite proves nothing broke. | Asserting on captured log records at each site would pin implementation detail of logging calls, be brittle to message edits, and test structlog rather than Infrahub. | +| No new tests for 27 changed call sites (Principle IV) | The change is a level-preserving logging substitution with no new behaviour. The lint gate (SC-001/002) is the durable regression guard, and the existing unit suite proves nothing broke. | Asserting on captured log records at each site would pin implementation detail of logging calls, be brittle to message edits, and test structlog rather than Infrahub. | | A new `per-file-ignores` entry added by a change whose purpose is *removing* a suppression | `backend/infrahub/auth/auth.py` is off-limits to the automated pipeline, but the rule must still be enforceable repo-wide. | Editing the 2 auth sites inline is the better end state and is what the reviewer will likely ask for — but it requires a human to own the auth-module change. Leaving the rule fully off instead would forfeit the other 34 sites. | diff --git a/dev/specs/005-ruff-try400-tracebacks/research.md b/dev/specs/005-ruff-try400-tracebacks/research.md index 5aca05ed0ac..aab1ed46691 100644 --- a/dev/specs/005-ruff-try400-tracebacks/research.md +++ b/dev/specs/005-ruff-try400-tracebacks/research.md @@ -56,7 +56,7 @@ silently stop being reported. mode a blind `--fix --unsafe-fixes` run would have introduced, and it is invisible in a diff review. -## R4 — Per-site decision: convert (28) vs. justified `# noqa` (6) +## R4 — Per-site decision: convert (27) vs. justified `# noqa` (7) All 34 in-scope sites were read at their call site. The rule applied: **convert unless the traceback would be actively harmful or worthless.** diff --git a/dev/specs/005-ruff-try400-tracebacks/tasks.md b/dev/specs/005-ruff-try400-tracebacks/tasks.md index 379e18b1a6d..7384464032b 100644 --- a/dev/specs/005-ruff-try400-tracebacks/tasks.md +++ b/dev/specs/005-ruff-try400-tracebacks/tasks.md @@ -147,7 +147,7 @@ Both sites are in `_init_infrahub_client`: Per critique P2/P3/P4, the PR body MUST: -- state **28 of 36** sites converted, not a clean sweep — 6 justified in-line `# noqa` + 2 +- state **27 of 36** sites converted, not a clean sweep — 7 justified in-line `# noqa` + 2 deferred in `auth/auth.py`; - lead with the `webhook/tasks/process.py:204` finding (converting it would have silently deleted the delivery-failure log record) — it is the reason this was not an autofix;