fix: use presence tests for loads() kwargs and reject invalid ones

[ryanhill1]

Fixes #356

Six documented loads() kwargs were stored behind a walrus truthiness test (if dev_qbts := kwargs.get("device_qubits")), so any falsy caller value was silently treated as "not passed". The kwarg→attribute map (_LOADS_KWARG_ATTRS) replaces the seven hand-written if blocks, so the kwarg list, the docstring and the storage loop can no longer drift apart.

Storage now tests is not None. An explicit None means "not passed", so module defaults are never clobbered — pass False to turn off a boolean kwarg.

Both adjacent gaps from the issue are closed as well:

• unknown kwarg names (loads(src, devise_qubits=5)) raise TypeError. This is breaking for any caller currently passing a stray or misspelled kwarg, and is filed under Improved / Modified accordingly.
• device_qubits / device_cycle_time / compiler_angle_type_size / frame_limit_per_port are validated at load time instead of surfacing later as a confusing validation message: non-positive raises ValueError, non-numeric raises TypeError. bool is rejected explicitly, since it is an int subclass and device_qubits=True would otherwise store as a count of 1.

load() forwards **kwargs, so it validates too — its errors name load() rather than loads(), and its docstring documents the exceptions.

Behaviour change worth calling out

frame_in_def_cal=None and play_in_cal_block=None previously were stored, and both are consumed as falsy guards, so an explicit None disabled the check that the True default enables. They now mean "not passed" and the default survives. Nothing is lost — False reaches the state None used to — but callers building kwargs from a config mapping should know. Covered in the CHANGELOG and the loads() docstring.

[argus-eye]

Argus review

Auto-review is off for this repo. Tick the box below to run a review on this PR.

• Trigger Argus review

Estimated cost

• Files changed: 3
• Diff lines (±): 137
• Historical avg: ~318.9k tokens · ~$1.35 · across last 6 review(s)

Tip: you can also comment @argus-eye review at any time.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 866bf498-a2ab-4403-bb20-1029d2258967

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[ryanhill1]

@Argus-Eye review

[argus-eye]

🔎 Argus · 9/10 — Loads kwargs validation is cleaner, with one compatibility warning to resolve

🔍 PR intent vs diff (LLM analysis)

Argus read the diff against the stated intent. This is not an execution log — reviewer still needs to test behavior.

Goal: Ensure loads() preserves falsy kwargs and rejects unknown or invalid values at the call site.
Stated acceptance criteria (from PR/issue — not independently verified):

• All documented loads() kwargs are handled with presence tests so falsy values are preserved.
• Unknown loads() kwarg names raise TypeError.
• Non-positive device_qubits, device_cycle_time, compiler_angle_type_size, and frame_limit_per_port raise ValueError at load time.
• An explicit None kwarg value is treated as not passed.
• The kwarg-to-attribute map replaces the seven hand-written if blocks.

⚠️ Intent not delivered

Although src/pyqasm/entrypoint.py adds loads() kwarg handling and tests, finding 0 identifies that explicit None values are stored rather than treated as omitted. This fails the stated explicit-None acceptance criterion for API validation.

Unmet criteria:

• An explicit None kwarg value is treated as not passed.

Verdict: This PR centralizes loads() kwarg handling, preserves falsy values, and adds call-site validation. It is close to merge-ready once the compatibility concern is addressed.

🟡 1 P1 · 3 files reviewed

Architecture: The map-based validation replaces repetitive branching cleanly; we should ensure its stricter behavior remains compatible with existing callers.

1 finding · 1 inline · 0 folded

_{🔢 81.0k tokens · $0.2005 total}

Stage	Tokens	Cost
Intent	2.7k	$0.0000
Triage	3.0k	$0.0000
Lead agent	1.4k	$0.0000
Review · bug_hunter	15.0k	$0.0523
Review · security	14.9k	$0.0451
Review · architecture	14.8k	$0.0512
Review · regression	15.1k	$0.0519
Review	11.3k	$0.0000
Acceptance	1.0k	$0.0000
Scoring	901	$0.0000
Synthesis	893	$0.0000

