Skip to content

Name an unchanged instruction limit reached through an in-tree link instead of refusing (#822) - #863

Merged
pengfei-threemoonslab merged 4 commits into
mainfrom
fix/822-linked-unchanged-limit
Sep 24, 2026
Merged

pengfei-threemoonslab merged 4 commits into
mainfrom
fix/822-linked-unchanged-limit

Conversation

@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor

Closes #822
Refs #808, #721, #700, #711, #812, #811

Problem

This entry cannot resolve some instruction files, such as a SKILL.md whose metadata holds a non-string value (internal: true). When a change leaves such a file alone, the file is named as an unchanged limit and everything else is still compared (#721). The same untouched file reached through an in-tree link that the reader follows (#700) behaved differently. The link could be .claude/skills -> ../.agents/skills, a per-skill link or a file link. In that case diff, verify and the manifest-free PR comment refused the whole comparison with base_inventory_incomplete; head_inventory_incomplete and no rows. That hid a removed deny rule sitting next to the skill.

Cause: unchanged_limits() calls unchanged(source), and both diff and verify answer that with blob_path_unchanged. For a linked file, the source is the path under the link, e.g. .claude/skills/review/SKILL.md. blob_path_unchanged only accepted a regular file in Git at exactly that path. A path through a link never is one, so the proof failed and the refusal covered everything.

Design

blob_path_unchanged (src/agents_shipgate/cli/verify/git.py) now works out, for each side, how the reader gets to the file:

  • Base and commit head: read from Git tree entries. _CommitEntries makes one exact ls-tree per path and one bounded cat-file blob for each link's text.
  • Working-tree head: read without following any link. Each component is typed with the reader's own _directory_member_kind, each link is read with readlink, and the file is hashed unfiltered, then compared against the base's entries.

The result of that walk is (links followed with their text, in-tree path read). The proof holds only if both sides give the same result and the file they land on is the same regular-file blob:

  • The link: at every link on the way, a link entry at the same path with the same text, and a directory at every other component.
  • The target: the same blob at the same in-tree path.

_reader_path follows a link only under the rules the reader (_resolve_in_tree_link, #700) and the base archive (_resolve_tree_link, #711) already use:

  • the link text is relative and lands inside the tree after normalization;
  • everything above where it lands is a directory, not a link;
  • at most eight links;
  • links at one component of the path only, because a directory the reader reads through contains no link.

A link the reader does not read through gives an unreadable issue, and unreadable is still never an unchanged-limit kind. A path with no link on it still costs one ls-tree, as before. Blob object IDs are still compared, so no filter or textconv runs (#721's guarantees). No metadata value is coerced: internal: true stays an unsupported structure (#811).

Every consumer of the proof moves together:

check's boundary result still cannot name a limit, so for these layouts it now refuses with unchanged_limits_not_representable. That is the reason it already gives when the limit sits at its own path; before, it gave base/head_inventory_incomplete. Its rows (empty), decision and violations do not change.

Surface discipline. No surface is added: no command, schema, member, reason code, check id or version. The fix corrects the existing unchanged-limit proof, as #822's headline-metric note says. In CONTRIBUTING terms it raises blocked-recall and the comparable rate on host diffs, because a refused comparison hides real rows.

The capability_diff row in docs/distribution-surfaces.md and its note in tests/test_distribution_surface_parity.py both record that this adds no claim. The row's roots include core/host_comparison.py, whose comment and docstring changed.

Docs updated:

  • CHANGELOG.md: an entry under ## Unreleased; 1.1.0 is untouched.
  • STABILITY.md: a Migration Note: Unreleased (#linked-unchanged-limits-822).
  • docs/host-boundary-support.md: one paragraph.
  • docs/distribution-surfaces.md and the parity test: as above.

Before / after (measured)

I built the issue's three repositories (direct, dirlink = .claude/skills -> ../.agents/skills, skilllink = .claude/skills/review -> ../../skills/review). I then ran the CLI from origin/main 8269922b (via git archive) and from this branch:

Repository Route origin/main 8269922b this branch
direct diff comparable; ⚠ low removed … deny: Bash(curl *) → gone; limit .claude/skills/review/SKILL.md — unsupported identical
dirlink diff Cannot compare against HEAD~1: base_inventory_incomplete; head_inventory_incomplete, no row comparable; the same removed-deny row; limits .claude/skills/review/SKILL.md (claude-code), and .agents/skills/review/SKILL.md for claude-code, codex and cursor
skilllink diff same refusal, no row comparable; the same row; limit .claude/skills/review/SKILL.md
dirlink, skilllink verify host_comparison and PR comment incomparable, 0 rows, "Host capability comparison unavailable" comparable, 1 row, the same unchanged_limits as diff, "Not compared: unchanged in this change…" in the comment
dirlink, skilllink check --format agent-boundary-json require_review, incomparable [base_inventory_incomplete, head_inventory_incomplete] require_review, incomparable [unchanged_limits_not_representable] (as direct); the same violations

Each limit's detail is byte-identical to the one for the direct path (frontmatter_invalid_structure).

Tests

New file tests/test_linked_unchanged_limits.py, 45 tests:

  • Every layout matches direct. Layouts: directory link, per-skill link, file link, and a chain of two file links. For each, diff text and JSON, verify verifier.json and text, the PR comment and check agree with the direct result.
  • Proof works on both head kinds. It holds between commits and against the working tree.
  • Negative controls stay refused, on every route and in the proof:
    • skill added behind the link;
    • skill edited behind the link;
    • link retargeted to an identical copy;
    • link text rewritten to land on the same file (../../skills/./review);
    • link replaced by a directory with the same bytes, and the reverse;
    • file-link target edited;
    • a later hop retargeted;
    • working-tree-only retargets and edits.
  • Parity with the reader. The proof holds exactly where the reader reads SOURCE. That is checked for all of the above plus: eight links (read), nine links (not read), dangling, looping, escaping, absolute (external) targets, a link above where another lands, and a link inside a linked directory. Every unreadable shape also stays refused on diff.
  • No coercion ([Feature]: Make deterministic permission review tangible through a change–correction–recurrence loop #811). internal: true stays unsupported. The issue's control internal: "true" is comparable with no limit.

On origin/main, the positive cases in this file fail and the negative controls pass.

Updated existing tests:

  • tests/test_host_comparison_coverage.py: retired the skill-metadata-through-link case in INCOMPARABLE, which pinned exactly this refusal. The nested-link case beside it still refuses.
  • tests/test_adapter_static_only.py: moved the two pinned call-site lines in cli/verify/git.py (2686→2855, 3086→3255). The bounded-collector rationale now also mentions the link-text cat-file blob.

Ran locally, all passing:

  • tests/test_unchanged_host_limits.py, test_linked_unchanged_limits.py, test_host_link_read_through.py, test_host_file_links.py, test_partial_host_comparison.py, test_host_comparison_coverage.py, test_host_change_route_parity.py, test_host_only_advisory_recipe.py, test_manifest_free_pr_rows.py, test_capability_diff.py, test_scoped_base_tree.py, test_unread_changed_inputs.py, test_claude_hook_loading_evidence.py, test_host_boundary_unread_surfaces.py, test_agent_control_envelope_rows.py, test_reusable_workflow_secret_mappings.py, test_workflow_label_redaction.py, test_workflow_step_action_references.py, test_host_diff_review_changes.py, test_enabled_plugin_hook_routing.py, test_distribution_surface_parity.py.
  • Benchmark replays: test_host_config_replay.py and test_cold_start_replay.py. Run-of-record scores reproduce.
  • Docs and surface-contract suites: test_adapter_static_only.py, test_public_surface_contract.py, test_host_diff_entry_docs.py, test_report_1_0_contract.py, test_release_pipeline.py, test_release_decision.py, test_release_engine_smoke.py, test_reviewer_summary.py, test_agent_action_summary.py, test_bootstrap.py, test_setup_control.py, test_fixture_no_import.py, test_severity_override_floor.py, test_host_audit.py.
  • Everything else importing the Git module or the host route: test_verify.py, test_verify_orchestrator.py, test_verify_auto_base.py, test_verification_git_snapshot.py, test_current_control.py, test_capability_diff_partial_clone.py, test_first_adoption.py, test_host_directory_inputs.py, test_host_boundary_check.py, test_prompt_disabling_settings.py, and others.
  • ruff check . is clean.

Not in scope

…nstead of refusing (#822)

An instruction file whose limit this entry cannot resolve, such as a
SKILL.md whose metadata holds a non-string value, is named as an unchanged
limit when a change leaves it alone (#721). Reached through an in-tree link
the reader reads through (#700) - `.claude/skills -> ../.agents/skills`, a
per-skill link or a file link - the same untouched file refused the whole
comparison: diff, verify and the manifest-free PR comment printed
`base_inventory_incomplete; head_inventory_incomplete` and no row, hiding a
removed deny rule beside it. `unchanged_limits` asked blob_path_unchanged
whether the source, the path the link is read under, was one regular file
in Git, and a path through a link never is.

blob_path_unchanged now resolves the path on each side the way the reader
reaches it: from Git tree entries for the base and a commit head, and for a
working-tree head without following any link (each component's own entry,
each link's own text, the file's unfiltered hash). It holds only when both
resolutions are equal - every link at the same path with the same text,
every other component a directory - and the file they land on is the same
regular-file blob at the same in-tree path. A link is followed only under
the rules the reader and the base archive already use (#700, #711): a
relative text that lands inside the tree after normalization, directories
above where it lands, at most eight links, and links at one component of
the path. A path no link reaches still takes the one `ls-tree` it took
before, and blob object IDs are still compared, so no filter or textconv
can make two byte sequences equal.

Everything that asks the proof moves together: unchanged_limits in
`diff --json` and verifier.json, the text and PR comment, the unchanged
limits a partial comparison may carry (#808) and the shared
plugin-reference limits check leaves out (#714). check's boundary result
still cannot name a limit, so it now refuses these comparisons with
unchanged_limits_not_representable, as it does for a limit at its own path;
its rows, decision and violations do not move. No schema, member, reason
code or check id is added. The metadata value is not coerced.

tests/test_linked_unchanged_limits.py holds every layout (directory link,
per-skill link, file link, a chain of file links) to the direct result on
diff, verify, the PR comment and check; keeps the negative controls refused
(skill added or edited behind the link, link retargeted to an identical
copy, link text rewritten to land on the same file, link replaced by a
directory or the reverse, a later hop retargeted, working-tree-only
retargets and edits); and holds the proof to exactly the links the reader
reads through, across dangling, looping, absolute, escaping, over-long,
intermediate and nested links. The #812 coverage case that pinned the
refusal is retired, and the static-only allowlist follows its two call
sites down the file.

@pengfei-threemoonslab pengfei-threemoonslab left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent coding-agent review/address cycle 1 at de9b9650.

Not mergeable yet: one P2 (a false statement in the STABILITY.md migration note about check).

The code change itself holds up. I reproduced the issue on origin/main 8269922b and confirmed the fix on this head with ./shipgate against synthetic repositories. I found no observable behaviour defect in the proof. The only blocking item is a published claim that the code contradicts.

Findings

  1. P2 — STABILITY.md says check refuses with its rows empty. For a plugin-reference limit behind an in-tree link, check now compares and publishes rows, and diff/verify go from partial to comparable.
    • Evidence. The migration note (STABILITY.md ~L665-670) says: "where the other routes now compare past one it refuses with unchanged_limits_not_representable … Its rows stay empty". The code does something else for a shared plugin-reference limit. _without_shared_plugin_reference_limits asks the same unchanged(source), which now proves a linked source. So check drops the limit, nothing is left to refuse, and check.py L494 only turns a comparison into unchanged_limits_not_representable when comparison.unchanged_limits is non-empty.

    • Reproduction.

      • Base: .claude/settings.json with deny: ["Bash(curl *)"]; plugins/demo/.claude-plugin/plugin.json -> ../../../vendor/plugin.json, where vendor/plugin.json is {not json; plugins/demo/cfg/hooks.json holds one hook.
      • Head commit: only the deny rule is dropped.
    • Results:

      Route origin/main 8269922b this head
      check --format agent-boundary-json incomparable [base_inventory_incomplete, head_inventory_incomplete], 0 rows comparable, 1 row (claude-code .claude/settings.json removed)
      check --format agent-control-json capability_rows.comparison_status: incomparable comparable, with the row
      diff --json, verify host_comparison partial (PR comment "Not compared: plugins/demo …") comparable, plugins/demo/.claude-plugin/plugin.json parse_failed in unchanged_limits

      In both columns, check's decision (require_review), its violations and its control_state stay the same.

    • Why it is not a code defect. This is exactly what the direct-path shape (plugin.json as a regular file) already gives on main, so the behaviour is consistent with #714/#721.

    • What is wrong. The note states the opposite. It also never mentions the partial → comparable move. The CHANGELOG.md check bullet only covers instruction files, so it is incomplete here rather than false.

    • Fix.

      • In the STABILITY note, limit the "refuses … rows stay empty" sentence to limits check cannot leave out. Say that a shared plugin-reference limit on an unchanged linked source is now left out by check just like one at its own path, so check compares and may publish rows. Say that a comparison that was partial only because of such a limit is now comparable, with the limit in unchanged_limits.
      • Mirror this in the CHANGELOG entry.
      • Pin both in tests/test_linked_unchanged_limits.py, so the note is backed by a test.

Non-blocking (P3)

  • The docstring and the PR description claim more parity with the reader than the code has. blob_path_unchanged says "a link either of them would not read through is never followed here", and the PR description says "The proof holds exactly where the reader reads SOURCE". _reader_path does not model the whole-target conditions in _reads_through_directory_link (no link anywhere under the target, no skipped name, boundary location).
    • Reproduction: base .claude/skills -> ../.agents/skills; the head adds .agents/skills/review/logo -> ../../../img/logo.txt. The head reader no longer reads SOURCE, yet blob_path_unchanged(main, HEAD, SOURCE) is True.
    • This is harmless. The head then carries an unreadable limit on .claude/skills, the two issue sets differ, and diff refuses (verified). Suggest wording the proof as necessary, not sufficient, with the issue-set equality in unchanged_limits doing the rest.
  • Ambiguous wording. "Not changed: … every row" in the same note reads oddly next to a table where rows now appear. "Every row's value" would be clearer.

Verified

  • Original defect. On origin/main, dirlink, skilllink and a file-link repository give Cannot compare against HEAD~1: base_inventory_incomplete; head_inventory_incomplete with no rows. On this head each matches direct:
    • the same ⚠ low removed … deny: Bash(curl *) → gone row;
    • limits named as the PR says (dirlink also names .agents/skills/review/SKILL.md for claude-code, codex and cursor);
    • byte-identical detail (frontmatter_invalid_structure).
  • Other routes.
    • verify host_comparison and the PR comment match diff.
    • check --format agent-boundary-json now gives [unchanged_limits_not_representable], with the same decision and violations.
    • The verifier's control.state is unchanged; only control.reason and headline change.
  • Proof edge cases (commit and working-tree head):
    • Link text with a trailing slash, ./, or other/..: the reader reads the file and the proof holds.
    • The parent of the landing directory turned into a link: refused.
    • A two-hop directory chain via a root link: the proof holds, but the reader's own unreadable on the root link refuses. That is an under-claim, not a false one.
    • A link at .claude itself: comparable.
    • Target mode 100644 → 100755: refused between commits and accepted in the working tree, the same as a direct path.
    • Linked paths stay None in blob_path_identities.
  • Acceptance criteria. Link text and target blob are both compared, and blob object IDs are compared. The cat-file blob runs under GIT_NO_LAZY_FETCH=1, and no textconv or filter runs. internal: true stays unsupported. Dangling, looping, absolute, escaping and 9-hop links, and a link nested inside the linked directory, all still refuse.
  • Tests. tests/test_linked_unchanged_limits.py has 45 tests, all passing on this head. Copied onto origin/main, 19 positive cases fail and the negative controls pass.
  • Also run and passing on this head:
    • test_unchanged_host_limits, test_host_link_read_through, test_host_file_links, test_partial_host_comparison, test_host_comparison_coverage, test_host_change_route_parity, test_manifest_free_pr_rows, test_capability_diff, test_scoped_base_tree;
    • test_adapter_static_only, test_distribution_surface_parity, test_enabled_plugin_hook_routing, test_public_surface_contract, test_host_diff_entry_docs;
    • the benchmark replays test_host_config_replay and test_cold_start_replay;
    • test_verification_git_snapshot, test_capability_diff_partial_clone, test_host_boundary_check, test_unread_changed_inputs, test_claude_hook_loading_evidence, test_host_only_advisory_recipe, test_agent_control_envelope_rows, test_host_boundary_unread_surfaces.
    • ruff check on the changed files is clean.
  • Contracts. No new surface, schema, member or reason code. The distribution-surfaces row and the parity note agree. CHANGELOG ## Unreleased sits above an untouched ## 1.1.0, and there is a Migration Note: Unreleased. No new sensitive text: sources were already published in the refusal's coverage block, and link texts are not published.
  • CI. Every check on de9b9650 passes; release-tag-consistency is skipped, as expected.

The STABILITY migration note said that where the other routes now compare
past a limit reached through an in-tree link, `check` refuses with
`unchanged_limits_not_representable` and its rows stay empty. That holds for
an instruction file's limit, which `check` cannot leave out, but not for a
plugin-reference limit: `_without_shared_plugin_reference_limits` asks the
same unchanged proof, so a `parse_failed` `plugin.json` that is a file link
to an unchanged target is now left out exactly as one at its own path has
been since #714. `check` then compares and publishes the removed `deny` row
in its boundary result and its control envelope's `capability_rows`, where
it refused with `base_inventory_incomplete` / `head_inventory_incomplete`
and no row, and `diff`, `verify` and the PR comment, which withheld
`plugins/demo` as `partial` (#808), are `comparable` with the limit in
`unchanged_limits`. `check`'s decision, violations and control state, and
`verify`'s control state and next action, do not move.

The note now splits `check` by whether it may leave the limit out, adds the
`partial` to `comparable` move, and says "every row's value" where it said
"every row". The CHANGELOG entry mirrors it, the distribution-surfaces row
and `docs/host-boundary-support.md` no longer say any change behind the link
refuses the comparison (an edited plugin manifest keeps it `partial`), and
`tests/test_linked_unchanged_limits.py` pins the plugin manifest at its own
path and behind a file link, on every route, with the edited-target control.

The `blob_path_unchanged` and `_reader_path` docstrings now state the proof
as necessary, not sufficient: it follows the links on the way to the path,
not every condition the reader puts on reading a whole linked directory, and
a side whose reader does not read the path carries no limit there. A test
holds a head that adds a link inside the linked directory to a refusal on
every route. The two pinned call-site lines in `cli/verify/git.py` move with
the docstrings.
@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor Author

Addressed review cycle 1. New head 8c6b816a (one commit on top of de9b9650; the branch already contained origin/main 8269922b, so no rebase).

P2: STABILITY.md said check refuses with empty rows (fixed)

I confirmed the finding before changing anything. _without_shared_plugin_reference_limits asks the same unchanged(source) question, so on this branch a shared plugin-reference limit behind a link is left out of check's comparison, exactly as one at its own path is.

I reproduced your case through CliRunner, with src/ taken from each tree. Base: deny: ["Bash(curl *)"]; plugins/demo/.claude-plugin/plugin.json -> ../../../vendor/plugin.json, where vendor/plugin.json is {not json; and plugins/demo/cfg/hooks.json. Head: only the deny rule is dropped.

Route v1.1.0 e3c6cb0c origin/main 8269922b this head
diff --json, verify host_comparison incomparable, 0 rows partial, 1 row, Not compared: plugins/demo, a plugin directory … comparable, 1 row, plugins/demo/.claude-plugin/plugin.json parse_failed in unchanged_limits
verify control agent_action_required, next action audit --host same state and next action; reason "Host comparison is partial: …" same state and next action; reason "1 repository-declared host capability change(s). …"
check --format agent-boundary-json incomparable [base_inventory_incomplete, head_inventory_incomplete], 0 rows same comparable, 1 row (Bash(<redacted-arguments>) removed)
check --format agent-control-json control_state: review_publishable, capability_rows incomparable same review_publishable, capability_rows comparable with the row
  • check's decision (require_review) and violations (HOST-PERMISSION-DENY-REMOVED) are the same in every column.
  • The same layout with plugin.json as a regular file gives the "this head" column on all three trees.
  • With vendor/plugin.json edited on the head, this head gives the origin/main column. The limit is kept, diff/verify stay partial, and check refuses with no row.

What changed:

  • STABILITY.md (#linked-unchanged-limits-822).

    • The check bullet is now split by whether check may leave the limit out. A limit it cannot leave out, such as the skill, still gives unchanged_limits_not_representable with empty rows. A shared plugin-reference limit is left out once the proof holds, so check compares and publishes the rows it finds, the same row as at the limit's own path. Edited behind the link, check refuses as before. Decision, violations and control state do not move.
    • A new partial becomes comparable bullet covers the Preserve independently established host changes when a plugin scope is incomparable #808 move, with the limit in unchanged_limits and no scope. It notes that 1.1.0 refused this case outright, and that verify's control state and next action stay the same while its reason becomes the comparable sentence.
    • The intro and Compatibility now say that check's comparison moves too.
    • "every row" is now "every row's value" (your P3 wording note).
  • CHANGELOG.md (## Unreleased). Mirrors the note: a plugin-manifest bullet and a corrected check bullet. I also fixed the wrap of the entry's first paragraph. ## 1.1.0 is untouched.

  • docs/distribution-surfaces.md (capability_diff row) and docs/host-boundary-support.md. Both said "any change to either refuses as before". That is false for an edited plugin manifest, which stays partial, so they now say it stays a blocking limit / is treated as before. The row also says that check compares past a shared plugin-reference limit behind such a link, and that a partial comparison becomes comparable. The row's parity note in tests/test_distribution_surface_parity.py records this; there is no new claim.

  • Tests (tests/test_linked_unchanged_limits.py); the first two are parametrised over direct and file-link:

    • test_check_leaves_out_a_shared_plugin_reference_limit_behind_a_link_as_at_its_own_path: boundary result and control envelope are comparable with the row, with decision, violation and control_state pinned.
    • test_a_shared_plugin_reference_limit_behind_a_link_is_named_not_withheld: diff text and JSON, verify and the PR comment are comparable, the limit is named, no scope, and no Not compared: plugins/demo or Partial line appears.
    • test_a_plugin_manifest_edited_behind_the_link_is_still_withheld: the negative control. The comparison stays partial, and check is incomparable with no row.

    Copied onto the origin/main source, the two file-link positive cases fail, and the direct cases and the negative control pass.

P3 (nonblocking, also addressed)

  • Necessary, not sufficient. The _reader_path and blob_path_unchanged docstrings now say the proof follows the links on the way to the path, and not every whole-directory condition in _reads_through_directory_link. So True is necessary, not sufficient. Its callers only ask about a limit both sides carry on the path: unchanged_limits requires equal blocking-issue sets, and check intersects them. A side whose reader does not read the path carries no limit there.
    • New test test_a_link_added_inside_the_linked_directory_still_refuses pins your reproduction: head adds .agents/skills/review/logo -> ../../../img/logo.txt. diff/verify are incomparable with no rows or limits and a head-side unreadable item, and check is incomparable with no rows.
    • I have not edited the PR description. Its sentence "The proof holds exactly where the reader reads SOURCE" should read "The proof holds wherever the reader reads SOURCE; where the reader does not, the side that does not read it carries no limit there, so nothing is named".
  • Pinned call sites. The docstrings moved the two pinned call-site lines in cli/verify/git.py again: Popen 2855→2872, run 3255→3272 in tests/test_adapter_static_only.py.

Tests run on this head (all passing)

  • tests/test_linked_unchanged_limits.py: 51 tests.
  • test_adapter_static_only, test_distribution_surface_parity, test_unchanged_host_limits, test_partial_host_comparison, test_host_link_read_through, test_host_file_links, test_host_comparison_coverage, test_host_change_route_parity, test_manifest_free_pr_rows, test_enabled_plugin_hook_routing, test_public_surface_contract, test_host_diff_entry_docs, test_capability_diff, test_scoped_base_tree.
  • test_host_boundary_unread_surfaces, test_unread_changed_inputs, test_verification_git_snapshot, test_capability_diff_partial_clone, test_host_boundary_check, test_host_only_advisory_recipe, test_agent_control_envelope_rows, test_claude_hook_loading_evidence, test_host_diff_review_changes.
  • Benchmark replays test_host_config_replay and test_cold_start_replay: the run-of-record scores reproduce.
  • ruff check on the changed Python files is clean.

@pengfei-threemoonslab pengfei-threemoonslab left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent coding-agent review/address cycle 2 at 8c6b816a.

No P0-P2 findings.

I reviewed the full diff from origin/main 8269922b (still the merge base, so no rebase is needed), the cycle-1 review and its address, and #822. I ran ./shipgate from a detached worktree of each tree against synthetic repositories.

Cycle-1 finding: fixed

  • P2, STABILITY.md said check refuses with its rows empty. Fixed.
    • I rebuilt the cycle-1 reproduction: plugins/demo/.claude-plugin/plugin.json -> ../../../vendor/plugin.json, where vendor/plugin.json is {not json, and a head that only drops deny: Bash(curl *).
    • Results on main → this head:
      • check --format agent-boundary-json: incomparable [base_inventory_incomplete, head_inventory_incomplete] with 0 rows → comparable with the redacted deny row.
      • agent-control-json: capability_rows incomparable → comparable with 1 row. control_state stays review_publishable.
      • diff --json: partial → comparable, with the manifest parse_failed in unchanged_limits.
      • Decision (require_review) and violations (HOST-PERMISSION-DENY-REMOVED) do not change.
    • This head gives exactly what the same layout with a regular plugin.json gives. The split check bullet, the new "partial becomes comparable" bullet, the CHANGELOG entry, the distribution-surfaces row and docs/host-boundary-support.md now state exactly that.

Verified

  • Original defect and fix. I built the issue's repositories: direct, dirlink, skilllink, and a file link at SOURCE.

    • On main, the three link layouts print Cannot compare against HEAD~1: base_inventory_incomplete; head_inventory_incomplete with no row.
    • On this head each is comparable, shows ⚠ low removed … deny: Bash(curl *) → gone, and names the limits the PR states.
    • verify agrees: host_comparison is comparable with 1 row. control.state (agent_action_required) and the next action (audit --host) are the same as on main; only reason changes.
    • The skilllink PR comment on this head is byte-identical to the direct comment on main, apart from paths and SHAs.
    • check gives [unchanged_limits_not_representable] for every layout, as for direct, with the same decision and violations.
  • Proof soundness. I checked _reader_path rule by rule against the reader's _resolve_in_tree_link / _reads_through_directory_link and the archive's _resolve_tree_link: lexical normalization, landed prefixes that must be directories, the eight-link bound, and a link at one component only.

    • The reader always opens the in-tree path it resolved lexically, never through a link, so the path the proof hashes is the file the reader parses on each side.
    • Where the proof is True but a side's reader does not read the path (a link added inside the linked directory), that side carries unreadable instead. The issue sets differ, and every route still refuses.
    • cat-file blob runs on a regex-validated object ID, under GIT_NO_LAZY_FETCH=1 with a 4096-byte cap. No textconv or filter runs.
  • Extra adversarial shapes. Each was run with a commit head and with a working-tree head. All refuse on diff, and the proof is False:

    • the target directory .agents/skills replaced by a link to an identical copy;
    • the file-link target's parent shared/ replaced by a link to an identical copy;
    • .claude replaced by a link to a copied directory;
    • link text rewritten with a trailing slash.

    Two further shapes behave like a direct path:

    • A mode change on a file-link target is refused between commits and accepted against the working tree, the same as a direct path.
    • A case-only rename of .agents on macOS makes the proof True, but the reader refuses. That is an under-claim, not a false one.
  • Plugin side effect. I checked the case where an enabled plugin's cfg/hooks.json changes while an unparseable manifest behind a link stays unchanged. The result moves from partial to comparable with no row. That is exactly what main already gives for the manifest at its own path.

    • A manifest the host can read but that has duplicate keys parses here too, and publishes the hook row on both layouts.
    • So this is parity with the released direct behaviour, not a regression from this PR.
  • Tests. tests/test_linked_unchanged_limits.py has 51 tests, and all pass here. Copied onto origin/main, 21 fail (every positive case) and 30 pass (the negative controls).

    • Also run and passing on this head:
      • test_unchanged_host_limits, test_host_link_read_through, test_host_file_links, test_partial_host_comparison, test_host_comparison_coverage, test_host_change_route_parity, test_manifest_free_pr_rows, test_capability_diff, test_scoped_base_tree;
      • test_adapter_static_only, test_distribution_surface_parity, test_enabled_plugin_hook_routing, test_public_surface_contract, test_host_diff_entry_docs;
      • the benchmark replays test_host_config_replay and test_cold_start_replay (the run-of-record scores reproduce);
      • test_verification_git_snapshot, test_capability_diff_partial_clone, test_host_boundary_check, test_unread_changed_inputs, test_host_only_advisory_recipe, test_agent_control_envelope_rows, test_host_boundary_unread_surfaces, test_claude_hook_loading_evidence, test_host_diff_review_changes;
      • test_reusable_workflow_secret_mappings, test_workflow_label_redaction, test_workflow_step_action_references, test_verify, test_verify_orchestrator, test_prompt_disabling_settings, test_host_directory_inputs, test_release_pipeline.
    • ruff check . is clean.
  • Contracts.

    • No new command, schema, member, reason code or check id.
    • The CHANGELOG entry sits under ## Unreleased, and ## 1.1.0 is untouched. STABILITY.md has Migration Note: Unreleased (#linked-unchanged-limits-822).
    • The distribution-surfaces row and the parity note agree.
    • No new sensitive text: sources were already published in the refusal's coverage block, and link texts are never published.
  • CI. Every check on 8c6b816a passes; release-tag-consistency is skipped, as expected.

Non-blocking (P3)

  1. Cost of the proof through a link.
    • Each linked source builds fresh _CommitEntries and re-lists every shared component: .claude, .claude/skills, the link text, .agents, .agents/skills. With a commit head that is 18 Git processes per source, against 4 for a direct path.

    • With 60 unchanged-limit skills behind .claude/skills -> ../.agents/skills, measured wall time:

      Route direct through the link
      diff 3.8 s 15.2 s
      verify 6.4 s 25.6 s
      check 6.2 s 25.7 s

      On main the linked case refused in 4.0 s.

    • This is correct but grows linearly with the number of limits. Memoizing the two _CommitEntries per commit across one comparison's unchanged calls would remove most of it. It could be a follow-up.

  2. The PR description has drifted from the head.
    • It still says 45 tests (now 51).
    • It still gives the pinned lines as 2855 / 3255 (now 2872 / 3272).
    • "The proof holds exactly where the reader reads SOURCE" overstates the proof. The docstrings now correctly say it is necessary, not sufficient.
    • The check paragraph says rows stay empty, but it does not mention that check now compares a shared plugin-reference limit behind a link. STABILITY.md and the CHANGELOG already say this.

@pengfei-threemoonslab pengfei-threemoonslab left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed original head 8c6b816a0e1783e6e67137502e317a15a08e026e and its prior review/address cycle. No new actionable findings. I checked the Git-tree/working-tree link identity proof, bounded link traversal, refusal paths, and shared plugin-reference handling.

Validation: 227 tests passed across linked and unchanged limits, partial comparison, host links, coverage and Git snapshots. After integrating merged #851, 550 tests passed and 1 skipped across linked limits, coverage, hook/MCP detail and distribution parity; Ruff is clean. Only the two overlapping distribution-documentation/comment conflicts needed manual combination; both feature descriptions are preserved. Fresh verification of the resolved tree returned control_state=complete. The integration commit is e61eba64; committed-head verification and GitHub CI must also pass before squash merge.

@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor Author

Final integration check at 624fe55e1150f9869655f3d38e53ffeee24d1aa6 against current main: required CI checks all pass, the reviewed change remains scoped, and fresh local Agents Shipgate verification reports control_state=complete with merge permission. Proceeding with the requested squash merge. No branch rules or release controls were bypassed.

@pengfei-threemoonslab
pengfei-threemoonslab merged commit 28d0644 into main Sep 24, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An unchanged instruction limit reached through an in-tree link refuses the whole comparison instead of being named unchanged

1 participant