Repository navigation
Conversation
A harness profile names its agent with an `overlay` key (claude-code: claude, codex, copilot). The guidance:overlay arm keeps the base AGENTS.md and the AGENTS.<overlay>.md of that agent, and removes every other AGENTS.<name>.md. The arm is refused without a profile overlay and when the base has no overlay for the agent. The reach probe now probes every overlay, even where the manifest globs miss it, and reports a foreign overlay that a harness loaded as a cross_loaded finding. own_loaded says whether the harness loads its own overlay at all. Refs agentsmd/agents.md#185 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
Critical symlink-validation and stale-inventory bugs can leave AGENTS.md dangling, retain foreign overlays, or cause cleanup to fail with FileNotFoundError.
2 open issues
What changed in this PR
2 new must-fix findings.
This PR adds per-agent overlay evaluation to dokimi’s repository workflow, allowing runs to isolate a harness’s own guidance and measure whether it reads other agents’ overlays.
Changes:
- Adds
guidance:overlaywith profile-based selection and dangling-link checks. - Extends reach probing and reports with own-overlay and foreign-overlay loading results.
- Updates built-in profiles, schemas, documentation, and regression tests.
| File | Description |
|---|---|
dokimi/tests/test_repo_overlay.py |
Tests overlay filtering, validation, links, and reporting. |
dokimi/dokimi/schemas/harness_profile.v1.json |
Adds the optional overlay profile key. |
dokimi/dokimi/schemas/guidance_reach.v1.json |
Adds overlay reach results. |
dokimi/dokimi/repos/workspace.py |
Implements overlay workspace preparation. |
dokimi/dokimi/repos/reach.py |
Probes overlays and reports cross-loading. |
dokimi/dokimi/repos/profiles/copilot.yaml |
Selects the Copilot overlay. |
dokimi/dokimi/repos/profiles/copilot-review.yaml |
Selects the Copilot review overlay. |
dokimi/dokimi/repos/profiles/codex.yaml |
Selects the Codex overlay. |
dokimi/dokimi/repos/profiles/codex-review.yaml |
Selects the Codex review overlay. |
dokimi/dokimi/repos/profiles/claude-code.yaml |
Selects the Claude overlay. |
dokimi/dokimi/repos/overlays.py |
Centralizes overlay filename recognition. |
dokimi/dokimi/repos/harness_profile.py |
Parses and validates overlay names. |
dokimi/dokimi/commands/repo_run.py |
Documents the new arm in CLI help. |
dokimi/docs/repo-evaluation-contract.md |
Defines overlay arm and reach behavior. |
dokimi/docs/preparing-a-repository.md |
Adds overlay evaluation guidance. |
dokimi/changelog.d/20261031_120000_napetrov_ce-overlay.md |
Records the new arm and reach reporting. |
Review effort: Balanced
| if arm.base == OVERLAY_ARM: | ||
| return _apply_overlay(ws, tracked, overlay or "") |
There was a problem hiding this comment.
Fixed in ee38237. The overlay inventory now comes from the workspace (git ls-files --cached --others, existing paths only), not the snapshot index, so an overlay added by a setup patch is seen. Removal is checked against that inventory.
| hop, seen = _canon(link), set() | ||
| while hop.is_symlink() and hop not in seen: | ||
| seen.add(hop) | ||
| hop = _canon(hop.parent / os.readlink(hop)) |
There was a problem hiding this comment.
Fixed in ee38237. _link_hits walks each path component, so a base link through a removed directory link is caught before _canon resolves it away. The arm refuses that base instead of leaving a dangling AGENTS.md. Test: test_overlay_arm_refuses_a_base_link_through_a_removed_directory_link.
| for n in names.get(r["file"], ()): | ||
| if n in foreign: | ||
| cross.setdefault(n, {})[r["scenario"]] = r["class"] | ||
| if n in own_files: | ||
| own_loaded.add(n) |
There was a problem hiding this comment.
Must-fix: a target's tokens do not establish which alias was read. With AGENTS.codex.md -> AGENTS.md and a harness that loads only AGENTS.md, resolve_links/build_workspace seed one shared token pair; this loop nevertheless reports AGENTS.codex.md as cross_loaded. An AGENTS.claude.md alias similarly falsely populates own_loaded. These are incorrect verdicts about overlay support/isolation, not merely alternative labels for the same content. Preserve ambiguous attribution rather than emitting a definitive finding, or collect path-specific access evidence; add a regression where base guidance shares its target with both own and foreign overlay aliases.
There was a problem hiding this comment.
Fixed in a52e675, with a follow-up in 4f33efc. A load of a file named by both a foreign and an own overlay is no longer attributed to either; it is listed under ambiguous. 4f33efc also covers a foreign target with an own alias, which was still counted as an own load. Tests: test_overlay_report_leaves_a_load_of_a_shared_target_unattributed, test_overlay_report_leaves_a_load_of_a_foreign_target_with_an_own_alias_unattributed.
| if arm.base == "guidance:bridge" and not bridge: | ||
| name = profile.name if profile is not None else "no harness" | ||
| raise PrepError(f"guidance:bridge needs a harness profile with a `bridge` key; {name} has none") | ||
| overlay = profile.overlay if profile is not None else None |
There was a problem hiding this comment.
Must-fix: the workspace treatment now depends on profile.overlay, but batch_ledger.execution_id omits it and runner.run_cell reuses a completed result before calling prepare_workspace. Run guidance:overlay with a custom profile selecting claude, then change only overlay to codex and resume the same batch: the execution id and cell name are unchanged, so the claude result is returned as skipped instead of running the codex treatment. Validate/pin the overlay selection for overlay cells before reuse (or incorporate it into their identity), with a resume regression. This can be scoped to the new arm to preserve existing non-overlay cell identities.
There was a problem hiding this comment.
Fixed in f442d5d. An overlay cell's execution id now includes the overlay it keeps, so a completed result for another treatment is not reused. Plain-arm reruns still match in a batch holding overlay cells (both arm ids are checked). Test: test_an_overlay_cell_is_identified_by_the_overlay_it_keeps.
| if arm.base == OVERLAY_ARM: | ||
| return _apply_overlay(ws, tracked, overlay or "") |
There was a problem hiding this comment.
Must-fix: tracked comes from the snapshot's index, but setup patches have already been applied with git apply (without updating that index). With a base containing AGENTS.claude.md and a setup patch adding AGENTS.codex.md, the new foreign overlay is absent from tracked, survives this arm, and is committed by the subsequent git add -A. A setup patch deleting an existing foreign overlay instead leaves a stale entry that causes unlink() to raise FileNotFoundError. Enumerate the post-setup workspace files for this arm, or refresh the index before enumeration, and cover both additions and deletions.
There was a problem hiding this comment.
Fixed in ee38237. The overlay check reads the workspace after setup patches, not the snapshot index, so an overlay a patch creates is inventoried. Test: test_overlay_arm_reads_the_workspace_after_setup_patches_not_the_index.
| hop = _canon(hop.parent / os.readlink(hop)) | ||
| if hop in gone: | ||
| return True |
There was a problem hiding this comment.
Must-fix: resolving the parent here erases removed overlays used as directory links. For example, with AGENTS.codex.md -> docs/, AGENTS.md -> AGENTS.codex.md/base.md, docs/base.md a regular file, and a regular AGENTS.claude.md, this walk jumps directly to docs/base.md and never matches gone. The arm then removes AGENTS.codex.md and leaves the kept AGENTS.md dangling, despite its link guard. Check symlink components before canonicalizing them (or reject foreign overlays that name directories), and add a regression for this case.
There was a problem hiding this comment.
Fixed in ee38237. The parent is no longer resolved through a removed overlay used as a directory link; the walk expands symlinks itself and stops on loops (_MAX_HOPS). Tests: test_overlay_arm_refuses_a_base_link_through_a_directory_link, test_overlay_arm_does_not_hang_on_a_symlink_loop_in_the_base.
| kept = sorted(p for p in tracked if overlay_of(p) == own) | ||
| if not kept: | ||
| raise PrepError(f"guidance:overlay: the base has no AGENTS.{own}.md to keep") |
There was a problem hiding this comment.
Must-fix: a tracked own-overlay name is accepted even if it cannot supply a file. For example, keep a regular AGENTS.md and track AGENTS.claude.md -> missing.md: kept is nonempty, _link_hits finds no removed overlay, and preparation succeeds with only usable base guidance. A cyclic link or a link to a directory also passes. _checkout_tree preserves these links and no earlier preparation step validates their targets. Validate that kept overlays resolve to usable files inside the workspace and reject unusable targets with PrepError; add dangling/cyclic/directory own-overlay regressions (the current cycle test covers unrelated links only).
There was a problem hiding this comment.
Fixed in ee38237. A kept own overlay must now resolve to a regular file in the workspace, otherwise the arm raises PrepError ("does not resolve to a usable file in the workspace"). Test: test_overlay_arm_refuses_an_own_overlay_that_supplies_no_file.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #338 +/- ##
==========================================
+ Coverage 94.40% 94.47% +0.06%
==========================================
Files 313 318 +5
Lines 40240 40672 +432
Branches 6828 6909 +81
==========================================
+ Hits 37990 38424 +434
+ Misses 1435 1432 -3
- Partials 815 816 +1
🚀 New features to boost your workflow:
|
The arm walks each path component before resolving it, so a removed directory link in the middle of a base link is a hit; an own overlay must resolve to a file inside the workspace; the arm lists the workspace (index and untracked files), so a setup patch applied with git apply is seen. Thread: PRRT_kwDOVBg7g86yWjv2 Thread: PRRT_kwDOVBg7g86yWjwG Thread: PRRT_kwDOVBg7g86yWj4D Thread: PRRT_kwDOVBg7g86yWj4E Thread: PRRT_kwDOVBg7g86yWj4G Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…y report A harness that loads a file of a native name, or a file that an own and a foreign overlay both name, cannot be pinned on one alias: those aliases are listed as ambiguous, not as own or cross loads. Thread: PRRT_kwDOVBg7g86yWj4A Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A resumed batch reused an overlay cell that kept another agent's overlay, since the execution id did not carry the overlay. The id now includes the profile's overlay for cells of the overlay arm only; other cells keep their ids. Thread: PRRT_kwDOVBg7g86yWj4C Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The overlay report's ambiguous key was not in the guidance_reach schema. Thread: PRRT_kwDOVBg7g86yWj4A Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A foreign target with an own alias was counted as an own load and a cross load of the target. The ambiguity check now looks at the target too, and an ambiguous link pins no name, as the docstring says. Thread: PRRT_kwDOVBg7g86yWj4A Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| ambiguous: set[str] = set() | ||
| for target, aliases in links.items(): | ||
| kinds = {"own" if overlay_of(n) == own else "foreign" for n in (target, *aliases) if overlay_of(n) is not None} | ||
| if aliases and (PurePosixPath(target).name in native or {"own", "foreign"} <= kinds): |
There was a problem hiding this comment.
Must-fix: native-name ambiguity is checked only on the target. With CLAUDE.md -> docs/shared.md and AGENTS.codex.md -> docs/shared.md, resolve_links supplies both names as aliases. A Claude harness loading only CLAUDE.md produces the shared tokens, but this branch attributes them to AGENTS.codex.md and reports a false cross-load. Check native names across (target, *aliases) and add this shared-native-alias regression; the existing native-target test does not cover it.
There was a problem hiding this comment.
Fixed in 3d7334b: a group of link names pins nothing when the target or an alias has a harness-native file name (_pinned_names), so a Claude load of docs/shared.md is not attributed to AGENTS.codex.md.
Test: test_overlay_report_does_not_pin_a_shared_target_read_through_a_native_alias (red on 4f33efc).
| hops += 1 | ||
| if hops > _MAX_HOPS: | ||
| return False | ||
| target = PurePosixPath(os.readlink(cand)) |
There was a problem hiding this comment.
Must-fix: os.readlink returns filesystem-native paths, but PurePosixPath does not split Windows backslashes. On Windows, with AGENTS.codex.md -> docs and a kept AGENTS.md -> AGENTS.codex.md\base.md, the entire target becomes one component. cand then names the regular base.md through the directory link, so the walker never checks the removed AGENTS.codex.md component. Preparation removes that link and leaves AGENTS.md dangling. Parse readlink results with native Path semantics and add a Windows regression for a backslash-separated target through a removed directory link.
There was a problem hiding this comment.
Fixed in 3d7334b: readlink targets are read with native Path semantics, so a Windows target such as AGENTS.codex.md\base.md splits into its names.
Test (Windows only, skipped here): test_overlay_arm_refuses_a_backslash_base_link_through_a_removed_directory_link. Not run on Windows in this round.
…rgets use native separators A shared target reached through a native alias (CLAUDE.md) is not pinned to the foreign alias. A Windows link target with backslashes splits into names when read as a native path. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| line. `own_loaded` lists the harness's own overlays that loaded in some scenario; | ||
| when none did, the text report says so. `own` is the profile's `overlay`. |
There was a problem hiding this comment.
Nit: document the alias-ambiguity exception here. With AGENTS.claude.md -> AGENTS.md and a Claude profile, reported tokens from AGENTS.md leave own_loaded empty but populate ambiguous; format_matrix deliberately suppresses the 'no overlay loaded' note. Thus an empty own_loaded does not always mean the own overlay was not loaded, and the text report does not always say so. Describe ambiguous and qualify both the own-load and cross-load claims so readers can distinguish an unobserved load from an unattributable one.
There was a problem hiding this comment.
Fixed in b880a3e. docs/repo-evaluation-contract.md section 7 now states the ambiguous set and when an own overlay gets the 'could not determine' note instead of 'not loaded'. Docs only; the rule is in _pinned_names and _own_overlay_note.
| for target, aliases in links.items(): | ||
| kinds = {"own" if overlay_of(n) == own else "foreign" for n in (target, *aliases) if overlay_of(n) is not None} | ||
| # a native name anywhere in the group (the target or an alias) cannot tell which file a harness read | ||
| if aliases and (any(PurePosixPath(n).name in native for n in (target, *aliases)) or {"own", "foreign"} <= kinds): |
There was a problem hiding this comment.
Must-fix: foreign-only alias groups still produce unsupported per-path findings. For example, overlay_report([{"file": "docs/shared.md", "scenario": "cwd=.", "class": "loaded"}], "claude", {"docs/shared.md": ("AGENTS.codex.md", "AGENTS.copilot.md")}, native={"CLAUDE.md", "AGENTS.md"}) reports both aliases in cross_loaded. A harness reading only one alias produces exactly this shared-token record, so the other finding: is false. The same issue occurs with a foreign overlay target and another foreign overlay alias. Treat these competing overlay names as ambiguous (or report a group-level finding), and add regressions for both arrangements rather than attributing the load to every name.
There was a problem hiding this comment.
Fixed in b880a3e. _pinned_names treats a group with two overlay names as ambiguous, whatever their owner, and adds the target to ambiguous. Tests: test_overlay_report_leaves_a_load_of_two_foreign_aliases_unattributed and ..._foreign_target_with_a_foreign_alias_unattributed (test_repo_overlay.py).
| if overlays.get("own_files") and not overlays.get("own_loaded") and not overlays.get("ambiguous"): | ||
| lines.append(f"note: no {overlays['own']} overlay ({', '.join(overlays['own_files'])}) was loaded in any scenario") |
There was a problem hiding this comment.
Must-fix: this reports a negative loading verdict even when the probe has no usable evidence. For example, with a regular AGENTS.claude.md and every scenario timing out, classify() produces only error records, overlay_report() leaves own_loaded empty, and this prints “no claude overlay ... was loaded in any scenario.” Error records explicitly mean loading is unknown, not not_loaded. Gate this note on usable negative records for the own overlays (and qualify incomplete runs), or report that loading could not be determined. Add a regression for an all-error run.
There was a problem hiding this comment.
Fixed in b880a3e. An errored scenario no longer yields 'no overlay was loaded': _own_overlay_note prints that only with a usable not_loaded record and no error, otherwise 'could not determine whether ... was loaded'. Test: test_text_report_does_not_claim_not_loaded_for_an_all_error_run.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 3d7334b. The arm and the link walk hold up under the cases I tried, and the earlier bot findings are fixed. Five points are worth fixing before merge: one wrong note in the reach text report, three gaps in tests that pin a reach verdict or the dangling-link refusal, and one mismatch between the contract and the arm.
What I ran, on a Linux review host at 3d7334b:
make ci-local: 28 passed, 2 failed, 7 skipped. The two failures are the gitleaks download (Install gitleaks, HTTP 504) and theScan commitsstep that needs it. Neither touches this PR. Changed-line coverage passes.tests/test_repo_overlay.py: 40 passed, 1 skipped (the Windows-only test). The tests write only undertmp_path.- Reproductions for the reach findings, with the LLM transport mocked.
- Mutation runs against the new code: 13 mutants of
reach.py, 2 of which pass the three reach/overlay test files; a set ofworkspace.pylink-walk mutants, 6 of which passtest_repo_overlay.py.
Findings not anchored inline:
- The arm does not require a base
AGENTS.md, but the contract says it keeps one.docs/repo-evaluation-contract.md(theoverlaybullet, around line 318) says the workspace "keeps the baseAGENTS.md" and that an overlay sits "beside anAGENTS.md"._apply_overlayonly requires that an own overlay exists anywhere. A repo holding onlyAGENTS.claude.mdandAGENTS.codex.mdruns: the call returns(['AGENTS.codex.md'], {'arm_overlay': {'name': 'claude', 'kept': ['AGENTS.claude.md']}}). A repo withAGENTS.md,sub/AGENTS.claude.mdandother/AGENTS.codex.mdkeeps thesuboverlay althoughother/has no base. The cell is then an overlay alone, which the contract sentence "so a cell cannot be the base alone by accident" does not cover. Suggestion: refuse when noAGENTS.mdexists, or reword the contract and say an overlay alone is allowed. The PR description lists this as open question 2, so it needs a decision before merge.
Bot points I checked that no longer apply at this head (10)
- Copilot's point on setup-patch additions and deletions missed by the index inventory does not apply:
_workspace_fileslists the workspace after the patches (git ls-files --cached --others, existing paths only), andtest_overlay_arm_reads_the_workspace_after_setup_patches_not_the_indexpins it. - Copilot's point on a base link through a removed directory link does not apply:
_link_hitswalks each path component before resolving, and the arm refusesAGENTS.md -> AGENTS.codex.md/base.mdinstead of leaving it dangling. - exai-bot's point on attributing a shared target to the wrong alias does not apply: a load of a file named by an own and a foreign overlay is listed under
ambiguous, not as an own or cross load. - exai-bot's point on resuming a batch after changing only
overlaydoes not apply: the overlay arm's execution id now carries the overlay, and plain cells keep their ids. - exai-bot's point on the index being stale after setup patches does not apply: the inventory comes from the workspace, as above.
- exai-bot's point on the parent walk erasing a removed directory link does not apply: the walk expands symlinks itself and stops on loops.
- exai-bot's point on an unusable own overlay does not apply: a kept overlay that is missing, cyclic or a directory link raises
PrepErrorbefore anything is removed. - exai-bot's point on a shared native alias such as
CLAUDE.mddoes not apply:_pinned_namespins nothing when the target or any alias has a native file name. - exai-bot's point on backslash readlink targets does not apply: targets are read with native
Pathsemantics. The Windows test is skipped on Linux, so I could not run it. - exai-bot's point on foreign-only alias groups does not apply: with
docs/shared.mdnamed byAGENTS.codex.mdandAGENTS.copilot.md, both are listed incross_loaded, but a foreign overlay was read in either case, so the finding that isolation failed is right. Which alias was read is the documented alias attribution the description declines to narrow.
…sing own overlay is unknown A native name in a link group pins nothing. Two overlay names in one group (own or foreign) are ambiguous, and the ambiguous set now includes the target. A load of a link target counts as unpinned too. The 'no overlay was loaded' note needs a usable negative record and no error, otherwise the note says it could not be determined. Adds the base-link refusal cases and moves the native-alias test to test_repo_reach_links.py to keep test_repo_reach.py under the length cap. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| def _pinned_names(links: dict[str, tuple[str, ...]], own: str | None, | ||
| native: set[str] | frozenset[str]) -> tuple[dict[str, tuple[str, ...]], set[str]]: |
There was a problem hiding this comment.
Nit: own is never read by _pinned_names; attribution depends only on the alias group and native names. Remove this parameter and change the sole caller to _pinned_names(links, native) so the helper's signature reflects its actual dependencies.
There was a problem hiding this comment.
| for rel in removed: | ||
| if (ws / rel).is_dir() and not (ws / rel).is_symlink(): | ||
| raise PrepError(f"guidance:overlay: {rel} is a directory, not an overlay file") | ||
| (ws / rel).unlink() |
There was a problem hiding this comment.
Must-fix: validate the parent of each removed path stays inside the workspace before unlinking. A setup patch can replace a tracked directory sub/ (formerly containing sub/AGENTS.codex.md) with a symlink to an external directory. Because setup patches use git apply without updating the index, _workspace_files() still includes the cached sub/AGENTS.codex.md when that external file exists. With a valid root AGENTS.claude.md, this loop then deletes the external file on the host. The kept-overlay containment check does not protect foreign overlays. Reject removed paths whose resolved parent escapes ws (while still allowing the final component itself to be a symlink), and add a regression covering a directory-to-symlink setup patch.
There was a problem hiding this comment.
Fixed in 6232e4d. _apply_overlay now checks every removed overlay before anything is unlinked: if its parent resolves outside the workspace it raises PrepError, so the host file stays. The final name may still be a link, as you suggested, since unlink removes the link itself.
Regression test_overlay_arm_does_not_unlink_through_a_setup_patch_that_made_a_directory_a_link fails without the fix (no PrepError) and passes now.
…d1031 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Review: the helper never reads own; attribution depends only on the alias group and the native names. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…linked out A setup patch can turn a tracked directory into a link to a host directory. The index still lists the overlay under it and the file now resolves outside the workspace, so the arm unlinked the host file. Refuse before anything is removed, and keep the removal of the final component itself as it was. Red before the fix: DID NOT RAISE, host file removed. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| gone = {_canon(ws / rel) for rel in removed} | ||
| removed_set = set(removed) | ||
| for rel in tracked: | ||
| if rel not in removed_set and (ws / rel).is_symlink() and _link_hits(ws / rel, gone): |
There was a problem hiding this comment.
Must-fix: check retained paths for removed symlinks in their ancestors too. A setup patch can replace an indexed directory AGENTS.codex.md/ with AGENTS.codex.md -> docs, where docs/AGENTS.claude.md exists. The stale indexed AGENTS.codex.md/AGENTS.claude.md passes the kept-file validation, but its final component is a regular file, so this condition skips it. Removing the foreign directory link then leaves a recorded kept overlay path dangling instead of refusing preparation. Walk the full workspace-relative path component by component; merely removing is_symlink() is insufficient because _link_hits resolves the parent before checking it. Add a regression for a retained regular file beneath a removed directory link.
There was a problem hiding this comment.
Fixed in e9a486f. Every path the arm keeps is now walked from the workspace root, component by component, before anything is removed. AGENTS.codex.md/AGENTS.claude.md under a removed AGENTS.codex.md link is refused ("resolves through an overlay the arm removes; the base would dangle"), and the codex link stays.
Test: test_overlay_arm_refuses_a_kept_overlay_under_a_removed_directory_link, red on 6232e4d (no refusal).
| for rel in removed: | ||
| if (ws / rel).is_dir() and not (ws / rel).is_symlink(): | ||
| raise PrepError(f"guidance:overlay: {rel} is a directory, not an overlay file") | ||
| (ws / rel).unlink() |
There was a problem hiding this comment.
Must-fix: deduplicate deletion targets before unlinking. After a setup patch replaces indexed sub/ with sub -> real, _workspace_files can contain both the stale indexed sub/AGENTS.codex.md and real/AGENTS.codex.md. Both pass the containment checks and refer to the same file. This loop deletes it on the first iteration, then raises FileNotFoundError on the second, failing an otherwise valid overlay preparation. Deduplicate by _canon(ws / rel) (while preserving the removed-path metadata), and add a regression with an internal directory alias.
There was a problem hiding this comment.
Fixed in e9a486f. The removed list keeps one name per file, so sub/AGENTS.codex.md and real/AGENTS.codex.md are unlinked once. No FileNotFoundError.
Test: test_overlay_arm_removes_a_file_a_directory_alias_lists_twice_once, both alias orders, red on 6232e4d.
Local: overlay/reach/ratchet tests 193 passed, 2 skipped; ruff clean.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 6232e4d. The delta since 3d7334b closes my three reach threads and the setup-patch escape, but two cases of the same setup-patch kind still break guidance:overlay. Both are in _apply_overlay and both reproduce. Two things are worth fixing before merge, and one earlier question is still open.
What I ran, on a Linux review host at 6232e4d:
make ci-local: 30 passed, 0 failed, 7 skipped; 12,054 tests passed; changed-line coverage passes- Reproductions for both findings, offline, in the repo's
tests/style. - Mutants of the new reach and workspace lines: 6 mutants, 2 of which pass the full overlay/reach suite. Both only change which "own overlay" note prints (dropping
and "error" not in seen; matching onlyr["file"]instead of its alias group), so I did not raise them.
Still open from my earlier review: the contract (docs/repo-evaluation-contract.md, the overlay bullet around line 318) still says the workspace "keeps the base AGENTS.md", and _apply_overlay still only requires an own overlay. This waits for a decision: refuse when no AGENTS.md exists, or reword the contract.
Checked, no issues
- Threads: the all-error note, the
ambiguousexemption and therun_reachwiring are fixed and pinned by tests. The base-link refusal cases from my last review (absolute,..,./x/.., chain, own overlay outside the workspace) now have tests. - Unlink escape: a removed overlay whose last component is a link to a file or directory outside the workspace is only unlinked; the target survives.
- Commits: every non-merge commit carries a
Signed-off-by.
Bot points I checked that no longer apply at this head (12)
- Copilot's point on setup-patch additions and deletions missed by the index inventory does not apply:
_workspace_fileslists the workspace after the patches (git ls-files --cached --others, existing paths only). - Copilot's point on a base link through a removed directory link does not apply:
_link_hitswalks each path component before resolving, and the arm refusesAGENTS.md -> AGENTS.codex.md/base.md. - exai-bot's point on attributing a shared target to the wrong alias does not apply: a load of a file named by an own and a foreign overlay is listed under
ambiguous. - exai-bot's point on resuming a batch after changing only
overlaydoes not apply: the overlay arm's execution id carries the overlay. - exai-bot's point on the index being stale after setup patches does not apply: the inventory comes from the workspace.
- exai-bot's point on the parent walk erasing a removed directory link does not apply: the walk expands symlinks itself and stops on loops.
- exai-bot's point on an unusable own overlay does not apply: a kept overlay that is missing, cyclic or a directory link raises
PrepError. - exai-bot's point on a shared native alias such as
CLAUDE.mddoes not apply:_pinned_namespins nothing when the group holds a native file name. - exai-bot's point on backslash readlink targets does not apply: targets are read with native
Pathsemantics (the Windows test is skipped on Linux, so I could not run it). - exai-bot's point on foreign-only alias groups does not apply: a group with two overlay names is now ambiguous, and
ambiguousholds the target. - exai-bot's point on the all-error note does not apply:
_own_overlay_noteprints "could not determine" unless anot_loadedrecord exists and none errored. - exai-bot's point on unlinking through a setup patch that made a directory a link out of the workspace does not apply:
_apply_overlayraisesPrepErrorbefore unlinking, and the regression fails with the check removed.
…once A setup patch can turn an indexed directory AGENTS.codex.md/ into a link to docs/. The stale kept path AGENTS.codex.md/AGENTS.claude.md then ends up dangling once the link is removed. The check only looked at symlink names, so it was not refused. The walk now runs over every non-removed path from the workspace root. A directory alias (sub -> real) lists one file under two names; the removal unlinked it twice and raised FileNotFoundError. The removed list is now one name per file. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| @functools.lru_cache(maxsize=1) | ||
| def native_names() -> frozenset[str]: | ||
| """Guidance names a builtin harness loads by its own convention (``AGENTS.override.md``): never an overlay.""" | ||
| return frozenset(n for name in builtin_profiles() for n in load_profile(name).guidance_names) |
There was a problem hiding this comment.
Must-fix: these names do not necessarily load the bundled profiles. load_profile() first checks for a file of that name in the current working directory. For example, with a plain executable or text file named codex in the working directory, overlay_of("AGENTS.claude.md") raises ProfileError while trying to parse that file as a profile. Because reach calls overlay_of() for every repository file, even probing Claude Code can now fail on this unrelated local file. Load each bundled YAML by its explicit PROFILE_DIR path here, and add a regression with a same-named file in the working directory.
There was a problem hiding this comment.
Already on head: native_names() loads each bundled profile by its explicit PROFILE_DIR path, so codex in the cwd is not read. Pinned by test_overlay_of_reads_the_bundled_profiles_not_a_same_named_file_in_the_cwd in 7e46426.
| else: | ||
| names[target] = group |
There was a problem hiding this comment.
Must-fix: a single overlay alias does not establish that the overlay name was read. For example, let CLAUDE.md import @docs/shared.md and let AGENTS.codex.md symlink to docs/shared.md. Claude can load the target through the import without loading the foreign overlay, but this branch attributes its tokens to AGENTS.codex.md and emits a cross-load finding. The reproducible report input is overlay_report([{"file": "docs/shared.md", "scenario": "cwd=.", "class": "loaded"}], "claude", {"CLAUDE.md": (), "docs/shared.md": ("AGENTS.codex.md",)}, native={"CLAUDE.md"}). The same attribution problem affects own_loaded and canonical-target tool reads classified as fetched. Preserve evidence identifying the name read, or leave these alias loads ambiguous when that evidence is unavailable; add a regression for a directly imported non-native target. (Read scenarios request REACH_PROBE.txt, not the canonical guidance target, so they do not themselves force the target read.)
There was a problem hiding this comment.
Fixed in 7e46426. A link target is pinned to an overlay alias only when no native file is in the probe, since CLAUDE.md can import the target. With one present the alias is ambiguous. Test: test_overlay_report_leaves_a_link_load_unattributed_when_a_native_file_may_import_it (your docs/shared.md example, red before). A probe with no native file keeps the pin, as before.
There was a problem hiding this comment.
Maintainer decision (2026-11-02): keep the ambiguous rule. A link load is pinned only when no native file can import the target, else it is listed under ambiguous. Unchanged in d70dd57, except that only native names present in the probe count. The PR body has a section on it.
There was a problem hiding this comment.
A consequence of the kept rule: codex and copilot declare imports: null, yet a repo with overlays nearly always has an AGENTS.md (native for every builtin profile), so every overlay that is a link is ambiguous for every harness and never a finding:. If intended, the contract could say so; otherwise apply the rule only to profiles with guidance.imports set.
There was a problem hiding this comment.
Kept the rule as the contract states, and documented the consequence in aa9fc49: codex and copilot have no imports, so with their AGENTS.md in the probe every overlay link is ambiguous.
Open for the maintainer: scope the rule to profiles with guidance.imports?
There was a problem hiding this comment.
Maintainer decision: keep the ambiguous rule. A link load is pinned only when no native file can import it. No change.
| if "not_loaded" in seen and "error" not in seen: | ||
| return f"note: no {overlays['own']} overlay ({files}) was loaded in any scenario" |
There was a problem hiding this comment.
Must-fix: this treats a refusal as evidence that the overlay was not loaded. Reproduce with the existing fake harness's FAKE_MODE=refuse, an AGENTS.claude.md file, and profile.overlay='claude': the successful but token-free refusal is classified as not_loaded, so this prints a definitive negative even though format_matrix also warns that the scenario's prose makes its negative cells untrustworthy. Check the relevant scenario rows for suspect prose before making this claim, and emit the indeterminate note when the negative evidence is unusable. Add a regression using the refusal mode.
There was a problem hiding this comment.
Fixed in 7e46426. A scenario that errored or answered with prose over PROSE_LIMIT gives no usable not_loaded record, so the note says it could not be determined. Test: test_text_report_does_not_claim_not_loaded_from_a_refusal, red before.
| for rel in removed: | ||
| if (ws / rel).is_dir() and not (ws / rel).is_symlink(): | ||
| raise PrepError(f"guidance:overlay: {rel} is a directory, not an overlay file") | ||
| (ws / rel).unlink() |
There was a problem hiding this comment.
Must-fix: deletion aliases can become inaccessible during this loop. With AGENTS.codex.md -> real, a usable AGENTS.claude.md, and inventory entries AGENTS.codex.md, AGENTS.codex.md/AGENTS.copilot.md, and real/AGENTS.copilot.md, _overlay_targets retains the nested alias for the copilot file. This loop removes the codex directory symlink first, then raises FileNotFoundError when unlinking its child. Setup patches can produce this inventory through stale indexed descendants, and the dangling-path guard skips both paths because they are foreign overlays. Unlink via the canonical parent paths computed before deletion, or choose aliases/order that remain accessible; add a regression where both the directory link and its descendant are removal targets.
There was a problem hiding this comment.
Fixed in 7e46426: same change as the thread above. Children are unlinked before their alias; regression test added.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at e9a486f. The two fixes since my last review hold for the cases they name, but the dedup in the unlink fix leaves a sibling of the same crash. One bug inline, plus one nit.
What I ran, on a Linux review host at e9a486f:
make ci-local: 30 passed, 0 failed, 7 skipped; changed-line coverage passes.- A reproduction for the bug below, offline, in the repo's
tests/style. - Mutants of
_apply_overlay,_overlay_targets,_path_hitsand_workspace_files: 34 mutants, 7 of which passtest_repo_overlay.py. Four are equivalent or only reorder a refusal; the other three (lexiststoexists, therealpath(ws)normalisation, the "is a directory" refusal) are test gaps on rare paths, so I did not raise them.
Still open from my earlier review:
dokimi/docs/repo-evaluation-contract.md(theoverlaybullet, around line 318): the contract says the workspace "keeps the baseAGENTS.md", and_apply_overlaystill only requires an own overlay. This waits for a decision: refuse when noAGENTS.mdexists, or reword the contract. The refusal list in the same bullet also names one refusal while the arm has several more.dokimi/dokimi/repos/workspace.py:612: the removal-order variant of the alias crash, inline.
Checked, no issues
- Kept overlay under a removed directory link:
AGENTS.codex.md -> docswith the stale index entryAGENTS.codex.md/AGENTS.claude.mdis refused before anything is unlinked, and_path_hitscovers chains,.., absolute targets and loops. - Unlink escape: a removed overlay whose parent resolves outside the workspace is refused; a final-name link out of the workspace is only unlinked.
- Commits: every non-merge commit carries a
Signed-off-by.
Bot points I checked that no longer apply at this head (16)
- Copilot's point on setup-patch additions and deletions missed by the index inventory does not apply:
_workspace_fileslists the workspace after the patches (git ls-files --cached --others, existing paths only). - Copilot's point on a base link through a removed directory link does not apply:
_path_hitswalks each path component before resolving, and the arm refusesAGENTS.md -> AGENTS.codex.md/base.md. - exai-bot's point on attributing a shared target to the wrong alias does not apply: a load of a file named by an own and a foreign overlay is listed under
ambiguous. - exai-bot's point on resuming a batch after changing only
overlaydoes not apply: the overlay arm's execution id carries the overlay. - exai-bot's point on the index being stale after setup patches does not apply: the inventory comes from the workspace.
- exai-bot's point on the parent walk erasing a removed directory link does not apply: the walk expands symlinks itself and stops on loops.
- exai-bot's point on an unusable own overlay does not apply: a kept overlay that is missing, cyclic or a directory link raises
PrepError. - exai-bot's point on a shared native alias such as
CLAUDE.mddoes not apply:_pinned_namespins nothing when the group holds a native file name. - exai-bot's point on backslash readlink targets does not apply: targets are read with native
Pathsemantics (the Windows test is skipped on Linux, so I could not run it). - exai-bot's point on foreign-only alias groups does not apply: a group with two overlay names is ambiguous.
- exai-bot's point on the all-error note does not apply:
_own_overlay_noteprints "could not determine" unless anot_loadedrecord exists and none errored. - exai-bot's point on unlinking through a setup patch that made a directory a link out of the workspace does not apply:
_apply_overlayraisesPrepErrorbefore unlinking. - exai-bot's point on a retained file under a removed directory link does not apply: every kept path is walked from the workspace root and refused.
- exai-bot's point on deduplicating deletion targets does not apply to the two-name case:
sub/AGENTS.codex.mdandreal/AGENTS.codex.mdare unlinked once. The nested-alias variant is the inline comment. - exai-bot's point on a single overlay alias of a non-native target does not apply as a blocker: the false
finding:needs a hand-built alias group, and the description declines narrowing alias attribution on purpose. The matrix names the alias, so a reader can check it. - exai-bot's point on the refusal-mode run does not apply: the "no overlay was loaded" note is printed together with the existing
warning: ... read its trace before trusting its not_loaded cellsline, which the same run emits.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| if overlays.get("own_files") and not overlays.get("own_loaded") and not overlays.get("ambiguous"): | ||
| lines.append(_own_overlay_note(payload, overlays)) |
There was a problem hiding this comment.
Must-fix: this suppresses the own-overlay diagnostic whenever any file has an ambiguous load, even if none of those names belongs to the own overlay. For example, with AGENTS.claude.md classified as not_loaded and AGENTS.codex.md classified as loaded with a CLAUDE.md alias, ambiguous contains only AGENTS.codex.md and CLAUDE.md, yet no note is printed about the unambiguously unloaded claude overlay. Limit this guard to ambiguity involving own_files, and add a regression combining an own-overlay negative record with an unrelated ambiguous foreign load.
Suggestion:
| if overlays.get("own_files") and not overlays.get("own_loaded") and not overlays.get("ambiguous"): | |
| lines.append(_own_overlay_note(payload, overlays)) | |
| if (overlays.get("own_files") and not overlays.get("own_loaded") | |
| and not set(overlays["own_files"]).intersection(overlays.get("ambiguous", ()) )): | |
| lines.append(_own_overlay_note(payload, overlays)) |
There was a problem hiding this comment.
Fixed in 7e46426. The own-overlay note is held back only when an own file is itself ambiguous. Test: test_text_report_still_says_not_loaded_for_an_own_overlay_next_to_an_unrelated_ambiguous_load, red before.
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.
I pushed a commit per nit, up to ba6dbfd, which fix the nit from my review:
ba6dbfd:dokimi/dokimi/repos/overlays.py:25:native_namesloads each built-in profile by its path underPROFILE_DIR, so a file named after a profile in the working directory is not read.
make ci-local at ba6dbfd: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the bug from my review; the earlier thread of mine above nit level.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/repos/reach.py
…ads honestly - workspace: remove the removed overlay paths children-first, so a file under a directory alias is unlinked before the alias itself. - reach: a load of a link target is pinned to an overlay alias only when no native file in the probe can import it, and two overlay names stay unpinned as before. - reach: a refusal or an errored scenario gives no usable not_loaded record for the own-overlay note; the note is printed when only an ambiguous foreign file is unrelated to the own overlay. - Tests: nested alias order, a directly imported non-native target, an own-overlay note beside an unrelated ambiguous load, a refusal, and a same-named profile file in the working directory. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| "scenarios": scenario_rows, | ||
| "records": records, | ||
| "overlays": overlay_report(records, profile.overlay, {s.path: s.aliases for s in seeded}, | ||
| {PurePosixPath(n).name for n in profile.guidance_names}), |
There was a problem hiding this comment.
Must-fix: this passes configured native names, not native files actually present in the probe. _pinned_names treats any nonempty native set as evidence of a possible import, so a profile with guidance.names: [CLAUDE.md] probing only AGENTS.codex.md -> docs/shared.md marks a loaded target ambiguous and leaves cross_loaded empty even though no CLAUDE.md exists to import it. The same problem suppresses own_loaded for an own-overlay link. Restrict the native set to names present among seeded targets and aliases, and add a regression with configured native names but no native files. Keep the existing native-import/native-alias regressions.
There was a problem hiding this comment.
Fixed in d70dd57. The native set is now the configured names present in the probe (native_in_probe), so CLAUDE.md with no CLAUDE.md file or link no longer makes a link group ambiguous. Test: test_native_in_probe_counts_only_the_native_names_a_probed_file_or_link_carries. Existing native-import/native-alias regressions unchanged.
| if rel not in listed_set and _path_hits(ws, rel, gone): | ||
| raise PrepError(f"guidance:overlay: {rel} resolves through an overlay the arm removes; " | ||
| "the base would dangle") | ||
| for rel in reversed(removed): # a name under a link sorts after the link: unlink it first, or its path is gone |
There was a problem hiding this comment.
Must-fix: reverse lexical order still breaks chained directory aliases, so the earlier deletion-order fix does not cover all dependencies. With AGENTS.z.md -> real, AGENTS.a.md -> AGENTS.z.md, a regular AGENTS.claude.md, and listed entries AGENTS.a.md/AGENTS.copilot.md and real/AGENTS.copilot.md, deduplication keeps the nested AGENTS.a.md/... name. This loop removes AGENTS.z.md first, then unlinking that nested name raises FileNotFoundError. Such a stale indexed entry can remain after a setup patch replaces a directory with a symlink; an otherwise valid overlay cell consequently fails after partially modifying the workspace. Unlink through the prevalidated canonical paths (captured before deletion), or order by actual symlink dependencies rather than spelling, and extend the deletion regression to this chained-alias inventory.
There was a problem hiding this comment.
Fixed in d70dd57. Each removed overlay is unlinked by the canonical path captured before any unlink, after all the checks, so order no longer matters. With AGENTS.z.md -> real, AGENTS.a.md -> AGENTS.z.md and both listings, the nested name and real/AGENTS.copilot.md collapse to one entry. Test: test_overlay_arm_removes_a_chained_directory_alias_listing_without_failing (red on 7e46426).
… count only when probed A chained directory alias (AGENTS.a.md -> AGENTS.z.md -> real/) failed the removal when AGENTS.z.md went first. Each removed overlay is now unlinked by the canonical path captured before any unlink, after every check. A configured harness name (CLAUDE.md) with no file in the probe no longer makes a link group ambiguous: native_in_probe keeps only the names present. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| `ambiguous` lists the names of a load that could have been read under another | ||
| name: a file reached through an alias group that holds a native guidance name | ||
| or two overlay names. Those loads are neither own nor cross loads. The text |
There was a problem hiding this comment.
Nit: this says a native guidance name must belong to the alias group, but _pinned_names marks a linked load ambiguous whenever any harness-native file is present anywhere in the probe. For example, with separate CLAUDE.md and AGENTS.codex.md -> docs/shared.md, a loaded docs/shared.md is ambiguous even though its alias group has no native name. Document the probe-wide condition so readers can correctly interpret missing cross_loaded findings; the implementation's conservative rule can remain unchanged.
Suggestion:
| `ambiguous` lists the names of a load that could have been read under another | |
| name: a file reached through an alias group that holds a native guidance name | |
| or two overlay names. Those loads are neither own nor cross loads. The text | |
| `ambiguous` lists the names of a load that could have been read under another | |
| name: a file with aliases when any harness-native guidance file is present in | |
| the probe (it could import the target), or when the alias group holds two overlay | |
| names. Those loads are neither own nor cross loads. The text |
| if "not_loaded" in negative and "error" not in seen: | ||
| return f"note: no {overlays['own']} overlay ({files}) was loaded in any scenario" |
There was a problem hiding this comment.
Must-fix: a usable negative in one scenario masks an indeterminate refusal in another. For example, with own-overlay records not_loaded for both cwd=. and read=sub, prose counts 0 and 40 respectively, negative contains not_loaded and seen contains no error, so this prints “was loaded in any scenario” despite read=sub providing no evidence about loading. The all-refusal test covers only the case where negative is empty. Require all relevant scenarios to provide usable negative evidence before making the all-scenarios claim, or qualify the note to cover only usable scenarios; add a mixed negative/refusal regression.
There was a problem hiding this comment.
Fixed in 19195e0. The "not loaded" note now needs every scenario that probed the own overlay to give a usable answer, so a refusal in one scenario prints "could not determine". Regression: test_text_report_does_not_claim_not_loaded_when_another_scenario_was_a_refusal.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at d70dd57. Both open threads are fixed, but the own-overlay note still overstates in a mixed run, and the base AGENTS.md question from my earlier review is still open. One bug and one nit inline.
What I ran, on a Linux review host at d70dd57:
make ci-local: 30 passed, 0 failed, 7 skipped; changed-line coverage passes.- Reproductions for the note finding, offline, with a hand-built reach payload.
- Mutants of the new
workspace.pyandreach.pylines: 19 of the canonical-path unlink, 3 of which pass the suite (2 are equivalent), plus a set for the reach note and pinning logic, a few of which also pass. I did not raise them: they change a printed note or a rare path.
Still open from my earlier review:
dokimi/docs/repo-evaluation-contract.md:347: the contract says the workspace "keeps the baseAGENTS.md", and_apply_overlaystill only requires an own overlay. A repo with onlyAGENTS.claude.mdandAGENTS.codex.mdruns underclaude: the call returns(['AGENTS.codex.md'], {'arm_overlay': {'name': 'claude', 'kept': ['AGENTS.claude.md']}}), so the cell is an overlay with no base. This waits for a decision: refuse when noAGENTS.mdexists, or reword the contract.preparing-a-repository.mdhas the same "keep only the base and the own overlay" wording.
Checked, no issues
- Threads: the nested and chained directory aliases no longer crash. Removed overlays are keyed by canonical path and unlinked once, after every refusal check. A final-name link out of the workspace is only unlinked, and the target survives.
- Resume ids:
execution_idadds the overlay only for overlay-arm cells, so existing cell ids are unchanged and the golden ledger test passes. - Schemas:
overlayandoverlaysare optional, so both changes widen. - Commits: all 14 non-merge commits carry a
Signed-off-by.
Bot points I checked that no longer apply at this head (21)
- Copilot's point on setup-patch additions and deletions missed by the index inventory does not apply:
_workspace_fileslists the workspace after the patches, not the snapshot index. - Copilot's point on a base link through a removed directory link does not apply:
_path_hitswalks each path component before resolving, and the arm refuses such a base. - exai-bot's point on attributing a shared target to the wrong alias does not apply: a load of a file named by an own and a foreign overlay is listed under
ambiguous. - exai-bot's point on resuming a batch after changing only
overlaydoes not apply: the overlay arm's execution id carries the overlay, and plain cells keep their ids. - exai-bot's point on the stale index after setup patches does not apply: the inventory comes from the workspace.
- exai-bot's point on the parent walk erasing a removed directory link does not apply: the walk expands symlinks itself and stops on loops.
- exai-bot's point on an unusable own overlay does not apply: a kept overlay that is missing, cyclic or a directory link raises
PrepError. - exai-bot's point on a shared native alias does not apply: a link group is ambiguous when a native file is in the probe.
- exai-bot's point on backslash readlink targets does not apply: targets are read with native
Pathsemantics (the Windows test is skipped on Linux, so I could not run it). - exai-bot's point on foreign-only alias groups does not apply: a group with two overlay names is ambiguous.
- exai-bot's point on the all-error note does not apply:
_own_overlay_noteprints "could not determine" unless a usablenot_loadedrecord exists and none errored. - exai-bot's point on unlinking through a setup patch that made a directory a link out of the workspace does not apply:
_overlay_targetsraisesPrepErrorbefore any unlink. - exai-bot's point on a retained file under a removed directory link does not apply: every kept path is walked from the workspace root and refused.
- exai-bot's point on deduplicating deletion targets does not apply: removed overlays are keyed by canonical path and unlinked once.
- exai-bot's point on a profile-named file in the working directory does not apply:
native_namesloads each bundled profile by itsPROFILE_DIRpath. - exai-bot's point on attributing an alias load does not apply: a link load is left
ambiguouswhen a native file is in the probe, which is the maintainer's stated rule. - exai-bot's point on treating a refusal as a negative does not apply to a single refusal scenario: it gives no usable
not_loadedrecord, so the note says loading could not be determined. - exai-bot's point on a directory link and its nested alias does not apply: unlinking is by canonical path.
- exai-bot's point on suppressing the own-overlay note does not apply: the note is held back only when an own file is itself ambiguous.
- exai-bot's point on configured native names does not apply:
native_in_probecounts only the native names a probed file or link carries. - exai-bot's point on chained directory aliases does not apply: canonical paths have real directories as parents, so unlink order does not matter.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 53aeecd, which fix the nit from my review:
53aeecd:dokimi/docs/repo-evaluation-contract.md:1095: theambiguoussentence now says a file with aliases is listed when any harness-native guidance file is in the probe, or when its alias group holds two overlay names;guidance_reach.v1.jsonis reworded the same way.
make ci-local at 53aeecd: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the bug from my review.
…as a refusal The note claimed the overlay was loaded in no scenario when one scenario read as not_loaded usably and another answered with prose or errored. Require every scenario that probed the own overlay to be usable. 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 53aeecd. The arm and the link walk hold up on Linux: removed overlays are unlinked once, by canonical path, after every refusal check, and reach follows the kept ambiguity rule. Still open: the guidance:none baseline keeps the overlays, two tests fail on Windows, and the mixed refusal note (the fix commit 19195e0 is not on the PR yet).
Agreed with @antonwolfy: the "not loaded in any scenario" note next to a refusal in another scenario; the arm still does not require a base AGENTS.md, though the contract says it keeps one (a repo with only AGENTS.claude.md and AGENTS.codex.md gives kept: ['AGENTS.claude.md'] and no refusal).
Agreed with @exai-bot on the same mixed refusal note.
What I ran, at 53aeecd:
make ci-local --offline: 19 passed, 0 failed, 13 skipped (network and Docker steps).- The CI
external-catalog-smokejob (docker-oracle,docker-probe): both pass, isolation probe closed. - Offline repros for the note, the
guidance:nonefixture,overlay_ofnames, a codex link load, a base with noAGENTS.md, and_apply_overlayon 50,000 files (1.8 s). - On Windows (Server 2025, Python 3.13):
pytest tests/test_repo_overlay.py tests/test_repo_reach_links.py: 63 passed, 2 failed. The backslash test that is skipped on Linux passes; the two failures are in the line comment onworkspace.py:572.
Checked, no issues
- Unlinking: all checks run before the first unlink, and a link out of the workspace is only unlinked.
- Resume ids: only overlay-arm cells add the overlay to
execution_id, so existing cell ids and the golden ledger stay the same. - Schemas:
overlayinharness_profile.v1andoverlaysinguidance_reach.v1are optional, so both widen. - Docs and changelog: the
ambiguoustext matches_pinned_names; the fragment is inAddedand names the new key and the refusals.
| `AGENTS.<agent>.md` beside an `AGENTS.md`, at any depth. The workspace keeps | ||
| the base `AGENTS.md` and the `AGENTS.<overlay>.md` files and removes every | ||
| other `AGENTS.<name>.md` (not the harness's own names such as | ||
| `AGENTS.override.md`, nor files under `.claude`). The files are recognised by |
There was a problem hiding this comment.
With the usual **/AGENTS.md globs, guidance:none keeps every AGENTS.<agent>.md, so the "no guidance" baseline holds more overlay guidance than guidance:overlay.
On this PR's test fixture, guidance:none removes AGENTS.md and sub/AGENTS.md and leaves AGENTS.claude.md, AGENTS.codex.md and sub/AGENTS.copilot.md. A none / overlay / raw comparison then counts overlays as "none".
Suggestion: have guidance:none (and rev:) also drop the files overlay_of recognises, or document that a repo with overlays needs **/AGENTS.*.md in guidance.globs.
There was a problem hiding this comment.
Documented in aa9fc49: none and rev: match only guidance.globs, so a repo with overlays lists **/AGENTS.*.md there.
Open for the maintainer: should none also drop the overlays overlay_of recognises?
There was a problem hiding this comment.
Maintainer decision: keep. guidance:none and rev: match only guidance.globs, as the contract says. No change.
| hops += 1 | ||
| if hops > _MAX_HOPS: | ||
| return False | ||
| target = Path(os.readlink(cand)) # the native separators: a Windows target splits on backslashes |
There was a problem hiding this comment.
Two tests in test_repo_overlay.py fail on Windows (Server 2025, Python 3.13).
...however_it_is_written[absolute]: no PrepError.os.readlinkreturns\\?\C:\...there, whilegoneholds realpath paths without the prefix, socand in gonenever matches.test_overlay_arm_keeps_a_base_link_whose_target_a_removed_link_also_names:OSError [Errno 22]. A Windows link made withsymlink_to("docs/shared.md")does not open; this one is the test.
Suggestion: strip the \\?\ prefix after os.readlink on nt, and use Path("docs", "shared.md") in the test.
There was a problem hiding this comment.
Fixed in aa9fc49: a link target read on Windows drops the \\?\ prefix, and the Windows test link is Path("docs", "shared.md"). Pinned by test_a_link_target_with_the_windows_long_path_prefix_is_read_without_it.
|
|
||
| from .harness_profile import PROFILE_DIR, builtin_profiles, load_profile | ||
|
|
||
| OVERLAY_RE = re.compile(r"AGENTS\.([A-Za-z0-9][A-Za-z0-9-]*)\.md") |
There was a problem hiding this comment.
OVERLAY_RE takes any AGENTS.<word>.md as an agent overlay. overlay_of gives ja for AGENTS.ja.md, local for AGENTS.local.md, template for docs/AGENTS.template.md, and override for AGENTS.Override.md (the native check is case-sensitive). The arm deletes them, and repo reach reports them as foreign-overlay findings.
Suggestion: treat as foreign only known agent names (the builtin profiles' overlay keys), or require an AGENTS.md beside the file, as the docstring says.
There was a problem hiding this comment.
Fixed in aa9fc49: AGENTS.<name>.md is an overlay only for a builtin profile's overlay key (claude, codex, copilot); ja, local, template and Override are ordinary files. Pinned in test_overlay_names_are_agent_files_beside_agents_md.
Open for the maintainer: should overlay names of profiles loaded from outside the builtin set count too?
There was a problem hiding this comment.
Maintainer decision: keep. Only the builtin profile overlay keys are overlays; other AGENTS..md files are ordinary files. No change.
| targets = _overlay_targets(ws, listed) | ||
| gone = set(targets) | ||
| listed_set = set(listed) | ||
| for rel in tracked: |
There was a problem hiding this comment.
Nit: _path_hits walks every workspace path even when there is no foreign overlay to remove. On 50,000 files with only AGENTS.md and AGENTS.claude.md, _apply_overlay takes 1.8 s per cell, all in this loop.
Suggestion: return early when targets is empty.
There was a problem hiding this comment.
Fixed in aa9fc49: the path walk runs only when an overlay is removed. Pinned by test_the_path_walk_runs_only_when_an_overlay_is_removed.
| > **Status: recommendations.** §1–§7 apply today. The measurement loop in §8 runs | ||
| > today for guidance arms (`guidance:none`, `guidance:raw`, `guidance:bridge`). | ||
| > today for guidance arms (`guidance:none`, `guidance:raw`, `guidance:bridge`, | ||
| > `guidance:overlay`). |
There was a problem hiding this comment.
Nit: Two arm lists still name only none/raw/bridge: docs/benchmarking-and-comparison.md:280 and the _parse_guidance docstring at dokimi/dokimi/repos/workspace.py:146. (Anchored here because neither line is in the diff.)
There was a problem hiding this comment.
Fixed in aa9fc49: the arm list in docs/benchmarking-and-comparison.md and the _parse_guidance docstring now name guidance:overlay.
…rgets; a no-op walk is skipped - AGENTS.<name>.md is an overlay only for a name a builtin profile declares (claude, codex, copilot) - a symlink target read on Windows drops the \\?\ long-path prefix before the path walk - the path walk for dangling links runs only when an overlay is removed - the guidance:none and rev: globs, the ambiguity rule for profiles without imports, and the arm list documented Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| a profile loaded from outside the builtin set has no recognised overlay name, so | ||
| its arm is refused. A base with no `AGENTS.<overlay>.md` is a |
There was a problem hiding this comment.
Nit: External profiles are not categorically refused. For example, copying claude-code.yaml to a custom path retains overlay: claude, and prepare_workspace accepts it when AGENTS.claude.md exists: recognition checks the overlay value against builtin names, not the profile's origin. Clarify that an external profile cannot introduce a new overlay name, but can reuse a builtin one.
Suggestion:
| a profile loaded from outside the builtin set has no recognised overlay name, so | |
| its arm is refused. A base with no `AGENTS.<overlay>.md` is a | |
| an external profile can reuse one of those names but cannot introduce a new | |
| recognised overlay name. A base with no `AGENTS.<overlay>.md` is a |
There was a problem hiding this comment.
Fixed in a49a8be: the contract now says an external profile can reuse a builtin overlay name; a non-builtin key is still refused.
Pinned by test_external_copy_of_a_builtin_profile_keeps_its_builtin_overlay.
|
Open for the maintainer (antonwolfy's base AGENTS.md point, 11-02 review): refuse the overlay arm when no AGENTS.md exists, or reword the contract to drop the base-AGENTS.md promise? Not changed yet. |
…erlay A profile loaded from a path may reuse a builtin overlay name; the test pins that the arm accepts it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| """The target of a symbolic link as a path. On Windows ``os.readlink`` gives an absolute target with the ``\\\\?\\`` | ||
| long-path prefix, which the canonical paths of the workspace do not carry, so the prefix is dropped.""" | ||
| target = os.readlink(link) | ||
| return Path(target[len("\\\\?\\"):] if target.startswith("\\\\?\\") else target) |
There was a problem hiding this comment.
Must-fix: stripping \\?\ from a UNC target such as \\?\UNC\server\share\ws\AGENTS.codex.md produces UNC\server\share\ws\AGENTS.codex.md, which Windows Path treats as relative. With a workspace at \\server\share\ws, a valid AGENTS.claude.md, and AGENTS.md linking absolutely to that foreign overlay, _path_hits misses the removal target and preparation deletes AGENTS.codex.md, leaving AGENTS.md dangling. Normalize the \\?\UNC\ prefix to \\ before handling the ordinary \\?\ prefix, and add a Windows UNC-target regression; the current long-path test covers only the ordinary prefix.
There was a problem hiding this comment.
Fixed in 87dd6b5: _link_target maps \\?\UNC\server\share\x to \\server\share\x, not the relative UNC\.... Test: test_a_link_target_on_a_windows_unc_share_keeps_the_server_name.
…GENTS.md A Windows UNC link target (\\?\UNC\server\share\x) lost its server name and became the relative path UNC\server\share\x, so a removal target could be missed. Map it to \\server\share\x. guidance:overlay now refuses a base with no AGENTS.md, or one that does not resolve to a usable file, as the contract already says it keeps it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Maintainer decision: the overlay arm refuses a base with no usable |
workspace.py went over the 800-line cap and _apply_overlay over the complexity cap of the growth ratchet. The symbolic-link path checks of the guidance:overlay arm (_canon, _link_target, _path_hits, _overlay_targets, _supplies_a_file) move to repos/overlay_paths.py. _apply_overlay is split into _kept_overlays, _refuse_dangling and _check_removable, with the same checks in the same order. Behaviour is unchanged; _link_target stays importable from workspace for the tests, and _path_hits is still looked up in workspace so the walk test's monkeypatch still applies. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Vladislav Perevezentsev <vladislav.perevezentsev@intel.com>
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
LGTM
I checked 8080f70. Every point from my earlier review is settled at 0ae5a97, and the one nit left there is fixed in this branch.
Merge: safe now: it merges cleanly into current main, 7 commits behind, and its overlay, reach, docs-refs and schema-compat tests pass on the merge. Once it is in, #333, #367, #363, #368 and #359 conflict with it (workspace.py, repo-evaluation-contract.md, reach.py, profiles/codex.yaml, preparing-a-repository.md, repo_run.py) and need a rebase.
What I ran, at 8080f70:
make ci-local --offline: 18 passed, 1 failed, 13 skipped. The failure isschema_compatagainst currentmain: it reportsheadroom.v1.jsonremoved, a schema #279 added onmainafter this branch's base. Not from this PR.- On the merge with current
main: the overlay, reach, docs-refs and schema-compat tests, 294 passed, 1 skipped. - On Windows (Server 2025, Python 3.13), at 0ae5a97:
pytest tests/test_repo_overlay.py tests/test_repo_reach_links.py: 75 passed, none skipped, including the long-path and backslash link tests.
Checked, no issues
- Mixed refusal note: "not loaded" now needs every scenario that probed the own overlay to be usable.
- Windows link targets:
_link_targetdrops\\?\and maps\\?\UNC\server\shareto\\server\share. - Base
AGENTS.md: a base without one, or whoseAGENTS.mddoes not resolve to a file, is refused before anything is unlinked. - Refactor:
overlay_paths.pymoves the link checks unchanged; it has the SPDX header.
Fixed in this branch, up to 8080f70:
- A profile with a non-builtin
overlaywas told its overlay file was missing when it was there: 8080f70 refuses such a profile by its name (overlay 'gemini' is not a builtin overlay name (claude, codex, copilot)) before it reads the base; the test fails on the old message.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 8080f70. The delta since 53aeecd holds up: the base AGENTS.md is now required, the mixed-refusal note needs every probed scenario to be usable, overlay names are limited to the builtin profiles, and the link checks moved to overlay_paths.py unchanged apart from the Windows prefix handling. One risk is left, inline: the refusal for an AGENTS.md that resolves to no file has no test.
What I ran, on a Linux review host at 8080f70:
make ci-local: 30 passed, 0 failed, 7 skipped; 12,616 tests; changed-line coverage 98%.- Mutants of the new lines:
probed <= usable, the builtin-name restriction and refusal, the missing-AGENTS.mdrefusal, the UNC branch of_link_targetand theif targets:skip. All are killed except the usable-file check onAGENTS.md(inline). - A reproduction for the finding, offline, with no model call.
Still open from my earlier review: none. The mixed-refusal note and the base AGENTS.md question are both fixed at this head.
Checked, no issues
- Refusal order: every new refusal comes before the first unlink, so a refused cell leaves the workspace as it was.
- Early skip: skipping the dangling-base walk when no overlay is removed is sound, since an empty
goneset can never hit;AGENTS.mdis still checked. - Schemas:
overlayandoverlaysare optional, so both changes widen; no new schema version is needed. - Commits: all 21 non-merge commits carry a
Signed-off-by.
Bot points I checked that no longer apply at this head (23)
- Copilot's point on setup-patch additions and deletions missed by the index inventory does not apply:
_workspace_fileslists the workspace after the patches, not the snapshot index. - Copilot's point on a base link through a removed directory link does not apply:
_path_hitswalks each path component before resolving, and the arm refuses such a base. - exai-bot's point on attributing a shared target to the wrong alias does not apply: a load of a file named by an own and a foreign overlay is listed under
ambiguous. - exai-bot's point on resuming a batch after changing only
overlaydoes not apply: the overlay arm's execution id carries the overlay, and plain cells keep their ids. - exai-bot's point on the stale index after setup patches does not apply: the inventory comes from the workspace.
- exai-bot's point on the parent walk erasing a removed directory link does not apply: the walk expands symlinks itself and stops on loops.
- exai-bot's point on an unusable own overlay does not apply: a kept overlay that is missing, cyclic or a directory link raises
PrepError. - exai-bot's point on a shared native alias does not apply: a link group is ambiguous when a native file is in the probe.
- exai-bot's point on backslash readlink targets does not apply: targets are read with native
Pathsemantics, and the Windows tests were run by another reviewer. - exai-bot's point on foreign-only alias groups does not apply: a group with two overlay names is ambiguous.
- exai-bot's point on the all-error note does not apply:
_own_overlay_noteprints "could not determine" unless a usablenot_loadedrecord exists and none errored. - exai-bot's point on unlinking through a setup patch that made a directory a link out of the workspace does not apply:
_overlay_targetsraisesPrepErrorbefore any unlink. - exai-bot's point on a retained file under a removed directory link does not apply: every kept path is walked from the workspace root and refused.
- exai-bot's point on deduplicating deletion targets does not apply: removed overlays are keyed by canonical path and unlinked once.
- exai-bot's point on a profile-named file in the working directory does not apply:
native_namesandoverlay_namesload each bundled profile by itsPROFILE_DIRpath. - exai-bot's point on attributing an alias load does not apply: a link load is left
ambiguouswhen a native file is in the probe, which is the maintainer's stated rule, now documented in the contract. - exai-bot's point on treating a refusal as a negative does not apply: a refusing scenario gives no usable
not_loadedrecord, so the note says loading could not be determined. - exai-bot's point on a directory link and its nested alias does not apply: unlinking is by canonical path.
- exai-bot's point on suppressing the own-overlay note does not apply: the note is held back only when an own file is itself ambiguous.
- exai-bot's point on configured native names does not apply:
native_in_probecounts only the native names a probed file or link carries. - exai-bot's point on chained directory aliases does not apply: canonical paths have real directories as parents, so unlink order does not matter.
- exai-bot's point on a usable negative masking a refusal in another scenario does not apply: the "not loaded" note now needs
probed <= usable, and the mutation check kills both dropping and flipping that condition. - exai-bot's point on a
\\?\UNC\...link target does not apply:_link_targetmaps it to\\server\share\..., and a test with a mockedos.readlinkpins it.
|
|
||
| def _check_removable(ws: Path, targets: dict[Path, str]) -> None: | ||
| """Refuse a removal that leaves the base without a file, or that would remove a directory.""" | ||
| if not _supplies_a_file(ws, "AGENTS.md"): # the base is kept too, so it must supply a file as well |
There was a problem hiding this comment.
The new refusal for an AGENTS.md that supplies no file is not pinned by any test. Mutation check: replacing the condition with if False: leaves test_repo_overlay.py, test_repo_reach.py, test_repo_reach_links.py and test_repo_reach_edges.py at 183 passed, 1 skipped. test_overlay_arm_refuses_a_base_with_no_agents_md covers only the absent file, which is the other refusal.
With AGENTS.md -> missing.md, AGENTS.claude.md and AGENTS.codex.md, _apply_overlay raises PrepError: guidance:overlay: AGENTS.md does not resolve to a usable file in the workspace. With the check removed it returns (['AGENTS.codex.md'], {'arm_overlay': {'name': 'claude', 'kept': ['AGENTS.claude.md']}}): a cell with a broken base is built and read as a valid arm. A cyclic or directory AGENTS.md behaves the same.
Suggestion: parametrize a test like test_overlay_arm_refuses_an_own_overlay_that_supplies_no_file over AGENTS.md linking to missing.md, to itself and to a directory, and assert match="AGENTS.md does not resolve" and that AGENTS.codex.md still exists.
There was a problem hiding this comment.
Fixed in c257c7e: test_overlay_arm_refuses_a_base_that_supplies_no_file, parametrized over AGENTS.md -> missing.md, itself and a directory; asserts AGENTS.md does not resolve and that AGENTS.codex.md survives. Mutation if False: → 3 failed; restored → 3 passed.
# Conflicts: # dokimi/docs/repo-evaluation-contract.md # dokimi/dokimi/repos/workspace.py
A dangling, cyclic or directory AGENTS.md link is refused before any overlay is removed; the existing test covered only an absent AGENTS.md.
An overlay is matched by its name, not by the guidance globs, so the none arm kept AGENTS.<agent>.md and its guidance still loaded. none now removes them with the matched files; rev: is unchanged. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Refs agentsmd/agents.md#185.
Summary
A harness profile names its agent with a new
overlaykey. The new arm keeps the base and that agent's overlay, and drops the rest:dokimi repo reachnow probes everyAGENTS.<name>.md. A foreign overlay that a harness loaded is reported as afinding:line and in theoverlaysblock ofguidance_reach.v1(cross_loaded).own_loadedsays whether the harness loads its own overlay at all. The arm does not make a harness load its overlay; the probe measures that.Evidence
make ci-localon the head commit: 0 failed, 30 passed, 7 skipped (docker, micromamba, latest-Python and push-only jobs).tests/test_repo_overlay.pyandtests/test_repo_reach.py: 122 passed offline. Includes the new regressions for a link reached through a directory link, a symlink loop in the base, a base link whose target a removed link also names, and a profileoverlayvalue with a trailing newline.test_repo_overlay.py,test_repo_reach.py,test_repo_reach_links.py,test_repo_reach_edges.py,test_growth_ratchet.py217 passed, 2 skipped; ruff and mypy clean on the touched files. Fullci-localnot rerun for this round.pr-reviewlocal mode, 3 passes): every confirmed blocker was fixed. The last fixes were verified by tests and ci-local, not by a fresh review pass. The review record is in the author's local scratch, not in this PR.Ambiguous overlay links
Kept as designed (maintainer decision, 2026-11-02). A load is pinned to an overlay link only when no other name of its file could have been read. Otherwise it is listed under
ambiguousinoverlaysand is neither an own nor a cross load, when:CLAUDE.md, which can@-import the target) is in the probe. Only native names present in the probe count, so a configured name with no file does not make a link ambiguous.Effect: a foreign link to a file a native file may import is
ambiguousin real runs, notcross_loaded. The rule is in_pinned_namesand_own_overlay_note;docs/repo-evaluation-contract.md§7 states it.Merge Danger
Door: two-way. The arm is new and the profile key is optional; reverting removes both with no data migration.
Blast radius: low for existing runs.
batch_ledger) does not include the newoverlaykey, so existing cell ids are unchanged.dokimi repo reachnow probes overlay files too. Repos withAGENTS.<name>.mdfiles will run more reach scenarios and cost more. Repos without overlays are unaffected.harness_profile.v1: optionaloverlay;guidance_reach.v1: optionaloverlays).Declined items
check_ancestorsguard that lacks overlay names could report an ancestor overlay as a false cross-load. Not probed.overlay_offolds case, soAGENTS.CODEX.mdcounts as codex. A case-sensitive harness would not load it. Kept on purpose; tests assert it.AGENTS.mdis reported as cross-loaded when only the base content was read. The docstring says a link counts as its target's overlay; say if you want it narrowed to reads by name.Open questions for maintainers
batch_ledger) does not include the profile'soverlayvalue. Editing it on a profile can reuse cells from a resumed batch built under the old overlay. Including it would change every claude-code cell id, so this PR does not change it.check_ancestorsalso guardAGENTS.<name>.mdfiles above the workspace?🤖 Generated with Claude Code