---

<sub>Contract: production/full · checked: bug_hunter, security, architecture, regression · review took 1m7s

Dashboard → · React 👎 to dismiss · Reply to any inline comment or use @argus-eye help to chat</sub>

[TheGupta2012]

Verdict

Request changes — the refactor itself is a clear improvement, but three items should be settled before merge.

Replacing seven hand-written if blocks with _LOADS_KWARG_ATTRS is the right structural call: the kwarg list, the docstring and the storage loop can no longer drift apart, and rejecting unknown names closes a real silent-typo hole. The concerns are about the edges of the new validator and about the CHANGELOG describing behaviour the code does not have.

Findings

#	Type	Severity	Item
1	Implementation	Medium	frame_in_def_cal=None / play_in_cal_block=None silently flip behaviour vs main (entrypoint.py:140)
2	Implementation	Medium	_validate_kwargs has no type check: bare comparison TypeError, and bools pass positivity (entrypoint.py:63)
3	Maintenance	Medium	CHANGELOG entry does not match the shipped behaviour, and the breaking part sits under Fixed (CHANGELOG.md:29)
4	Implementation	Low	load() inherits the new exceptions but not the docs; both messages name loads() (entrypoint.py:117)

Minor polish, no separate comments needed

• entrypoint.py:63 — value is not None and name in kwargs is redundant; kwargs.get(name) is not None already implies name in kwargs. The suggestion on finding 2 drops it.
• entrypoint.py:136-138 — the comment says "presence tests, not truthiness", but the code tests is not None, which is a third thing. Rewording it to name is not None would keep the comment true as the file evolves.
• entrypoint.py:141 — setattr with attribute names held as strings means a rename in modules/base.py creates a phantom attribute instead of failing. A one-line hasattr assert in _validate_kwargs, or a module-import-time check over _LOADS_KWARG_ATTRS.values(), would catch that.
• Strictness is now uneven across the public API: QasmModule.unroll(**kwargs) (modules/base.py:640) still forwards unrecognised names to QasmVisitor without complaint and still uses walrus truthiness (external_gates, consolidate_qubits). Harmless today, since both defaults absorb the falsy case, but a caller who learns that loads() rejects typos will reasonably assume unroll() does too.

How it was tested

• Behaviour matrix run on this head and on origin/main (7c31308), for all seven kwargs across omitted / None / 0 / 0.0 / False / True / {} / [] / normal / -5 / "5" / complex(1). Every claim below comes from that run.
• Full suite in a worktree at d1a85065: 729 passed, 2 failed, 4 skipped. The same 2 tests/cli/test_cli_commands.py failures reproduce on origin/main (714 passed, same 2 failed) and are an artifact of the long checkout path wrapping the expected filename, not this PR. Net effect of the PR: +15 passing tests, no regressions.
• End-to-end check of the None flip on a real OpenPulse program (newframe inside a defcal), unrolled on both trees.
• black --check passes on both changed files.
• gh pr checks 361: only Changelog and CodeRabbit ran — no test workflow executed on this PR, so the local suite above is the only test evidence.

Not verified: pylint and isort are not present in the available environment, so repo lint beyond black was not run.

Next steps

1. Address findings 1-4.
2. Rebase onto origin/main. The branch points at 7e05f4c and CHANGELOG.md conflicts; a rebase restores the #346 / #349 / #365 entries that the current diff appears to delete. No code conflict exists.

[TheGupta2012]

Type: Implementation
Severity: Medium

Rationale: For the two bool kwargs this is a silent behaviour reversal, not a no-op. On main the storage test was if "frame_in_def_cal" in kwargs, so an explicit None was stored. Under is not None it is discarded and the default True survives (modules/base.py:150, :152). Both attributes are consumed as falsy guards (pulse/visitor.py:714, :728), so the effective flag flips from off to on.

