Repository navigation
Conversation
Adds the repository config (.github/dokimi/config.yaml, or .dokimi/config.yaml, or an explicit path) that declares subjects (skills, or the repository itself), where each subject's evals live, plan defaults, shared paths and the budget and baseline store. dokimi.repo_config discovers and validates it, resolves subjects and suites, computes skill_sha and suite_hash, and maps changed files to subjects, suites, the shared list, or nothing. tree_digest moves from dokimi.commands.repo_run into dokimi.utils, so the build context and the repo config share one tree hash. Its output for a directory without exclusions is unchanged. Part of intel#318. Library only: no CLI reads the config yet. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ll dirs Self-review of the loader found three wrong results: - shared globs were matched raw, so './scripts/harness/**' never matched a changed path and the smoke set silently skipped it. They now go through the same normalisation as the suite globs, and a glob that climbs out is refused. - a suite that is an ancestor of the skill directory (suite: skills) passed the whole-skill guard, which only checked equality, and took SKILL.md edits away from the skill. The guard now refuses a suite that contains the skill. - a skill directory with [ ] in its name matched no suite files because the name went into the glob unescaped. The skill name is now glob-escaped. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The skill that holds a file was dropped from impact whenever any suite claimed the file, while skill_sha still hashed it. A suite that points into another skill's directory (skill/b with skills/a/notes/*) therefore moved skill/a's skill_sha and left skill/a out of the impact. Now a skill is named unless its own suite holds the file, which is the rule _hashed uses. Also drops the stale "--config" flag from the schema description: no such flag exists. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
github-actions
Bot
added
the
cli-interface-change
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): ~ `dokimi runs list` parameters changed:
~ changed option `--kind`: choices: ["'answers'", "'arms'", "'cli_routes'", "'effective_instructions'", "'eval'", "'eval_coverage'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'need_graph'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'redteam'", "'repo_budget'", "'repo_environment'", "'repo_readiness'", "'result'", "'skill_evidence'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"] -> ["'answers'", "'arms'", "'cli_routes'", "'dokimi_config'", "'effective_instructions'", "'eval'", "'eval_coverage'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'need_graph'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'redteam'", "'repo_budget'", "'repo_environment'", "'repo_readiness'", "'result'", "'skill_evidence'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"]Updated on every push; the |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
- Coverage 94.29% 94.27% -0.02%
==========================================
Files 308 310 +2
Lines 39322 39954 +632
Branches 6651 6793 +142
==========================================
+ Hits 37079 37668 +589
- Misses 1430 1457 +27
- Partials 813 829 +16
🚀 New features to boost your workflow:
|
|
🔍 OpenCodeReview found 4 issue(s) in this PR. By severity: 4 medium
Review details
|
…at-dokimi-config
tree_digest followed a directory symlink root into its target, and hashed a dangling root as an empty tree. Retargeting a suite-directory link between two identical trees left the suite hash unchanged. A symlink root is now one entry, hashed by its target text like an inner symlink. The rename check now compares against a digest taken right before the rename and renames to a name in the same sort position: the old final assertion passed even with file names left out of the hash. The executable-bit test starts from an explicit 0o644 and skips on Windows. Thread: PRRT_kwDOVBg7g86ySzNj Thread: PRRT_kwDOVBg7g86ySzNp Thread: PRRT_kwDOVBg7g86ySzNt Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The manifest is a literal path: escape it before it joins eval_globs, so manifests/repo[1].yaml is claimed as written. Refuse drive-qualified paths (C:\..., C:skill) in every path and glob field, as absolute ones are. A glob whose ** is not a whole path component raises ConfigError naming the field and the pattern, instead of ValueError on Python 3.11/3.12. A matched symlink is now hashed by its target through the tree_digest fix in the previous commit; the loader tests retarget a matched dangling and a matched directory symlink and expect the suite hash to move. Thread: PRRT_kwDOVBg7g86ySzNi Thread: PRRT_kwDOVBg7g86ySzNf Thread: PRRT_kwDOVBg7g86ySzNg Thread: PRRT_kwDOVBg7g86ySzNh Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Self-review of the symlink-root change: a skill directory that is a symlink was hashed by its link text, so editing the target no longer moved skill_sha. The skill is now digested through its resolved target, as before; only suite matches are hashed by their link. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| def _own_path(config: RepoConfig) -> list[str]: | ||
| """The config file is shared: a change to it reruns the smoke set. Empty when it sits outside the root.""" | ||
| try: | ||
| return [config.path.resolve().relative_to(config.root.resolve()).as_posix()] |
There was a problem hiding this comment.
Must-fix: the config's own filename is a literal path, but this return value is consumed as a glob by _claims. An explicit config named ci/config[1].yaml therefore leaves its own change unclaimed and incorrectly marks ci/config1.yaml as shared. Escape the filename as already done for the manifest, and test that the actual config change reaches the smoke set while the similarly named file remains unclaimed.
Suggestion:
| return [config.path.resolve().relative_to(config.root.resolve()).as_posix()] | |
| return [glob.escape(config.path.resolve().relative_to(config.root.resolve()).as_posix())] |
There was a problem hiding this comment.
Fixed in 479e791: config path escaped in impact globs.
| return not segs | ||
| if pat[0] == "**": | ||
| return any(_match(pat[1:], segs[i:]) for i in range(len(segs) + 1)) | ||
| return bool(segs) and fnmatch.fnmatchcase(segs[0], pat[0]) and _match(pat[1:], segs[1:]) |
There was a problem hiding this comment.
Must-fix: impact matching is always case-sensitive, whereas root.glob() used for suite resolution is case-insensitive on Windows. For a skill containing skills/a/EVALS/q.json with the default {skill}/evals suite, Windows resolution includes EVALS in the suite and excludes it from skill_sha, but subjects_for_changed(['skills/a/EVALS/q.json']) reports a skill change rather than a suite change. The same mismatch affects defaults.skills-discovered subjects with explicit suite globs. Use consistent case semantics for filesystem glob resolution and impact matching, and add a Windows regression test.
There was a problem hiding this comment.
Confirmed on Windows (Server 2025, Python 3.13), and it also breaks the hash, not only the impact.
defaults.skills: skills/* with skills/a/EVALS/q.json and the default {skill}/evals suite: after editing q.json, both suite_hash and skill_sha move, because root.glob matches EVALS without case but the exclude in tree_digest compares evals with case. The impact reports skills={'skill/a': ...} only. On Linux the same tree moves only skill_sha (EVALS is not the suite there), so the two systems also disagree on what the suite is.
Suggestion: resolve suites with the same case-sensitive matcher the impact uses. root.glob(..., case_sensitive=True) does it on 3.12+, but 3.11 has no such parameter; listing the tree once and filtering it with _match works on every version (and also removes the trailing-** difference between 3.12 and 3.13).
There was a problem hiding this comment.
Fixed in 479e791: case-sensitive matching; Windows run pending.
|
|
||
| def _skill_owner(subjects: list[Subject], segs: tuple[str, ...]) -> Subject | None: | ||
| """The skill whose directory holds ``segs``; the deepest one when skills nest.""" | ||
| owners = [s for s in subjects if s.kind == "skill" and segs[:len(_segs(s.path))] == _segs(s.path)] |
There was a problem hiding this comment.
Must-fix: supported symlinked skills have inconsistent hashing and impact attribution. For skills/a -> vendor/a, _hashed resolves the link and an edit to vendor/a/SKILL.md changes skill_sha (as the new symlink test confirms), but this ownership check only recognizes paths under skills/a, so subjects_for_changed(['vendor/a/SKILL.md']) reports the edit as unclaimed. Attribute in-repository target paths to the corresponding skill as well, preserving suite exclusions, and extend the symlink regression test to check impact mapping.
There was a problem hiding this comment.
Fixed in 479e791: symlinked skill hashes and impacts its target.
| def _skill_owner(subjects: list[Subject], segs: tuple[str, ...]) -> Subject | None: | ||
| """The skill whose directory holds ``segs``; the deepest one when skills nest.""" | ||
| owners = [s for s in subjects if s.kind == "skill" and segs[:len(_segs(s.path))] == _segs(s.path)] | ||
| return max(owners, key=lambda s: len(_segs(s.path)), default=None) |
There was a problem hiding this comment.
Must-fix: nested skills have inconsistent hashing and impact attribution. Using the outer/inner subjects in test_a_file_goes_to_the_deepest_of_two_nested_skills, editing skills/outer/inner/SKILL.md changes both subjects' skill_sha: the outer tree digest includes the inner directory. This selection reports only skill/inner, so impact-based evaluation can skip an outer subject whose content changed. Either attribute the change to every containing skill whose hash includes it, or exclude nested subjects from the outer hash. Add a test that checks hashes and impact together.
There was a problem hiding this comment.
Not changed: nested skills are separate subjects. Probe on head: editing skills/outer/inner/SKILL.md moved skill/inner's skill_sha only, and impact named skill/inner alone. Covered by test_a_file_goes_to_the_deepest_of_two_nested_skills.
| inside = [rel[len(subject.path) + 1:] for rel in files if rel.startswith(subject.path + "/")] | ||
| # A skill directory that is a symlink is its target's content (a suite match is hashed by its link instead). | ||
| skill_sha = "sha256:" + tree_digest((root / subject.path).resolve(), exclude=inside) |
There was a problem hiding this comment.
Must-fix: selective suite globs can still change skill_sha on an eval-only addition. With suite: '{skill}/evals/**/*.json', adding evals/new/task.json excludes the JSON file but leaves the new evals/new directory in tree_digest, which hashes directory entries. Impact analysis classifies this change only as a suite change, yet the skill hash changes too. Account for directories introduced solely by excluded suite files without dropping directories containing skill content, and add a regression test using a selective file glob (the current test excludes the entire evals directory).
There was a problem hiding this comment.
Not changed: this config is refused at load. Probe on head: suite '{skill}/evals/**/*.json' gives ConfigError, the file claims must be the directory. So an eval-only addition cannot move skill_sha; the directory form is the supported one.
A nested skill is its own subject: its directory leaves the outer skill_sha, and impact names only the inner skill. A skill's suite may hold only its evals; a suite that claims its SKILL.md, scripts or references is refused at load, naming the subject and the file. baseline.store accepts false (read as off). A budget number YAML reads as text (1e3) is refused with the plain decimal or the 1.0e+3 spelling. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ig error The plain-decimal hint used %f, so 1e-7 was hinted as 0. Format the Decimal instead. A budget text the regex matches but float() rejects (1.2.3e4, .e5) raised ValueError out of load(); it now falls through to the schema's type error, a ConfigError. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… target A budget of .nan or .inf passed the schema and disabled the cap check, so it is now refused with a message. A suite directory is walked file by file, and a symlink is hashed by the file it points to, so an edit to a shared eval moves suite_hash. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…t links to its skill A symlinked directory inside a suite was skipped, so an edit to a shared eval did not move suite_hash. The walk now follows symlinks, visiting each real directory once. An evals directory that resolves to its own skill (or an ancestor) is refused, since the suite would hold SKILL.md. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…files The evals-only rule refused a suite that claims a whole directory such as tests/** of its skill. A suite may now claim any directory of its skill whole; that directory leaves skill_sha. A suite that picks an individual file of its skill (SKILL.md, a script, a reference, one eval file) is refused at load, naming the subject and the file. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The rebase onto the review fixes on fork/feat/dokimi-config: a retargeted directory symlink between two identical trees left suite_hash unchanged, because the walk hashed only the content it reaches. A symlink in a suite now adds its target text to the digest, as tree_digest does for a link, so a retarget moves the hash and an edit to the content still moves it. os is imported again for readlink. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…et exponent Found in the self-review of the rebased head: a suite link such as evals/skill_link.md -> ../SKILL.md reached the skill's own files, so the suite held them; an explicit subject took the id of a default skill at another directory and dropped it without a word; a budget written as 1e99999999 expanded to a 100 MB error message. Each now has a red-then-green test. A link the suite walks that resolves into its skill is refused; the id clash is a ConfigError naming both directories; the budget hint gives no plain decimal past exponent 30. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at d2a5cea. The loader and the schema are in good shape and the earlier fixes hold, but skill_sha / suite_hash and the changed-file impact still disagree in a few layouts; since these hashes decide whether a baseline is comparable, it is worth fixing them before anything reads the config. Details are in the line comments.
Agreed with @exai-bot: nested skills, symlinked skills, selective suite globs, the unescaped config file name, and case on Windows (I added a Windows repro in that thread). One of its earlier fixes (the symlink root) now loses a directory link's content; I replied in that thread.
What I ran, at d2a5cea:
make ci-local --offline: 0 failed, 19 passed, 13 skipped (network/Docker jobs).- Repros through the loader for each point in the line comments: nested and symlinked skills, a symlinked suite directory,
store: off, a config namedci/config[1].yaml,{skill}/evals/**/*.json, andPath.glob('s/evals/**')on Python 3.12 and 3.13. - On Windows (Server 2025, Python 3.13):
pytest tests/test_repo_config.py tests/test_tree_digest.py: 63 passed, 12 failed, 6 skipped (symlinks and chmod). All 12 failures are one parametrized test that expects a POSIX path in the error message, so it comes from the test, not the loader. Also the same tree's digest on Linux and on Windows, and anEVALSdirectory against the default suite.
Checked, no issues
context_digest→tree_digest: for a directory root it hashes the same bytes as before, so existing build-context tags and pinnedeval_dirstamps stay valid; nothing else referenced the old function.- Path validation: absolute,
..and drive-qualified paths are refused in every field, and a malformed**is aConfigErroron 3.11 to 3.13. - Schema and registration: a new
v1kind with a fixture, inSCHEMA_VERSIONS, the runs kinds and the CLI snapshot; the changelog fragment says it is library-only and nothing reads the file yet.
| "required": ["store"], | ||
| "additionalProperties": false, | ||
| "properties": { | ||
| "store": {"type": "string", "pattern": "^(off|branch:.+)$", "description": "off, or branch:<name> for the baseline branch."} |
There was a problem hiding this comment.
baseline.store documents the value off, but yaml.safe_load reads an unquoted off as False, so the documented config is refused.
baseline:\n store: off gives ConfigError: ... baseline/store: False is not of type 'string'. A user has to know to write 'off' in quotes, and the error does not say so.
Suggestion: accept false as well ("oneOf": [{"const": false}, {"type": "string", "pattern": ...}], a widening, fine for v1) and treat it as off in the loader; or change the value to something YAML keeps as a string (none), before anything reads it. A test with an unquoted store: off would catch it.
| def _suite_files(root: Path, globs: tuple[str, ...]) -> dict[str, Path]: | ||
| found: dict[str, Path] = {} | ||
| for pattern in globs: | ||
| for match in sorted(root.glob(pattern)): | ||
| found.setdefault(match.relative_to(root).as_posix(), match) | ||
| return found |
There was a problem hiding this comment.
For a suite glob that ends in **, suite_hash depends on the Python version.
Path.glob('s/evals/**') returns only directories on 3.11/3.12 (s/evals, s/evals/sub) and directories and files on 3.13+ (s/evals/a.json, s/evals/sub/b.json too). _digest_files hashes one line per match, so the same tree gets two different suite_hash values, and a baseline recorded on 3.12 never matches a 3.13 run. Each nested match is also digested again with its whole subtree.
Suggestion: drop matches that sit under another match before digesting (both versions then hash just s/evals), and add a test that compares the hash of {skill}/evals/** with that of {skill}/evals.
There was a problem hiding this comment.
Fixed in 479e791: a ** suite glob hashes like its dir.
| root = _tree(tmp_path / "repo", {".github/dokimi/config.yaml": body}) | ||
| with pytest.raises(ConfigError, match=needle) as err: | ||
| load_repo_config(root) | ||
| assert ".github/dokimi/config.yaml" in str(err.value) |
There was a problem hiding this comment.
This test fails on Windows in all 12 cases, because the message has the native path ...\\.github\\dokimi\\config.yaml.
Run on Windows Server 2025, Python 3.13: pytest tests/test_repo_config.py tests/test_tree_digest.py gives 12 failed (all here), 63 passed, 6 skipped. CI does not run this file on Windows, so it does not show.
Suggestion: assert str(Path(".github/dokimi/config.yaml")) in str(err.value), or name the file with as_posix() in the ConfigError messages.
There was a problem hiding this comment.
Fixed in 479e791: message assertion uses the platform path.
| if path.is_symlink(): | ||
| h.update(f"L {rel}\0{os.readlink(path)}\0".encode()) | ||
| elif path.is_file(): | ||
| h.update(f"F {rel}\0{int(os.access(path, os.X_OK))}\0".encode()) |
There was a problem hiding this comment.
skill_sha is not the same on Linux and Windows for the same content, because the digest takes the mode bit from os.access(path, os.X_OK), which is true for every file on Windows.
The same tree (SKILL.md, evals/q.json, same bytes) digests to 5fdcbb78… on Linux and 6c1655e1… on Windows. An absolute symlink inside a skill also puts the checkout path into the digest (os.readlink). For a local build-context tag this did not matter; for skill_sha, which a baseline store compares across machines, it means a Linux baseline never matches a Windows run.
Suggestion: decide whether skill_sha should be portable. If yes, take the executable bit from git (git ls-files -s) or leave it out where the OS has none, and make link targets relative; if not, say in the docstring that it is only comparable on the same OS.
There was a problem hiding this comment.
Pending the maintainer's decision; no change in this push. The question is whether skill_sha must be portable across Linux and Windows (the mode bit and absolute link targets differ by OS).
Options: (a) make it portable: take the executable bit from git and relative link targets; (b) keep it OS-local and document that in the docstring and the baseline notes.
There was a problem hiding this comment.
Maintainer chose option (a): skill_sha must be portable across machines. Fixed in 1cb3efe.
The exec bit comes from the git index when the file is tracked (disk bit only for untracked files), and in-tree symlink targets are hashed as relative text, so the digest is the same across machines and checkout paths.
Tests: test_the_executable_bit_is_the_one_git_records_not_the_disk_one, test_an_absolute_link_inside_the_tree_digests_the_same_at_another_path.
| ``result`` (one status per planned cell), ``repo_budget`` (what a repository puts | ||
| into an agent's context) and ``task_inspect`` (which eval-task format each | ||
| path under a tree is). This module stamps each with a | ||
| path under a tree is) and ``dokimi_config`` (a repository's subjects and where their evals are). This module stamps each with a |
There was a problem hiding this comment.
Nit: The kind list now reads "... and task_inspect (...) and dokimi_config (...)", with two "and"s, on one long line.
Suggestion: make the earlier "and task_inspect" a comma and wrap the line like the rest of the docstring.
There was a problem hiding this comment.
Fixed in 479e791: docstring rewrapped, double "and" gone.
| def _map_one(rel: str, subjects: list[Subject], shared: list[str], impact: Impact) -> None: | ||
| segs = tuple(rel.split("/")) | ||
| is_shared = any(_claims(g, segs) for g in shared) | ||
| suites = [s.id for s in subjects if any(_claims(g, segs) for g in s.eval_globs)] |
There was a problem hiding this comment.
Must-fix: suite hashing follows symlink targets, but impact matching only checks the configured paths. In the new file-symlink test, skills/a/evals/q.json -> shared/q.json means editing shared/q.json changes suite_hash, yet subjects_for_changed(['shared/q.json']) returns it as unclaimed rather than attributing it to skill/a's suite. The directory-symlink case has the same mismatch. Include repository-relative resolved suite targets in impact matching and extend both hash tests with target-path attribution assertions; otherwise a consumer selecting affected suites can omit these changes.
There was a problem hiding this comment.
Fixed in 479e791: a change at a suite file's target maps to it.
| "so the suite would hold the skill's own files" | ||
| ) | ||
| for name, entry in _walk(path, rel): | ||
| if entry.is_symlink() and entry.resolve().is_relative_to(real_skill): |
There was a problem hiding this comment.
Must-fix: the ancestor check only applies to top-level suite matches, not directory links encountered inside them. With the default suite and skills/a/evals/up -> ../../.. (the repository root), this condition accepts up because its target is an ancestor of real_skill, not inside it. _walk then reaches skills/a/SKILL.md through that link, so editing the skill changes both skill_sha and suite_hash. Apply the ancestor-directory check to nested symlinks too, and add a regression test for this layout.
There was a problem hiding this comment.
Fixed in 479e791: suite links refused at any depth.
| "so the suite would hold the skill's own files" | ||
| ) | ||
| for name, entry in _walk(path, rel): | ||
| if entry.is_symlink() and entry.resolve().is_relative_to(real_skill): |
There was a problem hiding this comment.
Must-fix: on supported Python 3.11/3.12, Path.resolve() raises RuntimeError for a symlink loop. For example, with the default suite, create skills/a/evals/loop -> loop; subjects(hashes=False) then leaks that exception instead of returning subjects or raising the documented ConfigError. _walk retains the link because it is a symlink even though is_dir() is false, so its visited-directory guard does not handle this case. Catch resolution failures here and report a ConfigError naming the subject and link; add a regression test for a self-referential or mutually referential pair of links.
There was a problem hiding this comment.
Fixed in 479e791: symlink loop is a ConfigError on 3.11.
| load_repo_config(root) | ||
|
|
||
|
|
||
| def test_an_eval_that_is_a_symlink_hashes_the_file_it_points_to(tmp_path): |
There was a problem hiding this comment.
Must-fix: this test and test_an_eval_directory_that_is_a_symlink_hashes_the_files_it_reaches / test_an_evals_dir_that_links_to_its_own_skill_is_refused create symlinks without the Windows guard used by the other symlink tests in this file. On Windows without Developer Mode or symlink privileges, they fail with OSError: [WinError 1314] before testing the loader. Add the same skip decorator to all three, or use a capability-checking fixture. If directory-link tests are enabled on Windows, also pass target_is_directory=True.
Suggestion:
| def test_an_eval_that_is_a_symlink_hashes_the_file_it_points_to(tmp_path): | |
| @pytest.mark.skipif(os.name == "nt", reason="creating symlinks needs privileges on Windows") | |
| def test_an_eval_that_is_a_symlink_hashes_the_file_it_points_to(tmp_path): |
There was a problem hiding this comment.
Fixed in 479e791: Windows skip added; ../../.. case added.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved defects misattribute symlink-target changes, allow suites to include skill content, interpret literal config paths as globs, and cause three unguarded symlink tests to fail on Windows without the required privileges.
4 open issues
What changed in this PR
4 new findings.
This PR adds the library-only foundation for #318: a versioned repository configuration describing subjects, evaluation suites, and run defaults. Command integration comes later.
Changes:
- Add
dokimi_config.v1and a loader for discovery, validation, subject resolution, hashes, and changed-file impact. - Share tree hashing with repository build-context handling.
- Register the artifact kind and add fixtures, regression tests, and release notes.
| File | Description |
|---|---|
dokimi/tests/test_tree_digest.py |
Test tree hashing and exclusions. |
dokimi/tests/test_runs_show.py |
Add config summary expectations. |
dokimi/tests/test_runs_scan.py |
Add config scan expectations. |
dokimi/tests/test_repo_config.py |
Test loading, validation, hashes, and impact mapping. |
dokimi/tests/test_import_architecture.py |
Register the loader’s architectural layer. |
dokimi/tests/fixtures/artifacts/dokimi_config.v1.json |
Supply a representative config artifact. |
dokimi/dokimi/utils.py |
Add reusable tree hashing with exclusions. |
dokimi/dokimi/schemas/dokimi_config.v1.json |
Define the persisted configuration format. |
dokimi/dokimi/runs/kinds.py |
Define config artifact summary fields. |
dokimi/dokimi/repo_config.py |
Implement configuration loading and subject analysis. |
dokimi/dokimi/commands/repo_run.py |
Use the shared tree digest. |
dokimi/dokimi/artifacts.py |
Register the config schema version. |
dokimi/docs/reference/cli-interface.json |
Include the new artifact-kind choice. |
dokimi/changelog.d/20261030_180000_napetrov_feat-repo-config.md |
Document the library-only configuration support. |
Review effort: Balanced
| load_repo_config(root) | ||
|
|
||
|
|
||
| def test_an_eval_that_is_a_symlink_hashes_the_file_it_points_to(tmp_path): |
There was a problem hiding this comment.
Fixed in 479e791: symlink test has the Windows skip.
| def _own_path(config: RepoConfig) -> list[str]: | ||
| """The config file is shared: a change to it reruns the smoke set. Empty when it sits outside the root.""" | ||
| try: | ||
| return [config.path.resolve().relative_to(config.root.resolve()).as_posix()] |
There was a problem hiding this comment.
Fixed in 479e791: same escape as the config-path thread.
| def _map_one(rel: str, subjects: list[Subject], shared: list[str], impact: Impact) -> None: | ||
| segs = tuple(rel.split("/")) | ||
| is_shared = any(_claims(g, segs) for g in shared) | ||
| suites = [s.id for s in subjects if any(_claims(g, segs) for g in s.eval_globs)] |
There was a problem hiding this comment.
Fixed in 479e791: linked suite files and skill dirs map.
| ) | ||
| dirs = [rel for rel, path in matches.items() if path.is_dir() and _within(rel, subject.path)] | ||
| for rel, path in matches.items(): | ||
| if path.is_dir() or not _within(rel, subject.path) or any(_within(rel, d) for d in dirs): |
There was a problem hiding this comment.
Fixed in 479e791: suite claiming one skill file refused.
- The config's own path is escaped before it joins the impact globs, so a bracketed config name claims only its own change. - Suite and skill globs match names from the directory listing, so matching is case-sensitive on every platform, as impact is. ** matches zero or more directories and does not recurse through a symlink; a symlink loop is a ConfigError naming the link. - Impact follows links: a change at a symlink target maps to the skill or suite that reads it. Two skills that alias one directory both own its change. A skill linked into another skill's directory is refused. - A suite may hold only its own evals: a suite that links into its skill, or to an ancestor of it, is refused at load. - An unreadable directory is a ConfigError naming the path, not a PermissionError out of the loader. - Helpers split out of _glob_paths and _check_suite for the complexity cap. Thread: PRRT_kwDOVBg7g86yUBWw Thread: PRRT_kwDOVBg7g86yWE0a Thread: PRRT_kwDOVBg7g86yUBWz Thread: PRRT_kwDOVBg7g86yUBW3 Thread: PRRT_kwDOVBg7g86yWEHL Thread: PRRT_kwDOVBg7g86yWE0e Thread: PRRT_kwDOVBg7g86yWEHN Thread: PRRT_kwDOVBg7g86yWEHP Thread: PRRT_kwDOVBg7g86yWEHR Thread: PRRT_kwDOVBg7g86yWE0P Thread: PRRT_kwDOVBg7g86yWE0j Thread: PRRT_kwDOVBg7g86yVRTz Thread: PRRT_kwDOVBg7g86yVRT2 Thread: PRRT_kwDOVBg7g86yVRT6 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…the way, and undecodable names - A skill reached through a link keeps its suite globs rewritten to the resolved directory, so deleting the evals directory still routes a change there to the suite instead of the skill. - A symlink on the declared way to a suite file, a manifest or a skill directory maps a change to that link itself, so retargeting it moves the subject it feeds. - Suite digests encode names and link targets with os.fsencode, so an undecodable byte no longer raises UnicodeEncodeError. Refs PRRT_kwDOVBg7g86yYKWh, PRRT_kwDOVBg7g86yYKWl, PRRT_kwDOVBg7g86yYKWo Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| links.extend(_link_ancestors(rel, root)) | ||
| for _, entry in _walk(path, rel, dirs=True): | ||
| real = _inside(entry, root_real) |
There was a problem hiding this comment.
Must-fix: this only records symlinks in the declared path, not links encountered while resolving another link's target. For example, let skills/a/evals/q.json -> ../../../shared/current/q.json and shared/current -> one, with different q.json contents in shared/one and shared/two. Retargeting shared/current to two changes suite_hash, but subjects_for_changed(['shared/current']) returns it as unclaimed: reach contains the final file target, not this intermediate link. Chained skill-directory links and manifest links have the same gap. Collect repository-local links along the full resolution chain and map changes to each link; add chained-link regression tests.
There was a problem hiding this comment.
Fixed in 1cb3efe. Reach now follows every link on the way to a suite file, skill or manifest (_hops in repo_config.py), not only the declared path, so a change to an intermediate link claims the subject.
Red then green: test_a_link_on_the_way_to_a_suite_files_target_claims_the_suite, test_a_link_on_the_way_to_a_skill_claims_the_skill, test_a_link_on_the_way_to_a_manifest_claims_the_subject.
| target = path.resolve() if path.exists() else path | ||
| h.update(os.fsencode(f"{name}\0{tree_digest(target)}\n")) |
There was a problem hiding this comment.
Must-fix: symlink ancestors of a suite match never contribute their target text to this digest. With suite: evaluation/current/task.json, evaluation/current -> ../one, and identical task.json bytes and modes in one and two, retargeting current to ../two leaves suite_hash unchanged. _walk starts at task.json, which is not itself a symlink, and tree_digest receives only the resolved file. This violates the stated guarantee that retargeting a suite link changes its hash. Include links traversed to reach each match in the digest (also for manifests). Extend the existing ancestor-retargeting test to use identical target contents; its different contents currently mask this omission.
There was a problem hiding this comment.
Fixed in 1cb3efe. _digest_files hashes a link <segs> line with its target text for every hop on the way to a suite file, so suite_hash moves when an ancestor link is retargeted.
A link inside the skill that a suite reaches through is claimed by the skill too (test_a_link_inside_the_skill_that_a_suite_file_reaches_through_is_claimed_by_both).
- tree_digest reads the exec bit from the git index when the file is tracked, so a checkout with a different umask or core.fileMode does not move skill_sha. Untracked files keep the disk bit. - Symlinks inside the tree hash their target as a relative path from the link, so the same tree digests the same at another path. - Reach follows every link on the way to a skill, suite file or manifest (_hops), so a change to a link on that chain claims the subject, and a link inside the skill that a suite reaches through is claimed by the skill too. - _digest_files hashes each hop line, so suite_hash moves when a link on the way changes. Covered by red-then-green tests in test_repo_config.py and test_tree_digest.py. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| resolves to, so a config linked from elsewhere in the repository counts at either. A path outside the root is | ||
| skipped. Each is a literal path, but ``shared`` is matched as a glob, so its metacharacters are escaped.""" | ||
| found: list[str] = [] | ||
| for path, root in ((config.path, config.root), (config.path.resolve(), config.root.resolve())): |
There was a problem hiding this comment.
Must-fix: this records only the declared config path and its final target, not intermediate symlink hops. For .github/dokimi/config.yaml -> ../../ci/current/config.yaml and ci/current -> one, retargeting ci/current to another directory containing a valid config makes subjects_for_changed(['ci/current']) report it as unclaimed rather than shared. A configuration change therefore fails to trigger the smoke set. Include escaped repository-relative hops from _hops, as skill/suite/manifest reach already does, and test retargeting an intermediate config link.
There was a problem hiding this comment.
Fixed in 8f23573. _own_path now also claims every link on the way to the config, so retargeting ci/current moves the smoke set. The chain-bypass nit from Round 4 is fixed in the same commit: a skill link reached through another link is claimed by the skill. Test: test_retargeting_a_link_on_the_way_to_the_config_moves_the_smoke_set; chain: test_a_link_inside_a_suite_reached_through_another_link_is_not_a_skill_link.
| for rel, path in _suite_files(root, subject.suite_globs).items(): | ||
| for _, entry in _walk(path, rel, dirs=True): | ||
| _suite_entry(entry, root, root_real, real_skill, suites, links, skill_links) |
There was a problem hiding this comment.
Must-fix: symlink reach is still collected only from existing suite matches. For suite: evaluation/current/task.json, with evaluation/current -> ../one and one/task.json present, retarget current to ../two, where task.json is absent. The suite hash changes to the empty-suite hash, but _suite_files() now returns nothing, so subjects_for_changed(['evaluation/current']) reports the link as unclaimed. Likewise, deleting one/task.json makes its real changed path unclaimed while the hash changes. Preserve link hops and resolved claims for literal suite paths even when their final entry is missing, and add regressions for both cases; the existing ancestor-link test only covers retargeting to another existing file.
There was a problem hiding this comment.
Fixed in 8f23573. A literal suite path that matches nothing now still claims the links on its way and the place it resolves to (_unnamed_literals), so retargeting evaluation/current to a directory without the file moves the suite. Test: test_retargeting_a_link_to_a_suite_file_that_is_missing_there_moves_the_suite.
| nested = [ | ||
| s.path[len(subject.path) + 1:] | ||
| for s in all_subjects | ||
| if s.kind == "skill" and s.path != subject.path and _within(s.path, subject.path) | ||
| ] |
There was a problem hiding this comment.
Must-fix: nested exclusions use only the declared path. With subjects at skills/outer and skills/outer/inner, where inner -> actual and skills/outer/actual/SKILL.md exists, _check_nesting accepts the layout, but this excludes only inner, leaving actual in the outer digest. Editing actual/SKILL.md changes both skill hashes while _skill_owners attributes the change only to the inner subject. Exclude the nested subject's resolved directory relative to the outer resolved directory as well, or reject this layout consistently. Add a regression test for this declared-nested/resolved-sibling case.
There was a problem hiding this comment.
Fixed in 8f23573. _check_nesting refuses a nested skill whose link resolves to a place other than its declared path inside the outer skill, so actual/SKILL.md can no longer sit unexcluded in both digests. Test: test_a_nested_skill_linked_to_a_sibling_inside_the_outer_skill_is_refused.
| target = os.readlink(link) | ||
| if not os.path.isabs(target): | ||
| return target | ||
| target = os.path.normpath(target) |
There was a problem hiding this comment.
Must-fix: lexical normalization collapses .. across symlink components, although the filesystem resolves those components before applying ... For example, with root/hop -> /outside/sub, retargeting root/current from the absolute root/hop/../x to root/x changes which file it reaches, but both targets become root/x here and tree_digest stays unchanged. This can reuse a stale build-context tag or skill hash. Preserve target components when stripping an in-tree absolute prefix (and preserve external target text), rather than applying normpath to the target; add a regression with a symlink component followed by ...
There was a problem hiding this comment.
Fixed in 8f23573. link_text keeps .. after an in-tree link instead of collapsing it, so retargeting through the link moves the digest. Test: test_a_dotdot_after_a_link_is_kept_so_retargeting_through_it_moves_the_digest.
| # GIT_* variables (a hook's GIT_DIR or GIT_INDEX_FILE) would point git at some other repository's index | ||
| env = Env.only(allow=[k for k in os.environ if not k.startswith("GIT_")]) | ||
| try: | ||
| done = run(["git", "-C", os.fspath(base), "ls-files", "--stage", "-z", "--", spec], timeout=60, env=env) |
There was a problem hiding this comment.
Must-fix: pass capture_limit=None when parsing this Git output. process.run defaults to a 1 MiB capture limit and replaces the middle of larger output with a truncation marker, even when done.ok is true. In a tree whose staged-file listing exceeds that limit, tracked files in the omitted region silently fall back to their disk executable bit. For example, changing only the disk mode of an omitted tracked executable then changes the digest despite its unchanged index mode; Linux and Windows checkouts can also disagree. Capture the complete listing, as the other binary Git-data callers do.
Suggestion:
| done = run(["git", "-C", os.fspath(base), "ls-files", "--stage", "-z", "--", spec], timeout=60, env=env) | |
| done = run(["git", "-C", os.fspath(base), "ls-files", "--stage", "-z", "--", spec], timeout=60, env=env, capture_limit=None) |
There was a problem hiding this comment.
Fixed in 8f23573. _tracked_modes passes capture_limit=None. Test: test_a_tracked_mode_past_the_capture_limit_still_comes_from_the_index (7000 tracked files).
| # GIT_* variables (a hook's GIT_DIR or GIT_INDEX_FILE) would point git at some other repository's index | ||
| env = Env.only(allow=[k for k in os.environ if not k.startswith("GIT_")]) | ||
| try: | ||
| done = run(["git", "-C", os.fspath(base), "ls-files", "--stage", "-z", "--", spec], timeout=60, env=env) |
There was a problem hiding this comment.
Must-fix: treat spec as a literal Git pathspec. For a tracked file root named question[1].json, Git interprets [1] as a glob character class, so ls-files does not return that file. The digest then falls back to the disk executable bit instead of the index, breaking portability for these valid filenames (also when suite files are hashed individually). Add --literal-pathspecs before -C and cover a tracked filename containing brackets.
There was a problem hiding this comment.
Pushed in 8f23573: --literal-pathspecs on the ls-files call. On git 2.43 I could not reproduce the fallback with a tracked question[1].json; the index mode was read either way, so the test test_a_tracked_file_whose_name_has_brackets_takes_its_mode_from_the_index pins the behavior rather than showing a red state.
| ``link_text``); one pointing outside ``root`` is hashed as written. | ||
| """ | ||
| skip = tuple(e.strip("/") for e in exclude) | ||
| entries = [root] if root.is_symlink() or root.is_file() else sorted(root.rglob("*")) |
There was a problem hiding this comment.
Must-fix: sorting Path objects uses platform-dependent ordering: POSIX sorts B.txt before a.txt, while Windows folds case and sorts a.txt first. Since the entries are hashed sequentially, identical trees produce different digests across Linux and Windows, including skill_sha and build-context tags. Sort using a case-sensitive, platform-independent relative POSIX path key, and add mixed-case filename coverage with an explicit expected ordering.
Suggestion:
| entries = [root] if root.is_symlink() or root.is_file() else sorted(root.rglob("*")) | |
| entries = ( | |
| [root] | |
| if root.is_symlink() or root.is_file() | |
| else sorted(root.rglob("*"), key=lambda p: p.relative_to(root).as_posix()) | |
| ) |
There was a problem hiding this comment.
Pushed in 8f23573: tree_digest sorts entries by path parts, so the order is the same on every OS. On Linux the old order already matched, so test_mixed_case_names_are_ordered_by_code_point_on_every_os passes before and after here. It is a regression guard for Windows, not a red-then-green test.
- link_text keeps ".." after an in-tree link, so retargeting through it moves the digest - tree_digest sorts entries by path parts, the same order on every OS - _tracked_modes uses --literal-pathspecs and no capture limit - the config's links, and the links on the way to a literal suite path that matches nothing now, are claimed; a link a suite matches is not a skill link - _check_nesting refuses a nested skill that does not resolve inside the outer one Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if os.path.isabs(target): | ||
| for anchor in (root_real, root): # written as the resolved or the given spelling of the repository | ||
| try: | ||
| pending = [*Path(os.path.normpath(target)).relative_to(anchor).parts, *pending] |
There was a problem hiding this comment.
Must-fix: lexical normalization removes real symlink dependencies when an absolute target contains ... For example, let skills/a/evals/q.json link to <root>/shared/current/../q.json, with shared/current -> ../one/sub and identical one/q.json and two/q.json files (both sub directories exist). Retargeting current to ../two/sub changes the file actually read, but normpath collapses current/.. before traversing it. Consequently _hops never records shared/current, suite_hash stays unchanged, and subjects_for_changed(['shared/current']) leaves the change unclaimed. Preserve the target's .. components and process them in the existing traversal loop. The same helper also tracks skill, manifest, and config dependencies; add an absolute-target regression test.
| if literal in matches or any(c in literal for c in "*?["): | ||
| continue |
There was a problem hiding this comment.
Must-fix: empty wildcard suites lose their ancestor-link dependencies. For suite: evaluation/current/*.json, start with evaluation/current -> ../one and one/task.json, then retarget current to ../two, which contains no JSON files. The suite hash changes, but _reach has no matches and this branch skips the wildcard, so subjects_for_changed(['evaluation/current']) reports the link as unclaimed. Preserve link claims along glob prefixes even when the final pattern matches nothing, and add a wildcard counterpart to the existing missing-literal regression.
There was a problem hiding this comment.
Fixed in b109fc3 (Anton's commit): a wildcard that matches nothing claims the links on its literal prefix and its pattern under the resolved place. Test test_retargeting_a_link_to_a_wildcard_suite_that_matches_nothing_there_moves_the_suite, red on 8f23573. 94f43ac adds the case where other files still match.
| if real is None or real in seen: | ||
| return | ||
| seen.add(real) | ||
| for child in sorted(entry.iterdir()): |
There was a problem hiding this comment.
Must-fix: Path ordering here still makes suite hashes platform-dependent when directory aliases share a target. With suite: evaluation and evaluation/B and evaluation/a both linking to ../shared containing q.json, POSIX visits B first while Windows visits a first. Because seen permits only the first traversal of shared, the emitted file is respectively evaluation/B/q.json or evaluation/a/q.json, producing different suite hashes. Sorting the final results cannot restore the omitted alias's descendants. Sort children by their case-sensitive names before traversal and add a mixed-case directory-alias regression; the ordering fix in tree_digest does not cover this walk.
Suggestion:
| for child in sorted(entry.iterdir()): | |
| for child in sorted(entry.iterdir(), key=lambda p: p.name): |
There was a problem hiding this comment.
| return target | ||
| # the tree may be named by a path with links in it: match the target under the name as given and as resolved | ||
| # (a link that is the root itself is not resolved, or its own target would count as inside it) | ||
| anchors = [os.fspath(root)] if root.is_symlink() else [os.fspath(root), os.path.realpath(root)] |
There was a problem hiding this comment.
Must-fix: a symlink root must not be treated as a directory anchor. For an absolute link path, os.symlink(link, link) makes a self-referential link, but link_text(link, link) normalizes its target to .. Replacing it with os.symlink('.', link) therefore leaves tree_digest(link) unchanged even though it now resolves to the parent directory instead of a loop. Preserve the target text when the root itself is a symlink, and add a regression test for these two targets.
| anchors = [os.fspath(root)] if root.is_symlink() else [os.fspath(root), os.path.realpath(root)] | ||
| for anchor in anchors: | ||
| try: | ||
| inside = os.path.commonpath([target, anchor]) == anchor and target.startswith(anchor) |
There was a problem hiding this comment.
Must-fix: Windows os.readlink() returns the substitution path for an absolute symlink, typically \\?\C:\checkout\tree\q.json, whereas both anchors normally use C:\checkout\tree (Windows realpath() strips the extended-path prefix for an ordinary input). commonpath() treats these as different drives, so the internal target is left absolute. Two identical trees with internal absolute links at different checkout paths therefore get different digests on Windows. Normalize the extended-path representation for containment checks while preserving the target's remaining components, and add a Windows symlink regression test where privileges permit.
There was a problem hiding this comment.
Fixed in 94f43ac: _plain_path strips the \\?\ and \\?\UNC\ prefix from an absolute link target before the containment check. test_a_windows_extended_path_prefix_is_read_as_the_plain_path runs on every OS. The Windows symlink test runs only on Windows and was not run here.
| except ValueError: # different drives, or a relative path mixed in | ||
| continue | ||
| if inside: | ||
| rest = [part for part in target[len(anchor):].split(os.sep) if part not in ("", ".")] |
There was a problem hiding this comment.
Must-fix: stripping empty and . components makes semantically different symlink targets hash identically. On POSIX, create a regular root/q.json and a link targeting str(root / 'q.json'); replacing its target with that string plus / (or /.) breaks the link because the target must now be a directory, but link_text still returns q.json, so tree_digest and the skill hash remain unchanged. Preserve trailing directory requirements when converting absolute targets to relative ones, and add regression tests for both suffixes.
| root = _write(tmp_path / "t", {"run.sh": "echo\n"}) | ||
| (root / "run.sh").chmod(0o755) | ||
| subprocess.run(["git", "init", "-q", str(root)], check=True) | ||
| subprocess.run(["git", "-C", str(root), "add", "run.sh"], check=True) |
There was a problem hiding this comment.
Must-fix: on Windows, chmod(0o755) does not set an executable bit, so git add records mode 100644 here. Changing to 0644 then changes nothing, and this test still passes if _tracked_modes is removed entirely. The bracket-name and capture-limit tests have the same problem. Set the index mode explicitly with git update-index --chmod=+x -- <file> after adding the files, so all three tests actually exercise a tracked executable even on Windows or with core.filemode=false.
There was a problem hiding this comment.
Fixed in 94f43ac: the three tests that need a tracked exec bit set it with git update-index --chmod=+x after git add (helper _track_executable), so they hold where chmod sets no bit. The index is the source, not the disk.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 8f23573. The last round's fixes hold, but the two test gaps I named at c80ce22 are still open: no test was added for either, and both mutants still pass the full suite. A few smaller edge cases sit inline.
What I ran, on a Linux review host at 8f23573:
make ci-local: 30 passed, 0 failed, 7 skipped (network/Docker/other-Python legs); changed-line coverage 95%.- Reproductions through the loader for the impact and hashing cases below, with the LLM transport mocked.
- A mutation run over
repo_config.pyandutils.pyagainsttest_repo_config.py,test_repo_config_links.pyandtest_tree_digest.py: 45 mutants, 26 of which pass the full suite (about 10 of those are equivalent).
Findings that cannot be anchored inline:
- Two gate lines in the suite digest and the skill exclusions still have no test (from my earlier review).
repo_config.py:628: a mutant that leaves the file name out of the suite digest passes the full suite, so renamingskills/a/evals/x.yamltoy.yamlwith the same content keepssuite_hash.repo_config.py:754and:668:startswithinstead of_withinpasses too; with skillsskills/aandskills/abcd, an edit toskills/a/cdleavesskill/a'sskill_shaunchanged (False under the mutant, True in the real code), and suite pathstests/evandtests/evalsdrop the longer one. Tests to add: rename an eval file and expectsuite_hashto move; two skills with a shared name prefix and an edit under the shorter one. - Case sensitivity and
**/matching are still unpinned (from my earlier review). Lower-casing both sides in the glob walk (repo_config.py:583) and in_match(:254) passes the suite: withskills/a/Evalsnext toevals, an edit toEvals/ythen movessuite_hash, and withshared: ['docs/Guide/**']a change todocs/guide/xis claimed as shared. Making**need at least one directory (range(1, len(segs) + 1)at:253) passes too:shared: ['**/Makefile']then reports a changed rootMakefileas unclaimed. Add a test withskills/a/Evalsbesideevals, and one with**/Makefileagainst a root file.
Nit: four more surviving mutants decide a hash or an attribution. _hops resetting here = () for an absolute link (repo_config.py:317) is unpinned: with skills/a/evals -> ROOT/top and top -> ROOT/store, a change to top stops reaching the suite. The glob.escape on the config's link hops (:239) is unpinned: a config under a link named c[1] loses its shared claim. The [".."] * depth prefix in link_text (utils.py:118) is unpinned. --literal-pathspecs (utils.py:133) is unpinned: a tracked :x.sh takes its disk mode instead of the index mode.
Checked, no issues
- Digest move: on a plain directory
tree_digestand the oldcontext_digestgive identical bytes, so existing build-context tags stay valid. Only a git-tracked file whose disk mode differs from its index mode digests differently, which rebuilds once. - Portability: the same tree with an absolute in-tree link digests the same at two paths, a chmod on a tracked file does not move the digest, and a chmod on an untracked one does.
- Config errors: unreadable files and directories, a missing explicit config, broken and looping links, and odd changed-path strings give a
ConfigErroror no error, not a raw exception. - Hash and impact agree in 20 link layouts (relative and absolute targets, chains, parent links, linked skills); the exceptions are the two inline cases above.
- Repo rules: every commit is signed off, the kind is in
SCHEMA_VERSIONS,schema_compatand the CLI snapshot pass, and nothing reads the config yet.
Bot points I checked that no longer apply at this head (30)
- exai-bot's point on the unescaped config filename does not apply: the path is glob-escaped at this head.
- exai-bot's point on case-insensitive suite resolution does not apply: suite and skill globs match listed names case-sensitively at this head.
- exai-bot's point on symlinked skills in impact does not apply: a change at the link target maps to the skill at this head.
- exai-bot's point on nested skills does not apply: the inner directory leaves the outer hash, so only the inner skill moves and only it is named.
- exai-bot's point on selective suite globs does not apply: a suite that picks individual files is refused at load.
- exai-bot's point on suite symlink targets in impact does not apply: a change at a suite file's target maps to the suite at this head.
- exai-bot's point on nested ancestor links does not apply: suite links that reach an ancestor of the skill are refused at any depth.
- exai-bot's point on symlink loops does not apply: a loop is a ConfigError naming the link at this head.
- exai-bot's point on unguarded symlink tests does not apply: the Windows skip is in place at this head.
- Copilot's point on unguarded symlink tests does not apply: the Windows skip is in place at this head.
- Copilot's point on the config filename read as a glob does not apply: the path is escaped at this head.
- Copilot's point on symlink targets in impact does not apply: linked suite files and skill directories map to their subject at this head.
- Copilot's point on a suite claiming one skill file does not apply: such a suite is refused at load at this head.
- exai-bot's point on a config reached through a link does not apply: the config is shared at its declared path and at its target.
- exai-bot's point on deleted files under a linked suite directory does not apply: the suite's walked directories are in its reach.
- exai-bot's point on undecodable names in tree_digest does not apply: tree_digest encodes with os.fsencode at this head.
- exai-bot's point on a linked skill whose whole evals directory is deleted does not apply: the suite globs are rewritten to the resolved directory and the deleted file maps to the suite at this head.
- exai-bot's point on a symlinked parent of a suite file does not apply: a change to the link itself now claims the suite at this head.
- exai-bot's point on undecodable names in suite digests does not apply: both hash-update sites encode with os.fsencode at this head.
- exai-bot's point on intermediate links in a link chain does not apply: reach follows every hop of the chain at this head.
- exai-bot's point on ancestor links missing from the suite digest does not apply: each hop is hashed with its target text at this head.
- exai-bot's point on intermediate links of a linked config does not apply: the config's hops are claimed as shared at this head.
- exai-bot's point on a literal suite path that matches nothing does not apply: it still claims its links and the place it resolves to at this head.
- exai-bot's point on a nested skill linked to a sibling directory does not apply: such a layout is refused at load at this head.
- exai-bot's point on
..collapsed after a link in tree_digest does not apply:link_textkeeps..for in-tree targets at this head. - exai-bot's point on the 1 MiB capture limit does not apply:
_tracked_modespassescapture_limit=Noneat this head. - exai-bot's point on
ls-filespathspecs does not apply: the call uses--literal-pathspecsat this head. - exai-bot's point on platform-dependent sorting in tree_digest does not apply: entries sort by path parts at this head.
- exai-bot's point on Windows extended-path link targets is not something I could check: I have no Windows symlink run, so I leave it out of the findings.
- exai-bot's point on the tracked-mode tests under Windows does not apply to a gate:
windows-smokeruns only a few fast tests and nottest_tree_digest.py.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| if wild == 0 or _glob_paths(root, literal): | ||
| continue |
There was a problem hiding this comment.
Must-fix: preserve the resolved wildcard pattern even when other files still match. For suite: evaluation/current/*.json, with evaluation/current -> ../one and both one/task.json and one/keep.json, delete one/task.json. subjects_for_changed(['one/task.json']) returns it as unclaimed: _reach records only the surviving file, the declared glob does not match one/task.json, and this condition skips adding one/*.json. The deletion changes suite_hash but omits the affected subject from impact selection. Resolve the literal prefix for wildcard patterns regardless of surviving matches, and extend the existing only-file deletion test with a surviving match.
There was a problem hiding this comment.
| in_suite = any(_within(rel, m) for m in matches) | ||
| if real_skill is not None and _under(hop, real_skill) and hop not in declared_links and not in_suite: | ||
| skill_links.append(hop) |
There was a problem hiding this comment.
Must-fix: compare suite membership using resolved skill locations too. With skills/a -> ../vendor/a, the default suite at skills/a/evals, and vendor/a/evals/q.json -> ../../../shared/q.json, _hops() returns vendor/a/evals/q.json, but both matches and declared_links use skills/a/.... This condition therefore adds the eval link to skill_links. subjects_for_changed(['vendor/a/evals/q.json']) incorrectly reports both skill and suite impact, although retargeting this link leaves skill_sha unchanged because evals is excluded. Account for the suite's spelling under the resolved skill directory and add a linked-skill variant of the existing suite-link test.
| if link == root and target == anchor: | ||
| return target # the root links to itself, a loop: "." would read as a link to its own directory |
There was a problem hiding this comment.
Must-fix: the self-link fix still permits a digest collision for descendant targets. With root = Path('/tmp/link'), create os.symlink('/tmp/link/child', root), record tree_digest(root), then replace it with os.symlink('child', root). Both targets normalize to child, so the digest is unchanged, although the first loops through the root symlink and the second resolves to /tmp/child. The exact target == anchor guard only covers the simplest self-link. Return the target unchanged whenever the root itself is a symlink, rather than treating that link as a directory anchor, and add this descendant case to the root-link regression tests.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 809282b, which fix the four nits from my review:
f4d6cc7:dokimi/dokimi/repo_config.py:311: droppedos.path.normpathon an absolute link target so a..after a link is kept and the link is recorded as a hop; added a test with the absolute spelling.b109fc3:dokimi/dokimi/repo_config.py:365: a wildcard suite that matches nothing now claims the links on the way to its literal prefix and its pattern under the prefix's resolved place; added retarget and delete tests.90422e1:dokimi/dokimi/repo_config.py:657: directory children are sorted byp.name, so the walk order is the same on every OS.809282b:dokimi/dokimi/utils.py:115:link_textkeeps a trailing/for targets ending in/or/., and returns the target as written when the root links to itself; added a test for each.
Left as is:
dokimi/dokimi/repo_config.py:317: the four unpinned mutants (absolute-linkherereset,glob.escapeon hops, the..prefix inlink_text,--literal-pathspecs) need four new tests that were not added in this round.
make ci-local at 809282b: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the two risks from my review.
…refixes, keep root and Windows link targets - A suite reached through a linked skill is also matched by where its files resolve, so a link inside it is not reported as a skill link. - A wildcard suite claims the links on the way to its literal prefix and its own pattern under the place the prefix resolves to, also when other files still match. - A root symlink keeps its target as written, so a link to a descendant of itself no longer digests like the relative one. - Absolute link targets lose the Windows extended-length prefix before the containment check. - The tests that chmod a tracked file set the index exec bit explicitly (update-index --chmod=+x), so they hold on Windows too. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| target = os.readlink(link) | ||
| if os.path.isabs(target): | ||
| for anchor in (root_real, root): # written as the resolved or the given spelling of the repository |
There was a problem hiding this comment.
Must-fix: normalize Windows extended-path targets before comparing them with the repository anchors. os.readlink() can return \\?\C:\repo\shared\current\q.json, whereas root and root_real use C:\repo; relative_to() rejects that different drive spelling and traversal stops. For an absolute suite link targeting shared/current/q.json, retargeting the intermediate shared/current link therefore changes the suite hash but leaves that changed link unclaimed. This also affects intermediate hops for config, skill and manifest links. Apply the extended-prefix normalization already used by utils.link_text here, preserving .. components, and test intermediate-link attribution on Windows.
There was a problem hiding this comment.
Fixed in 7b121d8: _hops reads an absolute link target through _plain_path, so \\?\C:\... matches the plain root.
Test: test_a_link_target_with_the_windows_extended_prefix_is_matched_as_the_plain_path (red before, green after; the prefix is stubbed on Linux).
| prefix = root.joinpath(*segs[:wild]) | ||
| links.extend(_hops(prefix, root, root_real)) |
There was a problem hiding this comment.
Must-fix: symlink claims disappear when the link occurs after a wildcard and no longer yields a match. For suite: evaluation/**/task.json, create evaluation/link -> ../one with one/task.json, then retarget it to ../two where task.json is absent. The suite hash changes, but subjects_for_changed(['evaluation/link']) reports the link as unclaimed: _reach has no matched entry to walk, and this fallback checks only evaluation, before **. The same failure occurs with evaluation/*/task.json. Track symlinks traversed while matching wildcard prefixes even when the remaining pattern matches nothing, and add a regression test for this retargeting case.
There was a problem hiding this comment.
Fixed in 7b121d8: a glob now claims each symlink its walk reads through, matched or not (repo_glob.glob_links; the walk moved to dokimi/repo_glob.py).
Test: test_a_link_met_by_a_wildcard_suite_still_claims_it_after_its_target_loses_the_match (red before).
| if real is None or real == root_real or not real.is_relative_to(root_real): | ||
| return None |
There was a problem hiding this comment.
Must-fix: a supported symlinked skill can resolve to the repository root, but this branch drops its resolved ownership. For example, declare path: skills/a, create skills/a -> ../.., and leave the default evals directory absent. subjects() accepts this layout and hashes the repository tree as skill_sha; editing README.md changes that hash, yet subjects_for_changed(['README.md']) reports the file as unclaimed because _reach() retains only skills/a. Either reject a skill resolving to the repository root during subject validation, or represent and handle root ownership explicitly. Add a regression test checking hash/impact consistency for this layout.
There was a problem hiding this comment.
Fixed in 7b121d8: a skill that resolves to the repository root is now a ConfigError at load, so its hash never covers the whole repo.
Test: test_a_skill_linked_to_the_repository_root_is_a_config_error. I chose rejection over representing root ownership.
| anchors = [os.fspath(root)] if root.is_symlink() else [os.fspath(root), os.path.realpath(root)] | ||
| for anchor in anchors: | ||
| try: | ||
| inside = os.path.commonpath([target, anchor]) == anchor and target.startswith(anchor) |
There was a problem hiding this comment.
Must-fix: containment is case-sensitive on Windows even though filesystem paths are not. For a root C:\Checkout\skill and an absolute symlink target c:\checkout\skill\q.json, commonpath() preserves the target's spelling, so this comparison fails (as does startswith). The in-tree target is then hashed as an absolute path, and an equivalent tree at another checkout location gets a different digest. Compare normalized case on Windows while retaining the original target components, and add coverage for a target whose drive/directory casing differs from the root.
There was a problem hiding this comment.
Fixed in 7b121d8: containment compares normcase'd spellings (identity on POSIX); the returned text keeps the target's own spelling.
Test: test_an_absolute_link_inside_the_tree_matches_whatever_the_case_of_its_target, Windows-only, not run on this Linux box; the Windows CI job runs it.
| @pytest.mark.skipif(os.name != "nt", reason="the extended-path prefix is what Windows reads an absolute link back as") | ||
| @symlinks |
There was a problem hiding this comment.
Must-fix: this test is skipped on every platform. The first decorator skips non-Windows systems, while @symlinks skips Windows, so the Windows integration test never exercises os.readlink() and the extended-prefix containment fix, even when symlink creation is available. Remove @symlinks here and skip only if creating the Windows symlink actually fails because privileges are unavailable.
There was a problem hiding this comment.
Already fixed on the head by 54ffa03: @symlinks is gone from this test and a failed symlink creation skips. No change in this round.
| _track_executable(root, "question[1].json") | ||
| before = tree_digest(root / "question[1].json") # a file root: git is asked for its name as the pathspec |
There was a problem hiding this comment.
Must-fix: the earlier Windows test gap still exists after setting the index mode. On Windows, both chmod calls leave the disk executable bit unset, so replacing _tracked_modes with return {} still makes this equality pass. The directory-root and capture-limit tests have the same gap. Assert that changing only the index mode changes the digest while disk permissions stay constant. For directory-root tests, exclude .git so hashing the changed index file cannot satisfy the assertion by itself.
There was a problem hiding this comment.
Fixed in 7b121d8: the bracket, capture-limit and executable-bit tests hold the disk bit still and assert that an index-only change moves the digest; directory roots exclude .git.
Probe: with _tracked_modes returning {}, these three tests fail.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Not approving yet. The last commit's fixes hold and its new tests pin their lines, but two test gaps from my earlier review are still open, and two bot points reproduce at this head.
What I ran, on a Linux review host at 94f43ac:
make ci-local: 30 passed, 0 failed, 7 skipped (network/Docker/other-Python legs); 12,162 tests; changed-line coverage 95%- Reproductions through the loader for the impact and hashing cases below.
- A mutation run over
repo_config.pyandutils.pyagainsttest_repo_config.py,test_repo_config_links.pyandtest_tree_digest.py: 15 mutants, 9 of which pass the full suite (158 passed, 1 skipped).
Still open from my earlier review:
dokimi/dokimi/repo_config.py:654and:694: the gate lines in the suite digest and the skill exclusions have no test. Leaving the file name out of the suite digest passes the full suite, so renamingevals/x.yamltoy.yamlkeepssuite_hash.startswithinstead of_within(and in_check_match) passes too, so with skillsskills/aandskills/abcdan edit toskills/a/cdcan be attributed to the wrong skill. Tests to add: rename an eval file and expectsuite_hashto move; two skills with a shared name prefix and an edit under the shorter one.dokimi/dokimi/repo_config.py:254and:609: case sensitivity and**/matching are unpinned. Lower-casing both sides in_matchpasses the suite, soshared: ['docs/Guide/**']claims a change todocs/guide/x. Lower-casing the glob walk passes too, soskills/a/Evalsbesideevalsbecomes a suite. Making**need a directory (range(1, len(segs) + 1)at:253) passes, soshared: ['**/Makefile']leaves a changed rootMakefileunclaimed. Tests to add:skills/a/Evalsbesideevals, and**/Makefileagainst a root file.- Nit, not added in the last round: removing
glob.escapeon the config's link hops (repo_config.py:239), the[".."] * depthprefix inlink_text(utils.py:133) and--literal-pathspecs(utils.py:148) each still passes the full suite.
Open bot points I confirm:
- I agree with exai-bot that a link matched by a wildcard in the middle of a suite glob is unclaimed once its target lacks the file. New evidence: with
suite: evaluation/*/task.json(andevaluation/**/task.json) andevaluation/link -> ../one, retargeting the link to../twowithouttask.jsonmovessuite_hash, butsubjects_for_changed(["evaluation/link"])givesunclaimed=['evaluation/link', 'one/task.json', ...]. - I agree with exai-bot that a skill linked to the repository root loses its ownership. New evidence:
path: skills/awithskills/a -> ../..loads,skill_shamoves whenREADME.mdchanges, andsubjects_for_changed(["README.md"])givesskills={}andunclaimed=['README.md'].
Checked, no issues
- Last commit: the wildcard-prefix claims,
_resolved_spellings,_plain_pathand the root-link rule inlink_textare each pinned by a new test; removing them fails the suite. The wildcard change over-claims no file that the hash ignores. - Repo rules: every non-merge commit is signed off, the kind is in
SCHEMA_VERSIONS,schema_compatagainst the base passes, the CLI snapshot and changelog fragment are present. - Open design question: @vlad-perevezentsev's question on whether
skill_shamust be portable across Linux and Windows waits for the author, who held it.
Bot points I checked that no longer apply at this head (41)
- exai-bot's point on the unescaped config filename does not apply: the path is glob-escaped at this head.
- exai-bot's point on case-insensitive suite resolution does not apply: globs match listed names case-sensitively at this head.
- exai-bot's point on symlinked skills in impact does not apply: a change at the link target maps to the skill at this head.
- exai-bot's point on nested skills does not apply: the inner directory leaves the outer hash, so only the inner skill moves and only it is named.
- exai-bot's point on selective suite globs does not apply: a suite that picks individual files is refused at load.
- exai-bot's point on suite symlink targets in impact does not apply: a change at a suite file's target maps to the suite at this head.
- exai-bot's point on nested ancestor links does not apply: suite links that reach an ancestor of the skill are refused at any depth.
- exai-bot's point on symlink loops does not apply: a loop is a ConfigError naming the link at this head.
- exai-bot's point on unguarded symlink tests does not apply: the Windows skip is in place at this head.
- Copilot's point on unguarded symlink tests does not apply: the Windows skip is in place at this head.
- Copilot's point on the config filename read as a glob does not apply: the path is escaped at this head.
- Copilot's point on symlink targets in impact does not apply: linked suite files and skill directories map to their subject at this head.
- Copilot's point on a suite claiming one skill file does not apply: such a suite is refused at load at this head.
- exai-bot's point on a config reached through a link does not apply: the config is shared at its declared path and at its target.
- exai-bot's point on deleted files under a linked suite directory does not apply: the suite's walked directories are in its reach.
- exai-bot's point on undecodable names in tree_digest does not apply: tree_digest encodes with os.fsencode at this head.
- exai-bot's point on a linked skill whose whole evals directory is deleted does not apply: the suite globs are rewritten to the resolved directory.
- exai-bot's point on a symlinked parent of a suite file does not apply: a change to the link itself claims the suite at this head.
- exai-bot's point on undecodable names in suite digests does not apply: both hash-update sites encode with os.fsencode at this head.
- exai-bot's point on intermediate links in a link chain does not apply: reach follows every hop of the chain at this head.
- exai-bot's point on ancestor links missing from the suite digest does not apply: each hop is hashed with its target text at this head.
- exai-bot's point on intermediate links of a linked config does not apply: the config's hops are claimed as shared at this head.
- exai-bot's point on a literal suite path that matches nothing does not apply: it still claims its links and the place it resolves to.
- exai-bot's point on a nested skill linked to a sibling directory does not apply: such a layout is refused at load at this head.
- exai-bot's point on
..collapsed after a link in tree_digest does not apply:link_textkeeps..for in-tree targets at this head. - exai-bot's point on the 1 MiB capture limit does not apply:
_tracked_modespassescapture_limit=Noneat this head. - exai-bot's point on
ls-filespathspecs does not apply: the call uses--literal-pathspecsat this head. - exai-bot's point on platform-dependent sorting in tree_digest does not apply: entries sort by path parts at this head.
- exai-bot's point on
..after a link in an absolute target does not apply:_hopsno longer normalizes the target at this head. - exai-bot's point on a wildcard suite that matches nothing does not apply for a wildcard in the last segment: its prefix links are claimed at this head.
- exai-bot's point on the order of directory aliases in the suite walk does not apply: children are walked by name at this head.
- exai-bot's point on a root link to itself does not apply: the target text is kept as written at this head.
- exai-bot's point on Windows extended-path targets in
link_textis not something I could check: it needs a Windows symlink run;_plain_pathis covered by a test that runs on every OS. - exai-bot's point on trailing
/and/.in absolute targets does not apply: both suffixes are kept in the link text at this head. - exai-bot's point on the tracked-mode tests under Windows does not apply: they set the index mode with
git update-index --chmod=+xat this head. - exai-bot's point on a wildcard suite where other files still match does not apply: the pattern under the resolved place is claimed at this head.
- exai-bot's point on an eval link under a linked skill does not apply: suite matches are also named by where their directory resolves at this head.
- exai-bot's point on a root link to a descendant of itself does not apply: a root link's target is kept as written at this head.
- exai-bot's point on extended-path targets in
_hopsis not something I could check: it needs a Windows symlink run. - exai-bot's point on case-sensitive containment on Windows is not something I could check: it needs a Windows run.
- exai-bot's point on the Windows tracked-mode tests does not apply to a gate:
windows-smokedoes not runtest_tree_digest.py.
|
|
||
| def _norm(value: str) -> str | None: | ||
| """A repository-relative POSIX path or glob, or None when it is empty, absolute or climbs out.""" | ||
| norm = PurePosixPath(value.replace("\\", "/")).as_posix() |
There was a problem hiding this comment.
Must-fix: preserve literal backslashes in POSIX changed-file names. With the default suite skills/a/evals, a regular file named skills/a/evals\q.json is directly inside the skill (not inside evals) and contributes to skill_sha. Passing that Git path to subjects_for_changed() rewrites it to skills/a/evals/q.json, attributes it only to the suite, and omits the changed skill. The same conversion in _pattern() also prevents default discovery from resolving skill directories containing literal backslashes. Normalize Windows separators only where the platform/input contract requires it, and add a POSIX test asserting hash-to-impact consistency for this filename.
There was a problem hiding this comment.
Fixed in 7b121d8: _norm turns a backslash into / only on Windows; on POSIX it stays part of the file name.
Test: test_a_backslash_in_a_posix_file_name_is_part_of_the_name (red before).
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Not approving yet. Three bot points reproduce at this head and the merge waits for them, and test gaps from my earlier review are still open. The last commit's fixes hold: the wildcard-prefix claims, _resolved_spellings, _plain_path and the root-link rule are pinned by new tests.
What I ran, on a Linux review host at 8c71859:
make ci-local: 29 passed, 1 failed, 7 skipped; 12,405 tests; changed-line coverage passed. The failure istest_reference_checks.py::test_the_regex_limit_is_shared_by_every_pattern_of_one_check, a wall-clock bound that fails under load; it does not touch this PR and passes alone.- Reproductions through the loader for the three bot points below, with the LLM transport mocked.
- A mutation run over
repo_config.pyandutils.pyagainsttest_repo_config.py,test_repo_config_links.pyandtest_tree_digest.py: 119 mutants, 61 pass the full suite (158 passed, 1 skipped); about half of those are equivalent.
Still open from my earlier review:
dokimi/dokimi/repo_config.py:654and:694: the gate lines in the suite digest and the skill exclusions have no test. Leaving the file name out of the digest line, and usingstartswithinstead of_within, both pass together (158 passed). So renamingevals/x.yamltoy.yamlkeepssuite_hash, and with skillsskills/aandskills/abcdan edit toskills/a/cdleavesskill/a'sskill_shaunchanged. Tests to add: rename an eval file and expectsuite_hashto move; two skills with a shared name prefix and an edit under the shorter one.dokimi/dokimi/repo_config.py:246-254and:609: case sensitivity and**/matching are unpinned. Lower-casing both sides in_matchor the glob walk passes, and so does making**need a directory, soshared: ['**/Makefile']leaves a changed rootMakefileunclaimed. Tests to add:skills/a/Evalsbesideevals,shared: ['docs/Guide/**']againstdocs/guide/x, and**/Makefileagainst a root file.- Nit: removing
glob.escapeon the config's link hops (repo_config.py:239), the[".."] * depthprefix inlink_text(utils.py:133) and thecommonpathguard (utils.py:125: a link to the sibling/x/t2/q.jsondigests like a relative2/q.json) each still pass the full suite.
Open bot points I confirm:
- I agree with exai-bot that a link matched by a wildcard in the middle of a suite glob is unclaimed once its target lacks the file. New evidence: with
suite: evaluation/*/task.json(alsoevaluation/**/task.json) andevaluation/link -> ../one, retargeting the link to a directory withouttask.jsonmovessuite_hash, andsubjects_for_changed(["evaluation/link"])givesunclaimed=['evaluation/link']; deleting the link does the same. - I agree with exai-bot that a skill linked to the repository root loses its ownership. New evidence:
path: skills/awithskills/a -> ../..loads,skill_shamoves whenREADME.mdchanges, andsubjects_for_changed(["README.md"])givesskills={}andunclaimed=['README.md']. - I agree with exai-bot that backslashes in POSIX changed-file names are rewritten. New evidence: a file
skills/a/evals\q.jsoninside the skill movesskill_shaonly, yetsubjects_for_changed(["skills/a/evals\\q.json"])givessuites={'s': ['skills/a/evals/q.json']}, skills={}; a directorysk/we\irdfound bydefaults.skills: sk/*makes the whole config fail withConfigError: sk/we/ird is not a directory. - I agree with exai-bot that
_hopsdoes not strip the Windows extended-path prefix. New evidence: by reading the code,utils.link_textcalls_plain_pathon the target and_hopsinrepo_config.pyhas no such call, so an absolute in-repo link is hashed as inside but not followed for impact; I did not run it on Windows.
I agree with @vlad-perevezentsev's nit on the two "and"s in the kind list of the artifacts.py docstring.
I agree with exai-bot that the Windows-only extended-path test in test_tree_digest.py is skipped on every platform; skipif(os.name != "nt") and @symlinks (which skips on Windows) cancel out, and pytest -rs reports it skipped. It is the point of my open thread.
@vlad-perevezentsev's question on whether skill_sha must be portable across Linux and Windows waits for a decision; the author held it.
Checked, no issues
- Repo rules: all 24 non-merge commits carry a
Signed-off-by, the kind is inSCHEMA_VERSIONS,schema_compatagainst the base passes, the CLI snapshot and changelog fragment are present,growth_baseline.jsonis unchanged andtest_growth_ratchet.pypasses. - Link attribution: a fuzz over about 10,000 random trees with links found misses only in the two layouts above (a middle-wildcard link, and a skill that links to its own parent).
Bot points I checked that no longer apply at this head (40)
- exai-bot's point on the unescaped config filename does not apply: the path is glob-escaped at this head.
- exai-bot's point on case-insensitive suite resolution does not apply: globs match listed names case-sensitively at this head.
- exai-bot's point on symlinked skills in impact does not apply: a change at the link target maps to the skill at this head.
- exai-bot's point on nested skills does not apply: the inner directory leaves the outer hash, so only the inner skill moves and only it is named.
- exai-bot's point on selective suite globs does not apply: a suite that picks individual files is refused at load.
- exai-bot's point on suite symlink targets in impact does not apply: a change at a suite file's target maps to the suite at this head.
- exai-bot's point on nested ancestor links does not apply: suite links that reach an ancestor of the skill are refused at any depth.
- exai-bot's point on symlink loops does not apply: a loop is a ConfigError naming the link at this head.
- exai-bot's point on unguarded symlink tests does not apply: the Windows skip is in place at this head.
- Copilot's point on unguarded symlink tests does not apply: the Windows skip is in place at this head.
- Copilot's point on the config filename read as a glob does not apply: the path is escaped at this head.
- Copilot's point on symlink targets in impact does not apply: linked suite files and skill directories map to their subject at this head.
- Copilot's point on a suite claiming one skill file does not apply: such a suite is refused at load at this head.
- exai-bot's point on a config reached through a link does not apply: the config is shared at its declared path and at its target.
- exai-bot's point on deleted files under a linked suite directory does not apply: the suite's walked directories are in its reach.
- exai-bot's point on undecodable names in tree_digest does not apply: tree_digest encodes with os.fsencode at this head.
- exai-bot's point on a linked skill whose whole evals directory is deleted does not apply: the suite globs are rewritten to the resolved directory.
- exai-bot's point on a symlinked parent of a suite file does not apply: a change to the link itself claims the suite at this head.
- exai-bot's point on undecodable names in suite digests does not apply: both hash-update sites encode with os.fsencode at this head.
- exai-bot's point on intermediate links in a link chain does not apply: reach follows every hop of the chain at this head.
- exai-bot's point on ancestor links missing from the suite digest does not apply: each hop is hashed with its target text at this head.
- exai-bot's point on intermediate links of a linked config does not apply: the config's hops are claimed as shared at this head.
- exai-bot's point on a literal suite path that matches nothing does not apply: it still claims its links and the place it resolves to.
- exai-bot's point on a nested skill linked to a sibling directory does not apply: such a layout is refused at load at this head.
- exai-bot's point on
..collapsed after a link in tree_digest does not apply:link_textkeeps..for in-tree targets at this head. - exai-bot's point on the 1 MiB capture limit does not apply:
_tracked_modespassescapture_limit=Noneat this head. - exai-bot's point on
ls-filespathspecs does not apply: the call uses--literal-pathspecsat this head. - exai-bot's point on platform-dependent sorting in tree_digest does not apply: entries sort by path parts at this head.
- exai-bot's point on
..after a link in an absolute target does not apply:_hopsno longer normalizes the target at this head. - exai-bot's point on a wildcard suite that matches nothing does not apply for a wildcard in the last segment: its prefix links are claimed at this head.
- exai-bot's point on the order of directory aliases in the suite walk does not apply: children are walked by name at this head.
- exai-bot's point on a root link to itself does not apply: the target text is kept as written at this head.
- exai-bot's point on Windows extended-path targets in
link_textdoes not apply to the code:_plain_pathstrips the prefix at this head; the Windows run itself is not something I could check. - exai-bot's point on trailing
/and/.in absolute targets does not apply: both suffixes are kept in the link text at this head. - exai-bot's point on the tracked-mode tests under Windows does not apply: they set the index mode with
git update-index --chmod=+xat this head. - exai-bot's point on a wildcard suite where other files still match does not apply: the pattern under the resolved place is claimed at this head.
- exai-bot's point on an eval link under a linked skill does not apply: suite matches are also named by where their directory resolves at this head.
- exai-bot's point on a root link to a descendant of itself does not apply: a root link's target is kept as written at this head.
- exai-bot's point on case-sensitive containment on Windows is not something I could check: it needs a Windows run.
- exai-bot's point on the Windows tracked-mode tests does not apply to a gate:
windows-smokedoes not runtest_tree_digest.py.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| except OSError: | ||
| pass | ||
| return out |
There was a problem hiding this comment.
Must-fix: suite expansion treats permission failures as no matches. On POSIX as a non-root user, configure suite: evaluation/locked/tasks/*.json, create a task there, and chmod evaluation/locked to 000. subjects() succeeds with the empty-suite hash instead of raising ConfigError, because this handler discards the listing failure before _walk() can encounter it. This silently omits configured evaluations. Preserve the tested no-match behavior for defaults.skills if intended, but propagate permission/read errors during suite expansion (including the per-entry OSError handler above), and add coverage for an unreadable intermediate glob component.
There was a problem hiding this comment.
Fixed in 7b121d8: suite globs walk strictly, so an unreadable directory on the way is a ConfigError naming it. defaults.skills keeps its no-match behaviour (existing test).
Test: test_an_unreadable_directory_on_the_way_of_a_suite_glob_is_a_config_error (red before).
| if ".." in rest: | ||
| return "/".join([".."] * depth + rest) + slash or "." |
There was a problem hiding this comment.
Must-fix: commonpath() does not resolve .., so an absolute target /one/tree/../external/q.json is classified as inside /one/tree and rewritten to ../external/q.json. An otherwise identical tree at /two/tree with target /two/tree/../external/q.json then gets the same digest although the links name different external locations. This violates the external-target hashing contract and affects both tree_digest() and suite hashing through link_text(). Preserve the absolute text when traversal escapes the root, without lexically collapsing .. across symlinks, and add a regression test with these two checkout paths.
There was a problem hiding this comment.
Fixed in 7b121d8: a target whose .. leads out of the tree is hashed as written, so the two checkouts differ. A .. that comes back inside keeps the relative text.
Test: test_an_absolute_link_whose_dotdot_leaves_the_tree_is_hashed_as_written (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 54ffa03, which fix the nit from my review:
54ffa03:dokimi/tests/test_tree_digest.py:206: dropped@symlinksand the test now skips only ifos.symlinkraisesOSError.
Left as is:
dokimi/dokimi/repo_config.py:239: the nit aboutglob.escape,link_textdepth prefix and thecommonpathguard names no specific tests to add, so the fix is not certain and it stays.
make ci-local at 54ffa03: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: exai-bot on a link matched by a mid-glob wildcard is unclaimed once its target lacks the file (suite_hash moves, impact unclaimed); exai-bot on a skill linked to the repository root: skill_sha moves on README.md but the file is unclaimed; exai-bot on backslashes in POSIX changed-file names are rewritten to '/', so impact credits the wrong owner and a directory with a backslash breaks loading; exai-bot on _hops lacks the Windows extended-path normalization link_text has (read, not run); @vlad-perevezentsev on whether skill_sha must be portable across Linux and Windows; the author held it.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/runs/kinds.py # dokimi/tests/test_runs_scan.py
- a link a wildcard suite walks through claims its change even after its target loses the match (repo_glob.glob_links, in a new module that keeps repo_config under the length cap) - a skill that resolves to the repository root is a ConfigError - an absolute link target with the Windows extended-length prefix is matched as the plain path - a suite glob that cannot read a directory on its way is a ConfigError; skills globs still match nothing - a target whose ".." leads out of the tree is hashed as written; Windows link containment ignores case - the executable-bit tests assert that the index alone moves the digest, with the disk bit held still Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| for link in _glob_links(root, literal): | ||
| links.extend(_hops(link, root, root_real)) |
There was a problem hiding this comment.
Must-fix: links discovered after a wildcard contribute only their link locations, not the remaining pattern at their resolved targets. For example, declare suite: evaluation/*/task.json, create evaluation/current -> ../one and one/task.json, then delete one/task.json. Its deletion changes suite_hash, but subjects_for_changed(['one/task.json']) returns it as unclaimed: there are no current matches, and the only rewritten glob remains evaluation/*/task.json. The same gap affects evaluation/**/task.json. Preserve the remaining glob at each symlink target encountered by the glob walk, and add a deletion regression test so these changes still reach the suite.
There was a problem hiding this comment.
Fixed in ef9a0b1. A link a wildcard suite walks through now also keeps the rest of the pattern under the place it resolves to, so deleting one/task.json (behind evaluation/current -> ../one) claims it. Tests cover evaluation/*/task.json and evaluation/**/task.json; both failed before the change.
| if s.kind == "skill" and s.path != subject.path and _within(s.path, subject.path) | ||
| ] | ||
| # A skill directory that is a symlink is its target's content (a suite match is hashed by its link instead). | ||
| skill_sha = "sha256:" + tree_digest((root / subject.path).resolve(), exclude=[*inside, *nested]) |
There was a problem hiding this comment.
Must-fix: an unreadable non-suite subdirectory is silently omitted from skill_sha. For example, declare path: skills/a, add skills/a/scripts/run.py, and chmod scripts to 000 as a non-root user. subjects() still succeeds because tree_digest() uses Path.rglob(), which suppresses permission errors while descending directories; subsequent edits behind that directory do not move the digest. The except OSError in subjects() cannot catch an error that the walk suppresses. Use a strict walk for skill hashing (while pruning excluded suite/nested-skill directories), and add a permission test for a skill directory rather than only suite directories.
There was a problem hiding this comment.
Fixed in ef9a0b1. The skill digest now walks with a strict scandir, pruning excluded suite and nested-skill directories, so an unreadable directory raises and subjects() reports a ConfigError naming it. Permission test for a skill directory added (skipped as root, Windows).
| except OSError as exc: | ||
| if strict and exc.errno not in _MISSING: | ||
| raise |
There was a problem hiding this comment.
Must-fix: a self-referential symlink is dropped even when the final segment matches its name. For example, with evaluation/loop -> loop and suite: evaluation/* (or evaluation/loop), entry.is_dir() raises ELOOP and this handler omits the entry, so subjects() accepts an empty suite instead of raising the ConfigError enforced by _check_match() for looping suite links. Retargeting a previously matched link into a loop can therefore silently remove it from the suite. Preserve symlink entries when target classification fails with ELOOP (without descending them), so terminal matches reach suite validation; add a test for both literal and wildcard terminal segments.
There was a problem hiding this comment.
Fixed in ef9a0b1. A self-referential link stays in the listing (not dropped on ELOOP), so the suite check refuses it for literal and wildcard suites. Also: on Python 3.13+ resolve() returns a loop as itself, so _real now detects a loop with stat. Tests for both forms.
| if seg == "**": | ||
| _glob_walk(base, segs, i + 1, found, strict=strict, links=links) |
There was a problem hiding this comment.
Must-fix: the zero-directory branch accepts a regular-file base. For an existing evaluation/q.json, glob_paths(root, "evaluation/q.json/**") returns q.json, although ** is defined here to match directories only. _suite_files() does not filter this result, so a directory-oriented suite pattern unexpectedly hashes that file (or raises the individual-skill-file rejection when used inside a skill). Ensure the base is a directory before expanding **, preserving strict error handling, and add a regression test for a regular file followed by /**.
There was a problem hiding this comment.
Fixed in ef9a0b1. ** no longer takes its zero-directory match on a regular file, so evaluation/q.json/** matches nothing. Regression test added; it returned q.json before.
| load_repo_config(root).subjects() | ||
|
|
||
|
|
||
| def test_a_link_target_with_the_windows_extended_prefix_is_matched_as_the_plain_path(tmp_path, monkeypatch): |
There was a problem hiding this comment.
Must-fix: this test creates two real symlinks without the Windows guard used by the neighboring tests. On Windows without Developer Mode or symlink privileges, the first os.symlink() raises OSError (WinError 1314) before the assertion. Add the same platform skip, or mock symlink detection and readlink so the prefix-handling test does not require symlink privileges.
Suggestion:
| def test_a_link_target_with_the_windows_extended_prefix_is_matched_as_the_plain_path(tmp_path, monkeypatch): | |
| @pytest.mark.skipif(os.name == "nt", reason="creating symlinks needs privileges on Windows") | |
| def test_a_link_target_with_the_windows_extended_prefix_is_matched_as_the_plain_path(tmp_path, monkeypatch): |
There was a problem hiding this comment.
Fixed in ef9a0b1. Added the same skipif(os.name == "nt") marker as the neighbouring symlink tests.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/docs/reference/cli-interface.json # dokimi/dokimi/artifacts.py
- a link met by a wildcard suite keeps the pattern's rest under its target, so a deleted file there still claims - a self-referential link is listed, so the suite refuses it (3.13+ resolves a loop to itself: stat tells it apart) - ** does not match a regular file as its zero-directory case - the skill digest walks strictly: an unreadable directory is a ConfigError, not left out of skill_sha - the symlink test that creates links is skipped on Windows Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| seg = segs[i] | ||
| more = i + 1 < len(segs) # a link is read through only when a segment follows it | ||
| if seg == "**": | ||
| if more or not base.is_file(): # ** matches directories only: a file is not a zero-directory match |
There was a problem hiding this comment.
Must-fix: not base.is_file() also accepts non-directories such as FIFOs and Unix sockets. On POSIX, create evaluation/q with os.mkfifo(); glob_paths(root, "evaluation/q/**") returns that FIFO instead of [], despite ** matching directories only. Inside a skill, this false match also makes _check_claims() reject an otherwise non-matching suite as an individual-file claim. Check directory status for the terminal zero-directory branch and extend the existing regular-file regression test with a POSIX special-file case.
Suggestion:
| if more or not base.is_file(): # ** matches directories only: a file is not a zero-directory match | |
| if more or base.is_dir(): # terminal ** matches directories only |
There was a problem hiding this comment.
Real: reproduced with os.mkfifo, evaluation/q/** returned the FIFO.
Fixed in 4b18921: the terminal zero-directory branch now checks base.is_dir() (your suggestion).
New test test_a_fifo_followed_by_a_double_star_is_not_a_match failed before the fix, passes after.
…kets Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| if resolved != declared: # a linked skill: its suite globs, rewritten to where the files live | ||
| moved = [resolved + g[len(declared):] for g in globs if g == declared or g.startswith(declared + "/")] | ||
| globs = (*globs, *moved) | ||
| skill_links.extend(_hops(root / subject.path, root, root_real)) |
There was a problem hiding this comment.
Must-fix: collect skill link hops even when the resolved skill is outside the repository. For example, declare path: linked/a with suite: evaluation/*.json, and let the in-repository linked symlink point to an external directory containing a/SKILL.md. This layout is accepted and hashed, but retargeting linked to another external directory with different skill content makes subjects_for_changed(['linked']) return it as unclaimed: _inside() returns None, so this branch never records the link. Move hop collection outside the real_skill is not None branch (guarded by subject.kind == 'skill'), or explicitly reject external skill targets, and add a regression test.
| if real_skill is not None and _under(hop, real_skill) and hop not in declared_links and not in_suite: | ||
| skill_links.append(hop) |
There was a problem hiding this comment.
Must-fix: a declared intermediate suite link inside the skill can also change skill_sha. For example, declare path: skills/a and suite: skills/a/refs/current/tasks, with refs/current linking to common/one (and later common/two), both containing tasks/q.json. This layout is accepted, and tree_digest retains the refs/current symlink because the exclusion is only refs/current/tasks. Retargeting it therefore changes both hashes, but this condition excludes it from skill_links because it is a declared link; subjects_for_changed(['skills/a/refs/current']) reports only the suite. Preserve skill ownership for intermediate links inside the skill that are not themselves excluded suite matches, regardless of whether they occur on the declared path, and add a regression test.
| files = _suite_files(root, subject.suite_globs) | ||
| if subject.kind == "repo": | ||
| assert subject.manifest is not None # set by _subject for kind repo | ||
| files.setdefault(subject.manifest, root / subject.manifest) |
There was a problem hiding this comment.
Must-fix: avoid adding the manifest separately when a matched suite directory already covers it. For example, with manifest: evaluation/repo.yaml and suite: evaluation, _digest_files() hashes evaluation/repo.yaml once while walking the directory and again as this added entry. Listing the same suite files individually instead hashes the manifest only once, producing a different suite_hash for identical file membership and content. Apply the ancestor deduplication used by _suite_files() after adding the manifest, and add a regression test comparing these equivalent suite specifications.
| assert tree_digest(a) == tree_digest(b) | ||
|
|
||
|
|
||
| def test_a_tracked_file_whose_name_has_brackets_takes_its_mode_from_the_index(tmp_path): |
There was a problem hiding this comment.
Must-fix: when Git is not installed/on PATH, this test and test_a_tracked_mode_past_the_capture_limit_still_comes_from_the_index fail at subprocess.run(["git", ...]) with FileNotFoundError, rather than skipping. There is no shared Git availability guard in conftest. Add the same shutil.which("git") skip marker used by test_the_executable_bit_is_the_one_git_records_not_the_disk_one to both tests so this utility's tests also run in its supported no-Git environment.
Suggestion:
| def test_a_tracked_file_whose_name_has_brackets_takes_its_mode_from_the_index(tmp_path): | |
| @pytest.mark.skipif(shutil.which("git") is None, reason="the mode bit comes from git's index; needs git") | |
| def test_a_tracked_file_whose_name_has_brackets_takes_its_mode_from_the_index(tmp_path): |
Summary
Step 1 of #318: a repository declares its dokimi subjects (skills, or the repository itself), where each one's evals live, plan defaults, the
sharedsmoke paths and the budget and baseline store, in one config file. This PR adds the schema and a library loader. Nothing reads the file yet.Changes
schemas/dokimi_config.v1.json, registered inSCHEMA_VERSIONSasdokimi_config. The version key isschema_version, not theschema:key of the issue sample:save_artifactstampsschema_version, and the schema forbids extra keys. Deviation from the issue, on purpose.dokimi/repo_config.py: discovery (explicit path, then.github/dokimi/config.yaml, then.dokimi/config.yaml, under the nearest.gitabove the start), validation, subject and suite resolution,skill_sha,suite_hash, andsubjects_for_changed(paths)returning the unclaimed paths separately.dokimi.utils.tree_digest, moved fromcommands/repo_run.py. Same output for a directory without exclusions, so the build-context tag does not change.runs/kinds.py, the scan and show test rows, the import-architecture component, thecli-interface.jsonsnapshot (one added'dokimi_config'choice in--kind, frommake cli-snapshot), and a fixture.changelog.d/.Design choices
skill_shaleaves out the inner skill's directory, so editing the inner skill moves only the innerskill_sha, and changed-file impact names only the inner skill (the deepest owner). An outer skill that depends on the inner one's files declares that withshared:.evals(the default), or any other directory it names whole, such as{skill}/tests/**. Those directories leaveskill_sha, so editing an eval movessuite_hashand not the skill. A suite that picks an individual file of its skill (SKILL.md, a script, a reference, one eval file) is refused at load, with the subject id and the file named. A suite that contains the skill directory, or whose directory links to the skill, is refused too, and so is any link in a suite that resolves into its skill. A skill directory that is a symlink is hashed by its target's content.{skill}/evals/*.json, is refused for now, like any other file claim. Whether individual eval files may be claimed is the author's call.baseline.store: false(what an unquotedoffreads as) is accepted and meansoff. A budget number that PyYAML reads as text (1e3,1e-7) is refused with the exact plain decimal (1000,0.0000001) and the1.0e+3spelling in the message; text that is no number at all is a schema type error. A budget of.nanor.infis refused: it would disable the cap check.suite_hash.Evidence
test_process.py::test_sighup_kills_the_live_groups_tootimed out under load in this run and passed alone.check_comment_blocks.pyagainst intel/main,check_growth.py,audit_test_quality.py,check_changelog_fragment.py: pass.tests/test_repo_config.py(discovery order, both layouts, schema refusals, subject resolution, hashes, changed-file impact, nested skills, whole-directory suites,store: false, budget numbers),tests/test_tree_digest.py.store: false; budget text hints; non-number budget text; non-finite budgets; symlinked evals.pr-reviewlocal mode) of each round, on the frozen trees.Merge danger
defaults,subjects,shared,plan,budget,baseline) and theschema_versionkey are the one-way door. Review them before merge.tree_digestis shared with the build context. The change moves the function; the output is unchanged for a directory without exclusions, which the existing repo_run tests cover.UnicodeEncodeErrorfrom the digest, not aConfigError. The build context has the same encode; it is rare and left as is for this step.subjects:blocks) keeps the last one, as PyYAML does; a subject can vanish silently. Refusing duplicate keys needs a loader change, left for a follow-up.Part of #318.
🤖 Generated with Claude Code