Repository navigation
Conversation
dokimi:typecheck passes when the task's listed workspace files have no mypy error, under the image's pinned mypy version. repo scan --typecheck checks each Python fence of the guidance against --python-env: a failing fence is a stale-API candidate, an unparsable fence or a scan without an interpreter is unresolved. The report gains an optional typecheck block. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Self-review of the typecheck grader and repo scan --typecheck: a missing mypy (exit 1, no report) no longer passes every fence; truncated tool output is an error; a fence-less scan runs no tool; a relative --python-env is made absolute; the grader runs mypy from a neutral directory so a workspace mypy.ini cannot suppress a listed file; the error parser accepts paths with a colon and line-less errors. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…error is the fence line ast.parse can raise RecursionError or MemoryError on a deeply nested fence; that aborted repo scan with a traceback. Such a fence is now unresolved like any other unparsable one. mypy writes line -1 for an error with no line; that was added to the fence line, pointing one line above the fence. It now maps to the fence line. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…cases Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): ~ `dokimi repo scan` parameters changed:
+ added option `--python-env`
+ added option `--typecheck`
+ added option `--typecheck-tool`Updated on every push; the |
| "the report, its claims replacing the framework's on the same file, line and text. " | ||
| "A file that cannot be read or is not JSON exits 2; a schema-invalid document is " | ||
| "reported and makes the command exit 1") | ||
| scan_p.add_argument("--typecheck", nargs="?", const="mypy", default=None, choices=("mypy", "basedpyright"), |
There was a problem hiding this comment.
Must-fix: python cli.py repo scan --typecheck . exits 2 because nargs="?" consumes . as the checker name instead of leaving it as the required source. The bare flag therefore works only in some argument orders; the new test covers only source-first usage. Make the bare flag unambiguous (for example, use a boolean --typecheck with a separate checker-selection option) and test it both before and after the source.
There was a problem hiding this comment.
Fixed in cb07efb. --typecheck is now a plain flag; --typecheck-tool {mypy,basedpyright} picks the tool (default mypy) and needs --typecheck. So repo scan --typecheck . reads . as the source. CLI snapshot, flag allowlist and contract §8.2 follow.
|
|
||
| if mypy.version.__version__ != task["version"]: | ||
| raise ValueError(f"the image's mypy is {mypy.version.__version__}, the task pins {task['version']}") | ||
| workspace = Path(run_dir or "/eval/run") / "workspace" |
There was a problem hiding this comment.
Must-fix: resolve the workspace to an absolute path before changing directories. The script accepts a relative run directory, e.g. main.py grade typecheck tasks/typed run. With an existing run/workspace/pkg/api.py, the existence check succeeds in the original directory, but api.run subsequently resolves run/workspace/pkg/api.py under the empty temporary directory and exits 2. The entry point therefore writes pass: null for an otherwise valid run. Add a regression test using a relative run_dir.
Suggestion:
| workspace = Path(run_dir or "/eval/run") / "workspace" | |
| workspace = (Path(run_dir or "/eval/run") / "workspace").resolve() |
There was a problem hiding this comment.
Fixed in 05255f9. The workspace is resolved to an absolute path before anything runs, and the grader no longer changes directory, so a relative run directory works for mypy. Covered in tests/test_repo_typecheck_grader.py.
| stdout, stderr, status = api.run(["--no-incremental", f"--cache-dir={cache}", | ||
| *[str(workspace / p) for p in present]]) |
There was a problem hiding this comment.
Must-fix: changing to an empty directory removes the workspace root from mypy's import search path. For a workspace containing pkg/api.py with from pkg.other import broken and pkg/other.py, but no pkg/__init__.py (a valid namespace package), mypy searches from the listed file's directory rather than the workspace root and reports import-not-found on pkg/api.py. This falsely fails valid code; the current import test only covers regular packages. Explicitly include the workspace root in mypy's search path while retaining config isolation, and add a namespace-package regression test.
There was a problem hiding this comment.
Fixed in 05255f9. No chdir any more; the neutral config sets mypy_path to the workspace root, so pkg.other resolves from a namespace package. Covered in tests/test_repo_typecheck_grader.py.
| if status not in (0, 1): # 2: mypy itself failed (a bad config, a crash) | ||
| raise RuntimeError(f"mypy exited {status}: {stderr[-300:]}") |
There was a problem hiding this comment.
Must-fix: mypy also returns status 2 for blocking source errors, not just tool failures. For example, a listed pkg/api.py containing def broken(: emits an [syntax] error on stdout and exits 2; this branch turns it into pass: null with no measured errors instead of the required pass: false. Distinguish source diagnostics from invocation/internal failures and add a syntax-error regression test (including a blocking error in an imported module).
There was a problem hiding this comment.
Fixed in 05255f9. Exit 2 is a diagnostic when mypy printed an : error: line; only a 2 with no diagnostic is treated as a tool failure. A syntax error in a listed file now fails the grade with its message. Covered in tests/test_repo_typecheck_grader.py.
| def oracle(task: dict[str, Any], task_dir: Path) -> str: | ||
| """No reference answer: the grade reads the workspace only.""" | ||
| del task, task_dir | ||
| return "" |
There was a problem hiding this comment.
Must-fix: this no-op oracle makes every typecheck task fail repo tasks check. selfcheck._check_step prepares identical fresh workspaces for null and oracle checks, and grading.run_entry selects the built-in oracle even when the repository declares its own. Since grading ignores answer.txt, both checks necessarily produce the same result: a broken base fails the oracle check, while a clean base fails the null check. Provide a way to apply a reference workspace repair (or delegate the oracle to the repository) and cover the null/oracle self-check end to end.
There was a problem hiding this comment.
Fixed in 05255f9. oracle copies the task's reference/ directory over the workspace, so the reference repair is what gets graded (contract §4.5). Tasks without reference/ keep the no-op oracle and cannot pass the self-check until they add one.
There was a problem hiding this comment.
Maintainer decision: reference/ is optional. With it, the oracle copies it and its repair is what gets graded. Without it, the listed files are graded as they are, and the self-check runs the null step only (the untouched files must fail). Code, §4.3/§4.5 and the changelog match, in 75f338f.
| Path(tmp, f"fence_{i}.py").write_text(code + "\n", encoding="utf-8") | ||
| names.append(f"fence_{i}.py") | ||
| if check.tool == "mypy": | ||
| argv = [sys.executable, "-m", "mypy", "--python-executable", str(check.python), |
There was a problem hiding this comment.
Must-fix: --python-executable selects the interpreter used to discover installed packages, but does not set mypy's target Python version; that still defaults to the host interpreter. For example, with dokimi running on Python 3.12 and --python-env pointing to Python 3.11, a fence containing from itertools import batched can be marked ok, although that import fails in the repository environment. Query the repository interpreter's version and pass it as --python-version, with a regression test using different host/target versions. The earlier ast.parse(code) also uses the host grammar, so valid newer-target syntax (such as type Alias = int on a 3.12 target with a 3.11 host) is incorrectly left unresolved; align that validation with the target as well.
There was a problem hiding this comment.
Fixed in eb86685. Fences are parsed with ast.parse(feature_version=...) for the repository interpreter's version, read with -I, and mypy gets --python-version X.Y from the same value.
| argv = [sys.executable, "-m", "mypy", "--python-executable", str(check.python), | ||
| "--follow-imports=silent", "--no-incremental", "--cache-dir", os.path.join(tmp, "cache"), | ||
| "--output", "json", *names] |
There was a problem hiding this comment.
Must-fix: running in a temporary directory does not disable mypy's user-level configuration discovery. For example, a host ~/.mypy.ini containing [mypy]\nignore_errors = True makes the stale json.dumps(1).nosuch() fence report ok; user settings such as ignore_missing_imports also change the promised verdicts. Pass an explicit empty/controlled --config-file so fence results do not inherit unrelated host policy, and add a regression test with a user config that suppresses errors.
There was a problem hiding this comment.
Fixed in eb86685. mypy runs with --config-file pointing at an empty [mypy] file, so host and subject configs (~/.mypy.ini, ~/.config/mypy/config, pyproject) are not read.
| "--follow-imports=silent", "--no-incremental", "--cache-dir", os.path.join(tmp, "cache"), | ||
| "--output", "json", *names] |
There was a problem hiding this comment.
Must-fix: mypy skips bodies of unannotated functions by default, so a Python fence containing import json\ndef example():\n json.dumps(1).nosuch() is reported as ok, even though the same stale call at module scope fails. Guidance examples commonly use unannotated functions. Add --check-untyped-defs to this invocation and a regression test with the stale call inside an unannotated function so those bodies are actually checked.
There was a problem hiding this comment.
Fixed in eb86685. --check-untyped-defs is on, so the bodies of unannotated functions are checked and the stale call in your example now fails.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical flaws allow module shadowing to execute checkout code and host configuration to suppress diagnostics, while broken oracle checks and error handling prevent reliable grading.
5 open issues
Grade blocking source diagnostics instead of treating them as tool failures · New Support a reference solution in the oracle workspace · New Isolate the mypy version probe from checkout modules · New Use a neutral mypy config to prevent host settings overriding checks · New Resolve the run directory before changing the working directory · New
What changed in this PR
5 new findings.
This PR adds opt-in type checking to repository evaluation: a workspace-file grader and a Python-fence check for guidance documents.
Changes:
- Registers
dokimi:typecheckwith task validation and grading metrics. - Adds
repo scan --typecheckand--python-env, with fence claims and report metadata. - Documents both features and adds tests and CLI reference updates.
| File | Description |
|---|---|
dokimi/tests/test_repo_typecheck_grader.py |
Tests grader results, validation, and entry-point integration. |
dokimi/tests/test_repo_l0_typecheck.py |
Tests fence checks, CLI behavior, and tool failures. |
dokimi/tests/data/cli_flags_allowlist.txt |
Allows the two new CLI options. |
dokimi/scripts/growth_baseline.json |
Removes the refactored scan command’s complexity exemption. |
dokimi/pyproject.toml |
Exempts the grader’s runtime mypy import from dependency checks. |
dokimi/dokimi/schemas/repo_readiness.v1.json |
Adds optional typecheck metadata. |
dokimi/dokimi/repos/manifest.py |
Validates typecheck task fields. |
dokimi/dokimi/repos/l0/typecheck.py |
Extracts Python fences and runs type checkers. |
dokimi/dokimi/repos/l0/__init__.py |
Integrates fence checks into readiness reports. |
dokimi/dokimi/repos/graders/typecheck.py |
Implements the workspace-file mypy grader. |
dokimi/dokimi/repos/graders/review.py |
Accepts the shared run-directory argument. |
dokimi/dokimi/repos/graders/main.py |
Dispatches the new grader and supplies its run directory. |
dokimi/dokimi/repos/graders/__init__.py |
Registers the grader and task validator. |
dokimi/dokimi/commands/repo.py |
Adds scan options, validation, and text reporting. |
dokimi/docs/repo-evaluation-contract.md |
Defines grader and fence-check behavior. |
dokimi/docs/reference/cli-interface.json |
Records the new CLI options. |
dokimi/changelog.d/20261031_120000_napetrov_repo-typecheck.md |
Announces both opt-in features. |
Review effort: Balanced
| if status not in (0, 1): # 2: mypy itself failed (a bad config, a crash) | ||
| raise RuntimeError(f"mypy exited {status}: {stderr[-300:]}") | ||
| errors = _errors(stdout, set(present), str(workspace) + os.sep) |
There was a problem hiding this comment.
Fixed in 05255f9. A blocking diagnostic (a syntax error, a duplicate module) is a normal result: pass false with n_errors set, not pass: null. Only a tool failure (another exit code, or a 2 with no diagnostic) raises.
| def oracle(task: dict[str, Any], task_dir: Path) -> str: | ||
| """No reference answer: the grade reads the workspace only.""" | ||
| del task, task_dir | ||
| return "" |
There was a problem hiding this comment.
Fixed in 05255f9. With reference/ present, the oracle run grades the reference repair and the null run grades the broken workspace, so the two checks differ. Covered in tests/test_repo_typecheck_grader.py.
There was a problem hiding this comment.
Maintainer decision: reference/ is optional. With it, the oracle copies it and its repair is what gets graded. Without it, the listed files are graded as they are, and the self-check runs the null step only (the untouched files must fail). Code, §4.3/§4.5 and the changelog match, in 75f338f.
|
|
||
| def tool_version(check: Check) -> str: | ||
| """The version the tool reports, e.g. ``2.4.0``.""" | ||
| argv = [sys.executable, "-m", "mypy", "--version"] if check.tool == "mypy" else [check.tool, "--version"] |
There was a problem hiding this comment.
Fixed in eb86685. Both the version probe and the mypy run use python -I with cwd outside the checkout, so a mypy.py in the repository cannot shadow the installed mypy. Covered by the L0 typecheck tests.
| if check.tool == "mypy": | ||
| argv = [sys.executable, "-m", "mypy", "--python-executable", str(check.python), | ||
| "--follow-imports=silent", "--no-incremental", "--cache-dir", os.path.join(tmp, "cache"), | ||
| "--output", "json", *names] |
There was a problem hiding this comment.
Fixed in eb86685. mypy is given --config-file pointing at an empty file, so ~/.config/mypy/config and ~/.mypy.ini are not read. Covered by the L0 typecheck tests.
|
|
||
| if mypy.version.__version__ != task["version"]: | ||
| raise ValueError(f"the image's mypy is {mypy.version.__version__}, the task pins {task['version']}") | ||
| workspace = Path(run_dir or "/eval/run") / "workspace" |
There was a problem hiding this comment.
Fixed in 05255f9. The workspace is resolved before use, so mypy gets absolute paths for the files and for mypy_path, and nothing changes directory. Same fix as the relative run directory thread above.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #337 +/- ##
==========================================
+ Coverage 94.37% 94.47% +0.09%
==========================================
Files 312 319 +7
Lines 40068 40951 +883
Branches 6801 6956 +155
==========================================
+ Hits 37816 38688 +872
- Misses 1436 1440 +4
- Partials 816 823 +7
🚀 New features to boost your workflow:
|
mypy runs with the workspace absolute, under an explicit neutral config whose mypy_path is the workspace root, and without changing directory, so a relative run directory and a namespace package both resolve. A blocking error (exit 2 with a diagnostic) is a result, not a tool failure; when mypy stops before the listed files, that error fails the grade. A task's reference/ directory is copied over the workspace by oracle, so a reference repair is what the self-check grades. The typecheck tests need mypy on the test interpreter: it is added to requirements-test.txt (the Tests (py3.11) and (py3.14) legs failed). Thread: PRRT_kwDOVBg7g86yWj1M Thread: PRRT_kwDOVBg7g86yWj1Q Thread: PRRT_kwDOVBg7g86yWj1T Thread: PRRT_kwDOVBg7g86yWj1V Thread: PRRT_kwDOVBg7g86yWj9q Thread: PRRT_kwDOVBg7g86yWj90 Thread: PRRT_kwDOVBg7g86yWj-A Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Fences are parsed with the grammar of the repository interpreter's version, and mypy runs with that --python-version, an explicit empty config (no subject or host mypy settings), --check-untyped-defs, and -I, so a module in the checkout cannot stand in for mypy in the version probe or the run. Thread: PRRT_kwDOVBg7g86yWj1X Thread: PRRT_kwDOVBg7g86yWj1Y Thread: PRRT_kwDOVBg7g86yWj1Z Thread: PRRT_kwDOVBg7g86yWj94 Thread: PRRT_kwDOVBg7g86yWj97 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…e tool
A bare --typecheck with nargs="?" read the source after it as the
checker name, so `repo scan --typecheck .` exited 2. --typecheck is now
a flag and --typecheck-tool {mypy,basedpyright} selects the tool (default
mypy); either needs --typecheck. The CLI snapshot, the flag allowlist,
the contract and the changelog fragment follow.
Thread: PRRT_kwDOVBg7g86yWj1K
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if args.python_env is not None and not os.access(args.python_env, os.X_OK): | ||
| print(f"✗ --python-env {args.python_env}: not an executable interpreter", file=sys.stderr) | ||
| sys.exit(EXIT_USAGE) |
There was a problem hiding this comment.
Must-fix: executable permission does not establish that this is a Python interpreter. On POSIX, scanning guidance containing a Python fence with --typecheck --python-env /bin/true passes this check; the version probe exits 0 with empty stdout, and python_version() raises ValueError while unpacking the version. cmd_repo_scan catches only TypecheckError, so this invalid CLI input produces a traceback instead of the intended usage diagnostic and exit 2. Validate the probe's output and convert malformed/non-Python version responses to TypecheckError; add a CLI regression test for an executable that is not Python.
There was a problem hiding this comment.
Not reproduced on 6a9c800. --typecheck --python-env /bin/true exits 2 with --typecheck mypy: /bin/true did not report its version: '' and no traceback: the version probe refuses it before any fence runs. Regression test: test_a_true_python_env_is_a_usage_error_not_a_traceback.
| stdout, stderr, status = api.run(["--no-incremental", f"--cache-dir={cache}", f"--config-file={config}", | ||
| *[str(workspace / p) for p in present]]) |
There was a problem hiding this comment.
Must-fix: mypy_path makes namespace-package imports discoverable, but does not make mypy assign the same module names to explicitly listed files. With no pkg/__init__.py, list both pkg/api.py and pkg/other.py, where api.py imports pkg.other: mypy names the explicit source other and the imported source pkg.other, then stops with “Source file found twice under different module names”, failing an otherwise clean workspace. Add --explicit-package-bases alongside the workspace mypy_path, and extend the namespace-package regression test to list both files.
There was a problem hiding this comment.
Fixed in 9dfe9b2. Listed namespace modules are named as their importer names them: --explicit-package-bases, with mypy_path = the workspace, plus the source root of each regular package (so a src/ layout still resolves). Test test_a_namespace_package_resolves_from_the_workspace_root now lists both files and fails without the flag.
| runnable = [] | ||
| for f, fence, code in found: | ||
| try: | ||
| ast.parse(code, feature_version=target) |
There was a problem hiding this comment.
Must-fix: feature_version cannot teach the host's AST parser syntax introduced after the host Python version. Dokimi supports running on Python 3.11; if --python-env is Python 3.12, a valid fence such as type Alias = int\nx: Alias = 'wrong' is rejected by the 3.11 parser and marked unresolved, so its real type error is never checked. Parse using the selected repository interpreter, or explicitly reject unsupported newer target versions instead of silently treating valid repository syntax as invalid Python. Add coverage with a newer target than the host.
There was a problem hiding this comment.
Fixed in 9dfe9b2. The fence parse runs in the repository interpreter (-I, type_comments=True, no feature_version), so the repository's grammar decides, not the host's. Test test_a_fence_newer_than_the_repository_grammar_is_a_syntax_error (runs on interpreters older than 3.12).
| if done.timed_out or done.returncode not in (0, 1): # 1: the tool found errors; anything else is its failure | ||
| raise TypecheckError(f"{check.tool} exited {done.returncode}: {done.stderr[-300:]}") |
There was a problem hiding this comment.
Must-fix: mypy also exits 2 for blocking source diagnostics, which the initial ast.parse does not necessarily catch. For example, x = 1 # type: ??? passes this AST pre-check (type comments are disabled), but mypy rejects the type comment and exits 2. This branch then aborts the entire scan with a usage error instead of reporting a fence result. Distinguish exit 2 with source diagnostics from a checker failure, as the new grader already does, and avoid reporting other fences as ok if the blocking error prevented their checking. Add an L0 regression test for this case.
There was a problem hiding this comment.
Not reproduced with mypy 2.4.0: x = 1 # type: ??? exits 1 with code syntax, so the fence is a fail. Exit 2 (blocking errors that name a fence, e.g. a duplicate module) is handled in 9dfe9b2: fences mypy did not reach are unresolved, not ok. Pinned by test_a_blocking_type_comment_is_a_failing_fence_not_a_tool_failure (6a9c800).
|
|
||
| def _run_tool(argv: list[str], cwd: str): | ||
| try: | ||
| return process.run(argv, timeout=TIMEOUT, env=process.Env(allow=None), cwd=cwd) |
There was a problem hiding this comment.
Must-fix: the inherited environment still lets host MYPYPATH override the repository's installed dependencies. -I only ignores Python's own environment variables, and an empty mypy config does not disable MYPYPATH. For example, with MYPYPATH=/tmp/stubs and /tmp/stubs/unavailable.pyi defining def foo() -> int: ..., a fence containing import unavailable; unavailable.foo() can report ok even when the repository interpreter has no such package. Strip MYPYPATH from the tool environment (or explicitly clear it) and add a regression test so verdicts depend on the selected repository environment.
There was a problem hiding this comment.
Already fixed earlier: the tool environment is an allow-list (_TOOL_ENV in repos/l0/typecheck.py) that does not pass MYPYPATH. Test test_a_host_mypypath_cannot_supply_a_module_the_pinned_environment_lacks (tests/test_repo_l0_typecheck.py).
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
requirements-test.txt is an input of requirements.lock.txt, and the pytest
job installs with --require-hashes from that lock only. The mypy pin added
in the previous commit did not reach it, so the typecheck tests failed to
collect ("No module named 'mypy'") on the py3.11 and py3.14 Tests jobs.
The five blocks (mypy, mypy-extensions, pathspec, librt, ast-serialize) are
copied from requirements-ci.lock.txt, which pins the same versions and
hashes. The lock was merged, not recompiled with uv, so no other pin moves.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…hon-env is a usage error A tool run inherited the whole host environment, so a host MYPYPATH added modules to mypy's search path and a fence could pass on a module the pinned environment lacks. The run now gets PATH, locale and temp variables only. A --python-env that prints no version (e.g. /bin/true) raised an uncaught ValueError, and repo scan printed a traceback. It is now the usage error the other --python-env failures already are. Both are covered by a red-then-green test in test_repo_l0_typecheck.py. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| "--follow-imports=silent", "--no-incremental", "--cache-dir", os.path.join(tmp, "cache"), | ||
| "--output", "json", *names] | ||
| else: | ||
| argv = [check.tool, "--pythonpath", str(check.python), "--outputjson", *names] |
There was a problem hiding this comment.
Must-fix: basedpyright can discover configuration in ancestors of this temporary working directory. For example, start a fresh scan with TMPDIR pointing to a checkout containing pyrightconfig.json with {"reportAttributeAccessIssue":"none"}: the json.dumps(1).nosuch() fence can be reported as ok instead of fail. Ancestor extraPaths settings can likewise supply modules absent from the repository environment. Unlike the mypy branch, this invocation does not isolate configuration. Write a neutral basedpyright config inside tmp, pass it explicitly with --project, and add a regression test with a suppressing ancestor config.
There was a problem hiding this comment.
Fixed in 9dfe9b2. basedpyright runs with --project pointing at an empty pyrightconfig.json in the temp dir, so no config above the run applies. Test test_basedpyright_is_given_an_empty_project_so_no_config_above_the_run_applies captures argv. basedpyright is not installed here, so the real-tool path is not exercised.
… unresolved - grader: --explicit-package-bases, with the source root of each regular package on mypy_path, so a listed namespace module and a src/ layout both resolve as they are imported (pkg.other, pkg.x) - L0: fence syntax is parsed by the repository interpreter, with type_comments and no feature_version approximation - L0: mypy exit 2 is a result only when it names a fence; fences the run did not reach are unresolved, not ok - basedpyright runs with an empty --project, so no config above the run applies Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
mypy 2.4.0 reports the # type: ??? comment as a syntax error (exit 1), so the fence is a fail; the test pins that the parse passes it through to mypy. Not a regression test: it passes before and after the grader and L0 changes. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The comment block was four lines; check_comment_blocks fails a block of three or more lines that the PR touches. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| has its files copied over the workspace first, so the reference repair is what gets graded.""" | ||
| reference = task_dir / REFERENCE_DIR | ||
| if reference.is_dir() and run_dir is not None: | ||
| shutil.copytree(reference, Path(run_dir) / "workspace", dirs_exist_ok=True) |
There was a problem hiding this comment.
Must-fix: copytree(..., dirs_exist_ok=True) follows existing destination symlinks. For example, if the base checkout contains workspace/pkg/api.py -> ../../task.json and the task supplies reference/pkg/api.py, running the oracle overwrites run_dir/task.json with Python source; the subsequent grade then cannot load its task. A symlinked destination directory has the same escape behavior. The oracle self-check uses a fresh checkout, but repository symlinks are preserved, and the entire run directory is mounted writable. Reject symlinked destination components (including the workspace root), or safely replace them without following them, before copying. Add tests for both file and directory links to an outside sentinel.
There was a problem hiding this comment.
Fixed in 9a253f0 (symlink check before any copy) and 4a24291 (empty directories too).
The oracle refuses a destination reached through a symlink: the workspace root, a directory on the way, or the file itself.
It raises ValueError and copies nothing. Tests cover a file link, a directory link and a root link; each leaves the outside sentinel as it was (red on the old oracle).
| for i, code in enumerate(codes): | ||
| Path(tmp, f"fence_{i}.py").write_text(code + "\n", encoding="utf-8") | ||
| names.append(f"fence_{i}.py") |
There was a problem hiding this comment.
Must-fix: all generated snippets share an import directory, so they can supply modules missing from the repository environment. For example, with a first fence value: int = 1 and a second fence import fence_0\nprint(fence_0.value), mypy resolves the import to the generated first fence and reports no error even when the environment has no fence_0 module. This violates §8.2's missing-module failure contract and makes verdicts depend on other guidance snippets. Isolate generated fences from each other's import resolution for both tool paths, and add a regression asserting that this import fails.
There was a problem hiding this comment.
Fixed in 2745c47. Fences are now written as fence-.py: a hyphen cannot be imported, so import fence_0 in a later fence no longer resolves to another fence and is a fail.
Both mypy and basedpyright get the new names. Regression: test_a_fence_cannot_import_another_fence_as_a_module (red before).
mypy's main raises sys.setrecursionlimit for the whole process, so a later RecursionError-based check (journeys' too-deep registry) passed on py3.11 CI. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| stdout, stderr, status = api.run(["--no-incremental", f"--cache-dir={cache}", f"--config-file={config}", | ||
| "--explicit-package-bases", *[str(workspace / p) for p in present]]) |
There was a problem hiding this comment.
Must-fix: api.run() still reads MYPYPATH, which takes precedence over the configured mypy_path; Python's -I does not suppress it. This variable can reach the grader through the image's environment or image.env (EvalContext.grade_env preserves those settings). For example, with MYPYPATH=/eval/run/workspace/stubs, a stubs/unavailable.pyi containing def value() -> int: ... makes a listed file importing unavailable.value pass even though the runtime dependency is absent. The neutral config therefore does not fully control import resolution, and agent-written stubs can affect the verdict. Remove MYPYPATH around the mypy invocation, restoring it afterward for in-process callers, and add a regression test for an externally supplied stub directory.
There was a problem hiding this comment.
Fixed in 9a253f0. MYPYPATH is popped around api.run and restored in the finally, so an external stub directory cannot satisfy a missing import.
test_an_external_mypypath_cannot_supply_a_module_the_image_lacks is red on the old module and checks the variable is restored.
antonwolfy
left a comment
There was a problem hiding this comment.
Three bugs, three risks and two nits. The grader design is sound, and the earlier review rounds fixed most of the isolation problems. Three isolation gaps remain, and each changes a verdict or writes outside the workspace.
What I ran, on a Linux review host at 5d8b05d:
make ci-local: 28 passed, 2 failed, 7 skipped. The full suite is 12035 passed, 61 skipped; changed-line coverage is 95%. The two failures are the gitleaks install and the commit scan, whose download got a 504 from the proxy; no step that reads the PR's files failed. Thechangelogstep passes.- Reproductions for the three bugs, with the type-checker run for real and no model or network involved.
- A mutation run over the two new test files (partial, about 40 mutants): the survivors that matter are inline.
- All 14 commits carry a
Signed-off-by.
Other findings:
- The contract still tells grader authors the standard library and Python 3.8 are enough.
docs/repo-evaluation-contract.md:586(outside this diff) says built-in graders use "the standard library only and the image'spython3(3.8 or newer)", butdokimi:typecheckimports mypy, and the pinned mypy 2.4.0 declaresRequires-Python: >=3.10. An author on apython:3.9image who follows that sentence gets a grader failure at grade time. The new paragraph at lines 636-641 mentions the mypy requirement, but the sentence above it stays wrong. Suggestion: say "3.8 or newer, and 3.10 or newer plus the pinned mypy fordokimi:typecheck". The same section's "data only" (line 575) and "leaves the workspace alone" (the oracle description) also no longer hold for a task with areference/directory.
Bot points I checked that no longer apply at this head (19)
- exai-bot's point on
--typecheckswallowing the source does not apply:--typecheckis a plain flag at this head and--typecheck-toolpicks the tool. - exai-bot's point on a relative run directory does not apply:
graderesolves the workspace to an absolute path before mypy runs. - exai-bot's point on the namespace-package search path does not apply: the neutral config sets
mypy_pathto the workspace and each package root. - exai-bot's point on exit 2 for a syntax error does not apply:
gradetreats a 2 with an: error:line as a diagnostic and fails the grade. - exai-bot's point on the no-op oracle does not apply at this head:
oraclecopies the task'sreference/directory over the workspace (its destination handling is a separate problem, inline). - exai-bot's point on the target Python version does not apply: mypy gets
--python-versionfrom the repository interpreter and the fences are parsed by that interpreter. - exai-bot's point on user-level mypy config does not apply:
--config-filepoints at an empty file in the temporary directory. - exai-bot's point on unannotated function bodies does not apply:
--check-untyped-defsis passed. - Copilot's point on blocking source diagnostics in the grader does not apply: a 2 that printed an error line is graded as a failing diagnostic.
- Copilot's point on the oracle self-check does not apply at this head: with a
reference/directory the oracle run and the null run grade different workspaces. - Copilot's point on module shadowing in the version probe does not apply: the probe runs
python -Ifrom the system temporary directory. - Copilot's point on host mypy configuration in the fence check does not apply: mypy gets an explicit empty
--config-file. - Copilot's point on a relative run directory does not apply: the workspace path is resolved before mypy gets it.
- exai-bot's point on
--python-env /bin/truedoes not apply: the version probe rejects an output that is not two integers with aTypecheckError, so the command exits 2 without a traceback. - exai-bot's point on listed namespace modules does not apply:
--explicit-package-basesis passed with the workspace inmypy_path. - exai-bot's point on the system Python's grammar parsing a newer fence does not apply: the fences are parsed by the repository interpreter itself.
- exai-bot's point on a blocking exit 2 aborting the scan does not apply:
_runreturns the fence errors withblockedset, and fences mypy did not reach areunresolved. - exai-bot's point on
MYPYPATHin the L0 fence check does not apply: the tool environment is the allow-list_TOOL_ENV, which leavesMYPYPATHout (the grader path is different, see the inline comment). - exai-bot's point on basedpyright configuration does not apply: the run passes
--projectwith an emptypyrightconfig.json.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
…graded is_file() and mypy both follow symbolic links, so a listed file that the agent linked to the hidden reference repair was graded on that repair. A link on the way to a listed file, or a linked workspace root, now makes the path missing before any check or mypy run. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| missing = [p for p in paths | ||
| if _linked_on_the_way(linked_root, linked_root / p) or not (workspace / p).is_file()] |
There was a problem hiding this comment.
Must-fix: the symlink check covers only paths, but mypy also reads imported modules and package initializers. An agent can leave the listed pkg/api.py as a regular file that imports add from pkg.repair, and make the unlisted pkg/repair.py a symlink to /eval/content/tasks/<task-id>/reference/pkg/api.py. That target is unavailable to the agent but mounted during grading; mypy follows it and can produce a passing grade using the hidden repair. Reject workspace symlinks that mypy could follow (including unlisted .py/.pyi modules and __init__ files), or check a sanitized workspace without access to hidden task data. Add a regression with a regular listed file importing an unlisted symlink to an external repair.
There was a problem hiding this comment.
Fixed in c9f0e36: a link whose target lies outside the workspace now refuses the grade as a grader error, listed or not, instead of reading the link as missing. Regression test: test_a_listed_file_importing_an_unlisted_link_to_a_hidden_repair_is_refused.
Refusing rather than marking missing is a design call, open for the maintainer.
| Path(project).write_text("{}\n", encoding="utf-8") | ||
| argv = [check.tool, "--project", project, "--pythonpath", str(check.python), "--outputjson", *names] |
There was a problem hiding this comment.
Must-fix: the basedpyright branch ignores target. --pythonpath selects the interpreter used to discover import paths, but does not set basedpyright's target Python version; with {} it uses its default version. For a Python 3.11 repository, a fence containing from typing import override can therefore pass even though that API is unavailable in its interpreter. Set pythonVersion in the generated project (or pass --pythonversion) from target, and add equivalent target-version coverage for basedpyright.
There was a problem hiding this comment.
Fixed in c9f0e36: basedpyright's explicit project file now sets pythonVersion from the repository interpreter's version, not only the import paths.
Regression test: test_basedpyright_is_told_the_repository_interpreters_version.
…basedpyright's version A link whose target lies outside the workspace makes mypy read content this run cannot see, so the grade is refused as a grader error. basedpyright gets the repository interpreter's version as pythonVersion in its project file, since --pythonpath only sets import paths. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| `dokimi:typecheck` checks the listed files only. Its pass is "no error on a | ||
| listed file" (an error in a module a listed file imports belongs to that module, | ||
| not to the listed file), and a listed file that is missing fails. A symbolic link in the workspace whose target | ||
| lies outside it refuses the whole grade (a grader error, not a pass), listed or not: mypy follows an import through a |
There was a problem hiding this comment.
Must-fix: this promises a grader error for any external symlink, but grade() scans links only when present is nonempty. For example, when the sole listed file is a symlink outside the workspace, grading returns pass: false with n_missing: 1, rather than raising; the existing test_a_listed_path_reached_through_a_link_is_missing_not_graded explicitly requires that behavior. Qualify the contract to say external links refuse grading when mypy would run, and explain that listed paths reached through links count as missing. Otherwise consumers cannot reliably distinguish a failed task from a grader failure.
There was a problem hiding this comment.
Done in 67bbc3a: the contract now says the refusal applies when mypy runs (some listed file present). A listed path reached through a link is missing, so with every listed file missing the grade fails with n_missing. Code unchanged.
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at c9f0e36. Links to a hidden repair are now closed, both in listed paths and in imported modules. The open problems are in the line comments, one of them blocking.
Agreed with @antonwolfy: repo-evaluation-contract.md lines 604, 615 and 628 are still unchanged.
Agreed with @exai-bot: §4.5 says any link outside the workspace voids the grade, but a listed path through a link is counted as missing (and a test fixes that behaviour).
What I ran, at c9f0e36:
- The PR's tests: 106 passed, 2 skipped. I did not rerun
make ci-local --offlineon this head. - My repro that imports a module through a link to a file outside the workspace now stops with
ValueError: pkg/repair.py is a symbolic link to a file outside the workspace.
Checked, no issues
- Links in imported modules: the grader now refuses a link to a file outside the workspace instead of grading it. It fails rather than counting a pass, which I think is the right choice.
- basedpyright target version: the Python version is now passed to basedpyright as well.
| stdout, stderr, status = api.run(["--no-incremental", "--no-color-output", f"--cache-dir={cache}", | ||
| f"--config-file={config}", "--explicit-package-bases", | ||
| *[str(workspace / p) for p in present]]) |
There was a problem hiding this comment.
Blocking: An agent can pass dokimi:typecheck by silencing mypy in the workspace instead of fixing the code.
A listed a.py with x: int = 'a' grades pass: False. With # type: ignore or # mypy: ignore-errors as its first line, or # type: ignore on the line, it grades pass: True. A workspace json.pyi with def __getattr__(name: str) -> Any: ... hides json.nosuch() the same way, because the workspace is first on mypy_path.
Suggestion: count suppressions in the listed files and new .pyi files on the search path as a metric, and fail on any the null workspace did not have; or document the gap in §4.5.
There was a problem hiding this comment.
Fixed in 12ff812. A # type: ignore or # mypy: comment in a listed file fails the grade (n_suppressions), and so does any .pyi in the workspace. Tests cover the examples from the thread and a stub that replaces json.
There was a problem hiding this comment.
Blocking: 12ff812 catches the comments and the json.pyi stub, but a .py does the same: a workspace json.py with def __getattr__(name: str) -> Any: ... makes json.nosuch() grade pass: True. And refusing every .pyi means a repository that ships its own stubs (ext.pyi for a C extension) can never pass, even with the reference.
Suggestion: refuse a workspace module, .py or .pyi, whose top name is a standard-library module (sys.stdlib_module_names) or one the image's site-packages provides.
| if not errors: | ||
| scan.add(f, fence.line, "python_fence", _label(code), "ok", f"{check.tool}: no error") | ||
| continue | ||
| block["fail"] += 1 | ||
| line, message = errors[0] | ||
| more = f" (+{len(errors) - 1} more)" if len(errors) > 1 else "" | ||
| where = f"line {line}: " if line else "" # 0: the tool gave no line, so the claim is the fence itself | ||
| scan.add(f, fence.line + line, "python_fence", _label(code), "fail", | ||
| f"{check.tool}: {where}{message}{more}"[:MAX_WHY]) |
There was a problem hiding this comment.
A fence that is a fragment of a longer example is reported as a fail stale-API candidate, though no API is stale.
A fence client.decode("{}") after a fence that defines client gives FAIL ... Name "client" is not defined. A fence await asyncio.sleep(1) gives "await" outside function. With --fail-on-fail, ordinary snippets fail the gate.
Suggestion: make a fence whose only errors are missing context (name-defined, top-level-await, return/await outside a function; reportUndefinedVariable for basedpyright) unresolved with that reason.
There was a problem hiding this comment.
Fixed in 12ff812. A fence whose only errors are missing context (an undefined name, top-level await or return) is unresolved with that reason. A fence with a real error still fails.
| # run directory or PYTHON* variables can stand in for mypy or for the standard library. | ||
| config = os.path.join(tmp, "mypy.ini") | ||
| Path(config).write_text("[mypy]\n", encoding="utf-8") | ||
| version = [] if target is None else ["--python-version", f"{target[0]}.{target[1]}"] |
There was a problem hiding this comment.
repo scan --typecheck exits 2 for a repository on Python 3.9 or older, because the pinned mypy refuses that --python-version.
mypy 2.4.0 prints "Python 3.9 is not supported (must be 3.10 or higher)" and exits 2. The scan then stops with mypy exited 2 with no fence report. §8.2 does not mention the limit.
Suggestion: when the target is older than the tool supports, mark the fences unresolved with that reason, as for a missing --python-env.
There was a problem hiding this comment.
Fixed in 12ff812. For a repository older than mypy's floor (3.10) the fences are unresolved with that reason and mypy is not run, so there is no exit 2.
| def _oracle_applies(task: Task) -> bool: | ||
| """False for a built-in ``dokimi:typecheck`` task with no ``reference/``: its oracle has no repair to apply | ||
| (the grade reads the workspace as it is), so the self-check runs the null step only, and the untouched | ||
| listed files must fail the gate.""" | ||
| if builtin_name(task.raw.get("grader")) != "typecheck": | ||
| return True | ||
| return (task.dir / REFERENCE_DIR).is_dir() | ||
|
|
There was a problem hiding this comment.
A typecheck task without reference/ passes tasks check when a listed path does not exist, so no run can ever pass it.
Only the null step runs, and any failing grade satisfies it. With paths: ["src/pkg/modle.py"] the null grade fails with n_missing: 1, and the task is reported ok.
Suggestion: for a typecheck task with no oracle step, report a problem when the null grade has missing files.
There was a problem hiding this comment.
Fixed in 12ff812. A typecheck task with no reference now gets a problem when its null grade reports a listed path missing.
| stdout, stderr, status = api.run(["--no-incremental", "--no-color-output", f"--cache-dir={cache}", | ||
| f"--config-file={config}", "--explicit-package-bases", | ||
| *[str(workspace / p) for p in present]]) |
There was a problem hiding this comment.
mypy prints names relative to the current directory, so with another cwd a type error grades as a pass.
Listed a.py with x: int = 'a': cwd = the workspace gives pass: False; cwd = the run directory or / gives pass: True (mypy prints workspace/a.py). It works now only because the container's workdir is the workspace. On Windows, 6 of the new grader tests fail the same way.
Suggestion: pass --show-absolute-path.
There was a problem hiding this comment.
Fixed in 12ff812. mypy runs with --show-absolute-path, so the grade is the same from any working directory (test runs from the run directory).
| if not isinstance(raw.get("version"), str) or not raw.get("version"): | ||
| problems.append('version must be the pinned mypy version, e.g. "2.4.0"') | ||
| paths = raw.get("paths") | ||
| if not isinstance(paths, list) or not paths or not all(_safe_path(p) for p in paths): |
There was a problem hiding this comment.
Two listed names for the same file make mypy stop with "Duplicate module", so the task always fails.
paths: ["a.py", "./a.py"] with a clean a.py grades pass: False, a.py:0: Duplicate module named "a".
Suggestion: in task_problems, reject paths that are equal after posixpath.normpath.
There was a problem hiding this comment.
Fixed in 12ff812. task_problems rejects listed paths that are equal after normpath.
| "reported and makes the command exit 1") | ||
| scan_p.add_argument("--typecheck", action="store_true", default=False, | ||
| help="Type-check each Python fence of the guidance (§8.2): one claim per fence, fail when the " | ||
| "tool reports an error. The tool is mypy (the host's pinned copy) unless --typecheck-tool names " |
There was a problem hiding this comment.
The help and §8.2 say the fence check uses "the host's pinned copy" of mypy, but mypy is pinned only in requirements-test.txt and is mypy>=1.8 in the dev extra.
After a plain pip install dokimi the scan exits 2 with "No module named mypy". With the dev extra, two hosts can run different mypy versions on the same fences.
Suggestion: add an extra such as typecheck = ["mypy==2.4.0"] and name it in the help and §8.2.
There was a problem hiding this comment.
Not changed in 12ff812, needs your call: add a typecheck extra pinning mypy==2.4.0 (pyproject plus the uv.lock change it needs), or keep requirements-test.txt as the pin and reword the help and section 8.2? Which do you want?
There was a problem hiding this comment.
Maintainer decision: keep requirements-test.txt as the pin; help and §8.2 reworded in ac47ae9.
| if args.python_env is not None and not os.access(args.python_env, os.X_OK): | ||
| print(f"✗ --python-env {args.python_env}: not an executable interpreter", file=sys.stderr) | ||
| sys.exit(EXIT_USAGE) | ||
| if tool != "mypy" and shutil.which(tool) is None: | ||
| print(f"✗ --typecheck {tool}: not on PATH", file=sys.stderr) |
There was a problem hiding this comment.
Nit: The errors still name the old valued flag, and --python-env accepts a directory.
✗ --typecheck basedpyright: not on PATH (line 144) and ✗ --typecheck mypy: ... (line 163) read like the removed --typecheck <tool>. os.access(path, X_OK) is true for a directory, so --python-env .venv passes and fails later.
Suggestion: ✗ --typecheck-tool {tool}: ..., and os.path.isfile(...) and os.access(...).
There was a problem hiding this comment.
Fixed in 12ff812. The errors name --typecheck-tool, and --python-env must be a regular file.
The contract said a link out of the workspace always refuses the grade. The grader refuses only when mypy runs, that is when a listed file is present; a listed path reached through a link is missing, so with every listed file missing the grade fails with n_missing and no grader error. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…0 targets - A `# type: ignore` or `# mypy:` comment in a listed file, or a .pyi in the workspace, fails the grade (n_suppressions); the grader cannot pass by silencing mypy. - mypy runs with --show-absolute-path, so a type error is graded from any working directory; paths equal after normpath are a task problem. - repo scan: a fence that is only missing context is unresolved; a repository older than mypy's floor (3.10) is unresolved, mypy not run. - --python-env must be a file; the CLI names --typecheck-tool in errors. - tasks check: a typecheck task with no reference and a missing listed path is a problem (its null grade is the only step). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| found = [] | ||
| for p in listed: | ||
| text = (workspace / p).read_text(encoding="utf-8", errors="replace") | ||
| found += [f"{p}:{n}" for n, line in enumerate(text.splitlines(), 1) if _SILENCE.search(line)] |
There was a problem hiding this comment.
Must-fix: this scans raw source text, so a clean listed file containing message = "# type: ignore" is counted as suppressed and grades pass: False, even though mypy sees no suppression comment. Docstrings showing # mypy: examples fail for the same reason. The contract restricts actual comments that silence mypy, not marker text inside strings. Inspect Python COMMENT tokens (preserving their line numbers) instead, and add regressions for string literals and multiline docstrings alongside genuine suppression comments.
There was a problem hiding this comment.
Fixed in 47a95a6. The suppression check reads comment tokens, so a marker in a string or docstring no longer fails a clean file; a genuine comment still does, on its line.
| _FRAGMENT = re.compile(r'^(?:Name "[^"]+"|"[^"]+") is not defined(?: \[reportUndefinedVariable\])?$' | ||
| r'|^"(?:await|return|yield)" outside function$') |
There was a problem hiding this comment.
Must-fix: the outside-function alternative only matches mypy's wording. For example, a fence containing import asyncio\nawait asyncio.sleep(1) produces basedpyright's "await" allowed only within async function, which does not match this regex (and diagnostic rules, when present, are appended by _basedpyright_errors). _claims consequently marks this missing-context fragment as fail rather than unresolved, contrary to §8.2, and --fail-on-fail can exit 1 solely because basedpyright was selected. Recognize basedpyright's corresponding diagnostics, including their optional rule suffixes, and add basedpyright coverage for top-level await/return/yield fragments.
There was a problem hiding this comment.
Fixed in 47a95a6. basedpyright's top-level await/return/yield messages are fragments now (its own wording, with an optional rule suffix); covered by a basedpyright test.
| out = [] | ||
| for code in json.load(sys.stdin): | ||
| try: | ||
| ast.parse(code, type_comments=True) |
There was a problem hiding this comment.
Must-fix: the older-interpreter fallback still fails for Python 3.7. With --python-env /path/to/python3.7 and any Python fence, this raises TypeError because ast.parse gained the type_comments parameter in Python 3.8. _syntax_errors runs before _older_than_the_tool, so the scan exits 2 instead of reporting the fences as unresolved for an unsupported mypy target. Omit this keyword on older interpreters (or handle the unsupported target before parsing), and add a regression covering this parser API boundary.
There was a problem hiding this comment.
Fixed in 47a95a6. The fence parser omits type_comments where ast.parse does not take it (Python 3.7), so the version check runs instead of a crash; covered by a regression test.
The help and contract §8.2 called mypy "the host's pinned copy", but pip install dokimi does not install it: the pin is requirements-test.txt (mypy==2.4.0), and a missing mypy exits 2. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…7 parse - A listed file is suppressed by a `# type: ignore` or `# mypy:` comment, read with tokenize: a marker in a string or docstring no longer fails a clean file. A file the tokenizer cannot read is still scanned line by line. - basedpyright words top-level await/return/yield its own way (with an optional rule suffix): those fences are unresolved, as mypy's are. - The fence parser omits type_comments on Python 3.7 (the keyword is 3.8+). - Contract 4.5: dokimi:typecheck imports mypy (3.10+) and oracle copies reference/. - The Check.tool comment names mypy's real pin (requirements-test.txt). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if ch != ":": | ||
| continue | ||
| tail = _TAIL.match(text[k:]) | ||
| name = posixpath.normpath(text[:k]) |
There was a problem hiding this comment.
Must-fix: native Windows separators survive posixpath.normpath, so listed-file errors can be discarded. For example, _errors(r'C:\run\workspace\pkg\api.py:2: error: boom', {'pkg/api.py'}, 'C:\\run\\workspace\\') returns []: after stripping the root, the candidate is pkg\\api.py, not pkg/api.py. With mypy's normal error exit (1), grade() can consequently return pass=True. The same normalization occurs in the listed is None branch. Normalize native diagnostic paths to POSIX separators before matching (without changing literal backslashes in POSIX filenames), and add a Windows-path regression. This also leaves the earlier cwd/Windows diagnostic fix incomplete for direct Windows grading/tests.
There was a problem hiding this comment.
Fixed in a6340ce: the native separator is converted before the listed names are matched, so a Windows diagnostic path is still this file's error. Regression test added.
| """For a typecheck task with no reference the null grade is the only step: a listed path it reports missing | ||
| fails that step, so no run could pass the task.""" | ||
| doc, _ = read_grade_json(rd / "grade.json") | ||
| missing = doc.get("missing") if isinstance(doc, dict) else None |
There was a problem hiding this comment.
Must-fix: the missing-path fix does not hold for actual grader output. graders/main.py puts lists under details, so a task with no reference and paths: ["src/pkg/modle.py"] writes "details": {"missing": ["src/pkg/modle.py"]}. This top-level lookup returns None, leaving the task reported OK, as in the earlier thread. Read details.missing instead, and exercise this check with main.grade() output; the current regression fixture writes a top-level missing that the entry point never emits.
There was a problem hiding this comment.
Fixed in a6340ce: _missing_listed reads details.missing, where the grader writes lists. The fixture has the grader's shape.
…ts own parse script Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| try: | ||
| payload = scan_repo(tree, manifest, static, typecheck=check) | ||
| except TypecheckError as exc: | ||
| print(f"✗ --typecheck-tool {check.tool}: {exc}", file=sys.stderr) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in a6340ce: the error is labelled repo scan --typecheck, and its text names the failing step (tool or interpreter). Test added.
| return None | ||
| name = m.group("file") | ||
| return posixpath.normpath(name[len(root):] if name.startswith(root) else name), m | ||
| text = line[len(root):] if line.startswith(root) else line |
There was a problem hiding this comment.
Still open from the PR thread: on a Windows host mypy prints backslash paths (C:\run\workspace\pkg\api.py). root is built with os.sep, so the stripped name is pkg\api.py, and posixpath.normpath does not convert separators. name in listed then fails for pkg/api.py, the error is dropped, and a file with a type error grades pass: True. Repro: _errors(r'C:\run\workspace\pkg\api.py:2: error: boom', {'pkg/api.py'}, 'C:\\run\\workspace\\') returns []. Normalise the separators (e.g. .replace(os.sep, '/')) on both the mypy name and the root, or compare with pathlib.PurePath equality, and add a test with a backslash line.
There was a problem hiding this comment.
Same fix as the thread above, in a6340ce: _name converts the native separator on the mypy name; a POSIX backslash stays literal (test added).
| scan.add(f, fence.line, "python_fence", _label(code), "unresolved", | ||
| f"not valid Python ({why}): not type-checked") |
There was a problem hiding this comment.
With no --python-env, the fences are parsed by dokimi's own interpreter, which may be older than the repository's grammar (e.g. a 3.12+ type alias on a 3.11 host). Such a valid fence is labelled "not valid Python", which is inaccurate. Use a wording that says it did not parse under the parsing interpreter, or name that interpreter in the reason.
Suggestion:
| scan.add(f, fence.line, "python_fence", _label(code), "unresolved", | |
| f"not valid Python ({why}): not type-checked") | |
| scan.add(f, fence.line, "python_fence", _label(code), "unresolved", | |
| f"not parsed by {check.python or 'dokimi'} ({why}): not type-checked") |
There was a problem hiding this comment.
Fixed in a6340ce: the reason names the interpreter that parsed the fence (the repository's, or dokimi's without --python-env).
| fails += 1 | ||
| line, message = errors[0] |
There was a problem hiding this comment.
A fence whose import cannot resolve in the pinned environment (an optional extra, a dev-only dependency, or the import fence_0 case) is reported as fail, labelled a stale-API candidate. The failure is in the environment, not in the guidance, so it inflates the fail count the since-cutoff split relies on. The test test_a_fence_cannot_import_another_fence_as_a_module pins this as fail. Suggest treating mypy import-not-found (and the basedpyright equivalent) as unresolved with a 'module not in the repository environment' reason, and not counting it as fail.
There was a problem hiding this comment.
Should a module the pinned environment lacks stay fail (contract §8.2 says so, and a test pins it) or become unresolved? Not changed in a6340ce; your call.
| f"{tool}: {errors[0][1]} (a fragment of a longer example: not type-checked)"[:MAX_WHY]) | ||
| continue | ||
| fails += 1 | ||
| line, message = errors[0] |
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in a6340ce: the claim names the first error that is not a fragment; the count still covers all errors. Test added.
| """For a typecheck task with no reference the null grade is the only step: a listed path it reports missing | ||
| fails that step, so no run could pass the task.""" | ||
| doc, _ = read_grade_json(rd / "grade.json") | ||
| missing = doc.get("missing") if isinstance(doc, dict) else None |
There was a problem hiding this comment.
Must-fix: missing is a list, so graders/main.py:split_result moves it under details, not top level. doc.get("missing") returns None for real grader output, _missing_listed returns [], and a no-reference task with a nonexistent listed path is still reported OK. Read doc["details"]["missing"] (with a dict check on details). Add a test that feeds real main.grade output, not a hand-built top-level dict.
Suggestion:
| missing = doc.get("missing") if isinstance(doc, dict) else None | |
| details = doc.get("details") if isinstance(doc, dict) else None | |
| missing = details.get("missing") if isinstance(details, dict) else None |
There was a problem hiding this comment.
Same fix as the thread above, in a6340ce: the lookup reads details.missing.
| # repos/graders/typecheck.py imports mypy inside the grading image, where it is installed | ||
| # for the grader; the wheel does not ship it. The host never imports it (the L0 check runs it as a tool). |
There was a problem hiding this comment.
This new comment block is three # lines, and AGENTS.md says a block of three or more # lines fails scripts/check_comment_blocks.py once the PR adds or edits a line in it. Shorten it to two lines, e.g. # mypy is imported by repos/graders/typecheck.py inside the grading image only; the wheel and the host do not need it.
Suggestion:
| # repos/graders/typecheck.py imports mypy inside the grading image, where it is installed | |
| # for the grader; the wheel does not ship it. The host never imports it (the L0 check runs it as a tool). | |
| # repos/graders/typecheck.py imports mypy only inside the grading image; the host runs it as a tool (L0). |
There was a problem hiding this comment.
No change: the block is two lines at a6340ce (pyproject.toml 145-146), and scripts/check_comment_blocks.py --base 41b3d8df --head HEAD reports no block over two lines.
| raw = {"grader": "dokimi:typecheck", "tool": "mypy", "version": PINNED, "paths": ["src/pkg/modle.py"]} | ||
| rd = tmp_path / "rd" | ||
| rd.mkdir() | ||
| (rd / "grade.json").write_text(json.dumps({"pass": False, "missing": ["src/pkg/modle.py"]}), encoding="utf-8") |
There was a problem hiding this comment.
Must-fix: this fixture does not match what the grader writes. main.split_result moves lists (missing) under details, so real grade.json has details.missing, not a top-level missing. The test feeds the top-level shape, so it passes while selfcheck._verdict_problems misses the real case (the open selfcheck thread). Build the fixture with main.grade(...) output, or with {"pass": False, "metrics": {"n_missing": 1}, "details": {"missing": [...]}}, and add a test that fails on the current lookup.
There was a problem hiding this comment.
Fixed in a6340ce: the fixture is built with main.split_result, so missing sits under details as the grader writes it.
…he real error - graders: a diagnostic with the native separator (Windows) matches its POSIX listed name - selfcheck: a no-reference missing path is read from details (the grader puts lists there) - l0: the claim names the first real error, not a fragment error from the same fence - repo scan --typecheck: an interpreter failure is no longer labelled --typecheck-tool - l0: the unparsed-fence reason names the parsing interpreter Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Re-check at a6340ce. Nearly every point from my last round is fixed: fence fragments and pre-3.10 environments are now unresolved, the grade no longer depends on the working directory, duplicate paths are rejected, and the self-check catches a missing listed file. One point is only partly fixed: a workspace .py can still shadow a module, and refusing every .pyi locks out repositories that ship their own stubs (reply in its thread). The branch now conflicts with main in repo-evaluation-contract.md.
What I ran, at a6340ce:
make ci-local --offline: 18 passed, 1 failed, 13 skipped. The failure isschema_compat:headroom.v1.jsonwas added onmainby #279 after this branch's base, so it is not from this PR.- The real
grade()with mypy 2.4.0: ajson.pyistub and suppression comments now fail the grade; a rootjson.pywith a module__getattr__still passes; a clean file next to its ownext.pyifails. A type error fails from the run directory and from/. repo scan --typecheckon a scratch AGENTS.md: two fragment fences areunresolved, the stale call and the mixed fence fail, and the mixed one names its real error. With an interpreter that reports Python 3.9, all five fences areunresolvedand the scan exits 0.- On Windows (Server 2025, Python 3.13):
pytest tests/test_repo_l0_typecheck.py tests/test_repo_typecheck_grader.py: 124 passed, 2 failed, 4 skipped. The type-error-as-pass failures from my last round are gone; the 2 left are POSIX-only test setups (a:in a directory name, a 0700 mode).
Checked, no issues
- Self-check:
_missing_listedreadsdetails.missing, wheremain.split_resultputs the list, and runs only when there is no oracle step. - Contract: §4.5 now names
reference/, the mypy and Python 3.10 requirement and the oracle's copy; §8.2 says how mypy is installed, as the maintainer decided.
|
|
||
|
|
||
| @pytest.mark.parametrize("sep", [",", ":"]) | ||
| def test_a_source_root_with_a_mypy_path_delimiter_is_a_task_error(tmp_path, sep): |
There was a problem hiding this comment.
Nit: Two grader tests fail on Windows for reasons the grader does not have there.
This test cannot create a src:lib directory (WinError 123), and test_the_oracle_refuses_a_linked_workspace_even_with_an_empty_reference (line 396) checks a 0700 mode that Windows does not keep. The other 124 tests of both files pass there.
Suggestion: skip these two on win32, as the POSIX-script test already is.
There was a problem hiding this comment.
Done in f7cb3c5: both tests skip on win32 (the src:lib directory and the 0700 mode check), as the POSIX-script test does.
mypy stops on a blocking error (a top-level yield, break outside a loop), so one such fence left every other fence unresolved and --fail-on-fail passed a stale fence. The fences the blocking error did not name are checked in another run. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
A top-level async with or async for lacks the same context as a top-level await, yet both tools' messages for it were read as a stale API, so --fail-on-fail failed correct async snippets. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Section 8.2 said only a name an earlier fence defines is missing context, but every undefined name is, an omitted import included (the PR's own fragment test pins `asyncio` with no import). Say so, and that a removed builtin such as `unicode` is unresolved rather than a fail. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
The grader's mypy run lacked --check-untyped-defs, which the L0 check passes, so an agent could make a failing grade pass by deleting a function's annotations instead of fixing its body. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
@no_type_check (under any alias) makes mypy skip the function, so a listed file with a type error graded pass with no suppression counted. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
The workspace is first on mypy_path, so a json.py replaces json exactly as the refused json.pyi does: a __getattr__ -> Any there hid the listed file's error and the grade passed. It now counts as a suppression. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
With no reference/ the null grade is the only step. A suppression the workspace already has (a repository .pyi) fails every grade, so even a run that repairs the code cannot pass, yet tasks check reported ok. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
repo-evaluation-contract.md: intel#349's structure paragraph closes 8.1, then this branch's 8.2 on Python fences. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
…_scan.py With main merged in, commands/repo.py reached 823 lines, over the growth ratchet's 800 cap. The two helpers this branch adds move out unchanged; repo.py imports them back. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
Ran: full suite, lint, types and security on the head plus the commits below, and on the same merged with main. Also a CLI and grader repro for each point.
Pushed:
|
antonwolfy
left a comment
There was a problem hiding this comment.
Review at bc9bc0e. Not ready to merge. The delta since my last review is sound on the points it set out to fix, but the fragment rule still sends missing-context fences to fail, two new guards that decide a verdict have no test, and points raised in comments are still open. Five points of mine: four inline (three risks, one nit) and one nit below.
What I ran, on a Linux review host at bc9bc0e:
make ci-local: 29 passed, 1 failed, 7 skipped; 13017 tests passed, 1 failed. The failure istests/test_reference_checks.py::test_the_regex_limit_is_shared_by_every_pattern_of_one_check, a timing assertion (3163.69 - 3161.84 = 1.86 s against a 1.5 s limit) in a file this PR does not touch. The coverage-of-changed-lines step passes.- Reproductions with the real mypy 2.4.0 for the code findings, no model or network involved.
- A mutation run over the L0 check and its CLI wiring (the survivors that decide a verdict or exit code are inline) and over the grader (6 mutants, all killed).
- All 41 non-merge commits carry a
Signed-off-by.
Other findings:
- Nit: the PR description is stale against the code. It shows
repo scan --typecheck [mypy|basedpyright]and a known limitation aboutnargs="?"swallowing the source withinvalid choice. At this head--typecheckis a plain flag and--typecheck-tool {mypy,basedpyright}picks the tool (cli-interface.json, the changelog). Themake ci-localand self-review markers in the description named96d7db8and057ad88a, not the head. Suggestion: update the usage block, drop the limitation, and cite the head.
Still open from comments on this PR:
- I agree with @xaleryb on the installed package without
py.typed. New evidence: mypy 2.4.0 reportsimport-untypedfor it, so the fence isfailand--fail-on-failexits 1, while §8.2 fails only a module the environment lacks. - I agree with @xaleryb on the workspace
.venv. New evidence: a.venv/bin/pythonlink to/usr/bin/python3gradespass: null, and a.pyiunder.venv/.../site-packagesor.git/fails a clean file, since the.pyiwalk atgraders/typecheck.py:157-158covers the whole workspace. - @exai-bot's question on whether a module the pinned environment lacks stays
failor becomesunresolvedwaits for the author: the reply handed it to the maintainer, and it is unanswered.
Checked, no issues
- Symlinks: a listed file, a linked parent directory and a linked workspace root are each read as missing before
is_file()and before mypy runs (12 link tests pass). A link to a file outside the workspace refuses the grade. - Suppressions:
# type: ignore,# mypy:andno_type_checkare read from comment tokens, so a marker inside a string is not counted;# pyright: ignoreis not a mypy silence and the error is still reported. - Text report move:
repo_scan.pykeeps thewroteline, warnings, incomplete reasons and FAIL lines of the basecmd_repo_scan, and the JSON report carries thetypecheckblock. - Repo rules:
mypy==2.4.0matches in the test requirements and both lock files with hashes,schema_compatpasses, the schema change is an optional property, and no generated file is committed.
Bot points I checked that no longer apply at this head (44)
- Copilot's and exai-bot's points on
--typecheckswallowing the source, a relative run directory, the namespace-package search path, exit 2 for a syntax error, the no-op oracle, the target Python version, host mypy config, unannotated bodies, shadowing in the version probe,/bin/true, listed namespace modules, newer-grammar fences, blocking exit 2,MYPYPATH(fence check and grader), basedpyright config,copytreethrough links, fences importing each other, empty-reference linked root, report validation, colour output, line separators,: error:in names,,/:in roots and the win32 skip do not apply: each is fixed at this head and was checked in my earlier reviews. - exai-bot's points on a listed file linked to the hidden reference, links in unlisted modules, the basedpyright target version and the contract wording on external links do not apply: the link check runs before
is_file(), an outside link refuses the grade, the project file setspythonVersion, and the contract says the refusal applies when mypy runs. - exai-bot's points on marker text inside strings, basedpyright's top-level
await/return/yieldwording, Python 3.7type_comments, Windows separators anddetails.missingdo not apply: comment tokens are read, the basedpyright wording is matched, the keyword is omitted on old interpreters,_nameconverts the native separator, and the self-check readsdetails.missing(the fixture is built withsplit_result).
| # Fragment-only errors (a name an earlier fence defines, await/return/yield or async with/for outside a function), | ||
| # in mypy's or basedpyright's wording; any other error is real. | ||
| _FRAGMENT = re.compile(r'^(?:Name "[^"]+"|"[^"]+") is not defined(?: \[reportUndefinedVariable\])?$' | ||
| r'|^"(?:await|return|yield)" outside function$' |
There was a problem hiding this comment.
Other missing-context fences are still graded fail, a stale-API candidate. _FRAGMENT names await, return, yield, async with/for and undefined names, but not the other statements that only make sense inside a function or loop. Real mypy 2.4.0 through the fence check:
yield from range(3)->fail,"yield from" outside functioncontinue->fail,"continue" outside loop- a fence that ends in
break->fail,"break" outside loop(plusName "y" is not defined) nonlocal x->fail,nonlocal declaration not allowed at module level
All of them parse, so they reach mypy. A loop body or generator body in an AGENTS.md fence inflates the fail count that the since-cutoff split reads, and --fail-on-fail exits 1. Pyright's break wording (can be used only within a loop) is missing too.
Suggestion: add "(?:yield from|continue|break)" outside (?:function|loop) and nonlocal declaration not allowed at module level to _FRAGMENT, with the basedpyright wordings, and a real-mypy test per statement.
There was a problem hiding this comment.
Done in f7cb3c5: continue, break, yield from outside a loop or function, and nonlocal at module level are fragments now.
A real-mypy test per statement; basedpyright's wording is from its messages and was not run here.
| # Fragment-only errors (a name an earlier fence defines, await/return/yield or async with/for outside a function), | ||
| # in mypy's or basedpyright's wording; any other error is real. | ||
| _FRAGMENT = re.compile(r'^(?:Name "[^"]+"|"[^"]+") is not defined(?: \[reportUndefinedVariable\])?$' | ||
| r'|^"(?:await|return|yield)" outside function$' |
There was a problem hiding this comment.
Nothing pins the mypy wording of a top-level return. Mutation check: dropping return from (?:await|return|yield) here passes the full suite. Removing await or yield fails a test, but the mypy fragment tests cover only await, async with and async for; return and yield are covered only through the fake basedpyright test. If mypy changes that message, a fence with a top-level return turns from unresolved into fail and nobody notices.
Suggestion: add a real-mypy test with a fence return 1 that asserts unresolved, next to the await one.
There was a problem hiding this comment.
Done in f7cb3c5: a real-mypy test pins a top-level return fence as unresolved, next to the await one.
| target: tuple[int, int] | None) -> bool: | ||
| """True, with each runnable fence unresolved, when the repository is older than the tool checks (no run).""" | ||
| floor = MIN_PYTHON.get(check.tool, (0, 0)) | ||
| if target is None or target >= floor: |
There was a problem hiding this comment.
The Python 3.10 boundary is untested, and it decides whether a repository is checked at all. Mutation check: target >= floor changed to target > floor passes the full suite (64 tests). With that mutant every repository on exactly 3.10 is reported unresolved for all fences and never checked; the only test uses 3.9.
Suggestion: add a test with an interpreter that reports 3.10 and asserts a fence is checked (ok or fail), next to the 3.9 one.
There was a problem hiding this comment.
Done in f7cb3c5: a test pins Python 3.10 as checked (ok and fail), next to the 3.9 one. A > mutant fails it.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 9fcddfb, which fix the nit from my review:
9fcddfb:dokimi/tests/test_repo_l0_typecheck.py:526: added a test with failing python, bash and json fences, asserting only the python one is counted and fails.
Left as is:
PR description: the stale description lives on GitHub, not in the repo, so there is nothing to commit.
make ci-local at 9fcddfb: 28 passed, 2 failed (host: gitleaks download), 7 skipped.
Not approving yet. Still open: the three risks from my review; @xaleryb on an installed package without py.typed is a mypy fail (import-untyped), contrary to §8.2; same false fail in the grader; @xaleryb on a workspace .venv (link to the interpreter, or .pyi under site-packages) makes the grade pass:null or a false fail; exai-bot on whether a module the pinned environment lacks stays fail or becomes unresolved; the author handed the choice back to the maintainer.
…lled-package shadows refused - L0: a fence whose only errors are a module the repository environment lacks is unresolved, not fail; a real error in the same fence still fails it. The contract §8.2 says so. - L0: break, continue, yield from outside a loop or function, and nonlocal at module level are fragments (mypy's wording, and basedpyright's where known), not stale calls. - grader: a workspace .py or package named as a module the image's site-packages provides is a suppression, as a standard-library name already was (contract §4.5). - tests: a real-mypy top-level return, the Python 3.10 boundary, the two grader tests that cannot run on Windows are skipped there. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>


Summary
Two additions that check guidance against code, both opt-in.
Files:
repos/graders/typecheck.py(new),repos/l0/typecheck.py(new),repos/l0/__init__.py,commands/repo.py,repos/manifest.py,repos/graders/{__init__,main,review}.py,schemas/repo_readiness.v1.json(optionaltypecheckblock),docs/repo-evaluation-contract.md(§4.5, §8.2), changelog fragment, deptry exemption, CLI allowlist and snapshot.Since-cutoff: the contract already splits results by base commit date against the model's cutoff, so a stale-API
failfound here can be checked against that split.The notes from these checks can feed
patch:<file>arms (contract §3), so a guidance fix found by the fence check can be measured as its own arm.Evidence
repo scanhas no Python-fence check; a stale call in an AGENTS.md fence is not reported.After:
repo scan <tree> --typecheck --python-env <venv>reports the fence (testtest_mypy_against_an_interpreter_passes_the_good_fence_and_fails_the_stale_one).make ci-localond96d7db8:0 failed, 30 passed, 7 skipped; full suite12064 passed, 7 skipped; rc 0.cmd_repo_scancomplexity 14 → 7 (text output moved to_print_scan_text); its baseline entry is removed.passes: 2): blocking 0. Marker on057ad88a.Merge Danger
Door: two-way. The grader arm and the CLI flag are new; reverting removes them with no data migration. The schema change is an optional field (
oneOf null/object), so existing reports stay valid.Reviewer points
pyproject.toml:DEP004 = ["mypy"].repos/graders/typecheck.pyimports mypy inside the grading image, where it is installed for the grader; the wheel does not ship it and the host never imports it (the L0 check runs it as a subprocess). The reason comment is in the file; a reviewer may prefer a different mechanism.PATHand is recorded with the version it reports. dokimi does not pin it (contract §8.2).--python-envwithout--typecheckis refused.--typechecktakes an optional value (nargs="?"), so a source placed right after it is read as the value and the command exits 2 withinvalid choice. Put the source first:repo scan . --typecheck.a6a15234had a timing failure,tests/test_repo_environment.py::test_unbalanced_quote_in_hook_completes_quickly(1.33 s against a 1 s limit). It passes alone and was not touched; the next run ond96d7db8was clean (above).d96d7db8,057ad88a) were checked by targeted probes and tests, not by a third pass.Blast Radius: low. Affects only
repo scan --typecheckanddokimi:typecheck; repos that do not use either see no change. A tool-run failure (not a type error) is a usage error (exit 2), not a silent pass.Behaviour change
dokimi:typecheckrefuses silenced mypy: a# type: ignoreor# mypy:in a listed file fails the grade, including one that was already there (no exemption for pre-existing comments)..pyianywhere in the workspace is refused as a suppression, since it would replace the module it names.Both are deliberate (maintainer decision). A repository that relies on either sees a failing grade.
🤖 Generated with Claude Code