chore(backend): drop the stale no-untyped-def mypy override for core.node.base - #10146
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
CI is red for a reason outside this PR —
|
| Branch | BLE in ruff ignore list |
|---|---|
release-1.11 |
ignored (pyproject.toml:512 — "BLE", # flake8-blind-except) |
develop |
not ignored — enforced |
backend/infrahub/telemetry/utils.py was written on release-1.11, where BLE is ignored, so except Exception as exc: linted clean there. PR #10143 (release-1.11 → develop, merged today 12:01 UTC) carried that code into a branch that enforces BLE001, and develop went red.
Blast radius
Every PR based on post-merge develop inherits this. Other open PRs (#10147, #10144, #10141, #10137) still show python-lint green only because their last runs predate the 12:01 merge — they will go red on their next run. This PR is simply the first to be tested against post-merge develop.
Not fixing it here
The fix is a policy call that belongs on develop in its own PR, not inside an unrelated tech-debt change — either narrow the exception in safe_metric, add a scoped # noqa: BLE001, or reconcile the BLE ignore between release-1.11 and develop so the branches stop disagreeing. Escalated to a human on INBOX-30.
This PR's own verification is green: full mypy (1609 files), backend.test-unit (2233 passed), backend.validate-generated (no drift), uv lock --check.
🤖 platform-health-agent reconcile pass
|
This needs to be rebased and the CI should be fine. |
|
@ogenstad you were right — rebased, and When I escalated at 13:03 UTC the branch was 0 commits behind Verified locally on the rebased branch using the exact commands CI's
Also fixed a second, unrelated point raised by the cubic review: the changelog fragment was missing towncrier's One note that may be worth a separate ticket: this failure was invisible to the local pre-push gate. |
|
🤖 platform-health-agent — reconcile pass (CI red). All 4 failures share one root cause, and it is not this PR. Every failing job died at stack startup, not in a test assertion:
Evidence that it is external to this branch:
Action taken: re-ran only the failed jobs — no push, no branch change. That is the appropriate remedy for a container healthcheck failure and distinguishes a flake from a systemic break. If the re-run reproduces the same Thanks for the |
|
🤖 platform-health-agent — reconcile pass (CI red). Escalating to a human; no code change pushed. @ogenstad thanks for the approval. The rebase you called for did fix the
Why it isn't this PR: the entire diff vs Current state: 43 of 44 checks pass, approved, branch rebased onto Your call: re-run Separately, and only as an observation: this job runs the whole |
PR #10146 (INBOX-30) removed a stale mypy override with no source change, and its review threads surfaced three durable gaps. Two were suggested candidates going in; investigation confirmed one and sharpened the other, plus turned up a third while mining the same threads. - cubic-dev-ai flagged a changelog fragment missing towncrier's `+` orphan prefix. The rule already exists in four places (the skill file, two backend/AGENTS.md refs, git-workflow.md, and the PR template checklist), so this is "covered but still flagged," not missing: the doc states the rule but never the failure mode, which is the concrete detail cubic had to supply. Added the consequence (bogus issue link / skipped fragment) to creating-changelog-entries/SKILL.md's Common Mistakes list. - Reviewer @ogenstad's "this needs to be rebased" corrected an escalation that reasoned from "0 commits behind, so a rebase is a no-op." That reasoning was actually correct at the moment it was made — the defect was trusting it unchanged after time passed (waiting on the review) and the upstream base got fixed in the interim (#10147). Neither rebase/SKILL.md (which documents a different mistake: counting against a stale local ref instead of origin/<base>) nor monitoring-pull- requests/SKILL.md addressed re-verifying a stale snapshot. Added a callout to the latter's Phase 2: re-fetch and recompute immediately before deciding, reproduce against the base's current tip, and treat a reviewer's rebase suggestion as a cue to re-verify rather than repeat an earlier count. - Self-disclosed in the same thread: `invoke lint`/`invoke backend.lint` run `ruff check --diff` (tasks/main.py:39, tasks/backend.py:101), which reports only auto-fixable rewrites and exits 0 on an unfixable violation. CI's python-lint job runs a plain `ruff check` (.github/workflows/ci.yml:325-326), so a locally-green lint run is not proof CI's will be. Verified both invocations directly before writing this down. Documented next to the commands themselves in root AGENTS.md's Linting & Formatting section. Not encoded: @ogenstad's unresolved "not sure we need a changelog entry for something like this" — no reply landed and the fragment shipped as-is, so there's no correction to derive a rule from. The existing housekeeping rule already tells the author to ask when unsure; whether this specific case should have triggered that is a human call, not something this thread settles. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWG4Cvk9ZzseV73X9o1QTj
|
🤖 platform-health-agent — reconcile pass. Re-ran the failed job; nothing pushed. Correcting one detail in my previous comment: of the two
Attempt 3 wasn't a test result, so treating it as a second failure overstated the case. Meanwhile the same job executed successfully on two other runs today (14:18–14:38 and 14:59–15:20), so the That makes a re-run the right remedy rather than a further escalation, so I've triggered one (queued at 17:02, runner assigned). No code change, no push, no branch change — the diff is still just the @ogenstad your approval still stands and I won't merge. If this run comes back green the PR is yours to merge; if it OOMs again at the same place, that's a real capacity problem with running the whole |
|
🤖 platform-health-agent — reconcile pass. CI on That is an external cancellation, not a self-inflicted one: Action taken: re-ran the three failed runs (no code change, nothing pushed). They are queued now. For the record, this branch is 0 commits behind If the reruns get cancelled the same way again, that points at Actions infrastructure rather than this PR, and I'll escalate for a human rather than keep retrying. |
|
🤖 platform-health-agent — reconcile pass. Escalating to a human and standing down; nothing pushed, and I will not re-run CI again. The red checks are an Actions infrastructure problem, not this PR. The evidence is now conclusive, so I'm stopping rather than triggering a third re-run. Every red check is
Also cancelled at the same 15-minute mark: A 13-second markdown lint does not become a 15-minute hang because of a 6-line What I checked and ruled out
What a human needs to do: this needs someone with Actions/runner visibility — check runner-fleet capacity or a stuck queue for Card |
|
🤖 Automated reconcile pass — the red CI on this PR is not caused by this PR. No code pushed; the diff is unchanged ( The two genuine failures, both transient:
Why this diff can't be responsible: it deletes one Also present and not diagnosed: 8 jobs were cancelled at exactly 15m00s (±3s) — including Action: re-ran the failed jobs once (the fleet is idle, and re-running is the standard answer to identified flakes). Staleness was checked first per the CI playbook — the branch is 0 commits behind Tracked in |
|
🤖 platform-health-agent — correcting my earlier comments, and then I'll stop commenting here. CI is not failing on this PR anymore — it isn't running at all, repo-wide. The three workflows on I'm not re-running anything: that would enqueue into a queue nobody is servicing and forfeit the current position. For whoever picks this up: the full CI run on the previous head |
|
There must have been some GitHub issue yesterday, now the pipeline is green but most of the jobs have not run. Not sure if there's a way to retrigger it in the current state. A rebase with the latest changes from |
…node.base The module is already fully annotated, so the per-module suppression only hid future regressions. Removing it restores untyped-def checking on a foundational base class with no source change required. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ename towncrier is configured with orphan_prefix = "+", so a fragment without it is read as an issue reference and rendered with a bogus link instead of as an orphan entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Per review: the change touches no source code, so a user-facing changelog entry is noise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@ogenstad Rebased onto latest Re-ran Your approval predates the force-push, so it may need re-confirming before merge. |
A harvesting-review run over the week's review threads, restricted to the PRs the in-flight harvests (#10145, #10098) did not read: open and recently-merged PRs across develop, stable, and release-1.11. New rules and knowledge: - mutations.md: retry_db_transaction placement — wrap only rollback-able transaction scopes, skip under a caller-supplied transaction (#10121) - query-pattern.md: READ queries with insert_limit=False need their own LIMIT; auto-paginated reads need a total-order ORDER BY (#10132) - creating-migrations.md: fix data bugs at the migration layer, not in runtime save paths (#10105); per-item error collection (#10132); batch by the memory-bounding unit (#10132) - database-schema.md: generic kinds exist only as labels — n.kind never matches a generic; type concrete-only inputs as list[NodeSchema] (#9805) - events.md: changelog models mask secrets only at construction — post-hoc assignment leaks them into events (#10105) - async-tasks.md: InfrahubBatch is concurrent, not ordered (#10113) - testing.md + testing-python.md rules: wiring tests parse source with ast/inspect instead of instrumenting production code (#10121); poll-don't-sleep for async effects (#10133); branch-attributable removal assertions (#10132); pure-function extraction before skipping the cheap test tier (#10137) - checklist.md: new Settings fields must reach both compose entry points — one generated and CI-checked, one hand-maintained (#10122) - backend/AGENTS.md: new REST endpoints are ask-first — prefer existing GraphQL for SDK needs (#8594); route creating-migrations.md in Guides - pre-ci.md: invoke lint tasks run ruff check --diff, which exits 0 on unfixable violations — added the plain CI-parity ruff check (#10146) - development/grafana/AGENTS.md (new): only defined datasource variables, sweep drill-down links, regenerate the standalone compose (#10153) - docs/AGENTS.md: Excalidraw exports use an opaque white background (#9953) Strengthened in place: - creating-changelog-entries skill: removed the 'most maintenance still gets a fragment' push that contradicted the no-user-facing-effect boundary; added lint/typing config cleanups as a named example (#10146) Paid for by compressing testing.md's nested-dataclass example and query-pattern.md's duplicated accessor/return-properties sections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYisRTn3sPygP55cxbVqzG
|
Re-verified after the CI on the merge commit: 36 passed, 15 skipped, 4 still running (docker-integration, tests-component, tests-integration, E2E-playwright) — nothing failing; the Taking the changelog point: the fragment was both misnamed (this repo sets towncrier Ready for merge whenever CI settles — leaving that to a human. |
A harvesting-review run over the week's review threads, restricted to the PRs the in-flight harvests (#10145, #10098) did not read: open and recently-merged PRs across develop, stable, and release-1.11. New rules and knowledge: - mutations.md: retry_db_transaction placement — wrap only rollback-able transaction scopes, skip under a caller-supplied transaction (#10121) - query-pattern.md: READ queries with insert_limit=False need their own LIMIT; auto-paginated reads need a total-order ORDER BY (#10132) - creating-migrations.md: fix data bugs at the migration layer, not in runtime save paths (#10105); per-item error collection (#10132); batch by the memory-bounding unit (#10132) - database-schema.md: generic kinds exist only as labels — n.kind never matches a generic; type concrete-only inputs as list[NodeSchema] (#9805) - events.md: changelog models mask secrets only at construction — post-hoc assignment leaks them into events (#10105) - async-tasks.md: InfrahubBatch is concurrent, not ordered (#10113) - testing.md + testing-python.md rules: wiring tests parse source with ast/inspect instead of instrumenting production code (#10121); poll-don't-sleep for async effects (#10133); branch-attributable removal assertions (#10132); pure-function extraction before skipping the cheap test tier (#10137) - checklist.md: new Settings fields must reach both compose entry points — one generated and CI-checked, one hand-maintained (#10122) - backend/AGENTS.md: new REST endpoints are ask-first — prefer existing GraphQL for SDK needs (#8594); route creating-migrations.md in Guides - pre-ci.md: invoke lint tasks run ruff check --diff, which exits 0 on unfixable violations — added the plain CI-parity ruff check (#10146) - development/grafana/AGENTS.md (new): only defined datasource variables, sweep drill-down links, regenerate the standalone compose (#10153) - docs/AGENTS.md: Excalidraw exports use an opaque white background (#9953) Strengthened in place: - creating-changelog-entries skill: removed the 'most maintenance still gets a fragment' push that contradicted the no-user-facing-effect boundary; added lint/typing config cleanups as a named example (#10146) Paid for by compressing testing.md's nested-dataclass example and query-pattern.md's duplicated accessor/return-properties sections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYisRTn3sPygP55cxbVqzG (cherry picked from commit ca2190d)
Summary
Removes the
no-untyped-defmypy suppression oninfrahub.core.node.base, restoring untyped-def checking on that module.The module is already fully annotated — every def in
backend/infrahub/core/node/base.pycarries parameter and return types. The per-module override inpyproject.tomlwas therefore stale: it changed nothing about the current code and only served to hide future regressions. No source change was needed.Resolves
INBOX-30.Changes
pyproject.toml— delete the[[tool.mypy.overrides]]block formodule = "infrahub.core.node.base"(disable_error_code = ["no-untyped-def"]).That is the entire diff:
pyproject.toml | 6 ------.Testing
Re-verified on the current head (
a6c76a1e, after the merge fromdevelop):uv run mypy --show-error-codes backenduv run invoke backend.lint(ruff, ty, mypy)uv run invoke backend.test-unituv run invoke backend.validate-generateduv lock --checka6c76a1eEnforcement verified, not just assumed. To confirm the removal actually takes effect and is not shadowed by a broader override (
infrahub.core.nodeis a separate entry and does not cover submodules), an untyped def was temporarily appended to the module:mypy flagged it as expected; the probe was then reverted (the diff above is the whole change).
Status
a6c76a1e. The earlier red runs on this PR were never caused by its diff:python-lintwas broken ondevelopitself (fixed independently by Add ruff exception for broad exception #10147), and the remaining failures were Actions infrastructure (queued-then-cancelled jobs, one OOM, one Neo4j deadlock at test setup). All resolved by the rebase @ogenstad asked for.aa469642c, before that rebase and the subsequentdevelopmerge, so GitHub no longer counts it. A fresh look is needed before merge; review re-requested.[[tool.mypy.overrides]]removal has no user-visible effect, so it does not warrant a release note.🤖 Generated with Claude Code