Verified end to end on a defcal containing newframe:

                                    main            this PR
frame_in_def_cal omitted            unroll OK       unroll OK
frame_in_def_cal=None               ValidationError unroll OK   <-- flipped
frame_in_def_cal=False              ValidationError ValidationError

A caller building kwargs from a config mapping (cfg.get("frame_in_def_cal")) silently gets the opposite policy, with no error and no warning. Nothing is lost in capability — False reaches the same state None used to — but the change is undocumented, and the PR description's auto-generated section asserts the opposite ("Stores explicitly provided None values for supported loads() kwargs"). tests/test_entrypoint.py:55 locks in the new behaviour without noting that it differs from main.

Treating None as "not passed" is a defensible and arguably better contract; the problem is that it ships undeclared.

Change Requested: Keep the is not None semantics, and (a) correct the stale claim in the PR description, (b) add the frame_in_def_cal / play_in_cal_block None change to the CHANGELOG entry, and (c) note in the loads() docstring that an explicit None means "not passed" for every kwarg — the docstring currently gives no way for a reader to predict this.

[ryanhill1]

Agreed on all three counts — keeping is not None and declaring it properly, in 81b764d.

(a) The PR description's auto-generated section was flatly wrong and is gone. (b) The changelog entry now spells out that frame_in_def_cal=None / play_in_cal_block=None mean "not passed", that this differs from main where an explicit None was stored and read as falsy, and that False reaches the old state. (c) The loads() docstring now says an explicit None means "not passed" for every kwarg and to pass False to turn off a boolean.

Your framing is right: nothing is lost in capability, the problem was that it shipped undeclared.

[TheGupta2012]

Type: Implementation
Severity: Medium

Rationale: value <= 0 runs without a type check, which produces two results that work against the PR's stated goal of failing clearly at the call site:

1. Any non-numeric value raises a bare comparison error that names neither the kwarg nor the function:

   >>> loads(src, device_qubits="5")
   TypeError: '<=' not supported between instances of 'str' and 'int'
   

   Same for {}, [] and complex(1). A caller who reads a device config from JSON and gets "5" learns nothing about which argument was wrong. On main the value was stored as-is, so this is not a regression — but it is the exact failure mode this validator exists to prevent.

2. bool is an int, so True <= 0 is False and loads(src, device_qubits=True) passes validation and stores True. It then behaves as 1 at visitor.py:502 (total_qubits > True). The mirror case, device_qubits=False, produces ValueError: ... must be positive, got False, which reads as though False were a plausible qubit count.

The redundant name in kwargs is folded into the suggestion: kwargs.get(name) is not None already implies the key is present.

Change Requested: Guard the type before comparing, and reject bool explicitly.

Suggested change

	for name in _POSITIVE_KWARGS:
	value = kwargs.get(name)
	if value is not None and name in kwargs and value <= 0:
	raise ValueError(f"loads() kwarg '{name}' must be positive, got {value!r}")
	for name in _POSITIVE_KWARGS:
	value = kwargs.get(name)
	if value is None:
	continue
	if isinstance(value, bool) or not isinstance(value, (int, float)):
	raise TypeError(f"loads() kwarg '{name}' must be a number, got {type(value).__name__}")
	if value <= 0:
	raise ValueError(f"loads() kwarg '{name}' must be positive, got {value!r}")

Worth adding parametrised cases for "5", [] and True to tests/test_entrypoint.py — the current suite covers only 0, 0.0 and negatives, so none of the above is caught.

[ryanhill1]

Both correct, applied essentially verbatim in 81b764d — including the redundant name in kwargs, which is gone.

The bool case is the one I would have missed: device_qubits=True passing validation and behaving as 1 at visitor.py:502 is worse than a loud rejection, and must be positive, got False reading as though False were a plausible qubit count is exactly backwards.

Added the parametrized cases you asked for — "5", [], complex(1), {} and both bools.

[TheGupta2012]

Type: Maintenance
Severity: Medium

Rationale: Two separate problems with this entry.

It does not describe the shipped behaviour. "Fixed loads() silently dropping falsy kwarg values" implies falsy values are now honoured. Measured against main, per kwarg:

• device_qubits=0, device_cycle_time=0.0, compiler_angle_type_size=0, frame_limit_per_port=0 — previously dropped, now raise ValueError. Not honoured. (Rejecting is the right call: modules/base.py:628 guards the device-qubit check with if self._device_qubits:, so a stored 0 would have been skipped anyway. Issue loads() silently drops falsy kwarg values (device_qubits=0, device_cycle_time=0.0, ...) #356 offers this as an acceptable resolution.)
• frame_in_def_cal=False, play_in_cal_block=False — already honoured on main via the in kwargs test. No change.
• extern_functions={} / [] — the only values genuinely newly stored, and the issue itself calls this case harmless.

So the sole behavioural gain in the "no longer dropped" direction is the one the entry cites parenthetically, while the four cases a reader would most expect now raise. The clause "by testing truthiness instead of presence" also inverts the direction of the fix, and the mechanism is in fact neither: it is is not None (entrypoint.py:140).

The breaking part is filed under Fixed. TypeError on unknown kwarg names changes loads(src, devise_qubits=5) from a silent no-op to a hard failure. Any existing caller passing a stray or misspelled kwarg breaks on upgrade. This repository states it follows Keep a Changelog and SemVer, and loads() is the primary public entrypoint, so this belongs where a reader scanning for upgrade risk will find it.

Change Requested: Rewrite the entry to state that non-positive values are now rejected rather than honoured, name extern_functions as the case that is newly stored, drop the inverted "truthiness instead of presence" clause, and add the frame_in_def_cal=None / play_in_cal_block=None change. Move the unknown-kwarg rejection to Improved / Modified with an explicit note that it is a breaking change for callers passing unrecognised kwargs.

[ryanhill1]

Both points taken — rewritten in 81b764d.

The Fixed entry now walks the kwargs individually: extern_functions={} / [] is the case genuinely newly stored, the four numeric kwargs are rejected rather than honoured, and the frame_in_def_cal / play_in_cal_block None change is named. The "truthiness instead of presence" clause is gone; the mechanism is described as is not None, which is what the code does.

The unknown-kwarg TypeError moved to Improved / Modified prefixed Breaking, spelling out that a caller passing a stray or misspelled kwarg breaks on upgrade. You are right that it does not belong under Fixed in a repo that claims Keep a Changelog and SemVer for its primary entrypoint.

Also rebased onto origin/main — the #346 / #349 / #365 entries are back, and it was a changelog-only conflict as you predicted.

[TheGupta2012]

Type: Implementation
Severity: Low

Rationale: load() forwards **kwargs straight through (entrypoint.py:82), so both new exceptions surface from load() as well — but its docstring documents neither **kwargs nor a Raises section, while loads() gained both. The messages also hardcode the wrong name: load("f.qasm", devise_qubits=5) reports loads() got unexpected keyword argument(s): devise_qubits, pointing the caller at a function they did not call.

Change Requested: Add a Raises section to load() covering TypeError and ValueError and cross-reference loads() for the supported **kwargs. Optionally pass the caller's name into _validate_kwargs (_validate_kwargs(kwargs, func="load")) so the message names the entrypoint that was actually invoked.

[ryanhill1]

Fixed in 81b764d. _validate_kwargs takes a func name, and load() calls it directly before process_include_statements so the message names the entrypoint the caller invoked. load() also gains the Raises section and a **kwargs pointer to loads().

Pinned with test_load_errors_name_load_not_loads, covering both the TypeError and the ValueError.