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'", "'eval'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'repo_budget'", "'repo_environment'", "'repo_readiness'", "'result'", "'subject_scorecard'", "'task_inspect'", "'task_runs'", "'tool_preference'"] -> ["'answers'", "'arms'", "'cli_routes'", "'dokimi_config'", "'eval'", "'explore_runs'", "'guidance_reach'", "'matrix_rollup'", "'novelty'", "'plan'", "'plugin_delta'", "'questions'", "'repo_budget'", "'repo_environment'", "'repo_readiness'", "'result'", "'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.06% 94.05% -0.01%
==========================================
Files 297 298 +1
Lines 38018 38580 +562
Branches 6424 6546 +122
==========================================
+ Hits 35760 36286 +526
- Misses 1451 1474 +23
- Partials 807 820 +13
🚀 New features to boost your workflow:
|
|
🔍 OpenCodeReview found 3 issue(s) in this PR. By severity: 3 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 config file is shared: a change to it reruns the smoke set. Empty when it sits outside the root. | ||
| It is a literal path, but ``shared`` is matched as a glob, so its metacharacters are escaped.""" | ||
| try: | ||
| return [glob.escape(config.path.resolve().relative_to(config.root.resolve()).as_posix())] |
There was a problem hiding this comment.
Must-fix: resolving the config path here loses the declared symlink path. For .github/dokimi/config.yaml -> ../../ci/config.yaml, subjects_for_changed(['.github/dokimi/config.yaml']) returns that change as unclaimed, including when the link is retargeted to a different config. Only ci/config.yaml is registered as shared, so a configuration change misses the promised smoke-set trigger. Register both the declared absolute path (without resolving symlinks) and its resolved target when they are inside the root, escaping both for glob matching; test changes to both paths.
There was a problem hiding this comment.
Fixed in c80ce22. The config is shared under its declared path and under the file it resolves to, each glob-escaped, so a link like .github/dokimi/config.yaml -> ../../ci/config.yaml triggers the smoke set at either path. Test: test_a_config_reached_through_a_link_is_shared_at_its_declared_path_and_its_target.
| for rel, path in _suite_files(root, subject.suite_globs).items(): | ||
| for _, entry in _walk(path, rel): | ||
| real = _inside(entry, root_real) | ||
| if real is not None: | ||
| suite_roots.append(real) |
There was a problem hiding this comment.
Must-fix: resolved suite reach omits ordinary directories because _walk returns only files and symlinks. With skills/a -> ../vendor/a, the default suite, and vendor/a/evals/q.json deleted while evals still exists, subjects_for_changed(['vendor/a/evals/q.json']) reports a skill change rather than a suite change. The declared glob does not match the target path, and the deleted file is no longer in this walk, although its deletion changes only suite_hash. Register resolved suite directories as well as files so their deleted descendants remain covered, and add a regression test. The same gap applies to suite directories reached through a symlinked parent.
There was a problem hiding this comment.
Fixed in c80ce22. Suite reach now includes the directories the suite walks, so a deleted file under a directory that still exists maps to the suite (and not the skill), including through a symlinked parent. Test: test_a_deleted_file_in_a_linked_suite_directory_still_maps_to_the_suite.
| if any(rel == e or rel.startswith(e + "/") for e in skip): | ||
| continue | ||
| if path.is_symlink(): | ||
| h.update(f"L {rel}\0{os.readlink(path)}\0".encode()) |
There was a problem hiding this comment.
Must-fix: filesystem names and link targets can contain surrogate-escaped bytes on POSIX. For example, creating a file with os.open(os.fsencode(root) + b'/bad-\xff', os.O_CREAT | os.O_WRONLY, 0o644) makes tree_digest(root) raise UnicodeEncodeError at the file header; a link target containing that byte fails here too. Although inherited from the old build-context helper, this now also breaks the new skill-hashing API, whose caller catches only OSError. Encode filesystem metadata with os.fsencode() (or an explicit surrogateescape policy) in all three header branches and add POSIX regression tests for undecodable filenames and link targets.
There was a problem hiding this comment.
Fixed in c80ce22. tree_digest encodes names and link targets with os.fsencode, so an undecodable POSIX name or link target digests without UnicodeEncodeError; valid names keep the same bytes. Test: test_an_undecodable_name_and_link_target_digest_without_error (POSIX only).
… undecodable names - The config file is shared under its declared path and under the file it resolves to, so a config linked from elsewhere in the repository counts at either path. - A suite's reach includes the directories it walks, so a deleted file under a directory that still exists maps to the suite, not the skill. - tree_digest encodes names and link targets with os.fsencode, so an undecodable POSIX name or link target digests instead of raising UnicodeEncodeError. Valid names keep the same bytes. Tests: red on b4504b0 for the three, green here. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| for rel, path in _suite_files(root, subject.suite_globs).items(): | ||
| for _, entry in _walk(path, rel, dirs=True): |
There was a problem hiding this comment.
Must-fix: resolved suite reach still depends on the suite directory existing. Using defaults.skills: skills/* and skills/a -> ../vendor/a, remove the entire vendor/a/evals directory, then call subjects_for_changed(['vendor/a/evals/q.json']). _suite_files now returns no matches, so the default skills/a/evals glob cannot match the target-side path and the deleted eval is reported in skills rather than suites. The existing regression keeps evals/.keep, so it does not cover this case. Translate changed paths through declared/resolved skill aliases before matching suite globs (including nonexistent paths), and add a whole-directory deletion regression.
There was a problem hiding this comment.
Fixed in 5a23ae0. A linked skill keeps its suite globs rewritten to its resolved directory, so deleting vendor/a/evals now still routes vendor/a/evals/q.json to skill/a suite. Pinned by test_a_linked_skill_still_claims_its_evals_after_their_whole_directory_is_deleted.
| for rel, path in _suite_files(root, subject.suite_globs).items(): | ||
| for _, entry in _walk(path, rel, dirs=True): |
There was a problem hiding this comment.
Must-fix: reach omits symlinks in parent components of a matched path. For a skill with suite: evaluation/current/task.json, let evaluation/current -> ../one, with different task contents in one/task.json and two/task.json. Retargeting current to ../two changes suite_hash, but subjects_for_changed(['evaluation/current']) reports only an unclaimed change: the declared glob requires task.json, and this walk records only the resolved file, not the parent link. The same omission affects manifests and skills beneath symlinked parents. Track the ancestor symlink paths as dependencies and add a retargeting regression test so the actual Git-changed link reaches each affected subject.
There was a problem hiding this comment.
Fixed in 5a23ae0. A symlink on the declared way to a suite file, a manifest or a skill directory now maps a change to that link itself, so retargeting evaluation/current moves the subject. Pinned by test_retargeting_a_link_on_the_way_to_a_suite_file_claims_the_link_itself, and test_retargeting_a_link_above_a_skill_claims_the_skill_by_its_link for the skill side.
| if path.is_dir(): | ||
| continue # a linked directory: its files are walked and hashed one by one | ||
| target = path.resolve() if path.exists() else path | ||
| h.update(f"{name}\0{tree_digest(target)}\n".encode()) |
There was a problem hiding this comment.
Must-fix: these UTF-8 encodings still crash on valid POSIX filesystem bytes, despite the os.fsencode fix in tree_digest. With the default suite, create skills/a/evals/bad-\xff using os.open(os.fsencode(evals) + b'/bad-\xff', ...); subjects() raises UnicodeEncodeError when encoding name here. A suite symlink with a target containing b'\xff' fails at the preceding header too. Use os.fsencode for filesystem names and link targets at both hash-update sites, and extend the undecodable-byte regression test to RepoConfig.subjects().
There was a problem hiding this comment.
Fixed in 5a23ae0. Suite digests encode names and link targets with os.fsencode (both hash-update sites), so the undecodable-byte case no longer raises in subjects(). Pinned by test_undecodable_names_and_link_targets_under_a_suite_load_and_digest.
There was a problem hiding this comment.
Review at c80ce22. Two things are worth fixing before merge, and the three bot points I confirm below block the approval. The earlier rounds' fixes hold: I reran the claims in the description (store false, budget hints, nan/inf, id clash, individual-file suites refused, discovery order) and they behave as stated.
What I ran, on a Linux review host at c80ce22:
make ci-local: 30 passed, 0 failed, 7 skipped (network/Docker/other-Python legs); changed-line coverage gate passed.- Reproductions through the loader for the impact and hashing points, with the LLM transport mocked.
- About 85 deliberate breaks of
repo_config.pyagainsttest_repo_config.pyandtest_tree_digest.py.
Findings that cannot be anchored inline:
- Two gate lines in
_hashedandsuite_hashhave no test.repo_config.py:492: a mutant that leaves the file name out of the suite digest passes the full suite, so renaming an eval with the same content keepssuite_hashand a baseline keyed on it is reused.repo_config.py:613-619and:532: astartswithinstead of a path-component check passes too; with skillsskills/aandskills/abcd, an edit toskills/a/cdthen stops movingskill/a'sskill_sha(False under the mutant, True in the real code). 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 unpinned. Mutants that lowercase both sides in the glob walk and in_match, or that make**need at least one directory (shared: ['**/Makefile']then leaves the rootMakefileunclaimed), pass the suite. Add a test withskills/a/Evalsnext toevals, and one with**/Makefileagainst a root file.
Open bot points I confirm:
- I agree with exai-bot that deleting a linked skill's whole evals directory is attributed to the skill, not the suite. New evidence: with skills/a -> ../vendor/a and the default suite, vendor/a/evals/q.json deleted gives skills={'skill/a': [...]}, suites={}, while suite_hash moves to the empty digest.
- I agree with exai-bot that retargeting a symlinked parent of a suite file is unclaimed. New evidence: with suite evaluation/current/task.json and evaluation/current -> ../one, a change to evaluation/current gives unclaimed=['evaluation/current'].
- I agree with exai-bot that suite hashing still raises UnicodeEncodeError on undecodable bytes. New evidence: a suite file named q\xff.json makes subjects() raise it, as does a suite link whose target holds \xff; the same name in the skill outside the suite hashes fine.
@vlad-perevezentsev's question on whether skill_sha must be portable across Linux and Windows waits for the author.
Checked, no issues
- Digest move: the old
context_digestand the newtree_digestgive identical bytes on a tree with an executable, a non-ASCII name, relative, dangling and self links and an empty directory, so existing build-context tags stay valid. - Schema and rules:
schema_compatagainst the base passes, the kind is inSCHEMA_VERSIONS, the CLI snapshot and changelog fragment are present, and no baseline ingrowth_baseline.jsonis exceeded. - Config errors: an empty, list, non-UTF-8 or NUL-byte config file, absolute,
..and drive paths, and malformed**globs give aConfigError.
Bot points I checked that no longer apply at this head (16)
- 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 the inner skill alone moves and alone 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 while the directory exists: 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.
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.
| 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): |
| 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.
| 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.
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.
| 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.
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