Repository navigation
Conversation
A nested repository inside a skill or suite now applies its own ignore rules (one git ls-files per nested repository, blob ids as in the parent). Its ignored files no longer move skill_sha or suite_hash; a tracked or untracked change that git keeps still does, and a nested .git is still skipped. A suite may claim individual files under the skill's evals directory (default evals). Those files leave skill_sha and move suite_hash. Individual claims of SKILL.md, scripts, references or any other file of the skill stay refused. Follow-up to intel#323. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
Untracked nested repositories can omit ancestor directories from the strict skill walk, leaving skill_sha unchanged after file edits; ancestor handling and an intermediate-directory regression test are needed.
1 open issue
What changed in this PR
1 new must-fix finding.
This follow-up to #323 refines repository-config hashing for nested repositories and individual eval files, supporting the per-subject evaluation work in #318.
Changes:
- Apply nested repositories’ own ignore rules when computing hashes.
- Allow individual suite file claims under
{skill}/evals, while retaining refusals elsewhere. - Update regression tests, schema documentation, and the unreleased changelog.
| File | Description |
|---|---|
dokimi/tests/test_repo_config.py |
Tests individual eval claims, hash separation, impact, and refusals. |
dokimi/tests/test_repo_config_git_blobs.py |
Tests ignored and retained changes inside nested repositories. |
dokimi/dokimi/utils.py |
Adds cached nested-repository filtering for tree digests. |
dokimi/dokimi/schemas/dokimi_config.v1.json |
Documents the individual eval-file claim rule. |
dokimi/dokimi/repo_config.py |
Permits individual claims under a skill’s evals directory. |
dokimi/changelog.d/20261030_180000_napetrov_feat-repo-config.md |
Describes both updated hashing and suite-claim rules. |
Review effort: Balanced
| if text.endswith("/"): # an untracked repository, as ls-files gives it: a directory | ||
| dirs.add(path) | ||
| continue | ||
| files.add(path) | ||
| parts = path.split("/") | ||
| dirs.update("/".join(parts[:i]) for i in range(depth + 1, len(parts))) |
There was a problem hiding this comment.
Fixed in c567e37: every ancestor of a listed nested entry is now kept, so the walk reaches an untracked repository inside a nested one. Red test: test_a_change_inside_an_untracked_repository_under_a_nested_repository_moves_a_hash.
|
OpenCodeReview: Commit a2246b2 was not reviewed. This PR has reached its review limit (6 reviews). CI at `a2246b2`: 7 still running, 7 passing, 2 skipped
To review this commit: re-request review from exai-bot in the Reviewers list. <details><summary>Last review (8d6f05c): no findings</summary> Open threads from the bot: 0 </details> |
| if text.endswith("/"): # an untracked repository, as ls-files gives it: a directory | ||
| dirs.add(path) | ||
| continue |
There was a problem hiding this comment.
Must-fix — missing ancestor directories for untracked nested repositories. Trigger: initialize a repository at skills/a/tools, then another untracked repository at skills/a/tools/vendor/child, with vendor containing no ordinary allowed files. Evidence: Git lists vendor/child/; this branch adds child but skips the ancestor-population logic below. Consequently skipped(vendor) is true, and tree_digest(..., strict=True), used for skill_sha in repo_config.py, prunes vendor before reaching child. Impact: edits to child’s non-ignored files do not change skill_sha. Populate ancestor directories for directory entries as well as file entries, and add a regression test for this layout. Workaround: put child directly under tools or add a non-ignored ordinary file under vendor. Severity justification: medium, a bounded but material hash-correctness failure.
There was a problem hiding this comment.
Fixed in c567e37: every ancestor of a listed nested entry is now kept, so the walk reaches an untracked repository inside a nested one. Red test: test_a_change_inside_an_untracked_repository_under_a_nested_repository_moves_a_hash.
A nested repository whose git call fails (safe.directory, a timeout, a broken or missing .git) used to be hashed without its ignore rules, silently. Now the nested .git is passed to git as its git dir, so a .git git cannot read fails there instead of git walking up to the enclosing repository, and any failed git call raises an OSError that the config load turns into a ConfigError naming the repository path and git's message. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #388 +/- ##
=======================================
Coverage 94.78% 94.78%
=======================================
Files 353 354 +1
Lines 46527 46598 +71
Branches 7967 7976 +9
=======================================
+ Hits 44100 44169 +69
- Misses 1540 1541 +1
- Partials 887 888 +1
🚀 New features to boost your workflow:
|
…d one A nested repository lists an untracked repository inside it as a directory (vendor/inner/). The entry's own directory was kept, but its ancestors (vendor/) were not, so the strict skill walk skipped vendor and never reached the files under it. Keep every ancestor of a listed entry, as is already done for tracked files. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… too A root with a .git, or inside a repository, that git cannot read raised nothing and hashed the bytes on disk, unlike a nested repository. Now git_tree raises OSError naming the path and git's message, which the config load reports as a ConfigError. A root with no .git above it on its filesystem (no work tree) still hashes its bytes. Test fixtures that faked a repository with an empty .git now run git init. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| own words (no silent fallback: the digest would count other bytes than git stores).""" | ||
| if not _in_work_tree(base): | ||
| return None | ||
| top_dir = os.fsdecode(_work_git(base, ["rev-parse", "--show-toplevel"], 60).stdout_bytes).strip() |
There was a problem hiding this comment.
Must-fix — bind discovery to the nearest .git marker. Trigger: initialize an outer repository and an inner repository, remove inner/.git/HEAD, then call git_tree(inner). Evidence: _in_work_tree accepts the inner marker, but ordinary git -C inner rev-parse --show-toplevel skips the invalid repository directory and discovers the readable outer repository; unlike _nested_allowed, this call does not supply --git-dir. Impact: the promised unreadable-work-tree error is replaced by an outer-tree snapshot, potentially applying the wrong ignore rules and blob metadata. Workaround: repair the inner repository or move it outside the outer work tree. Return the nearest marker from discovery and explicitly validate it with --git-dir before accepting the work tree; add a regression with a readable parent and broken inner HEAD. Severity justification: medium because this is a bounded but material hashing and error-contract failure.
There was a problem hiding this comment.
Fixed in ecbf85b. git_tree now takes the nearest .git (_work_marker) and reads it alone with --git-dir/--work-tree before accepting the work tree.
A broken inner HEAD now raises the same rev-parse failed OSError as the other broken-repo cases, instead of hashing the outer tree.
Regression test_a_broken_inner_repository_under_a_readable_one_is_an_error_not_the_outer_tree: red before the fix, green after.
Full suite: only the 4 known test_repo_typecheck_grader failures.
git_tree found the work tree with a plain rev-parse, which skips a .git that git cannot read and returns an outer repository. A nested repository with a broken HEAD was then hashed as part of the outer tree. The nearest .git is now read alone (--git-dir/--work-tree), and a failure raises the same OSError as the other broken-repo cases. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A nested repository inside a nested repository follows the innermost repository's ignore rules. A submodule inside a skill is hashed by its checked-out files, not by the gitlink commit. A repository owned by another user (git's safe.directory refusal, simulated with GIT_TEST_ASSUME_DIFFERENT_OWNER on the calls that name it) raises ConfigError. The three pass on the code as it was: no production change. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…tory wildcard An empty safe.directory entry on the refused calls clears any allow-list, so the test fails the same way on a runner whose global config is safe.directory = * (CI red on py3.11 and py3.14: DID NOT RAISE). Red reproduced locally with HOME set to such a config. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
The nested-repository logic holds up: ignore rules, negations, submodules, nested-in-nested and the new git-failure errors all behave as described, and the three earlier bot points are fixed at this head. One test gap on the new evals claim rule is worth closing before merge, and there is one nit inline.
What I ran, on a Linux review host at ef5fa63:
make ci-local: 28 passed, 2 failed, 7 skipped; 15,610 tests passed; changed-line coverage passes. The two failures are not the PR's.test_unbalanced_quote_in_hook_completes_quicklymissed its 1 s limit (2.1 s) while the machine was loaded; it passes alone, together with the rest oftest_repo_environment.pyandtest_audit_test_quality.py(680 passed). The benchmark step reports 40 docs underdocs/against a maximum of 39; this PR changes no docs file.- Reproductions for the findings, with no model or network access.
- A mutation run of 36 mutants over
utils.pyandrepo_config.pyagainst the six repo-config and tree-digest test files. The new tests fail on the base modules (14 failed). Most mutants of the ancestor directories, thels-filesflags, the innermost-repo choice, the--git-dirpin and the error raising are killed; seven survive, and the one that decides a verdict is inline. - All seven commits carry a
Signed-off-by.
Checked, no issues
- Ignore rules: negations, root-anchored patterns, spaces, newlines and unicode names, tracked files that match an ignore, a
.gitfile in a submodule, a linked worktree and a sparse checkout give the expected skipped set. - Error paths: the new
OSErrorfromgit_treeandtree_digestreaches the config load as aConfigError; a broken.gitinside an excluded directory does not raise. - Claims:
evalsas a link into the skill,Evals,evals-oldand..are all refused.
Bot points I checked that no longer apply at this head (3)
- Copilot's point on the missing ancestor directories of an untracked repository inside a nested one does not apply: every ancestor of a listed entry is kept at this head.
- exai-bot's point on the same missing ancestors does not apply: the ancestor loop in
_nested_allowedcovers slash-terminated entries, and the mutation run killed its removal. - exai-bot's point on binding discovery to the nearest
.gitdoes not apply:git_treepins the nearest marker with--git-dirand--work-tree, and a broken inner repository now raises.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Thread: PRRT_kwDOVBg7g860EqNK Review: PRR_kwDOVBg7g88AAAABWNNG7Q#1 A prefix match on the evals directory (evals-old) or on a claimed directory name (ref, against refs.md) passed every test. Both boundaries are now pinned: each mutant fails one of the new cases, and head passes them. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Thread: PRRT_kwDOVBg7g860EqNT: already fixed in bf991bc Review: PRR_kwDOVBg7g88AAAABWNNG7Q#2: already fixed in bf991bc Review: PRR_kwDOVBg7g88AAAABWB1DIw#1: already fixed in c567e37 bf991bc (keep spaces in a nested repository's directory name) is the reviewer's own commit; c567e37 keeps the ancestors of an untracked repository in a nested one. Empty commit: no code change. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…config-followup-323 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Review: PRR_kwDOVBg7g88AAAABWB1DIw: already fixed in c567e37 Review: PRR_kwDOVBg7g88AAAABWB1jpQ: already fixed in c567e37 Review: PRR_kwDOVBg7g88AAAABWDU_sw: already fixed in ecbf85b Review: PRR_kwDOVBg7g88AAAABWNNG7Q: already fixed in bf991bc Empty commit: no code change. The review bodies are covered by their inline threads (c567e37, ecbf85b) and by the evals and nit points (1c7ae48, bf991bc). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Test gap: fixed in 1c7ae48. Nit: fixed in bf991bc (antonwolfy). Already fixed in c567e37 (ancestors kept), with a regression test. Its point (ancestor directories) is fixed in c567e37. Record: dc7b0aa. Its point (nearest .git) is fixed in ecbf85b, with a regression test. Not a code claim: the review-limit notice for bf991bc. Nothing to change. Not a code claim: the Codecov install notice. Nothing to change. |
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at dc7b0aa. The nested ignore rules, the evals claim rule and the new git errors work as described. Two cases are regressions from the base: a nested repository's allowlist drops a file whose name ls-files spells differently (core.ignorecase), and it drops a file the top tracks. The rest are small gaps, all in the line comments. #390 changes the same code with a denylist and conflicts with this branch in utils.py, so one of the two needs to be rebased onto the other.
What I ran, at dc7b0aa:
make ci-local --offline: 19 passed, 0 failed, 13 skipped (network and Docker steps).- The PR's five test files on the merge-base code: 14 of their tests fail there, and they pass at this head.
- Repros on the head and on the base, with no model or network: the case-only rename under
core.ignorecase, a top-tracked file under a nested repository that ignores it, a top directory whose name ends in a space, and a required clean filter that fails. tree_digestover 5,000 tracked files: 0.77 s against 0.60 s on the base, from the.gitlookup on each ancestor. That is acceptable at skill sizes.- On Windows I could not run anything: the VM's venv did not resolve from the lock (
litellmagainstfilelock), and this PR does not change the lock.
Checked, no issues
- Earlier threads: the ancestors of an untracked repository inside a nested one are kept (the ancestor loop runs before the slash check), and
git_treepins the nearest.gitwith--git-dir/--work-tree, so a broken inner repository raises. - Claims: the
evals-oldandrefs.mdcases pin the boundary of theevalsallowance; directory claims are unchanged. - Error path: every new
OSErrorreachessubjects()and leaves it as aConfigError. The onlygit=Truecaller is the config loader. - Docs and changelog: the unreleased
feat-repo-configfragment and the schema description follow the code. The schema change is in a description only.
…config-followup-323
… win; git errors raise Address review (vlad-perevezentsev, round 2): - a nested repository is read with ls-files --others --ignored --directory, as the top is, so a case-only rename under core.ignorecase keeps counting - a path the top's index lists is never dropped by a nested repository's rules - git_tree keeps a trailing space in --show-toplevel - hash-object goes through _work_git and raises on failure (a required clean filter) - _nested_git is gone; the nested calls use _work_git Thread: PRRT_kwDOVBg7g860MHqZ Thread: PRRT_kwDOVBg7g860MHqk Thread: PRRT_kwDOVBg7g860MHqp Thread: PRRT_kwDOVBg7g860MHq3 Thread: PRRT_kwDOVBg7g860MHq9 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Thread: PRRT_kwDOVBg7g860MHqZ Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Thread: PRRT_kwDOVBg7g860MHq9 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
git_tree stripped --show-toplevel's whole output, so such a top read as no work tree; strip the newline only. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
…ository's rules git -C walked past a .git it cannot read (no HEAD) and listed the outer repository's ignored paths. Check that git reads the nested directory as its own top first; its message goes into the error. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
Merged
|
rstrip("\n") also removed a newline that ends the directory name, so a
readable nested repository named that way failed the config load, and a
top named that way was hashed without its ignore rules.
Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at a2246b2. With #390 on main, the branch now uses #390's GitTree. That settles the question of which of the two lands first. The code is right. The port adds an error for a nested .git that git reads as another repository, the space and newline fix in --show-toplevel, and the evals claim rule. All five points from my first round are fixed or no longer apply.
Agreed with @xaleryb: the PR body still says that a broken top .git, a garbage index and a failing clean filter give a ConfigError, and it names three tests that are gone. The subjects of d417932 and 728837b say the same. After the merge of main these cases hash the bytes on disk, and only a nested repository git cannot read is an error. Please update the body before the squash.
What I ran, at a2246b2:
make ci-local --offline: 19 passed, 0 failed, 13 skipped (network and Docker steps).- The PR's
test_repo_config.pyandtest_repo_config_git_blobs.pyon the merge-base code: 8 tests fail there (theevalsclaim, the top name ending in a space or a newline, the nested error cases), and all pass at this head. - The repo-config and tree-digest tests on a merge with current
main: 258 passed, 2 skipped. - Repros for my first-round points: a case-only rename under
core.ignorecase, a top-tracked file under a nested repository that ignores it, and a top directory namedtop. All three behave as the base did before the PR.
Checked, no issues
- Nested
.gitgit cannot read:_not_own_topasks git for the top from the nested directory and compares real paths. A broken or empty.gitnow raises and names the directory, as the edited #390 fragment says, wheremainused the enclosing repository's rules. - Claims and docs: the
evalsallowance, itsevals-oldandrefs.mdboundary tests, the schema description and thefeat-repo-configfragment agree with the code.
Summary
Follow-up to #323, which left two open questions for the author. Both are now decided and implemented:
skill_shaorsuite_hash. A tracked or untracked change that git keeps still moves them, and a nested.gitis still skipped.evalsdirectory. Those files leaveskill_shaand movesuite_hash. Individual claims ofSKILL.md, scripts, references or any other file of the skill stay refused.Changes
dokimi/utils.py:GitTree.skippedasks the innermost nested repository (the deepest directory above the path that holds a.git) what it ignores: onegit ls-files --others --ignored --exclude-standard --directory -zper nested repository (a denylist, as for the top, so a namecore.ignorecasespells otherwise still counts), cached on the tree snapshot. A path the top's index lists is never dropped by a nested repository's rules. Blob ids stay the parent's. The nested.gitis passed to git as its git dir, so one git cannot read fails there instead of git walking up to the enclosing repository. A git failure inside a nested repository (safe.directory, a timeout, a broken.git) raises a config error naming the repository path and git's own message; there is no silent fallback. The same holds for the top work tree: a root with a.git(or below one, on its filesystem) that git cannot read, or whose index it cannot read, raises the same way. A root with no.gitabove it still hashes the bytes on disk.dokimi/repo_config.py:_check_claimsaccepts a file match under{skill}/evals; every other individual file claim is still refused, with the evals directory named in the message.dokimi/schemas/dokimi_config.v1.json: thesuitedescription states the rule.changelog.d/20261030_180000_napetrov_feat-repo-config.md: the unreleased feat(repo): dokimi_config.v1 schema and repo_config loader #323 fragment now states both rules (edited in place, the feature has not shipped).tests/test_repo_config_git_blobs.py(ignored and changed files inside a nested repository, atskills/a/toolsandskills/a/evals/tools);tests/test_repo_config.py(individual eval files: hashes and impact; refused outside evals).Design choices
ls-files --directory: that lists an untracked directory whole and hides a nested repository inside it.hash-object(for example a required clean filter) is a config error too.test_a_submodule_inside_a_nested_repository_is_hashed_by_its_checked_out_files.{skill}/evalsonly. A suite that picks files under another directory ({skill}/tests/*.json) is refused; claiming{skill}/tests/**as a whole stays accepted. Possible later widening: accept individual claims under whichever directory the suite names. Kept refused for now.Evidence
test_a_broken_top_level_git_is_an_error_not_a_silent_fallback,test_a_directory_inside_a_broken_work_tree_is_an_error_not_a_silent_fallbackandtest_a_corrupt_index_is_an_error_not_a_silent_fallbackfail onc567e37b(no error raised) and pass on the head.test_a_directory_outside_any_work_tree_hashes_its_bytespasses on both (guard). Fixtures that faked a repository with an empty.git(tests/test_repo_config.py,_links.py,_links_v1.py,_globs.py) now rungit init, since such a.gitis a broken repository under the new rule.pytest tests/test_repo_config_git_blobs.py tests/test_repo_config.py -k "nested or individual or evals_directory or outside_the_evals"): the two ignored-file nested tests and the per-file eval test fail. The change-moves-hash guards and the refusal tests pass on both. Red on the first fix (test_a_nested_repository_git_cannot_read_is_an_error_not_a_silent_fallback): both cases returned a digest; green after the fix.d3d122ef(Copilot and exai inline): an untracked repository inside a nested repository kept its own directory but not the ancestors on the way to it, so the walk never reached it. Red:test_a_change_inside_an_untracked_repository_under_a_nested_repository_moves_a_hash[skills/a/tools]failed ond3d122ef; fixed by keeping every ancestor of a listed entry.pytest tests/test_repo_config*.py tests/test_tree_digest.py: 223 passed, 2 skipped ond3d122ef;pytest tests/test_repo_config_git_blobs.py tests/test_repo_config.py tests/test_tree_digest.py: 178 passed, 2 skipped on the bot-round commit.8d6f05cb's tests (pytest tests, the worktree.venvviauv run --no-project --python 3.13 --with-editable '.[dev]'): 15608 passed, 10 skipped, 4 failed; the test-only refinements after that run were rerun green (test_repo_config*.py,test_tree_digest.py). The 4 aretests/test_repo_typecheck_grader.py(_image_provides('mypy')), which fail the same way on the base run (base-failing.log) in this environment.ruff check,mypyondokimi/utils.pyanddokimi/repo_config.py,check_growth.py,check_comment_blocks.py --base intel/main,audit_test_quality.py,check_changelog_fragment.py --base intel/main: pass.Merge danger
skill_shaorsuite_hashof a skill that has a nested repository changes once: an ignored file inside it stopped counting. Values written before are comparable only with values written after this change for skills without nested repositories..gitowned by another user under safe.directory, or a.gitfile that points at a missing git dir in a copied checkout) now fails the config load with a message naming its path. Before, it was hashed without its nested rules and nothing was reported..git, safe.directory, a timeout, a stray empty.gitwith no outer repository above it) now fails the config load the same way. Before, its files were hashed from disk without a word.evalsremain refused. A config that names a non-evals directory per file must claim it whole.Part of #318. Follow-up to #323.
🤖 Generated with Claude Code