Repository navigation
Conversation
Input loaders raise dokimi.utils.InputError, a ValueError whose message names the file. Only cli.main turns it into "Error: <message>" and exit 2 (bad input). Exit 1 stays the verdict of the gates (--strict, judge-validity), so the exit code now says which kind of failure it was. - read_json_input(path, *, flag=None) is the one read-or-fail path: a missing or unreadable file, non-UTF-8 bytes, and invalid or too deeply nested JSON each raise InputError naming the file. - load_artifact, load_atoms_record, load_artifact_ref, records.load and model_compare.load_run raise InputError; the *_or_exit helpers are gone. The routing loaders move to commands/novelty_routing_inputs.py. - tests/test_load_or_fail.py: every artifact kind and the atoms, judged-run and answers loaders, over missing, non-UTF-8, bad JSON, non-object and wrong-schema inputs. - tests/test_import_architecture.py: only the frontend ends the process; EXIT_BASELINE is empty, and a planted exit outside the frontend fails. Changelog fragment lists the exit-code and message changes. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
- read_json_input turns a bare ValueError (a NUL byte in the path) into InputError, as the other read failures are. - The all-vs-all field loader lets InputError through unwrapped, so a refused skill field keeps its own message. - novelty view tests pin the reason for each invalid file again. - exit_sites resolves sys/os aliases (import sys as s; from sys import exit as bye), so an aliased exit outside the frontend is counted. - Changelog: the novelty routing field refusals exit 2 too; the schema failure wording matches the code. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…onflict Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
🔍 OpenCodeReview found 4 issue(s) in this PR. By severity: 4 medium
Review details
|
| names the file and the reason; for a refused field it names the skill or option at fault. | ||
| Exit 1 stays the verdict of the gates | ||
| (`--strict`, `eval judge-validity`), so a script that branches on the code must | ||
| treat 2 as "fix the input". The same exit and `Error:` line now come from any input |
There was a problem hiding this comment.
Nit (accuracy): “any input file the loaders reject” is broader than the implemented CLI guarantee. subjects run --novelty-artifact <bad-file> calls the artifact loader but catches its InputError via ValueError in commands/subjects.py:113–115, prints Error loading novelty artifact: ..., and exits 1. Restrict this statement to errors that reach cli.main, or name the retained command-handler exceptions, so scripts do not rely on a universal exit-2 contract.
There was a problem hiding this comment.
Fixed in a172082. The sentence now says the same exit and Error: line come from the input files those commands reject, and names subjects run --novelty-artifact as the exception: it still prints its own message and exits 1.
The changelog fragment carries the sentence; the code is unchanged.
| guarded = {id(n) for stmt in tree.body | ||
| if isinstance(stmt, ast.If) and ast.unparse(stmt.test) == "__name__ == '__main__'" | ||
| for n in ast.walk(stmt)} |
There was a problem hiding this comment.
Must-fix: walking the entire ast.If also exempts its else branch, which runs when the module is imported. For example, import sys\nif __name__ == '__main__':\n pass\nelse:\n sys.exit(1) produces no exit sites and passes this rule despite terminating the importing process. Exempt only descendants of stmt.body, and add this case as a regression test.
Suggestion:
| guarded = {id(n) for stmt in tree.body | |
| if isinstance(stmt, ast.If) and ast.unparse(stmt.test) == "__name__ == '__main__'" | |
| for n in ast.walk(stmt)} | |
| guarded = {id(n) for stmt in tree.body | |
| if isinstance(stmt, ast.If) and ast.unparse(stmt.test) == "__name__ == '__main__'" | |
| for body_stmt in stmt.body for n in ast.walk(body_stmt)} |
There was a problem hiding this comment.
Fixed in a172082. The guard exempts only the body of the if __name__ == '__main__' block, not its else, so else: sys.exit(1) is an exit site.
Test: the planted dokimi/e.py case in tests/test_import_architecture.py.
| bad = tmp_path / "novelty.json" | ||
| bad.write_text(json.dumps({"schema_version": "novelty.v1"}), encoding="utf-8") | ||
| with pytest.raises(ArtifactValidationError): | ||
| with pytest.raises(InputError, match=str(bad)): |
There was a problem hiding this comment.
Must-fix: pytest.raises(..., match=...) treats its argument as a regular expression, not a literal substring. On Windows, str(bad) contains backslashes (for example, C:\Users\...\novelty.json), so regex escapes such as \U can raise re.error instead of matching the InputError. Capture the exception and assert str(bad) in str(exc.value), or pass re.escape(str(bad)).
Suggestion:
| with pytest.raises(InputError, match=str(bad)): | |
| with pytest.raises(InputError) as exc: | |
| load_novelty_artifacts([bad]) | |
| assert str(bad) in str(exc.value) |
There was a problem hiding this comment.
Fixed in a172082. The check is assert str(bad) in str(exc.value), so the Windows path is not read as a regex.
Test: tests/test_novelty_gate.py (the artifact-load refusal).
…ent to two lines Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #316 +/- ##
==========================================
+ Coverage 94.20% 94.32% +0.11%
==========================================
Files 304 309 +5
Lines 38748 39283 +535
Branches 6561 6651 +90
==========================================
+ Hits 36504 37052 +548
+ Misses 1430 1421 -9
+ Partials 814 810 -4
🚀 New features to boost your workflow:
|
…s the nesting Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The else branch of 'if __name__ == "__main__":' runs on import, so only the guarded body is the process boundary. Exempt stmt.body, not the whole If. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
…uiltins Main's dokimi.exit_codes.usage_exit raises a SystemExit bound to a name, so the rule saw neither it nor its callers. Count calls to usage_exit (any import form), 'raise e' with e = SystemExit(...), and builtins.exit/quit; exempt the helper module itself, as the spawn rule exempts dokimi.process. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
pytest.raises(match=) is a regex; a Windows tmp path such as C:\Users\... raises re.error on the \U escape before the InputError is checked. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
| continue | ||
| if isinstance(node, ast.Raise) and node.exc is not None: | ||
| exc = node.exc.func if isinstance(node.exc, ast.Call) else node.exc | ||
| if isinstance(exc, ast.Name) and (exc.id == "SystemExit" or exc.id in bound): |
There was a problem hiding this comment.
Must-fix: the rule silently accepts qualified or imported aliases of SystemExit. For example, a scratch library module containing import builtins\nraise builtins.SystemExit(2) or from builtins import SystemExit as Stop\nraise Stop(2) produces no exit sites, so exit_problems(imports, {}) passes despite the module terminating its caller. _exit_imports does not track SystemExit aliases, and this branch only recognizes the literal name. Resolve qualified/aliased SystemExit constructors in both the raise check and the bound-exception assignment check, and add these cases to the planted-violation tests.
There was a problem hiding this comment.
Fixed in a172082. exit_sites resolves from builtins import SystemExit as X, builtins.SystemExit, and a SystemExit bound to a name and then raised.
Tests: planted cases in tests/test_import_architecture.py, which also pass on the merged tree.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at cccb356. The design is right: loaders raise InputError naming the file, cli.main is the one place that exits 2, and the architecture test holds library code to that. The branch predates #285, though, so it conflicts with main in 12 files, and the merge decides which exit codes survive. The line comment on commands/novelty.py lists the places to watch.
Agreed with @xaleryb: describe the change as a message change after the merge, since main already exits 2 for these inputs; drop the follow-ups main already fixed.
Agreed with @exai-bot on the over-broad "any input file" sentence in the changelog and on the SystemExit aliases the exit rule misses. Its __main__ else-branch and match= path comments are fixed in 19b1118 and cccb356.
What I ran, at cccb356:
make ci-local --offline: 10,990 passed, 0 failed, 57 skipped.schema_compatfails onharness_profile.v1.json, whichmainwidened after this branch's base; this PR does not touch it.- Bad-input runs on the merge-base, this head and current
main(novelty items --probes,novelty view,report model-compare,report eval,arms plugin-deltawith a bad, missing or non-object file): this head exits 2 fornovelty viewandreport model-compare(including a non-object file, a traceback on both base andmain);mainexits 2 for all of them except that non-object file. - This branch's
test_import_architecture.pyonmain's tree: it flags 18usage_exitcalls in four library modules, which the merge has to turn intoInputError. exit_siteson aliasedSystemExitforms, for the open bot thread.
Checked, no issues
InputErroras aValueError: I scanned for loaders that now raise it inside anexcept ValueError/except Exception. Onlyarms plugin-delta,subjects runand the answer-key check wrap one, and each handles it as a refusal; this PR does not touch those handlers.- Loader matrix:
test_load_or_fail.pycovers every artifact kind, atoms, judged runs and answer records against a missing file, non-UTF-8 bytes, bad JSON, a non-object and a wrong schema, plus a NUL byte in the path. - New files: both carry the SPDX header; the rule is in
docs/architecture.md.
| if not verdict.ok: | ||
| if verdict.problem == "unloadable": # the artifact cannot be read: bad input, exit 2 | ||
| raise InputError(verdict.message) | ||
| raise SystemExit(verdict.message) |
There was a problem hiding this comment.
When you merge main, resolve the bad-input exits toward raise InputError, not toward this branch's raise SystemExit(msg): the new architecture test cannot catch the difference.
On main, #285 already made these refusals exit 2 through usage_exit. This branch was cut before it, so in commands/novelty.py it still has 19 SystemExit/sys.exit sites (exit 1) where main has 5. Examples on this head: novelty probe with a stale answer key (here, line 187), novelty shorten with a bundle entry_point_only refuses (line 553), novelty items --probes bad.json (line 243; I ran it: exit 1 here, exit 2 on main). commands/ is the frontend layer, so test_only_the_frontend_ends_the_process passes whichever side you keep. Also keep main's default code=EXIT_USAGE in write_text_or_exit (this branch moves it with code=1).
The library side is caught: I ran this branch's test_import_architecture.py on main's tree, and it fails on 18 usage_exit calls in novelty/artifact.py:48, novelty/atoms.py:672, report/model_compare.py (15) and utils.py:315. Those need to become InputError in the merge.
Suggestion: after the merge, turn main's usage_exit(...) calls in commands/novelty.py into raise InputError(...) too, and add one cli.main test for novelty items --probes <bad file> that expects exit 2.
There was a problem hiding this comment.
Fixed in a172082. The merge resolves the bad-input exits in commands/novelty.py to raise InputError, which cli.main maps to exit 2; the usage_exit and SystemExit(msg) sites that were refusals are gone.
The spend-cap exit stays a SystemExit, as it is not an input error. Test expectations moved to InputError in tests/test_novelty_routing.py, tests/test_novelty_journey.py, and tests/test_novelty_cli.py.
| atoms, spans, _record = load_atoms_record(path) | ||
| return atoms, spans |
There was a problem hiding this comment.
Nit: load_atoms now goes through load_atoms_record, so novelty view also runs _check_bundle_shape, though it never reads bundle.
An atoms file with, say, "bundle": {"n_files": "3"} used to render in novelty view; now it is refused with exit 2. That may be what you want (one entry point), but the changelog does not say so.
Suggestion: mention it in the fragment, or keep load_atoms without the bundle check.
There was a problem hiding this comment.
Fixed in a172082 (changelog). novelty view now names its bundle check in the fragment: a malformed bundle in an atoms file is refused with exit 2.
That is the one entry point, so load_atoms keeps the check.
| return records(run_data.get(section) or [], source=section, kind=section) | ||
| except RecordError as exc: | ||
| raise SystemExit(f"Error: {exc}") from exc | ||
| return records(run_data.get(section) or [], source=section, kind=section) |
There was a problem hiding this comment.
Nit: The _section docstring still says it returns the records "or a clean exit", but it now lets a RecordError (an InputError) propagate.
Suggestion: "The answers or evaluations records of run_data; a RecordError (an InputError) if malformed." (closest changed line; the docstring is on line 96).
There was a problem hiding this comment.
Fixed in a172082. The _section docstring now reads: the answers or evaluations records of run_data; a RecordError (an InputError) if malformed.
Resolves the conflicts with main's exit-code work (intel#285). Bad inputs now raise InputError from the command layer and cli.main maps it to exit 2, instead of SystemExit(msg) or sys.exit in the commands. - commands/novelty.py, novelty/measure.py, novelty/atoms.py, novelty/artifact.py, report/model_compare.py: refusals raise InputError; the spend-cap exit stays a SystemExit. - commands/_output.py: an unwritable output exits EXIT_USAGE (2). - commands/arms.py: InputError from the matrix input check propagates to cli.main instead of being caught as ValueError. - tests: the refusal expectations move from SystemExit to InputError (test_novelty_routing, test_novelty_journey, test_model_compare_exit_codes, test_novelty_cli, test_novelty_routing_all_vs_all, and others). - test_import_architecture: the exit-site scan also covers `from builtins import SystemExit as X` and `builtins.SystemExit`, and a SystemExit bound to a name and then raised. - scripts/growth_baseline.json regenerated. - changelog: the "any input file" sentence is narrowed to what reaches cli.main; the `novelty view` bundle check is named. Threads: PRRT_kwDOVBg7g86yMjfC, PRRT_kwDOVBg7g86yMjfE, PRRT_kwDOVBg7g86yMjfK, PRRT_kwDOVBg7g86yOv9G, PRRT_kwDOVBg7g86yVRgk, PRRT_kwDOVBg7g86yVRgn, PRRT_kwDOVBg7g86yVRgr. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| except (ExtractionError, ValueError) as exc: # a model failure under way | ||
| raise SystemExit(str(exc)) from None | ||
| raise InputError(str(exc)) from None |
There was a problem hiding this comment.
Must-fix: this also classifies model execution failures as bad user input. For example, with a valid artifact and an extractor transport that raises RuntimeError("service unavailable") on every call, extract_atoms raises ExtractionError, which now reaches cli.main as InputError and exits 2 instead of the previous 1. No input file or flag is invalid in that case. The same conversion occurs in cmd_novelty_atoms and in cmd_novelty_probe for measurement ValueErrors. Keep input-validation refusals as InputError, but use a separate runtime-failure exception handled by the frontend with exit 1; add a CLI-boundary test with a failing extractor transport.
There was a problem hiding this comment.
Fixed in fcb6bac. An extraction or measurement failure now exits 1 again (SystemExit, as on main) in analyze, atoms and probe; input refusals such as the self-judge check keep exit 2. Test: test_extraction_failure_exits_1_not_2.
| bound = {t.id for n in ast.walk(tree) if isinstance(n, ast.Assign) and isinstance(n.value, ast.Call) | ||
| and isinstance(n.value.func, ast.Name) and n.value.func.id == "SystemExit" | ||
| for t in n.targets if isinstance(t, ast.Name)} |
There was a problem hiding this comment.
Must-fix: bound exceptions only recognize the literal SystemExit constructor, so the alias fix still misses from builtins import SystemExit as Stop\nerror = Stop(2)\nraise error and import builtins\nerror = builtins.SystemExit(2)\nraise error. Both produce an empty exit_sites result and let a library process exit pass the architecture rule. Resolve constructor names/attributes using the same alias logic as direct raises when building bound, and add planted cases for both combinations.
There was a problem hiding this comment.
Fixed in 8a8ddaa. Names bound to SystemExit under an alias (from builtins import SystemExit as Stop; builtins.SystemExit) are now resolved, so both planted cases are caught; the test adds dokimi/h.py and dokimi/i.py for them.
There was a problem hiding this comment.
🟡 Changes recommended
Both novelty handlers incorrectly map extraction failures to exit code 2 instead of the documented runtime-failure code 1.
2 open issues
What changed in this PR
2 new findings: 1 must-fix and 1 nit.
This PR advances the architecture cleanup in #4 by centralizing input failures under InputError, letting library callers handle exceptions while the CLI reports bad input with exit 2.
Changes:
- Unifies input-loading errors across artifacts, records, novelty, and model comparison.
- Extracts routing-input helpers and moves output-exit handling into the command layer.
- Adds loader and process-exit architecture tests, updates existing assertions, and documents the error boundary.
| File | Description |
|---|---|
dokimi/tests/test_replicates.py |
Expects typed errors for invalid replicate comparisons. |
dokimi/tests/test_records.py |
Updates file-error message assertions. |
dokimi/tests/test_record_readers.py |
Checks typed record and judge-input refusals. |
dokimi/tests/test_novelty_view_inputs.py |
Tests frontend exit codes and detailed input errors. |
dokimi/tests/test_novelty_shorten_artifact.py |
Expects typed artifact-loading failures. |
dokimi/tests/test_novelty_settings_parity.py |
Adapts parity capture to typed refusals. |
dokimi/tests/test_novelty_routing.py |
Updates routing refusal assertions. |
dokimi/tests/test_novelty_routing_field.py |
Updates invalid-field error assertions. |
dokimi/tests/test_novelty_routing_all_vs_all.py |
Updates all-vs-all refusal assertions. |
dokimi/tests/test_novelty_journey.py |
Expects typed answer-key and self-judge errors. |
dokimi/tests/test_novelty_gate.py |
Checks invalid artifacts raise named input errors. |
dokimi/tests/test_novelty_cli.py |
Updates error assertions and adds an items exit-code test. |
dokimi/tests/test_novelty_atoms_input.py |
Exercises malformed atoms through the CLI boundary. |
dokimi/tests/test_model_compare.py |
Checks typed loading and comparison errors. |
dokimi/tests/test_model_compare_exit_codes.py |
Expects InputError for incompatible runs. |
dokimi/tests/test_load_or_fail.py |
Adds a shared malformed-input loader matrix. |
dokimi/tests/test_import_architecture.py |
Enforces frontend-only process exits. |
dokimi/tests/test_eval_output_write_errors.py |
Uses the relocated output helper. |
dokimi/scripts/growth_baseline.json |
Lowers size and complexity baselines. |
dokimi/dokimi/utils.py |
Defines the shared input-error and JSON-reading behavior. |
dokimi/dokimi/runner/compare.py |
Makes comparison errors inherit InputError. |
dokimi/dokimi/report/model_compare.py |
Replaces library exits with typed errors. |
dokimi/dokimi/records.py |
Unifies record-loading failures under InputError. |
dokimi/dokimi/novelty/routing.py |
Makes routing errors inherit InputError. |
dokimi/dokimi/novelty/measure.py |
Types selected arm-validation refusals. |
dokimi/dokimi/novelty/atoms.py |
Consolidates validated atoms loading. |
dokimi/dokimi/novelty/artifact.py |
Replaces the exiting artifact loader with a raising helper. |
dokimi/dokimi/eval/question_text.py |
Makes empty-question errors inherit InputError. |
dokimi/dokimi/commands/novelty.py |
Adopts typed errors and extracted routing loaders. |
dokimi/dokimi/commands/novelty_view_inputs.py |
Delegates validation to shared loaders. |
dokimi/dokimi/commands/novelty_routing_inputs.py |
Houses routing-field loading and error conversion. |
dokimi/dokimi/commands/judge_reliability.py |
Uses shared input errors and relocated output handling. |
dokimi/dokimi/commands/explore.py |
Imports the relocated output helper. |
dokimi/dokimi/commands/evaluate.py |
Propagates record errors to the frontend. |
dokimi/dokimi/commands/arms.py |
Preserves typed matrix-input refusals. |
dokimi/dokimi/commands/_output.py |
Houses command-level text-writing exit handling. |
dokimi/dokimi/cli.py |
Handles unified input errors with exit 2. |
dokimi/dokimi/artifacts.py |
Wraps file and schema failures in named input errors. |
dokimi/docs/architecture.md |
Documents the input-error boundary. |
dokimi/changelog.d/20261030_120000_napetrov_arch-loader.md |
Describes user-facing error handling and validation changes. |
Review effort: Balanced
| raise InputError(str(exc)) from exc | ||
| except (ExtractionError, ValueError) as exc: # a model failure under way | ||
| raise SystemExit(str(exc)) from None | ||
| raise InputError(str(exc)) from None |
There was a problem hiding this comment.
Same fix as the reply above (fcb6bac): ExtractionError exits 1 in both analyze and atoms (line 160), and probe's measurement ValueError too.
| runs that disagree on the treatment, prompt or judge), and the skill fields that | ||
| `novelty routing` refuses (a missing competitor, a skill listed twice, a skill compared | ||
| with itself, an all-vs-all catalog that holds one skill twice with different content), | ||
| now exit 2 instead of 1, with `Error: <message>` on stderr. For a bad file the message |
There was a problem hiding this comment.
Reworded in c03dd38: refusals that already exited 2 are no longer described as a migration. The PR description repeats the same sentence; I have not edited the body.
The merge a172082 re-added dokimi/uv.lock, which intel/main does not track. It fails the license-header and large-file hooks. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
An extraction or measurement failure is a runtime failure, as it was before the bad-input change: it exits 1 through SystemExit. Input refusals (self-judge, bad files) keep exit 2. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A name bound to SystemExit under an alias (from builtins import SystemExit as Stop, or builtins.SystemExit) was not recorded, so error = Stop(2); raise error passed the exit rule. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Refusals that already exited 2 (model-compare, routing, view) are not described as a migration; model failures stay at exit 1. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| for a in node.names: | ||
| if a.name in _EXIT_MODULES: | ||
| module_aliases[a.asname or a.name] = a.name | ||
| elif isinstance(node, ast.ImportFrom) and node.module and not node.level: |
There was a problem hiding this comment.
Must-fix: relative imports bypass this check. A library fixture containing from .exit_codes import usage_exit\nusage_exit('bad')\n has no detected exit sites, so exit_problems passes despite the call raising SystemExit. from . import exit_codes as ec; ec.usage_exit('bad') is missed too. Resolve relative imports using the caller's package before matching _EXIT_CALLS / _EXIT_MODULES, and add planted-violation tests for both forms (including a nested package).
| with pytest.raises(InputError, match=message): | ||
| check_run_consistency(runs, "regular") |
There was a problem hiding this comment.
Must-fix: this test no longer verifies the exit-2 behavior it names. It calls only the library function, so it would still pass if the model-compare command caught InputError and exited 1 or failed to propagate the refusal to cli.main. The existing model-compare CLI test covers only an unwritable output, not these consistency refusals. Exercise these cases through cli.main using temporary run files, and assert SystemExit.code == 2 plus the expected stderr message; retain the InputError assertion as a separate library-contract test if needed.
There was a problem hiding this comment.
Fixed in 565c1fe. test_inconsistent_run_files_exit_2_through_cli_main (parametrized) runs report model-compare through cli.main on the refused run pairs. It asserts exit 2, the message on stderr, and no output file.
tests/test_novelty_cli.py grew past the 1000-line test cap (1006), and the growth ratchet refuses a new entry without a split. The two tests that drive cli.main (exit codes) move to tests/test_novelty_cli_exit.py. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if isinstance(node, ast.Import): | ||
| for a in node.names: | ||
| if a.name in _EXIT_MODULES: | ||
| module_aliases[a.asname or a.name] = a.name |
There was a problem hiding this comment.
Must-fix: this module-wide alias map lets imports in unrelated functions overwrite each other. For example, def f():\n import sys as s\n s.exit(2)\ndef g():\n import os as s\n s.getcwd() leaves s mapped to os, so exit_sites() returns [] and the architecture rule passes despite f() terminating the process. Resolve aliases in their lexical scope and add this conflicting-local-alias fixture to the regression tests.
There was a problem hiding this comment.
Evidence, no code change. The alias map is a set per name, so a conflicting alias over-counts and does not hide an exit. Your s fixture is dokimi/c.py in test_relative_helper_imports_and_a_reused_module_alias_are_counted: f's s.exit(2) gives [3] at 565c1fe.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at a5a5d7c. The loaders now raise one InputError, cli.main is the single place that exits 2, and model failures and gates keep exit 1; the bad-input runs I made across novelty, eval, arms, dataset and report model-compare all exit 2 with an Error: line naming the file. Three things are worth fixing before merge: one wrong sentence in the changelog and two groups of exit-code lines that no test pins. Two nits inline.
What I ran, on a Linux review host at a5a5d7c:
make ci-local: 30 passed, 0 failed, 7 skipped; changed-line coverage 94%.- About 40 bad-input cases through
cli.main(missing file, directory, bad JSON, non-UTF-8, NUL in path, wrong shape), on this head and on the base. - Mutation runs on the new code and tests; the survivors are in the inline comments.
Checked, no issues
- Messages: every message the changelog quotes matches the CLI output, and each exits 2.
ValueErrorcatches:InputErroris aValueError; I checked eachexcept ValueErrorthat wraps anInputErrorsource.parse_armsandcheck_armsrun before thetryblocks inanalyzeandprobe, so none turns into exit 1.- Mutations killed:
cli.mainexiting 1 or not catchingInputError, a loader raisingSystemExit, dropping the path from a message, andInputErrorno longer being aValueErrorall fail the new tests. - Housekeeping: all 13 non-merge commits carry
Signed-off-by; the growth baselines match the files.
Bot points I checked that no longer apply at this head (5)
- The exiting-in-
elsepoint from exai-bot on the__main__guard does not apply: only theif __name__ == '__main__'body is exempt, and theelsebranch is counted. - exai-bot's point on
match=and Windows paths in the novelty gate test does not apply: the test assertsstr(bad) in str(exc.value). - exai-bot's point on aliased
SystemExitconstructors does not apply:import builtinsandfrom builtins import SystemExit as Xforms are counted at this head. - exai-bot's point on a bound alias of
SystemExit(error = Stop(2); raise error) does not apply: the bound name is resolved at this head. - Copilot's point on extraction failures exiting 2 does not apply:
novelty atomsandnovelty analyzeraiseSystemExit(msg)forExtractionError, so a model failure exits 1 (the unpinned branches are in my first inline comment).
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| bound = {t.id for n in ast.walk(tree) if isinstance(n, ast.Assign) and isinstance(n.value, ast.Call) | ||
| and is_system_exit(n.value.func) for t in n.targets if isinstance(t, ast.Name)} |
There was a problem hiding this comment.
Must-fix: annotated assignments bypass the bound-exception check. For error: SystemExit = SystemExit(2)\nraise error, exit_sites() returns []: the binding is an ast.AnnAssign, and constructing SystemExit is not counted as an exit call. Thus the architecture rule accepts a library that terminates its caller. Handle annotated assignments alongside ordinary assignments and add a regression fixture asserting the raise line is detected.
There was a problem hiding this comment.
Fixed in 565c1fe. Annotated bindings (error: SystemExit = SystemExit(2)) are read in _exit_bindings. Pinned by test_annotated_and_scoped_bindings_of_a_raised_exit_are_counted_per_scope.
| continue | ||
| if isinstance(node, ast.Raise) and node.exc is not None: | ||
| exc = node.exc.func if isinstance(node.exc, ast.Call) else node.exc | ||
| if ((isinstance(exc, ast.Name) and (exc.id == "SystemExit" or exc.id in bound or exc.id in exc_names)) |
There was a problem hiding this comment.
Must-fix: bound combines names from unrelated function scopes, so an ordinary exception can be reported as a process exit. For def f():\n error = SystemExit(2)\n return error\ndef g():\n error = ValueError('bad')\n raise error, the checker reports line 6 even though neither function terminates the process. Same-scope reassignment has the same problem. Resolve a raised variable's binding within its scope rather than treating every occurrence of its name as SystemExit, and add negative fixtures for these cases.
There was a problem hiding this comment.
Fixed in 565c1fe. Bindings are per scope, and a raise walks outward through its enclosing scopes (_bound_to_exit). The same test covers a function that binds its own error and another that raises an unrelated one.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 97059b0, which fix the two nits from my review:
ff8a48b:dokimi/dokimi/commands/novelty.py:185: removed the redundantunloadablebranch, so oneraise InputError(verdict.message)remains; lowered the file's growth baseline from 1100 to 1098.97059b0:dokimi/tests/test_import_architecture.py:420:_exit_importsnow resolves relative imports ofexit_codesand keeps a set of modules per alias name; added a test with the three planted fixtures.
make ci-local at 97059b0: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the three risks from my review.
The exit-site detector resolves `error = SystemExit(2); raise error` per scope (annotated and per-function bindings included) and is split into helpers to stay under the complexity cap. Refused novelty and model-compare inputs are checked through cli.main for exit 2, treatment-arm refusals name the arm, and a model failure in analyze/probe is checked to exit 1 with its message. The arch-loader changelog states the subjects run exit as 2. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if isinstance(node, ast.Import): | ||
| for a in node.names: | ||
| if a.name in _EXIT_MODULES: | ||
| module_aliases.setdefault(a.asname or a.name, set()).add(a.name) |
There was a problem hiding this comment.
Must-fix: import aliases are collected across unrelated scopes, so a local import can cause compliant code elsewhere to fail the architecture gate. For def f():\n import sys as s\n s.exit(2)\ndef g(s):\n s.exit(), the checker reports both lines 3 and 5, although g may receive an object with a non-terminating exit method. _builtin_exit_names similarly makes a function-local SystemExit as Stop import affect unrelated raise Stop sites. Resolve imported names in their owning scope and respect parameter/reassignment shadowing, with regression tests ensuring only the genuine exit is counted.
There was a problem hiding this comment.
Fixed in d3e08f6 (annotation follow-up 43a7726): exit names now resolve per Python scope in tests/test_import_architecture.py.
A function-local import sys as s no longer makes s.exit() in an unrelated g(s) count, and a function-local SystemExit as Stop no longer affects raise Stop elsewhere.
Red first: test_a_name_in_one_scope_does_not_change_what_another_scope_means gave [3, 6] on the old checker, [3] now.
| if id(scope) not in owner: # the module: no enclosing scope | ||
| return False | ||
| scope = owner[id(scope)] |
There was a problem hiding this comment.
Must-fix: this lookup does not follow Python's lexical scoping. For error = SystemExit(2)\ndef f(error):\n raise error, exit_sites() reports line 3 even when f receives a ValueError, because parameters are not recorded as local bindings. Conversely, error = SystemExit(2)\nclass C:\n error = ValueError()\n def f(self):\n raise error returns no exit site: the checker consults the class binding, but Python skips class namespaces when resolving a method's bare names, so calling C().f() raises the module-level SystemExit. Account for parameter/local shadowing and skip class namespaces when walking outward from methods; add both regression fixtures.
There was a problem hiding this comment.
Fixed in d3e08f6: a parameter shadows a module-level name, and a method no longer sees its class body when it resolves a bare name.
Both fixtures are in that test: def f(error): raise error is not counted, and raise error in a method of a class that sets error = ValueError() counts the module's SystemExit.
Full suite in the worktree: 12159 passed, 28 skipped, 6 xfailed.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…li.main exit_sites read imports and aliases from the whole file: a function-local `import sys as s` or `from builtins import SystemExit as Stop` counted in unrelated scopes, parameters did not shadow module names, and a method saw its class body. Names now resolve per scope like Python's (parameters, assignments and imports; class bodies skipped from methods), with a red regression test. test_skill_repo_shapes ran the commands directly, so the InputError that cli.main now maps to exit 2 escaped as a crash (11 CI failures after the merge of intel/main). The helper goes through cli.main as the console script does. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Self-review nit on the scope resolution: `x: int` with no value binds x in Python, so a later `raise x` in that function is not the module's SystemExit. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| _EXIT_MODULES = {m for m, _ in _EXIT_CALLS} | ||
|
|
||
|
|
||
| _SCOPE_NODES = (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda, ast.ClassDef) |
There was a problem hiding this comment.
Must-fix: comprehension targets are currently recorded as bindings in the enclosing scope. For import sys\n[x for sys in []]\nsys.exit(2)\n, exit_sites(ast.parse(source)) returns [] instead of [3], although Python leaves the module's sys binding intact. Thus an unrelated comprehension can hide a subsequent process exit from the architecture gate. Isolate targets for list/set/dict comprehensions and generator expressions, while evaluating their outermost iterable in the enclosing scope, and pin this example with a regression test.
There was a problem hiding this comment.
Fixed in 5629960. Comprehension targets bind in their own scope; the outermost iterable still resolves in the enclosing scope. import sys / [x for sys in []] / sys.exit(2) now returns [3]. Regression fixtures in dokimi/tests/test_exit_sites.py, red before the fix and green after.
| node, scope = stack.pop() | ||
| for child in ast.iter_child_nodes(node): | ||
| owner[id(child)] = scope | ||
| stack.append((child, child if isinstance(child, _SCOPE_NODES) else scope)) |
There was a problem hiding this comment.
Must-fix: definition-time expressions are assigned the function's scope, but Python evaluates defaults and decorators in the enclosing scope. For import sys\ndef f(sys=sys.exit(2)):\n pass\n, exit_sites(ast.parse(source)) returns [] instead of [2]: the recorded parameter shadows the module while resolving the default. This lets a library that exits during import pass the new gate. Keep defaults and decorators in the enclosing scope (including async functions and lambda defaults), and add regression fixtures.
There was a problem hiding this comment.
Fixed in 5629960. Defaults and decorators resolve in the enclosing scope, for def, async def and lambda. def f(sys=sys.exit(2)) now returns [2]. Fixtures in dokimi/tests/test_exit_sites.py.
| kind = "sysexit" if _is_sysexit_expr(n.value, owner[id(n)], n.lineno, table, owner) else "other" | ||
| for t in (n.targets if isinstance(n, ast.Assign) else [n.target]): | ||
| if isinstance(t, ast.Name): | ||
| _bind(table, owner[id(n)], t.id, n.lineno, kind) |
There was a problem hiding this comment.
Must-fix: assignments declared global or nonlocal are still recorded as local bindings, allowing real library exits to pass the architecture gate. For import sys\ndef f():\n global sys\n sys.exit(2)\n sys = None\n, exit_sites(ast.parse(source)) returns [] rather than [4]: the later assignment makes _resolve treat sys as an unbound local despite the global declaration. The same problem occurs with nonlocal aliases of SystemExit. Resolve declared binding scopes before recording assignments/imports and looking up names, and add fixtures for both declarations.
There was a problem hiding this comment.
Fixed in 5629960. Names declared global or nonlocal resolve to the declared scope, so a later assignment no longer makes them local. Both declarations have fixtures in dokimi/tests/test_exit_sites.py; global sys / sys.exit(2) / sys = None now returns [4].
| if scope is use: | ||
| found = [b for b in found if b[0] <= lineno] or [(lineno, "other", None)] |
There was a problem hiding this comment.
Must-fix: a class body does not treat a name assigned later as an unbound local; it falls back to the enclosing scope until that assignment executes. For import sys\nclass C:\n sys.exit(2)\n sys = None\n, this lookup substitutes an other binding because the class assignment is below the use, so exit_sites(ast.parse(source)) returns [] instead of [3]. The module actually exits while defining C, but the architecture gate passes. When the current scope is a class and there is no preceding binding, continue resolving in the enclosing scope instead of returning the function-local unbound sentinel, and add this fixture.
There was a problem hiding this comment.
Fixed in 5629960. In a class body with no earlier binding, lookup continues in the enclosing scope, as Python does. import sys / class C: / sys.exit(2) / sys = None now returns [3]. Fixture in dokimi/tests/test_exit_sites.py.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 43a7726. The scope-aware exit rule and the new cli.main tests are a good step. The merge still waits for a red CI check, one changelog sentence and the legacy-format model-compare refusals, listed below. One nit inline.
What I ran, on a Linux review host at 43a7726:
make ci-local: 29 passed, 1 failed, 7 skipped. The failure isComment blocks(inline comment). 12,128 tests passed, 59 skipped, 6 xfailed; changed-line coverage 96%.- Mutation runs on the
InputErrorraises inreport/model_compare.pyand the model-failure branches incommands/novelty.py: the novelty branches and the new-formatextract_scoresraises are killed; two survive (below). exit_siteson the fixtures from the open bot comments, with the LLM transport mocked where a model is involved.
I agree with @xaleryb that the changelog misstates the exit change. New evidence: at the PR's base, novelty view, probe --atoms (missing and non-object), shorten --atoms and items --probes on a non-object file already exit 2; the changelog still says the novelty file refusals "exited 1 before" (lines 11-12).
Still open from my earlier review:
dokimi/dokimi/report/model_compare.py:528and:535: the legacy-format (noevaluationskey)--treatment-armnot found / multiple non-baseline arms raises are still not pinned. Changing either toraise SystemExit(...)leaves the 106test_model_compare*tests green, so a legacy run would exit 1 instead of 2. The new-format twins at:478and:485are killed. Add onecli.maincase with a run file that has noevaluationskey.
Checked, no issues
- Model failures: changing the
SystemExit(str(exc))branches inanalyze,atomsandprobetoInputErrorfails a test each. cli.main: exiting 1 instead of 2 onInputErrorfails the novelty and model-compare suites.subjects run --novelty-artifact: the changelog now says it keeps exit 2, which matches the code.- Commits: all 18 non-merge commits carry
Signed-off-by.
Bot points I checked that no longer apply at this head (13)
- exai-bot's point on an exit in the
elseof the__main__guard does not apply: only theifbody is exempt and theelsebranch is counted. - exai-bot's point on
match=and Windows paths in the novelty gate test does not apply: the test assertsstr(bad) in str(exc.value). - exai-bot's point on aliased
SystemExitconstructors does not apply: thebuiltins.SystemExitandfrom builtins import SystemExit as Xforms are counted at this head. - exai-bot's point on model failures exiting 2 does not apply:
novelty analyze,atomsandproberaiseSystemExit(msg)for a model failure, and acli.maintest pins exit 1 foranalyzeandprobe. - exai-bot's point on a bound alias of
SystemExit(error = Stop(2); raise error) does not apply: the bound name is resolved at this head. - Copilot's point on extraction failures exiting 2 does not apply: they exit 1 with the model's message at this head.
- exai-bot's point on relative imports of
usage_exitdoes not apply:from .exit_codes import usage_exitandfrom . import exit_codes as ecare counted. - exai-bot's point on
test_model_compare_exit_codes.pynot checking exit 2 does not apply: the refusals now run throughcli.mainand assert exit 2, stderr and no output file. - exai-bot's point on conflicting function-local import aliases hiding an exit does not apply:
f'ss.exit(2)is still counted whengimportsos as s. - exai-bot's point on annotated assignments of
SystemExitdoes not apply:error: SystemExit = SystemExit(2); raise erroris counted. - exai-bot's point on bindings shared between unrelated functions does not apply: bindings are per scope and a
ValueErrorraised in another function is not counted. - exai-bot's point on aliases collected across unrelated scopes does not apply: a function-local
import sys as sno longer makess.exit()ing(s)count. - exai-bot's point on parameters and class bodies in the lookup does not apply: a parameter shadows the module name and a method does not see its class body.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| elif isinstance(n, ast.Name) and isinstance(n.ctx, ast.Store) and id(n) not in assigned: | ||
| _bind(table, owner[id(n)], n.id, n.lineno, "other") |
There was a problem hiding this comment.
Must-fix: annotation-only targets shadow runtime bindings even at module scope. For import sys\nsys: object\nsys.exit(2)\n, this records the annotation as other, so exit_sites() returns [] instead of [3] and the architecture gate accepts a real library exit. Python does not rebind a module or class name for an annotation without a value; only function scopes make that name local. Exclude annotation-only targets from these bindings outside function scopes, and add module/class regression cases alongside the existing function-annotation test.
There was a problem hiding this comment.
Fixed in 5629960. Annotation-only targets (sys: object) no longer rebind outside function scopes: import sys / sys: object / sys.exit(2) now returns [3]. Inside a function they still make the name local. Module and class fixtures sit next to the function-annotation test in dokimi/tests/test_exit_sites.py.
| """``x = <SystemExit>`` binds x to SystemExit. In source order, so a later value sees the earlier bindings.""" | ||
| assigns = [n for n in ast.walk(tree) if isinstance(n, (ast.Assign, ast.AnnAssign)) and n.value is not None] | ||
| for n in sorted(assigns, key=lambda n: n.lineno): | ||
| kind = "sysexit" if _is_sysexit_expr(n.value, owner[id(n)], n.lineno, table, owner) else "other" |
There was a problem hiding this comment.
Must-fix: assignment-based exit aliases are recorded as other, so exit_sites(ast.parse("import sys\nstop = sys.exit\nstop(2)\n")) returns [] instead of [3]. The module-alias form (s = sys; s.exit(2)) also escapes detection. This lets a library terminate the process while passing the new architecture gate. Preserve fn and mod bindings when resolving assignment values, and add regression fixtures for both forms (including annotated assignments).
There was a problem hiding this comment.
Fixed in 5629960 and 26c33c2. Assignments keep fn and mod kinds: stop = sys.exit / stop(2) returns [3], s = sys / s.exit(2) is caught, annotated assignments included. 26c33c2 lets an enclosing exit-kind binding count after a later non-exit rebinding. Known limit, documented in the exit_sites docstring: after import sys, a later module-level rebinding (except ImportError: sys = None) is read as the rebinding for later module code.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 744d212, which fix the nit from my review:
744d212:dokimi/tests/test_import_architecture.py:559: theexit_sitesdocstring now says names resolve by function scope and lists the five known scoping gaps.
make ci-local at 744d212: 29 passed, 1 failed, 7 skipped (same Comment blocks failure as at the reviewed head).
Not approving yet. Still open: the risk from my review; the earlier thread of mine above nit level; @xaleryb on changelog says the novelty file refusals 'exited 1 before'; at the PR's base they already exit 2, so the entry misdescribes the exit change.
…ugh cli.main The per-answer (legacy) branch of extract_scores raises InputError for an unknown --treatment-arm and for several treatment arms, as the evaluations branch does. Parametrise the cli.main refusal test over both formats: with the legacy raises changed to SystemExit the new cases fail. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ed cases - comprehensions are scopes; the outermost iterable, defaults, decorators, annotations, return annotations and class bases are evaluated in the enclosing scope - global and nonlocal declarations move the bindings to the scope they name - a class body reads an enclosing name until its own assignment runs - an annotation without a value binds nothing outside a function - assignments copy what they name: sys.exit, sys, SystemExit, aliases An enclosing scope is read as its first binding of the name. Known gap kept and documented: a module-level rebinding after an import, such as except ImportError: sys = None. The docstring block is cut to two lines (Comment blocks check). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…r rebinding A function reads an enclosing name as its last exit-kind binding, so stop = None; stop = sys.exit; def f(): stop(1) is still an exit site. Self-review found that reading the first binding missed it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
test_import_architecture.py is over the 1000-line growth cap with the new scoping cases. The exit-site tests move unchanged to their own module; the new cases stay with them. No assertion changes. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| elif isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): | ||
| _bind(table, owner[id(n)], n.name, n.lineno, "other") |
There was a problem hiding this comment.
Must-fix: the definition's name shadows the existing binding before its header is evaluated. For import sys\ndef sys(x=sys.exit(2)):\n pass\n, this records sys as other on line 2, so exit_sites() returns [] rather than [2]. Python evaluates the default before binding the new function, so importing this module actually exits. The same problem affects class sys(sys.exit(2)): and async definitions. Moving headers to the enclosing scope does not fully fix definition-time resolution: exclude the definition's own new binding while resolving its defaults, decorators and bases, and add regression fixtures.
There was a problem hiding this comment.
| if isinstance(expr, ast.Name): # ``stop = exit_alias`` copies the binding it names | ||
| b = _resolve(expr.id, scope, lineno, table, owner) | ||
| if b is not None and b[1] in ("fn", "mod"): | ||
| return b[1], b[2] |
There was a problem hiding this comment.
Must-fix: unshadowed builtin exit and quit are recognized when called directly, but not when assigned to an alias. For stop = exit\nstop(2)\n, _resolve('exit', ...) returns None, so this branch records stop as other and exit_sites() returns [] instead of [2]. The same applies to quit, allowing a library process exit to pass the new architecture gate. Recognize these unbound builtin names as fn bindings here (while retaining shadowing behavior), and add fixtures for both aliases and a shadowed builtin.
| for n in sorted(assigns, key=lambda n: n.lineno): | ||
| kind, payload = _value_binding(n.value, owner[id(n)], n.lineno, table, owner) |
There was a problem hiding this comment.
Must-fix: processing assignments once in source order misses enclosing bindings established after a function definition. For import sys\ndef f():\n stop = end\n stop(2)\nend = sys.exit\nf()\n, this records stop as other before end is recorded, so exit_sites() returns [] although calling f() exits at line 4. The same gap affects aliases of a later-bound SystemExit. Resolve enclosing assignment dependencies after their bindings are populated (e.g. a fixed-point pass), and add regression fixtures for both cases so the architecture gate rejects these library exits.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at d78866b. The commits since my last review fix both open threads: the legacy-format --treatment-arm refusals now exit 2 through cli.main, and the Comment blocks check passes. The merge still waits for one changelog sentence, which was raised earlier and is unchanged.
I agree with @xaleryb that the changelog misstates the exit change (line 12). New evidence: the novelty file refusals already exit 2 at the PR's base; only the message changes.
exai-bot's three later points on exit_sites (a def or class whose header reads the name it rebinds, an alias of the builtin exit or quit, and an enclosing binding made after the function) are real gaps in the test helper, but only a hand-built module reaches them, so they need not hold the merge.
What I ran, on a Linux review host at d78866b:
make ci-local: 30 passed, 0 failed, 7 skipped; 12,134 tests passed, 59 skipped, 6 xfailed; changed-line coverage 97%.- Mutation runs: each of the four
raise InputErrorinextract_scoreschanged toraise SystemExit(...)failstest_model_compare_exit_codes.py, alone and together. 37 mutants of the exit-site resolver: 12 pass the full suite, all in helper branches that only a hand-built module reaches. - 54 further fixtures through
exit_sites(aliases,from sys import exit, lambdas,async def,os._exit, walrus,with ... as sys): every ordinary call or raise of an exit is found. - Bad-input runs for 18
noveltycommands and inputs on the base and on this head, with the model transport blocked.
Checked, no issues
- Commits: all 23 non-merge commits carry
Signed-off-by. - Treatment-arm refusals: the legacy and new formats both exit 2 with the message on stderr.
- Real tree:
EXIT_BASELINEis empty and the exit rule passes.
Bot points I checked that no longer apply at this head (19)
- exai-bot's point on an exit in the
elseof the__main__guard does not apply: only theifbody is exempt. - exai-bot's point on
match=and Windows paths in the novelty gate test does not apply: the test assertsstr(bad) in str(exc.value). - exai-bot's point on
builtins.SystemExitand aliasedSystemExitraises does not apply: both forms are counted at this head. - exai-bot's point on
novelty analyze/atoms/probemodel failures exiting 2 does not apply: they raiseSystemExit(msg)and exit 1, pinned throughcli.main. - exai-bot's point on a bound alias (
error = Stop(2); raise error) does not apply: the bound name is resolved at this head. - Copilot's point on extraction failures exiting 2 does not apply: they exit 1 with the model's message at this head.
- exai-bot's point on relative imports of
usage_exitdoes not apply:from .exit_codes import usage_exitandfrom . import exit_codes as ecare counted. - exai-bot's point on
test_model_compare_exit_codes.pynot checking exit 2 does not apply: the refusals run throughcli.mainand assert exit 2, in both the new and the legacy format. - exai-bot's point on conflicting function-local import aliases hiding an exit does not apply: resolution is per scope at this head.
- exai-bot's point on annotated assignments of
SystemExitdoes not apply:error: SystemExit = SystemExit(2); raise erroris counted. - exai-bot's point on bindings shared between unrelated functions does not apply: bindings are per scope at this head.
- exai-bot's point on aliases collected across unrelated scopes does not apply: a function-local
import sys as sno longer makess.exit()ing(s)count. - exai-bot's point on parameters and class bodies in the lookup does not apply: a parameter shadows the module name and a method does not see its class body.
- exai-bot's point on comprehension targets does not apply: they bind in their own scope, and
[x for sys in []]followed bysys.exit(2)is counted. - exai-bot's point on defaults and decorators does not apply: they resolve in the enclosing scope, and
def f(sys=sys.exit(2))is counted. - exai-bot's point on
globalandnonlocaldoes not apply: declared names resolve to the declared scope. - exai-bot's point on class-body fall-through does not apply: a class body with no earlier binding resolves in the enclosing scope.
- exai-bot's point on annotation-only names does not apply: outside function scopes they no longer rebind the name.
- exai-bot's point on
stop = sys.exitands = sysaliases does not apply: both keep their exit kind and are counted.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/scripts/growth_baseline.json
|
|
||
| def _assignment_bindings(tree: ast.Module, owner: dict[int, ast.AST], table: ExitTable) -> None: | ||
| """``x = <value>`` binds x to what the value is. In source order, so a later value sees the earlier bindings.""" | ||
| assigns = [n for n in ast.walk(tree) if isinstance(n, (ast.Assign, ast.AnnAssign)) and n.value is not None] |
There was a problem hiding this comment.
Must-fix: assignment expressions lose exit aliases. For import sys\nif (stop := sys.exit):\n stop(2)\n, exit_sites(ast.parse(source)) returns [] instead of [3]: the NamedExpr target is recorded as other by _shadowing_bindings, and this assignment pass never updates it. Consequently the architecture test accepts a real process exit outside the frontend. Include ast.NamedExpr in value-binding analysis and add a regression fixture; for named expressions inside comprehensions, bind their targets in the enclosing scope as Python does.
| if target is not None and target is not scope: | ||
| table.setdefault((id(target), name), []).extend(table.pop((id(scope), name), [])) |
There was a problem hiding this comment.
Must-fix: moving bindings does not make uses obey global declarations. For import sys\ndef outer(sys):\n def inner():\n global sys\n sys.exit(2)\n return inner\n, exit_sites(ast.parse(source)) returns [], although outer(None)() exits: _resolve still walks through outer and selects its parameter instead of the module import. The same lookup problem applies to a globally declared SystemExit alias. Record declaration targets and consult them during resolution, with a regression fixture for a global name shadowed by an enclosing function parameter.
There was a problem hiding this comment.
| before = [b for b in found if b[0] <= lineno] | ||
| if before: | ||
| return sorted(before, key=lambda b: b[0])[-1] |
There was a problem hiding this comment.
Must-fix: binding resolution loses statement order within a line. For import sys; stop = sys.exit; stop(2); stop = None, exit_sites(ast.parse(source)) returns [] instead of [1]: both assignments qualify as preceding the call, and the final other binding wins. The same problem affects module aliases and raised SystemExit aliases. Track column positions (and resolve assignment values before binding their targets), and add same-line regression fixtures so the architecture gate rejects these library-side exits.
A def or class name binds after its header, so its own defaults see the enclosing name. Builtin exit and quit can be aliased. A walrus binds its target. A global or nonlocal declaration sends the lookup to the scope that binds the name. Bindings in one statement count by column, and an alias of a later module binding resolves once the assignments settle. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
_declared_scopes ran before _assignment_bindings, so a nonlocal whose binder was a plain assignment found no binder. Global declarations still run first; nonlocal ones now run after the assignments are bound. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…xit-site resolver A second global declaration of a name in one function re-added its redirect to the module, and the lookup looped on it. A scope that only declares a name nonlocal binds nothing, so the binder search now skips it and reaches the enclosing binder. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| _bind(table, owner[id(n)], n.name, _at(n.body[0]), "other") | ||
| elif isinstance(n, ast.ExceptHandler) and n.name: | ||
| _bind(table, owner[id(n)], n.name, _at(n.body[0]), "other") | ||
| elif isinstance(n, ast.Name) and isinstance(n.ctx, ast.Store) and id(n) not in assigned: |
There was a problem hiding this comment.
Must-fix: structural-pattern captures are missing from the binding table because their names are strings, not ast.Name(Store) nodes. For import sys\ndef f(value):\n match value:\n case sys:\n sys.exit(2)\n, the detector reports line 5 even though sys is the captured argument, not the imported module. This can reject valid library code calling an unrelated object's exit method. Record MatchAs.name, MatchStar.name, and MatchMapping.rest as shadowing bindings, with regression tests.
There was a problem hiding this comment.
Fixed in 5957355. Match captures (case x:, [*x], {**x}) now bind their name after the pattern (_capture_bindings in tests/exit_site_scan.py), so sys.exit(2) in a case body that captured sys is not counted. Red cases are in the parametrized list of test_exit_sites_resolves_names_by_scope_and_source_order.
| elif isinstance(n, ast.Name) and isinstance(n.ctx, ast.Store) and id(n) not in assigned: | ||
| # ``x: int`` binds nothing at run time outside a function, where it makes x local | ||
| if id(n) not in bare or isinstance(owner[id(n)], _FUNC_NODES): | ||
| _bind(table, owner[id(n)], n.id, _after(n), "other") |
There was a problem hiding this comment.
Must-fix: loop targets are recorded as bound before Python evaluates the iterable. For import sys\nfor sys in sys.exit(2):\n pass\n, exit_sites(ast.parse(source)) returns []: the target's position precedes the call, so it shadows the import. Python actually exits on line 2 before assigning the target. Augmented assignments such as import sys\nsys += sys.exit(2)\n have the same problem. Record these bindings after their iterable/RHS evaluation and add regression cases so the architecture gate cannot silently accept these exit sites.
There was a problem hiding this comment.
Fixed in 5957355. for, with and augmented-assignment targets bind after the iterable, context manager or value (_late_targets). for sys in sys.exit(2) and sys += sys.exit(2) now count their line. Red cases added to the same test.
| for t in targets: | ||
| if isinstance(t, ast.Name): |
There was a problem hiding this comment.
Must-fix: unpacked aliases silently bypass the new architecture gate. For import sys\nstop, = (sys.exit,)\nstop(2)\n, exit_sites() returns [], although line 3 exits the process. _assigned ignores tuple/list targets, while _shadowing_bindings classifies their names as other. Resolve matching tuple/list literal values into their targets (including nested unpacking), and add regression cases for exit callables, module aliases, and SystemExit aliases.
There was a problem hiding this comment.
Fixed in 5957355. A name unpacked from a literal tuple or list of the same shape keeps the callable or SystemExit it is given (_pairs), so stop, = (sys.exit,) and nested a, (b, c) = 1, (sys.exit, 2) count. Starred and arity-mismatched targets still bind as other names; cases added.
| if not isinstance(use, ast.ClassDef): # a function's local, not bound yet | ||
| return (pos, "other", None) |
There was a problem hiding this comment.
Must-fix: this applies function-local unbound-name rules to module scope too. For raise SystemExit(2)\nSystemExit = ValueError\n, exit_sites(ast.parse(source)) returns [], although Python exits on line 1: the later assignment does not shadow the builtin until it executes. The same bypass affects exit(2)\nexit = print\n and quit(2)\nquit = print\n. Restrict the unbound-local result to function/comprehension scopes and let a module with no preceding binding fall back to builtins; add regression cases for all three names.
There was a problem hiding this comment.
Fixed in 5957355. The function-local rule now applies only to function and comprehension uses, so a module-level name read before its own binding falls back to the builtin: raise SystemExit(2) before SystemExit = ValueError and exit(2) before exit = print count. Red cases added.
…_bind's docstring The four-line comment failed the Static checks comment-block gate. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
test_import_architecture.py was 1035 lines, over the 1000-line test cap with no baseline. The AST scan that finds exit sites moves to its own module; the rules and tests stay. Function bodies are unchanged. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…l unpacking and module names before their binding Four gaps the review found in the exit-site scan: - match captures (case x:, *x, **x) bind x, so a later exit call through x is not counted - for, with and augmented assignment targets bind after the iterable, context or value - a name unpacked from a literal tuple or list (stop, = (sys.exit,)) keeps the callable - a module-level name read before its own assignment is the builtin, not an unbound local Red cases added to test_exit_sites_resolves_names_by_scope_and_source_order first. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| pos = _after(node) | ||
| if isinstance(node, ast.Import): # ``import sys as s`` binds s; ``import dokimi.exit_codes`` binds that dotted name | ||
| for a in node.names: | ||
| _bind(table, scope, a.asname or a.name, pos, "mod", frozenset({a.name})) |
There was a problem hiding this comment.
Must-fix: an unaliased dotted import is recorded independently of its root binding. For import dokimi.exit_codes\ndef f(dokimi):\n dokimi.exit_codes.usage_exit('x')\n, the scanner reports line 3 even though dokimi is the argument, not the imported package. Rebinding the root at module level has the same problem. Resolve the dotted reference through the root name's current binding before recognizing the helper, and add parameter-shadowing and root-rebinding tests.
| elif isinstance(n, ast.ExceptHandler) and n.name: | ||
| _bind(table, owner[id(n)], n.name, _at(n.body[0]), "other") |
There was a problem hiding this comment.
Must-fix: deleted bindings remain in the table, so a real exit can bypass the architecture gate. For SystemExit = ValueError\ndel SystemExit\nraise SystemExit(2)\n, the scanner returns []: it retains the other assignment even though Python deletes it and resolves the final name to builtin SystemExit. Exception targets have the same lifetime issue: after try: 1 / 0\nexcept Exception as SystemExit: pass, Python automatically clears SystemExit, but the scanner keeps its shadowing binding. Track explicit deletion and exception-target cleanup, with regression cases asserting the final raise is counted.
| _assignment_bindings(tree, owner, table) | ||
| _declared_scopes(tree, owner, table, "nonlocal") |
There was a problem hiding this comment.
Must-fix: assignment aliases are settled before nonlocal bindings move to their enclosing scope, and are never recomputed afterward. For import sys\ndef outer():\n x = None\n def set_exit():\n nonlocal x\n x = sys.exit\n def run():\n stop = x\n stop(2)\n set_exit()\n run()\nouter()\n, the scanner returns [], although line 9 exits. While resolving stop = x, the outer binding is still None; moving the exit binding later leaves stop classified as other, so the architecture gate accepts the violation. Resolve nonlocal destinations before settling assignment aliases, or settle both together, and add a regression covering this case (and the equivalent aliased SystemExit raise).
| if found and not here: # a function may run later: its exit-kind binding counts | ||
| exits = [b for b in found if b[1] != "other"] or found | ||
| return sorted(exits, key=lambda b: b[0])[-1] |
There was a problem hiding this comment.
Must-fix: enclosing lookups use deferred-function rules even for eagerly executed class bodies. For E = SystemExit\nE = ValueError\nclass C:\n raise E(2)\n, this reports line 4 as a process exit, although Python raises ValueError. The same issue affects enclosing module aliases used by class bodies and eager comprehension bodies. Preserve source-position lookup until crossing a deferred execution boundary, and add regression cases alongside the existing class-body tests.
Part of #4.
Summary
Every input loader raises one typed error, and only the frontend turns it into an exit code.
Before,
load_*_or_exithelpers calledSystemExitfrom library code, so a bad file and a failed gate both looked like exit 1. Now a script can branch on the code: 2 means fix the input, 1 means the gate said no.Evidence
tests/test_load_or_fail.py::test_a_bad_input_file_is_an_input_error_naming_it, parametrised over every artifact kind plusatoms,judged-runandanswers-records(22 loaders) and five inputs (missing, not UTF-8, bad JSON, not an object, wrong schema), 110 cases. Each case assertspytest.raises(InputError)and that the file path is in the message. On main the atoms and artifact loaders callSystemExitinstead, which that assertion rejects.tests/test_import_architecture.py::test_only_the_frontend_ends_the_process: no module outside the frontend may callsys.exitoros._exit, through an import alias too (import sys as s,from sys import exit as bye), or raiseSystemExit, except a__main__guard.EXIT_BASELINEis empty. A planted violation (test_a_process_exit_outside_the_frontend_fails) fails the rule.tests/test_novelty_atoms_input.py: an unreadable--atomsfile throughcli.main()exits 2 withError:and the path on stderr, fornovelty probeandnovelty shorten, over every malformed shape in the test's table.--randomly-seed=101and--randomly-seed=202: 11041 passed, 5 skipped eachmypywith the project config: no issues in 286 filesmypy --stricton the new module: clean; no strict error lands on a line this PR addsruff checkandpre-commit runon the changed files: passscripts/check_growth.py: no violation; the three baselines went down (novelty.py1190 → 1102,model_compare.py1207 → 1195,cmd_novelty_routingentry removed)Merge Danger
Door: one-way, for the exit code and the two changed messages. A script that treats any non-zero as "gate failed" will now see 2 for a bad input file.
Blast Radius: cli
novelty view,novelty probe,novelty shorten,novelty routingandreport model-compareexit 2 instead of 1 on a bad input file (missing, unreadable, not UTF-8, not JSON, not the artifact it names). The stderr line names the file and the reason (Error: cannot read <file>: …,Error: <file> is not valid JSON: …).novelty routingalso exits 2 for a refused field (a missing competitor, a skill listed twice or compared with itself, an all-vs-all catalog that holds one skill twice with different content). Those messages name the skill or option, not a file.report model-comparemessages:File not found: <file>is nowcannot read <file>: No such file or directory;Invalid JSON in <file>is now<file> is not valid JSON.novelty viewnames the reason for an invalid artifact (is not valid JSON,is not UTF-8 text,is not a novelty artifact: ...) where it saidis not a valid novelty artifact.--strict,eval judge-validity) and for verdicts.subjects run --novelty-artifactexits 2 on a malformed artifact, as before; itsError loading novelty artifact:line now names the reason.Follow-ups, deliberately not in this PR:
report generateandquestions analyzestill exit 1 on bad input (documented exception);baseline saveandlist; theREADMEbenchmark command;cmd_novelty_probeanswer-key refusals other thanunloadable(wrong_artifact,files_changed,no_file_rows) still exit 1; the remaining usage-errorSystemExitcalls innovelty(--all-vs-all, missing--artifact);ProbeFileErrorincmd_novelty_items; command-layer exits injudge_reliabilityandevaluateare kept.Checklist
pytestpassesruff,mypy, and schema checks passAGENTS.md.github/.licenserc.yaml)🤖 Generated with Claude Code