Skip to content

fix: use identity checks for type comparisons; remove mutable default argument - #2277

Open
harshadkhetpal wants to merge 1 commit into
Open-Source-Legal:mainfrom
harshadkhetpal:fix/type-identity-and-mutable-default
Open

fix: use identity checks for type comparisons; remove mutable default argument#2277
harshadkhetpal wants to merge 1 commit into
Open-Source-Legal:mainfrom
harshadkhetpal:fix/type-identity-and-mutable-default

Conversation

@harshadkhetpal

Copy link
Copy Markdown

Summary

  1. opencontractserver/llms/tools/tool_factory.py — the annotation→JSON-schema dispatch chain compares type objects with == five times (param.annotation == int, float, bool, list, dict) — ruff E721. Identity (is) is the intended semantics for type objects and avoids surprises with metaclass-defined __eq__.

  2. opencontractserver/pipeline/utils.pyrun_post_processors(..., input_kwargs: dict[str, Any] = {}) shares one dict across every call (ruff B006). Changed to the None sentinel with input_kwargs = input_kwargs or {} after the docstring; identical behavior for all callers.

Testing

python -m py_compile passes on both files; ruff check --select B006,E721 on them goes clean.

🤖 Generated with Claude Code

… argument

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Harshad Khetpal <harshadkhetpal@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

JSv4 commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Reviewed this as part of a batch pass over the open PRs. Both changes look correct — I found nothing blocking.

The branch also merges cleanly with current main, so there's no rebase needed.

tool_factory.py==is on the annotation dispatch

Semantically identical here, and the ruff E721 rationale holds. The values reaching those five comparisons are the builtin type objects, which are singletons, so identity and equality agree. I specifically checked the cases where they could diverge and none of them reach this code:

  • Typing generics (list[str], dict[str, int], Optional[int]) already failed the == comparison — list[str] == list is False — so they fell through to the untyped default before this PR and still do. No behavior change.
  • from __future__ import annotations would make annotations strings, which likewise compare unequal both ways. Also unchanged.
  • A metaclass-defined __eq__ returning True against a builtin is the one case where the old code differed, and is is the safer reading — which is the point of the lint.

pipeline/utils.py — mutable default → None sentinel

The B006 fix is right and input_kwargs = input_kwargs or {} is safe in this specific function: the value is only ever splatted into processor.process_export(..., **input_kwargs), never stored, mutated, or returned, so the one behavioral difference between or {} and if input_kwargs is None (an explicitly-passed empty dict becoming a fresh object) is unobservable.

Coverage

Checked this against .codecov.yml, since patch.default has no informational: true and therefore gates: every added or changed executable line is exercised by the existing suite — run_post_processors through test_pipeline_utils.py and test_pdf_redaction.py, and the annotation dispatch through the tool-factory tests. setup.cfg's [coverage:run] has no branch key, so coverage is line-based and the per-branch question doesn't arise. codecov/patch should pass.

The one thing blocking this PR

Nothing in the code. CLAAssistant is failing because the CLA isn't signed yet — you can clear it by posting a comment on this PR containing exactly:

I have read the CLA Document and I hereby sign the CLA

No other CI has run at all, because workflows on fork PRs need a maintainer to approve the run. Once both of those happen this should go green.

One heads-up unrelated to your change: the linter job is currently red on main itself (an unpinned flake8-isort drifted to isort 9.0.1 while the isort hook is pinned to 6.0.1, so the two disagree on files nobody edited). Fix is in #2281 — if your run trips on I001/I005 findings in pydantic_ai_agents.py or compact_pawls.py, that's why, and it isn't yours.


Generated by Claude Code

@JSv4

JSv4 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

@harshadkhetpal can you please acknowledge the CLA?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants