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 |
github-actions
Bot
added
the
cli-interface-change
|
🔍 OpenCodeReview found 1 issue(s) in this PR. By severity: 1 medium
Review details
|
| "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.
| 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
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.
|
|
||
| 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.05% 94.05%
========================================
Files 297 299 +2
Lines 38018 38310 +292
Branches 6424 6474 +50
========================================
+ Hits 35756 36033 +277
- Misses 1452 1459 +7
- Partials 810 818 +8
🚀 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
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.
| 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.
oracle writes through symlinks in the workspace, so a checkout can make it overwrite files outside the workspace. shutil.copytree(..., dirs_exist_ok=True) opens existing destination symlinks and writes to their targets.
- Workspace has
pkg/api.pyas a symlink to a file outside it (holdingORIGINAL), the task hasreference/pkg/api.py(holdingREFERENCE). Afteroracle(task, task_dir, run_dir)the outside file holdsREFERENCE, andpkg/api.pyis still a symlink. - Workspace has
pkgas a symlink to an outside directory: the outsideapi.pyis overwritten the same way.
The run directory is mounted writable and repository symlinks are preserved, so task.json next to the workspace is reachable by the same path (../../task.json).
Suggestion: before copying, walk the files of reference/ and refuse (raise ValueError) when any component of the destination path, including the workspace root, is_symlink(). Add one test per case (file link, directory link) asserting that an outside sentinel is unchanged.
There was a problem hiding this comment.
| roots = [*_package_roots(workspace, present), workspace] | ||
| config = Path(neutral, "mypy.ini") | ||
| config.write_text(f"[mypy]\nmypy_path = {':'.join(str(r) for r in roots)}\n", encoding="utf-8") | ||
| stdout, stderr, status = api.run(["--no-incremental", f"--cache-dir={cache}", f"--config-file={config}", |
There was a problem hiding this comment.
MYPYPATH from the grader's environment changes the grade, because api.run reads it. The neutral config sets mypy_path, but mypy puts MYPYPATH entries ahead of it, so an external stub directory can stand in for a missing dependency.
- Workspace:
m.pywithfrom unavailable import valueandstubs/unavailable.pyiwithdef value() -> int: ...; taskpaths: ["m.py"]. MYPYPATHunset:passisFalse,m.py:1: Cannot find implementation or library stub for module named "unavailable" [import-not-found].MYPYPATH=<workspace>/stubs:passisTrue,errorsis[].
image.env reaches the grader, and the agent's workspace can hold the stubs. The L0 fence check already drops MYPYPATH (_TOOL_ENV in repos/l0/typecheck.py), so the two paths disagree.
Suggestion: pop MYPYPATH from os.environ around api.run and restore it in the existing finally; add a test that sets MYPYPATH to a stub directory and expects the import failure.
There was a problem hiding this comment.
Fixed in 9a253f0: MYPYPATH is out of the run during a grade and restored afterwards, as in the other thread.
Regression: test_an_external_mypypath_cannot_supply_a_module_the_image_lacks (red before the fix).
| with tempfile.TemporaryDirectory(prefix="dokimi-typecheck-") as tmp: | ||
| names = [] | ||
| for i, code in enumerate(codes): | ||
| Path(tmp, f"fence_{i}.py").write_text(code + "\n", encoding="utf-8") |
There was a problem hiding this comment.
A fence can import another fence as fence_N, so the verdict depends on the other snippets in the guidance. All fences are written as fence_<i>.py in one directory that mypy searches.
With an AGENTS.md holding three python fences and Check("mypy", sys.executable):
value: int = 1isok.import fence_0plusprint(fence_0.value)isok("mypy: no error"), although nofence_0module exists in the environment.import nosuchmod_xyzisfail("Cannot find implementation or library stub").
That contradicts contract §8.2, where a module the environment lacks is a fail, and --fail-on-fail passes on it.
Suggestion: isolate the fences from each other's imports, for example run each fence from its own temporary directory (or one run per fence), and add a test whose second fence imports fence_0; it must be a fail.
There was a problem hiding this comment.
Fixed in 2745c47, the same change as the shared-directory thread: fences are fence-.py, which no import can name.
test_a_fence_cannot_import_another_fence_as_a_module: the second fence is a fail, as §8.2 requires (red before).
| 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]}"] | ||
| argv = [sys.executable, "-I", "-m", "mypy", "--python-executable", str(check.python), *version, |
There was a problem hiding this comment.
No test pins --python-executable, so the fence verdicts can silently stop depending on --python-env. Mutation check: removing "--python-executable", str(check.python) from this argv passes all of test_repo_l0_typecheck.py (36 passed, 2 skipped).
Without the flag mypy resolves imports against the interpreter dokimi runs under, not the repository's. A fence importing a package only the repository has would fail, and one using a package only dokimi's environment has would pass. Every real-mypy test passes sys.executable as the repository interpreter, which is also the interpreter the tool runs under, so the two cannot be told apart.
Suggestion: add a test that uses a second interpreter (a venv with a package dokimi's own environment lacks) and expects ok for a fence importing it, or at least assert that the argv of the tool run carries --python-executable <check.python>.
There was a problem hiding this comment.
Pinned in 2745c47: test_mypy_is_given_the_repository_interpreter asserts the tool argv carries --python-executable with the repository's interpreter.
Removing that flag fails the test.
| done = _run_tool(argv, tmp) | ||
| # 1: the tool found errors. mypy's 2: it stopped on a blocking one ("errors prevented further checking"), | ||
| # which is a result only when it names a fence. Any other exit is the tool's own failure (pyright's 2 is fatal). | ||
| blocked = check.tool == "mypy" and done.returncode == 2 |
There was a problem hiding this comment.
**Neither the exit-code handling nor the severity filters of the tool output are pinned, so a fence verdict can flip without a failing test.**The comment above says pyright's 2 is fatal, and only mypy's 2 means "stopped on a blocking error".
Mutation check, each passes all of test_repo_l0_typecheck.py (36 passed, 2 skipped):
blocked = done.returncode == 2(dropping thecheck.tool == "mypy"term).done.returncode not in (0, 1, 2)on the next line.- the mypy severity filter in
_errors(line 229) replaced withTrue, or widened to("error", "note"): a note on a fence file would turn anokfence intofail. - the basedpyright severity filter in
_errors(line 236) replaced withTrue: warnings and information diagnostics would count as errors and flip a fence fromoktofail.
The exit-2 tests use mypy, and the basedpyright test is skipped when the tool is not on PATH, so nothing runs this branch for basedpyright. With either mutant a fatal basedpyright run no longer raises TypecheckError, and the scan carries on with whatever it printed.
Suggestion: add a test with _run_tool faked to return exit 2 for Check("basedpyright", ...), and expect TypecheckError. It pins the first two mutants and needs no installed tool. For the severity filters, call _errors directly: for mypy with a canned note object and an error object, for basedpyright with a canned document holding one warning and one error, and expect only the errors.
There was a problem hiding this comment.
Pinned in 2745c47. basedpyright exit 2 and any exit outside 0-2 are the tool's own failure (TypecheckError), and mypy's 2 stays a blocking-error result.
test_only_error_diagnostics_count_as_fence_errors covers mypy note/warning and basedpyright warning/information, which must not count.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| "unknown: no repository environment (--python-env), not type-checked") | ||
| block["unknown"] = len(runnable) | ||
| return block | ||
| block["tool_version"] = tool_version(check) |
There was a problem hiding this comment.
Must-fix: this version probe runs even when runnable is empty. For example, in a normal installation without the dev-only mypy dependency, scanning guidance containing no Python fences with --typecheck --python-env <valid-python> exits 2 because mypy --version fails, instead of returning a zero-checked report. The same happens when every Python fence is unparsable. Return the block before probing the checker when there are no runnable fences, and strengthen test_an_interpreter_with_nothing_to_check_runs_no_tool to reject a version/tool invocation; its counter assertions currently miss this behavior.
There was a problem hiding this comment.
Fixed in 2745c47. With no runnable fence the --version probe is not run, and the report's tool_version is null.
test_nothing_to_check_runs_no_tool_not_even_a_version_probe and test_fences_that_do_not_parse_run_no_tool_either assert no tool run (red before).
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to b6a8dac, which fix the two nits from my review:
8122aa2:dokimi/docs/repo-evaluation-contract.md:642: the L0 check now "runs mypy or basedpyright on the host, as--typecheck-toolselects"; the grader docstring (typecheck.py:12) says the same.b6a8dac:dokimi/tests/test_repo_l0_typecheck.py:459: new test withCLAUDE.mda symlink toAGENTS.mdholding one failing fence asserts onepython_fenceclaim andfences == 1.
Left as is:
docs/repo-evaluation-contract.md:586: the point is not labelled Nit or Non-blocking, so it is the author's.
make ci-local at b6a8dac: 28 passed, 2 failed (host), 7 skipped.
Not approving yet. Still open: the three bugs and the three risks from my review.
… MYPYPATH The oracle copied the reference over the workspace with copytree(dirs_exist_ok=True), which writes through an existing destination symlink, so a checkout could make it overwrite a file outside the workspace. It now refuses a destination reached through a link (the workspace root, a directory on the way, or the file) before copying anything. mypy reads MYPYPATH ahead of the neutral mypy_path, so an externally supplied stub directory could satisfy an import the image lacks. MYPYPATH is out of the run during a grade and restored afterwards. Thread: PRRT_kwDOVBg7g86yYW3V Thread: PRRT_kwDOVBg7g86yaepL Thread: PRRT_kwDOVBg7g86yYfCX Thread: PRRT_kwDOVBg7g86yaepR Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…out a fence The fences were written as fence_<i>.py in one directory that mypy searches, so 'import fence_0' resolved to another fence and the verdict depended on the other snippets. The files are now fence-<i>.py: a hyphen cannot be imported, so every fence is checked against the repository's environment alone. A repository with no runnable fence (none, or all unparsable) no longer runs the tool's --version probe; the report's tool_version is then null. Pinned, with tests: the --python-executable argv, the exit codes that are the tool's own failure (basedpyright 2, any other), and that only error diagnostics count as fence errors. Thread: PRRT_kwDOVBg7g86yaepV Thread: PRRT_kwDOVBg7g86yaepX Thread: PRRT_kwDOVBg7g86yaepc Thread: PRRT_kwDOVBg7g86yal_R Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…under a link copytree creates the reference's directories as well as its files, so an empty directory whose parent is a workspace symlink was created at the link's target. The check now covers every entry of the reference, not only its files. Thread: PRRT_kwDOVBg7g86yaepL Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| reference = task_dir / REFERENCE_DIR | ||
| if reference.is_dir() and run_dir is not None: | ||
| workspace = Path(run_dir) / "workspace" | ||
| for source in reference.rglob("*"): # directories too: copytree creates an empty one through a link |
There was a problem hiding this comment.
Must-fix: reference.rglob("*") does not descend into symlinked source directories, but copytree() follows them by default. For example, let reference/pkg link to ../repairs, containing api.py, and let workspace/pkg/api.py link to a writable sentinel outside the workspace. The preflight checks only workspace/pkg, then the copy follows the nested destination link and overwrites the sentinel. Task export preserves source symlinks, so this can reach the oracle. Reject source directory symlinks or traverse exactly the source entries that copytree will copy before checking every destination; add a regression combining a symlinked reference directory with a nested destination symlink.
There was a problem hiding this comment.
Fixed in dc5732f. The oracle now refuses any symbolic link inside reference/ before it copies: copytree follows a linked directory, and the walk did not see the entries behind it.
Test: test_the_oracle_refuses_a_linked_reference_directory_with_a_linked_destination. Red on 4a24291, where the sentinel was overwritten.
| except ValueError: | ||
| continue |
There was a problem hiding this comment.
Must-fix: reject malformed diagnostic reports rather than treating them as no errors. For example, _run_tool returning exit 1 with stdout not JSON passes the nonempty-report guard, but this branch discards the report, so _run returns no errors and every fence becomes ok. The basedpyright branch has the complementary problem: the same output raises JSONDecodeError, which cmd_repo_scan does not catch (it handles only TypecheckError). Validate the expected report format and wrap parsing/shape failures in TypecheckError; add regressions for nonempty malformed output from both tools.
There was a problem hiding this comment.
Fixed in dc5732f. A nonempty report that is not the tool's JSON now raises TypecheckError instead of passing every fence: mypy for a non-JSON or non-object line, basedpyright for non-JSON or a wrong shape. The basedpyright JSONDecodeError is now a TypecheckError, which repo scan reports.
Tests: test_a_nonempty_report_that_is_not_the_tools_json_is_a_tool_failure, four cases, all red before.
… the reference - A nonempty mypy or basedpyright report that is not the tool's JSON now raises TypecheckError, not zero errors. - The oracle refuses a symbolic link inside reference/: copytree follows it and the walk did not see the entries behind it. - The B108 finding (a literal /tmp path in a test) uses tmp_path. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| raise TypecheckError(f"{check.tool} output was truncated; not every error is known") | ||
| if done.returncode == 1 and not done.stdout.strip(): # exit 1 with no report: the tool did not run | ||
| raise TypecheckError(f"{check.tool} exited 1 with no report: {done.stderr[-300:]}") | ||
| errors = _errors(check.tool, done.stdout) |
There was a problem hiding this comment.
Must-fix: valid JSON is not necessarily a valid tool report. For either tool, mocking _run_tool to return returncode=1, stdout="{}" makes _run() return ({}, False), so check_fences() marks every runnable fence ok despite the error exit. Mypy accepts any object without a severity; basedpyright defaults a missing generalDiagnostics to an empty list. Validate the required diagnostic/report fields and raise TypecheckError for invalid shapes. Extend the existing malformed-report parameterized test with (tool, "{}") for both tools.
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.🤖 Generated with Claude Code