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.
|
🔍 OpenCodeReview found 2 issue(s) in this PR. By severity: 2 medium
Review details
|
| 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.21% 94.23% +0.02%
==========================================
Files 304 305 +1
Lines 38766 38910 +144
Branches 6567 6599 +32
==========================================
+ Hits 36524 36668 +144
+ Misses 1429 1428 -1
- Partials 813 814 +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.
| 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.
I agree with exai-bot here; the repro is below.
A setup patch that turns an indexed directory into a link to another directory in the workspace crashes the arm with FileNotFoundError. _workspace_files lists both the stale index entry sub/AGENTS.codex.md (it still exists through the new link) and real/AGENTS.codex.md. Both pass the containment check and name the same file. The loop unlinks it once, then fails on the second path.
Repro: commit AGENTS.md, AGENTS.claude.md, sub/AGENTS.codex.md and real/AGENTS.codex.md. Replace sub/ with a symlink to real without staging, then call _apply_arm(..., "claude"). Result: FileNotFoundError: [Errno 2] No such file or directory: '.../ws/sub/AGENTS.codex.md', not a PrepError or a clean run.
Suggestion: dedupe removed by _canon(ws / rel) before unlinking (keep the first name for arm_files), or skip a path that no longer exists with unlink(missing_ok=True). 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.
There was a problem hiding this comment.
Thanks, the two-name case is fixed. A nested alias under a removed directory link still crashes: with AGENTS.codex.md -> real, AGENTS.codex.md/AGENTS.copilot.md and real/AGENTS.copilot.md, the link goes first and the nested path is then gone (FileNotFoundError). Repro and a one-line fix in the new comment at workspace.py:612.
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in 7e46426: the nested alias under a removed directory link (AGENTS.codex.md/AGENTS.copilot.md with real/AGENTS.copilot.md) is unlinked before the link. Same test, first listing.
…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.
| 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.
| 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.
I agree with exai-bot here; the repro is below.
A removed directory link and a nested alias under it crash the arm with FileNotFoundError; the dedup keeps the name that stops existing. _overlay_targets keeps the first name in order for each file, and removed is then unlinked in sorted order (workspace.py:609-612). With AGENTS.codex.md -> real, real/AGENTS.copilot.md, a usable AGENTS.claude.md, and the inventory AGENTS.claude.md, AGENTS.codex.md, AGENTS.codex.md/AGENTS.copilot.md, real/AGENTS.copilot.md, the nested name sorts before real/... and stands for the file. The loop unlinks the AGENTS.codex.md link first, then the child path is gone:
_apply_overlay(ws, tracked, "claude")
FileNotFoundError: [Errno 2] No such file or directory: '.../AGENTS.codex.md/AGENTS.copilot.md'
This is the same crash as my earlier thread on the alias, reached through a different inventory. The fix covers only two names for one file where neither sits under the other's link.
Suggestion: unlink in reverse sorted order, so a path under a link goes before the link: for rel in reversed(removed):. Add a regression with this inventory next to test_overlay_arm_removes_a_file_a_directory_alias_lists_twice_once, and assert AGENTS.codex.md and real/AGENTS.copilot.md are both gone.
There was a problem hiding this comment.
Fixed in 7e46426: unlink runs in reverse sorted order, so the path under the link goes first. The regression uses your inventory and asserts AGENTS.codex.md and real/AGENTS.copilot.md are both gone.
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.
| 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.
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.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.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.AGENTS.md?docs/repo-evaluation-contract.md§3 says it keeps the base, but the code only requires an overlay.check_ancestorsalso guardAGENTS.<name>.mdfiles above the workspace?🤖 Generated with Claude Code