Skip to content

Re-enable ruff BLE (blind-except) rule and fix all violations - #10002

Merged
saltas888 merged 15 commits into
developfrom
pha/INBOX-19
Aug 6, 2026
Merged

Re-enable ruff BLE (blind-except) rule and fix all violations#10002
saltas888 merged 15 commits into
developfrom
pha/INBOX-19

Conversation

@saltas888

@saltas888 saltas888 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Removes BLE from the global ruff ignore list in pyproject.toml and resolves all 78 violations it surfaces (the card's original ~32 estimate was from a February analysis; the count grew — reconciled and re-measured as part of this work, see dev/specs/002-ruff-ble-reenable/data-model.md).
  • 8 sites (test/tooling code) got genuine exception narrowing to the specific type(s) the guarded code can raise.
  • 70 sites kept except Exception with a targeted # noqa: BLE001 plus a one-line justification comment, where a broad catch is a deliberate design choice (best-effort telemetry, top-level migration/error-collection loops, etc.) — annotation-only, no logic or control-flow changes.
  • Full spec-kit trail in dev/specs/002-ruff-ble-reenable/ (spec, plan, critique, tasks, alignment check, final implementation report).

Reviewer note: 24 of the changed files fall in paths this repo normally reviews extra carefully — 19 under backend/infrahub/core/migrations/graph/, 4 auth modules (api/auth.py, api/oauth2.py, api/oidc.py, auth/auth.py), and core/schema/update_coordinator.py. Every touch in those files is the identical mechanical pattern (justification comment + # noqa: BLE001 above an existing except Exception:) — confirmed by an explicit diff-audit task (SC-007) and independently re-verified by the review pass below. No behavior change. Worth a close look given the sensitivity of those paths regardless.

Source: Engineering Inbox INBOX-19, from Patrick's pyproject suppression-analysis thread (ranked BLE re-enable priority #1 — direct bug risk, small bounded count, lowest effort).

Test plan

  • ruff check --select=BLE . — 0 violations repo-wide
  • uv run invoke backend.lint (ruff + ty + mypy) — clean
  • uv run invoke backend.test-unit — 1846 passed
  • Two previously-non-collecting component tests (test_process_idempotency.py, test_uniqueness_propagation.py) confirmed passing — 12 passed (the earlier "no tests collected" signal was a CLI-proxy summarizer false negative, not a real defect)
  • Suppression audit — 70/70 sites carry a justification comment; enforcement canary (reintroducing one unjustified BLE001) confirmed the rule now fails as expected, then reverted
  • uv lock --check, schema.validate-graphqlschema, schema.validate-jsonschema, backend.validate-generated, docs.validate — all clean, zero diff
  • Two independent review passes (error-handling, simplification) across the full diff — no CRITICAL/HIGH findings; one pre-existing/accepted design tradeoff and a few cosmetic nits noted in the implementation report, none blocking

🤖 Generated with Claude Code


Summary by cubic

Re-enabled the ruff BLE (blind-except) rule and fixed all 78 violations by narrowing where safe and documenting intentional broad handlers. Updated two suppressions to clearly note rollback-in-transaction and retry-bypass caveats. Aligns with INBOX-19.

  • Refactors
    • Removed BLE from pyproject.toml ignore; repo passes ruff with the rule active.
    • Narrowed 8 sites to specific exceptions: SchemaNotFoundError (component tests), httpx.HTTPError (git integration fixtures), packaging.InvalidVersion (tasks/release.py).
    • Kept 70 deliberate broad handlers with # noqa: BLE001 and a one-line justification; revised comments in m066_consolidate_duplicate_number_pools.py and core/validators/tasks.py to state the rollback and retry caveats; no behavior change.
    • Added changelog fragment and spec-kit docs under dev/specs/002-ruff-ble-reenable/.
    • Lint and tests remain green (invoke backend.lint and existing test suites).

Written for commit 5ec7bc0. Summary will update on new commits.

Review in cubic

saltas888 and others added 13 commits July 22, 2026 07:25
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
78-site inventory with per-site treatment matrix (8 narrow / 70 suppress),
research decisions, and validation quickstart.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Verdict PROCEED, no must-address findings. Applied: PR-narrative note
(plan), rollback + release.py sanity checks (quickstart), changelog
assumption harmonized (spec).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
22 tasks across 3 user stories: 8 parallel suppress batches (US3),
3 parallel narrow batches (US2), config flip + verification (US1),
plus setup drift-check and polish audits.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Source PRD resolved: inline card text + Slack suppression-analysis
thread (fetched). No missing/changed/dropped requirements; 0 of 2
remediation passes used.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ciled, no drift (INBOX-19)

Re-ran `uv run ruff check --select=BLE --output-format=concise .` from the
repo root: 78 BLE001 violations (77 `Exception`, 1 `BaseException` at
backend/tests/helpers/test_worker.py:107), byte-identical to the
data-model.md batch tables — zero line drift, zero new sites, zero
removed sites, no python_sdk findings. data-model.md unchanged; only the
T001 checkbox is ticked.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Tasks T002–T004: annotation-only SUPPRESS treatment for all 30 Batch A
sites per data-model.md — a justification comment above each except plus
a line-targeted `# noqa: BLE001`. Zero semantic tokens changed (SC-007);
m066/m073 and shared.py:157/:245 use the transaction-safe wordings.
The noqas read as RUF100 "non-enabled" until T015 removes BLE from the
ignore list — expected interim state.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…NBOX-19)

Tasks T005–T007: annotation-only SUPPRESS treatment for the 8 Batch B
auth sites and 16 Batch C backend-runtime sites per data-model.md — a
justification comment above each except plus a line-targeted
`# noqa: BLE001`. Zero semantic tokens changed; auth diffs are
comment/noqa-only (SC-007). Two flagged sites got noqa only, keeping
their existing justification comments: git/sync.py:120 (lines 121-122)
and telemetry/tasks.py:129 (line 125 already states why broad).
The noqas read as RUF100 "non-enabled" until T015 removes BLE from the
ignore list — expected interim state.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…green (INBOX-19)

Batch D (7 backend-test sites) and Batch E (9 tooling sites) receive the
SUPPRESS treatment from data-model.md: a justification comment above each
except plus a line-targeted noqa. Annotation-only — zero semantic tokens
changed; test_worker.py keeps its BaseException catch and the load tester's
latent missing-return stays untouched by design.

US3 checkpoint (T010): remaining BLE001 violations are exactly the 8 NARROW
sites; ruff format clean on all 42 touched files; migrations+auth diff vs
base is mechanically verified comment/noqa-only (SC-007).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…een (INBOX-19)

Narrow the 8 analyzable BLE001 handlers per the data-model treatment
matrix (Batches D/E NARROW rows), handler bodies untouched:

- schema_branch component tests: except Exception -> SchemaNotFoundError
  in the duplicated _describe_hash_diff helper (2 sites per file)
- integration/git/conftest.py poll loops: except Exception -> httpx.HTTPError,
  dropping the now-stale noqa: S110 (typed excepts are S110-exempt)
- tasks/release.py version probes: except Exception -> InvalidVersion,
  extending the deliberate function-local packaging imports

US2 checkpoint: repo BLE001 = 0 (--exclude python_sdk, per the CI lint
gate); ruff format clean; InvalidVersion probe and invoke --list green;
the 12 touched component tests pass locally against the neo4j
testcontainer (12 passed in 65.16s).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…green (INBOX-19)

Phase 5 (US1), tasks T015-T018:
- T015: remove "BLE" from the [tool.ruff.lint] ignore list in pyproject.toml
- T016: towncrier housekeeping fragment for the BLE enforcement
- T017: full gates green — ruff --select=BLE (0, python_sdk excluded:
  initialized submodule is out of scope), full ruff check, ruff format
  --check, invoke backend.lint (ruff + ty + mypy) all exit 0
- T018: canary mutation check — 1 x BLE001 reported on a planted blind
  except in tasks/utils.py, canary reverted, tree clean

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…quickstart replay (INBOX-19)

T019: 70/70 noqa: BLE001 suppressions counted (= data-model.md SUPPRESS total);
68 carry the justification on/above the except line, 2 use the tabled alternate
positions (git/sync.py:120 below-line, telemetry/tasks.py:129 try-block header);
E722 backstop clean.
T020: backend unit suite green — 1846 passed, 17 warnings in 33.63s.
T021: constraint-area diff from feature fork point 9247c67 is annotation-only —
23 files, +76/-38; every addition is a comment or a byte-matched noqa'd except
line (0 violations). Note: the stable merge-base 01a1ab5 predates the fork and
includes unrelated upstream develop work; audit uses the fork parent.
T022: full quickstart replay green (BLE=0, CI gates, backend.lint, canary
mutation check, suppression audit, InvalidVersion probe); component tests cited
from the prior local run (12 passed).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…een (INBOX-19)

Verification + review tail for the ruff BLE re-enablement feature. All 22
tasks were already [X] on entry (completed by two prior, killed sessions);
this run independently re-verified every gate rather than trusting the
inherited state, then ran the review phase and wrote the final report.

Re-confirmed fresh: BLE=0 repo-wide (--exclude python_sdk), full
invoke backend.lint (ruff+ty+mypy), full ruff check/format --exclude
python_sdk, E722=0, 70/70 noqa: BLE001 suppressions, SC-007 diff audit
(migrations/auth annotation-only), SC-006 enforcement canary (1 x BLE001,
reverted clean), tasks/release.py InvalidVersion + invoke --list sanity,
full backend unit suite (1846 passed), and the two previously-flagged
component tests (12 passed, 44.33s).

Root-caused the "pytest component-test collection issue" flagged by prior
runs: it was a false negative from the rtk CLI proxy's output summarizer
("Pytest: No tests collected" on a fully passing run), not a real
collection defect — reproduced and confirmed via the raw (unfiltered)
command path. No code change needed.

Ran the error-handling and simplification review agents in parallel across
the full feature diff (9247c67..06fd34d). No CRITICAL/HIGH findings;
2 LOW/MEDIUM findings recorded (one is a pre-existing, deliberately
accepted planning tradeoff already documented in data-model.md/
quickstart.md, not a new defect) plus 3 LOW simplify nits — none block
merge, all deferred per the report.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@saltas888
saltas888 requested a review from a team as a code owner July 22, 2026 10:02

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 59 files

Confidence score: 4/5

  • In utilities/infrahub_load_tester.py, the exception path says failures are absorbed, but all_branches can remain undefined, so later branch-deletion logic may raise follow-on errors and mask the original request failure; initialize all_branches to a safe default (for example, an empty list) and gate deletion when branch fetch fails.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="utilities/infrahub_load_tester.py">

<violation number="1" location="utilities/infrahub_load_tester.py:48">
P2: The new justification comment claims this except "absorb[s] any request failure and continue[s]," but `all_branches` is never defined on failure, so every subsequent branch-deletion attempt will fail with a confusing `UnboundLocalError` message instead of cleanly skipping. Consider returning (like `delete_admin_branches` does) or defaulting `all_branches = []` in the except block.</violation>
</file>

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread backend/infrahub/core/validators/tasks.py
log.info(f"✅ Created proposed change for branch {branch_name}")
except Exception as e:
# Load test: absorb any request failure and continue
except Exception as e: # noqa: BLE001

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The new justification comment claims this except "absorb[s] any request failure and continue[s]," but all_branches is never defined on failure, so every subsequent branch-deletion attempt will fail with a confusing UnboundLocalError message instead of cleanly skipping. Consider returning (like delete_admin_branches does) or defaulting all_branches = [] in the except block.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At utilities/infrahub_load_tester.py, line 48:

<comment>The new justification comment claims this except "absorb[s] any request failure and continue[s]," but `all_branches` is never defined on failure, so every subsequent branch-deletion attempt will fail with a confusing `UnboundLocalError` message instead of cleanly skipping. Consider returning (like `delete_admin_branches` does) or defaulting `all_branches = []` in the except block.</comment>

<file context>
@@ -44,7 +44,8 @@ async def _create_one(idx: int, client: InfrahubClient, prefix: str, log: loggin
         log.info(f"✅ Created proposed change for branch {branch_name}")
-    except Exception as e:
+    # 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}")
 
</file context>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Treat this as a separate issue if we want to clean this up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 platform-health-agent — understood, leaving it as-is. For the record, the UnboundLocalError path cubic describes is real but pre-existing: this PR adds only the comment + # noqa: BLE001. Flagged as a follow-up candidate (default all_branches = [] or return early in the except, mirroring delete_admin_branches) alongside the m066 and validators findings — happy to file those as Engineering Inbox cards if you want them tracked rather than left in this thread.

@github-actions github-actions Bot added group/backend Issue related to the backend (API Server, Git Agent) type/spec A specification for an upcoming change to the project labels Jul 22, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name=".specify/feature.json">

<violation number="1">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**

The `.specify/feature.json` file is updated to point to `specs/001-entities-arch-migration`, but that directory does not exist in the repository. The PR's stated purpose is re-enabling the ruff BLE rule with spec-kit documentation located at `dev/specs/002-ruff-ble-reenable/` (and `specs/002-ruff-ble-reenable` also exists). This appears to be an unintended change — likely a leftover from a different branch or feature — and contradicts the PR's scope. It should remain `specs/002-ruff-ble-reenable` to align with the BLE re-enable work being merged.</violation>
</file>

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

@codspeed-hq

codspeed-hq Bot commented Jul 22, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 12 untouched benchmarks


Comparing pha/INBOX-19 (5ec7bc0) with develop (10e39b7)

Open in CodSpeed

await service.event.send(event=event)
except Exception as ex:
# Login event emission is best-effort telemetry; it must never fail a successful login
except Exception as ex: # noqa: BLE001

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this was needed in the first place. There would be an exception here if the event itself was malformed i.e. we didn't properly create the event. That should never happen as we control the data used as input. When sending the events we'd be sending them to an internal queue which is then later picked up. I don't think this ever blocks the current code path even if we fail to send the event to Prefect. In some other cases we do send things like this as a background task that gets executed after the response is returned the user which could be a better option. The same would be true for oauth2 and oicd, as this PR isn't really changing the behavior this part could be done as a follow up job.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 platform-health-agent — thanks, and agreed with your own conclusion that this is a follow-up rather than something for this PR: the try/except around the event emission already existed, and this PR adds only the comment + # noqa: BLE001, so behaviour is unchanged here.

To make sure the follow-up captures your point accurately: the suggestion is that the defensive catch is unnecessary in the first place (event construction is fully under our control, and emission goes to an internal queue that cannot block the login/logout response), so the better shape is to drop the try/except — or move emission to a background task that runs after the response is returned — rather than keep a suppressed broad catch. Same treatment for api/oauth2.py and api/oidc.py, which carry the identical pattern.

I have not changed anything here. If you would like this tracked as an Engineering Inbox card (all three files, "remove/relocate best-effort event-emission catches in auth endpoints"), say so and I will file it with your reasoning quoted.

Comment thread backend/infrahub/core/validators/tasks.py
obj_after = after.get(name=name, duplicate=False)
dump_after = obj_after.model_dump()
except Exception:
except SchemaNotFoundError:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are great when we can replace a catch all error (including Ctrl+C breaks) with a more specific one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 platform-health-agent — thanks, that matches the split this change deliberately made: 8 sites were narrowed to specific exception types (all in test/tooling code, where the raisable set is knowable and a stray KeyboardInterrupt/SystemExit catch is pure downside), and the remaining 70 kept except Exception with a justification comment rather than being narrowed speculatively in code paths where guessing the exception set could change production behaviour.

I am harvesting this preference — narrow where the exception set is knowable, suppress with justification only where a broad catch is a deliberate design choice — into the repo's internal convention docs via the maintained pha/conventions PR, so the next agent run applies it by default instead of relearning it from review.

log.info(f"✅ Created proposed change for branch {branch_name}")
except Exception as e:
# Load test: absorb any request failure and continue
except Exception as e: # noqa: BLE001

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Treat this as a separate issue if we want to clean this up.

saltas888 added a commit that referenced this pull request Aug 5, 2026
…NBOX-19]

PR #10002 (INBOX-19) re-enabled ruff's BLE (blind-except) rule and resolved
78 violations: 8 narrowed to specific exception types, 70 kept `except
Exception` with a justification comment + `# noqa: BLE001`. Reviewer
@ogenstad's threads on that PR, plus an independent cubic-dev-ai finding,
surfaced three durable gaps beyond "narrow when you can" (already
documented in python.md's Exception Handling section):

- No existing guidance asked whether a catch is needed *before* choosing
  its scope. @ogenstad on api/auth.py: the guarded event-emission goes to
  an internal queue that can't block the response, so the defensive catch
  is likely unnecessary altogether - a background task after the response
  is the better shape when isolation is genuinely required. Same pattern
  flagged on oauth2.py and oidc.py.
- "Deliberate design choice" (the bar for `# noqa: BLE001`) was previously
  illustrated only by "top-level boundary" (task worker loop, request
  handler) - too narrow for the batch/collection and best-effort shapes
  this PR actually used 70 times over. Named all three explicitly with a
  worked example.
- cubic-dev-ai flagged that the justification comment on
  m066_consolidate_duplicate_number_pools.py's except ("failures become
  MigrationResult errors") overstates safety: the `return` happens inside
  `async with db.start_transaction()`, so `InfrahubDatabase.__aexit__`
  (backend/infrahub/database/__init__.py:298-307) sees no exception and
  *commits* the partial work instead of rolling it back. Verified directly
  against both files. The new subsection requires a justification comment
  to state the actual commit/rollback consequence when nested inside a
  transaction, with a before/after example built on the real bug shape.

Also documented (@ogenstad, twice, on validators/tasks.py and
infrahub_load_tester.py): a lint-suppression PR that surfaces a
pre-existing behavioral bug should ship the annotation only and file the
bug separately, not fix it inline. New git-workflow.md bullet under Pull
Requests, cross-linked from the noqa subsection above since the same
transaction/commit bug is the concrete example of exactly this pattern.

Extended dev/knowledge/backend/async-tasks.md's existing "post-commit
follow-up is best-effort" contract with one sentence: it was scoped to
per-item batch dispatch, but the same shape (confirm the call can fail,
then isolate via a background task rather than an inline try/except)
applies to a single inline side effect in a request handler too.

Not encoded as a lesson: the three specific bugs flagged in review
(m066 commit-not-rollback, tasks.py retry-defeating catch,
infrahub_load_tester.py's undefined `all_branches` on failure) are
PR-local defects, not conventions - the review thread itself already
treats them as follow-up candidates pending a human filing decision.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K6uYDCYNXbWUdkFEJLW3Zx
Address review feedback on the justification comments: both sites keep
their existing broad catch (no behaviour change), but the comments now
state the caveat each one hides rather than implying a clean failure path.

The number-pool consolidation handler returns from inside the transaction
context, so that context commits and a mid-loop failure can persist a
partial consolidation. The schema-validation handler converts transient
errors into permanent violations on the first attempt, bypassing the
configured task retries. Both narrowings are separate follow-ups.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@saltas888

Copy link
Copy Markdown
Contributor Author

🤖 platform-health-agent — reconcile pass (session pha-test-2026-07-22-0710)

Pushed: 5ec7bc0aa — the two comment-wording changes I held back last pass (I did not want to mark fresh review comments outdated while @ogenstad was still commenting). Comment-only, no behaviour change anywhere; ruff --select=BLE still clean under the CI invocation (--exclude python_sdk) and invoke main.lint passes. Threads for those two sites are resolved.

Left alone, as directed: the behavioural fixes in m066, core/validators/tasks.py, utilities/infrahub_load_tester.py, and the auth event-emission catches. All are pre-existing; this PR only ever added a comment + # noqa: BLE001 at those sites.

Open question — four follow-ups, not yet filed. @ogenstad across three threads you said these belong in separate issues, and I offered to file them but did not want to create Jira cards off my own initiative without your word. They are, in rough value order:

  1. Number-pool consolidation commits on failure (m066) — verified real: the handler returns from inside the transaction context, so that context commits and a mid-loop failure can persist a partial consolidation while the migration reports an error. Data-integrity risk; the strongest candidate of the four.
  2. Auth event-emission catches (api/auth.py, api/oauth2.py, api/oidc.py) — your point that the defensive catch is unnecessary in the first place, since event construction is under our control and emission goes to an internal queue that cannot block the response; better shape is to drop it or move emission to a post-response background task.
  3. Schema-validation broad catch defeats task retries (core/validators/tasks.py) — narrowing so transient/infra errors can propagate and trigger the configured retries instead of becoming permanent violations on the first attempt.
  4. UnboundLocalError on the load-tester failure path (utilities/infrahub_load_tester.py) — default the variable or return early in the except. Tooling-only, lowest value.

Say the word and I will file them as Engineering Inbox cards with your reasoning quoted; otherwise they are captured here and on INBOX-19 so they are not lost when this merges. Nothing here blocks the merge — your approval stands and CI is re-running on the new commit.

Separately, the review preference you noted ("great when we can replace a catch-all with a more specific one") is harvested into the maintained conventions PR #10030 so future agent runs apply it by default.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 2 files (changes from recent commits).

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Shadow auto-approve: would auto-approve. Re-enables a lint rule and fixes all violations with annotation-only changes (justification comments + noqa) on existing except blocks; no behavior, logic, or operational tradeoffs.

Re-trigger cubic

@saltas888

Copy link
Copy Markdown
Contributor Author

🤖 platform-health-agent — reconcile pass (session pha-test-2026-07-22-0710)

CI was red on E2E-testing-playwright; diagnosed as unrelated flake and re-triggered. No code change.

Two specs failed on run 30995873309:

  • branches/branch-selector.spec.ts:51 — 180s timeout, getByLabel('New branch name *') element not found
  • objects/list/object-list-select-range.spec.ts:66apiRequestContext.post: socket hang up on POST /graphql, then a cascading Branch: … not found during teardown

237 passed, 2 failed. Why this isn't from this PR:

  1. E2E-testing-playwright is currently failing on develop itself — the two most recent develop runs both failed (10e39b70 today 06:27Z, c10dc61f 08-04 15:14Z). The 06:27Z develop failure is a different spec (objects/object-relationships.spec.ts:97, getByText('NameLoopback0') not visible) with the same signature: 237 passed / 1 failed, UI timeout.
  2. Failure modes are environmental, not logical — element-not-found timeouts and a socket hang-up, i.e. the app/runner dropping connections under load, not an application error surfacing.
  3. This PR has zero frontend changes (git diff --name-only origin/develop...HEAD -- frontend/ is empty). The three backend/infrahub/api/ files in the diff (auth.py, oauth2.py, oidc.py) are comment + # noqa: BLE001 only — no logic. Login clearly works in the failing run, since the branch-selector spec got as far as the authenticated branch-selector UI.

Different specs failing per run, on a base branch that is itself red, is a flaky-suite signature rather than a regression — so I re-ran only the failed job rather than "fixing" anything. Re-run started 11:03Z; the next tick will read the result.

If it fails again, this is likely worth a human call: the flake appears to predate and be independent of this PR (it is red on develop), so it may need its own investigation rather than blocking this change.

Review side: no action needed — @ogenstad approved at 08:25Z, and all three open threads already have replies from the previous pass. They are deliberately left unresolved because each ends in an open question to you (whether to file the api/auth.py catch removal, the infrahub_load_tester.py UnboundLocalError, and the m066/validators findings as Engineering Inbox cards). Say the word and I will file them.

@saltas888

Copy link
Copy Markdown
Contributor Author

🤖 platform-health-agent — reconcile pass (session pha-test-2026-07-22-0710)

CI is now fully green — the E2E flake cleared on re-run. No code change this pass.

E2E-testing-playwright and all its shards (branches_repo, sites_a, sites_b, foundation, tutorial, version-upgrade) pass: 47 passed, 0 failed on 5ec7bc0aa. That confirms the two failures I re-triggered last pass were unrelated flake, not a regression from this branch. Merge state is CLEAN / MERGEABLE against develop.

One thing for whoever merges: @ogenstad's approval landed at 08:25 on 89e7ec451. The only change since is 5ec7bc0aa — the two comment-wording edits requested in review, comment-only with no behaviour change, which cubic then re-reviewed with 0 issues found. So the approval predates the current head by one cosmetic commit; flagging it rather than treating the approval as automatically covering it.

Three review threads are intentionally left unresolvedutilities/infrahub_load_tester.py:48, backend/infrahub/api/auth.py:64, backend/tests/.../test_uniqueness_propagation.py:49. Each is a discussion that concluded the underlying issue is pre-existing and a follow-up, not something this PR should change (this PR only adds a comment + # noqa: BLE001 at those sites). I've replied on each; I'm deliberately not resolving them, since closing a reviewer's thread on "we're not doing it here" is the reviewer's call, and their open state keeps the follow-ups visible. Resolve them if you'd rather they not block the view.

Nothing else outstanding: no unaddressed review comments, no failing checks. This PR is ready for a human to merge — the agent never merges.

@saltas888

Copy link
Copy Markdown
Contributor Author

🤖 platform-health-agent — reconcile pass (session pha-test-2026-07-22-0710)

No code change this pass. Closing out the one review finding that was never answered: cubic's .specify/feature.json flag is a false positive.

The second cubic review flagged .specify/feature.json as "an unintended change — likely a leftover from a different branch" because it points at specs/001-entities-arch-migration. I verified that against the tree rather than taking it at face value, and this PR does not touch that file:

  • The blob is identical at the merge base and at branch head — 1be33c1f in both (git ls-tree 9247c6763 -- .specify/feature.json vs git ls-tree origin/pha/INBOX-19 -- .specify/feature.json).
  • It is absent from the PR's own file list: gh api repos/opsmill/infrahub/pulls/10002/files returns 58 files, none under .specify/.

So the stale 001-entities-arch-migration pointer is pre-existing on develop and unrelated to this branch. It did briefly appear in the diff before 89e7ec451 ("resolve conflicts"), which is what cubic reviewed — that commit dropped it. Nothing to fix here; the observation about the stale pointer itself may still be worth a separate cleanup, since it presumably affects anyone running spec-kit on develop.

Everything else is unchanged from my last pass and still green:

Check State
CI 47 passed, 0 failed on 5ec7bc0aa
Merge state MERGEABLE / CLEAN
Human review @ogenstad approved (08:25) — still the standing opinionated review
Bot re-review after last push cubic, 0 issues, "would auto-approve"

The approval-predates-head caveat from my previous comment still applies (approval landed on 89e7ec451; head is 5ec7bc0aa, comment-only). The three unresolved threads remain deliberately unresolved for the reviewer to close — see my previous comment for the rationale.

My offer to file the three pre-existing findings (load_tester UnboundLocalError, m066, validators/tasks) as Engineering Inbox cards is still open and unanswered — say the word and I'll file them; I'm not filing unbidden.

This PR is ready for a human to merge — the agent never merges.

@saltas888
saltas888 merged commit a9b746f into develop Aug 6, 2026
108 of 109 checks passed
@saltas888
saltas888 deleted the pha/INBOX-19 branch August 6, 2026 10:18
saltas888 added a commit that referenced this pull request Aug 6, 2026
…NBOX-19]

PR #10002 (INBOX-19) re-enabled ruff's BLE (blind-except) rule and resolved
78 violations: 8 narrowed to specific exception types, 70 kept `except
Exception` with a justification comment + `# noqa: BLE001`. Reviewer
@ogenstad's threads on that PR, plus an independent cubic-dev-ai finding,
surfaced three durable gaps beyond "narrow when you can" (already
documented in python.md's Exception Handling section):

- No existing guidance asked whether a catch is needed *before* choosing
  its scope. @ogenstad on api/auth.py: the guarded event-emission goes to
  an internal queue that can't block the response, so the defensive catch
  is likely unnecessary altogether - a background task after the response
  is the better shape when isolation is genuinely required. Same pattern
  flagged on oauth2.py and oidc.py.
- "Deliberate design choice" (the bar for `# noqa: BLE001`) was previously
  illustrated only by "top-level boundary" (task worker loop, request
  handler) - too narrow for the batch/collection and best-effort shapes
  this PR actually used 70 times over. Named all three explicitly with a
  worked example.
- cubic-dev-ai flagged that the justification comment on
  m066_consolidate_duplicate_number_pools.py's except ("failures become
  MigrationResult errors") overstates safety: the `return` happens inside
  `async with db.start_transaction()`, so `InfrahubDatabase.__aexit__`
  (backend/infrahub/database/__init__.py:298-307) sees no exception and
  *commits* the partial work instead of rolling it back. Verified directly
  against both files. The new subsection requires a justification comment
  to state the actual commit/rollback consequence when nested inside a
  transaction, with a before/after example built on the real bug shape.

Also documented (@ogenstad, twice, on validators/tasks.py and
infrahub_load_tester.py): a lint-suppression PR that surfaces a
pre-existing behavioral bug should ship the annotation only and file the
bug separately, not fix it inline. New git-workflow.md bullet under Pull
Requests, cross-linked from the noqa subsection above since the same
transaction/commit bug is the concrete example of exactly this pattern.

Extended dev/knowledge/backend/async-tasks.md's existing "post-commit
follow-up is best-effort" contract with one sentence: it was scoped to
per-item batch dispatch, but the same shape (confirm the call can fail,
then isolate via a background task rather than an inline try/except)
applies to a single inline side effect in a request handler too.

Not encoded as a lesson: the three specific bugs flagged in review
(m066 commit-not-rollback, tasks.py retry-defeating catch,
infrahub_load_tester.py's undefined `all_branches` on failure) are
PR-local defects, not conventions - the review thread itself already
treats them as follow-up candidates pending a human filing decision.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K6uYDCYNXbWUdkFEJLW3Zx
saltas888 added a commit that referenced this pull request Aug 6, 2026
…NBOX-19]

PR #10002 (INBOX-19) re-enabled ruff's BLE (blind-except) rule and resolved
78 violations: 8 narrowed to specific exception types, 70 kept `except
Exception` with a justification comment + `# noqa: BLE001`. Reviewer
@ogenstad's threads on that PR, plus an independent cubic-dev-ai finding,
surfaced three durable gaps beyond "narrow when you can" (already
documented in python.md's Exception Handling section):

- No existing guidance asked whether a catch is needed *before* choosing
  its scope. @ogenstad on api/auth.py: the guarded event-emission goes to
  an internal queue that can't block the response, so the defensive catch
  is likely unnecessary altogether - a background task after the response
  is the better shape when isolation is genuinely required. Same pattern
  flagged on oauth2.py and oidc.py.
- "Deliberate design choice" (the bar for `# noqa: BLE001`) was previously
  illustrated only by "top-level boundary" (task worker loop, request
  handler) - too narrow for the batch/collection and best-effort shapes
  this PR actually used 70 times over. Named all three explicitly with a
  worked example.
- cubic-dev-ai flagged that the justification comment on
  m066_consolidate_duplicate_number_pools.py's except ("failures become
  MigrationResult errors") overstates safety: the `return` happens inside
  `async with db.start_transaction()`, so `InfrahubDatabase.__aexit__`
  (backend/infrahub/database/__init__.py:298-307) sees no exception and
  *commits* the partial work instead of rolling it back. Verified directly
  against both files. The new subsection requires a justification comment
  to state the actual commit/rollback consequence when nested inside a
  transaction, with a before/after example built on the real bug shape.

Also documented (@ogenstad, twice, on validators/tasks.py and
infrahub_load_tester.py): a lint-suppression PR that surfaces a
pre-existing behavioral bug should ship the annotation only and file the
bug separately, not fix it inline. New git-workflow.md bullet under Pull
Requests, cross-linked from the noqa subsection above since the same
transaction/commit bug is the concrete example of exactly this pattern.

Extended dev/knowledge/backend/async-tasks.md's existing "post-commit
follow-up is best-effort" contract with one sentence: it was scoped to
per-item batch dispatch, but the same shape (confirm the call can fail,
then isolate via a background task rather than an inline try/except)
applies to a single inline side effect in a request handler too.

Not encoded as a lesson: the three specific bugs flagged in review
(m066 commit-not-rollback, tasks.py retry-defeating catch,
infrahub_load_tester.py's undefined `all_branches` on failure) are
PR-local defects, not conventions - the review thread itself already
treats them as follow-up candidates pending a human filing decision.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K6uYDCYNXbWUdkFEJLW3Zx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

group/backend Issue related to the backend (API Server, Git Agent) type/spec A specification for an upcoming change to the project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants