Skip to content

Keep Hub tests/ Paths Out of the Hub-Only Detector - #3540

Merged
ptr727 merged 9 commits into
developfrom
feature/3528-audit-hub-tests-collision
Oct 28, 2026
Merged

ptr727 merged 9 commits into
developfrom
feature/3528-audit-hub-tests-collision

Conversation

@ptr727

@ptr727 ptr727 commented Oct 27, 2026 •

Copy link
Copy Markdown
Owner

Summary

spec/audit.py reported a carrier's own tests/<name>.py as hub-only: whenever the hub has a test module of the same name. Its remedy then suggested deleting the file. The hub's tests/ tree is never carried, so such a hit is always a path collision.

This closes the class rather than adding one gaps entry per instance.

  • spec/files.json gains a repoOwnedTrees declaration, listing tests with its reason.
  • spec/files.schema.json describes the new array.
  • hub_only_paths drops every hub path below a declared repo-owned tree. Both of its consumers inherit this: the audit's hub-only check and the fidelity-honesty manifest gap pass.
  • A new hub_paths_below helper holds the declared-source walk, shared by both hub_only_paths loops and check_tree_verbatim.
  • AUDIT.md's carried-scope bullet, the carried.hubonly.absent assertion in spec/project-types.json, and RESYNC.md section 4 each name the exclusion.
  • spec/validate.py checks the repoOwnedTrees shape beside its trees checks. It refuses a missing source or reason, a source at or above the repository root or not in normalized form, a source with a symlinked component, and a source that is not a directory.
  • tests/test_audit.py adds a case where a carrier's tests/test_a.py shares a constructed hub test's name and is not reported. A control shows a hub-only path outside tests/ is still reported. Its fixture loads the shipped declaration, so a malformed entry or a dropped tests fails the suite.
  • tests/test_spec_validate.py adds RepoOwnedTreeGateCase, running the real gate against a scratch tree for each refusal and a plain entry. It shares a new run_gate_on_files runner with TreePathGateCase.

Verification

  • The full unittest suite passes under the OPERATIONS.md invocation.
  • With the tests declaration renamed, both new audit cases fail.
  • ruff check, ruff format --check, mypy, spec/validate.py, and scripts/prose_lint.py --diff origin/develop are clean.

Pre-existing findings, not filed

  • AUDIT.md:88, pre-existing: prose_lint.py --check sentence-length flags three sentences of 36, 53, and 35 words in the carried-scope bullet. This change added one 25-word sentence there and wrote none of the three.
  • RESYNC.md:92, pre-existing: the same check flags a 41-word sentence in section 4. This change added a shorter sentence before it.
  • AUDIT.md:88, RESYNC.md:92, spec/project-types.json:161, pre-existing: each derivation names the baseline and now the repo-owned exclusion, but not the carried trees exclusion hub_only_paths has always applied.

Dropped findings

  • scripts/canonical_review.py:303, consolidation on text this change did not touch: its tree walk could reuse hub_paths_below.

Closes on promotion: #3528

🤖 Generated with Claude Code

ptr727 and others added 3 commits October 27, 2026 16:04
The hub's tests/ tree is never carried, so a carrier's tests/ file at a
hub test path is a path collision rather than a hub-hosted copy. Declare
tests as a repo-owned tree in spec/files.json, and have hub_only_paths
drop every hub path below a declared repo-owned tree.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Factor the declared-source walk into hub_paths_below, used by both
hub_only_paths loops and check_tree_verbatim. Test the shipped
repoOwnedTrees shape, since no gate reads the schema. Name the exclusion
in AUDIT.md's carried-scope bullet.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The audit cases already load the shipped declaration, so a missing
source fails them loudly, and no code reads the reason field.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings October 27, 2026 23:15
@ptr727

ptr727 commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

A recorded local strict-review pass covers head 30a9ff09f5e40e72aaba57471e1060b8a1751044, the content this pull request carries at that commit against develop, and it recorded 0 findings.

@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Active issues remain because the carried-scope contract is inconsistent with the implementation and repo-owned-tree declarations lack boundary validation.

2 open issues
What changed in this PR

This pull request prevents the hub-only audit from reporting false positives for carrier-owned files under the hub’s tests/ tree while preserving detection of hub-only paths elsewhere.

Changes:

  • Adds and documents the repoOwnedTrees manifest declaration.
  • Centralizes repo-owned path filtering through hub_paths_below.
  • Applies filtering to hub-only and fidelity checks.
  • Adds regression coverage for test-path collisions and control paths.
  • Updates the carried-scope documentation.
File Summary Review findings
tests/​test_audit.py Adds regression coverage for repo-owned test paths and hub-only control paths. None
spec/​files.schema.json Defines the repoOwnedTrees manifest structure. Moderate, 3 votes: validate declarations at the manifest boundary, including required shape, valid paths, and rejection of the repository root.
spec/​files.json Declares tests as a repo-owned hub tree. Moderate, 1 vote: add corresponding validation in spec/validate.py so malformed or invalid declarations fail validation rather than at audit time.
spec/​audit.py Adds shared hub-path filtering and excludes repo-owned trees from audit consumers. None
AUDIT.md Documents the carried-scope exclusion behavior. Three documentation findings at line 88: update the carried-scope description and related spec/project-types.json and RESYNC.md guidance to include the new exclusion.

Review effort: Lite

Comment thread spec/files.schema.json
Comment thread AUDIT.md
@codecov

codecov Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 26.66667% with 22 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (develop@4752a2e). Learn more about missing BASE report.

Files with missing lines Patch % Lines
spec/validate.py 0.00% 21 Missing ⚠️
spec/audit.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #3540   +/-   ##
==========================================
  Coverage           ?   81.47%           
==========================================
  Files              ?       26           
  Lines              ?     8995           
  Branches           ?        0           
==========================================
  Hits               ?     7329           
  Misses             ?     1666           
  Partials           ?        0           
Flag Coverage Δ
python-3.13 81.47% <26.66%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

spec/validate.py refuses a repoOwnedTrees entry with no usable source or
reason, a source at or above the repository root, and a source that is
not a directory, since a misspelled one would exclude nothing. The
carried.hubonly.absent assertion now names the exclusion.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ptr727

ptr727 commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

A recorded local strict-review pass covers head db231921eb242c06cb01fe6445cad547c462abba, the content this pull request carries at that commit against develop, and it recorded 3 findings.

ptr727 and others added 3 commits October 27, 2026 16:26
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A source such as ./tests passes the directory check but excludes
nothing, so the gate now refuses any source that is not in normalized
form. Both scratch-tree gate cases run through one helper.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ptr727

ptr727 commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

A recorded local strict-review pass covers head aceb6583d3dd434072ee058dce42959358295b3d, the content this pull request carries at that commit against develop, and it recorded 0 findings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A moderate active finding remains because symlinked repo-owned source directories can pass validation while being interpreted differently by Git tree traversal, so the declared hub tree may be silently excluded or mismatched.

1 open issue
2 resolved since last review

Review effort: Lite

Comment thread spec/validate.py
git tracks no file below a symlink, so a source with a symlinked
component passes the directory check but excludes nothing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ptr727

ptr727 commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

A recorded local strict-review pass covers head 4d1a409cd3056e6e110dca69ba2a2eecfba8ce52, the content this pull request carries at that commit against develop, and it recorded 0 findings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The fixture omits the declared repo-owned tests directory, causing the gate to report an active manifest error and leaving the clean-path test insufficiently verified.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Create declared tests directory in scratch tree

tests/​test_spec_validate.py:580

[P2] Make the scratch tree satisfy the shipped manifest. run_gate_on_files copies the current spec/files.json, which now declares tests as a repo-owned source, but this fixture only creates real/sub. Every invocation therefore emits files.json: repo-owned tree tests is missing or not a directory; notably, test_a_plain_entry_draws_no_error can still pass because it only searches for real/sub, so it does not prove the clean declaration is accepted. Create root/tests (or replace the copied declaration) before running the gate.

1 resolved since last review

Review effort: Lite

The copied manifest declares tests as repo-owned, so the scratch tree
creates it, and a new case proves the shipped declaration passes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ptr727

ptr727 commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

A recorded local strict-review pass covers head 1fa73a0f68736b0875ee575726b13b159d5f465b, the content this pull request carries at that commit against develop, and it recorded 0 findings.

@ptr727

ptr727 commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Answering the previously missed finding (1) from the Copilot round on 4d1a409, #3540 (review):

tests/test_spec_validate.py:580 [P2] Make the scratch tree satisfy the shipped manifest. run_gate_on_files copies the current spec/files.json, which now declares tests as a repo-owned source, but this fixture only creates real/sub.

Fixed in 1fa73a0. run_gate_on_files now creates tests in the scratch tree. A new test_the_shipped_declaration_draws_no_error runs the gate on the shipped declaration and asserts no repo-owned error names it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is warranted because the change’s behavior is only partially covered, leaving the manifest_gap_pass consumer of hub_only_paths without verification.

No open issues 🎉

Review effort: Lite

@ptr727

ptr727 commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Answering the verdict headline (1) of the Copilot round on 1fa73a0, #3540 (review):

Human assessment is warranted because the change’s behavior is only partially covered, leaving the manifest_gap_pass consumer of hub_only_paths without verification.

No change needed. manifest_gap_pass adds no filtering of its own. It takes audit.hub_only_paths(spec) whole at spec/fidelity_honesty.py:173 and only intersects each carrier tree with it at line 185. The exclusion lives entirely inside hub_only_paths, which AuditRepoHubOnlyCase in tests/test_audit.py drives with the shipped declaration, a collision case, and a control. A second test through manifest_gap_pass would exercise the same function again.

@ptr727
ptr727 merged commit 9457128 into develop Oct 28, 2026
14 checks passed
@ptr727
ptr727 deleted the feature/3528-audit-hub-tests-collision branch October 28, 2026 00:15
ptr727 added a commit that referenced this pull request Oct 28, 2026
…y Tests, and Copilot Transient Fixes (#3544)

## Summary

Promotes `develop` to `main`, carrying round 1 of the resync-issue
burndown:

- #3538: `AGENTS.md` "Model Tiers" now states the no-model dispatch
result once, conditionally, in the `highest` bullet. The two bullets
that contradicted it lost their unconditional claims. The three skills
including the section are regenerated.
- #3539: `GOVERNANCE.md` "Durable Knowledge and Self-Improvement"
describes the standing-issue handoff lifecycle, matching `AGENTS.md`
"Session Scope". `agent-conduct` is regenerated.
- #3540: `spec/files.json` gains a `repoOwnedTrees` list declaring
`tests`, and `spec/audit.py` `hub_only_paths` leaves those paths out. A
carrier's own `tests/` file is no longer reported as hub-only.
`spec/validate.py` shape-checks the list, and `AUDIT.md`, `RESYNC.md`
and the project-types assertion state the exclusion.
- #3542: `pr_review.py wait` and `status` read the Copilot run log
before calling an error refusal quota. A `too many files changed` line
is transient and earns one re-request, as `COPILOT_ERROR_TRANSIENT` on
the existing exit 41.
- #3545: promotion fix for Copilot's finding on this pull request.
`spec/validate.py` refuses a `repoOwnedTrees` source that overlaps a
carried `trees[].source` or holds a `baseline` path.

Fixes #3529
Fixes #3504
Fixes #3528
Fixes #3510

🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.

2 participants