docs(research): CODEOWNERS expresses the boundary; a naive CI check would fail on drops (#369) - #429
docs(research): CODEOWNERS expresses the boundary; a naive CI check would fail on drops (#369)#429tucktuck101 wants to merge 2 commits into
Conversation
e16d543 to
e2dd35b
Compare
Updated for the handbook source-material contractForce-pushed an amended commit bringing this document into line with two conventions introduced after it was written, since it is unmerged and retrofitting after merge is the expensive case: 1. Every reference pinned to a full 40-character SHA. Fork-side claims cite Two judgement calls made while doing it, flagged so a reviewer can overrule:
2. Recommendations separated from evidence and attributed. Each No finding, figure or caveat changed. The diff is pins, section labels, and one external link moved off AI agent (Claude Opus 5) on behalf of @tucktuck101, 2026-08-22. |
…I check breaks on drop PRs (#369) Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
e2dd35b to
6d1ffa7
Compare
Revised for the fork's horizon (#357)Force-pushed an amended commit adding a No evidence, figure or caveat changed. Every measurement and quotation stands exactly as reviewed. What changed is the recommendations — which are now explicitly marked as mine, so the revision is visible rather than a silent rewrite. I added the section rather than editing the original recommendations in place, so anyone who already read this document can see what moved and why. Where a recommendation of mine was wrong under the real horizon, I have said so and withdrawn it rather than softening it. The reversals are named in the section. AI agent (Claude Opus 5) on behalf of @tucktuck101, 2026-08-22. |
serina-mcfall
left a comment
serina-mcfall
left a comment
There was a problem hiding this comment.
Blocker — "27 contested files" is 8
Line 118, in "The other three, briefly":
Also
lefthook.ymlis one of the 27 contested files
$ MB=f8692fa9b; UP=$(git rev-parse upstream/main); FORK=$(git rev-parse origin/launchpad)
$ comm -12 <(git diff --name-only $MB $FORK | sort) <(git diff --name-only $MB $UP | sort) | wc -l
8
Eight. And #425 — same author, same day, same pinned refs — states 8 and lists them by name.
27 is #425's "in-place edits" figure, reused here under a different and incompatible meaning. "Contested files" is the ledger's scope number: a maintainer reading this will size ADR-0022's adjudication surface at 27 when it is 8, and the figure now appears in two of your documents carrying two meanings.
(The lefthook.yml membership claim itself is fine — it is in the contested 8. Only the count is wrong.)
High — "the CI check breaks on drop PRs" is a deduction presented as a measurement
It is in the PR title and the summary line:
the CI check breaks on drop PRs … the check is useless exactly when the ledger matters most
The note's own table at line 22 records the check's status as "No — no required checks exist", and line 108's heading is "Neither can block today" with {"checks":null}.
So nothing was observed breaking. The 981-file input is real, the deduction is sound, and I am not disputing the conclusion — but the qualifier sits below the claim, which is the pattern that gets a document quoted wrongly. Rewording the summary to "would break" costs one word.
Medium — the pasted partition was computed against a tree two files older than the pinned SHA
note: tracked 4351 / unowned 322 / "321 fork additions"
mine: tracked 4353 / unowned 324 / 323 fork additions
Owned matches exactly at 4029, so the entire skew is on the fork side, and the note's internal arithmetic is self-consistent (4029 + 322 = 4351). But the document pins 5d76799d6 and says "the CODEOWNERS partition was computed against that same tree", and it wasn't quite. Small — except that this is a pinned-reference document, and the pinning discipline is the whole point of the genre.
What is right — the headline result reproduces exactly
I implemented your six-line pattern set as printed (last-match-wins, no-owner line un-owns) and ran it over every tracked blob at the pinned fork SHA:
tracked files: 4353
CODEOWNERS-owned: 4029 <- your figure, exact
CODEOWNERS-unowned: 324
fork-added files STILL owned: 2
desktop/src-tauri/src/managed_agents/runtime/summary.rs
scripts/test-ci-changed-paths-filter.sh
4,029 exactly, and a residue of exactly those two files, by name. The six lines really are six lines, every excluded path exists at the counts you give (launchpad/ 303, .claude/ 8, .github/ISSUE_TEMPLATE/ 7, .github/workflows/launchpad-* 5, LAUNCHPAD.md 1), and the .claude/skills/* accuracy argument holds — those 8 files would be false positives for a path-based check.
gh pr view 216 --json changedFiles → 981, exact. The lefthook.yml quotation at lines 14-15 is verbatim.
You also flag your own main limit — that the pattern set was not validated against GitHub's own CODEOWNERS parser — before recommending adoption rather than after. That is the right shape, and it is what the "breaks on drop PRs" line should have done too.
Reviewed at head 6d1ffa79c. The contested-file recount was run by me directly.
🤖 Review drafted by Claude Code (claude-opus-5) for @serina-mcfall.
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
tucktuck101
changed the title
2 participants
Summary
Adds one research document assessing five mechanisms for enforcing the upstream-file boundary. CODEOWNERS — absent from #301's options — expresses the boundary in six lines, tested against the real 4,351-file tree with a two-file residue that is arguably correct. Measured the CI-check option's real cost against 60 merged PRs: 11 fired, 8 correctly, 2 false positives CODEOWNERS gets right, and one catastrophic case — the 981-file drop PR.
Related issue
Closes #369
Issue type
Task
Agent provenance
Objective
Add
launchpad/Research/369-enforcing-the-upstream-boundary.mdrecording what mechanisms could enforce a ledger row for upstream-owned files and what each costs a contributor.Impacted components
Approach and rejected alternatives
Did the two things this issue's definition of done actually asks for and that are usually skipped: tested the CODEOWNERS pattern set against the real file list rather than reasoning about the syntax, and measured the CI check's false-positive rate against real history rather than estimating it.
Classified all 11 firing PRs individually by what they did, so the rate splits into correct firings, false positives and the catastrophic case — which is what turns "18% of PRs" from a number into a design finding.
Rejected: reporting the CI check's cost as a single rate. 11-of-60 hides the shape. Eight firings were correct, two were a specific class CODEOWNERS handles better, and one was a 981-file drop PR. The bimodality is the finding; the average would have concealed it.
Rejected: excluding the two residue files with two more no-owner lines. Both are fork-only files living inside upstream's directory structure, and
summary.rsis where #360 found the drop's semantic conflict. They deserve boundary review, so the document argues for leaving them owned rather than tuning the pattern set to look cleaner.Rejected: treating "can it block" as the discriminator. Neither can today, and both need the same single admin toggle — so it separates nothing. The real choice is between two different assurances, and the document says so.
Rejected: recommending a mechanism. #301 owns the decision. The document notes that neither half alone closes the drop-PR hole and stops there.
Verification
Command run:
Raw output:
Branch-protection state quoted from #358's measurement:
{"approvals":1,"checks":null,"codeowners":false,"enforce_admins":false}.Not verified
The CODEOWNERS test used a local implementation of the documented semantics, not GitHub's parser. GitHub exposes
GET /repos/{owner}/{repo}/codeowners/errors, but it validates a file committed to the repository and committing one is a change I am not making. The partition is correct per the documented rules; whether GitHub's implementation agrees on every edge — particularly/.github/workflows/launchpad-*.yml, the only pattern using a wildcard — is unconfirmed. That is the one thing to check before adopting it, and it costs one commit on a throwaway branch.I sampled the 60 most recent merged PRs, not all ~216, so 11/60 is recent-history rather than lifetime.
@launchpad-26/boundary-reviewersdoes not exist — the pattern set names a team that would have to be created, and I did not check whether the org lets repository admins create teams. I did not enablerequire_code_owner_reviewsto observe its behaviour. I did not test thelefthookhook option at all: the bypass claim rests on--no-verifybeing documented and on PR #216 having used it, not on my running it. I did not measure how long a boundary reviewer would take on a drop PR, which is the real cost of the CODEOWNERS option and the number I would most want. No builds were run — disk is at 99% capacity with 5.2 GiB free — and nothing here needed one.Security implications
None from the diff. Two findings bear on assurance and are recorded rather than acted on. First, a ledger check must exempt drop PRs to be usable at all, and the drop is exactly when a lost divergence decision is most likely — a gate that switches itself off during the event it guards provides less assurance than its presence implies. Second, under ADR-0022 a contested file with no ledger row is never presented for adjudication, so a missing row is a lost decision rather than a documentation gap; that raises the stakes on whichever mechanism is chosen and is why the document argues neither half alone closes the hole.
Escalations
#301's options are missing the cheapest and most accurate mechanism. CODEOWNERS is not in that issue. Six lines, tested, O(1) cost per PR versus O(files) for a CI check, and it correctly excludes the
.claude/skills/*class the path-based check gets wrong.#301 is choosing between two different assurances and should say which it wants. "Someone reviewed the boundary" and "the divergence is written down" are different properties and neither implies the other. Framing it as an implementation choice obscures the actual decision.
The "needs an org admin" premise that shaped #301 is false, per #358 — four people including the owner of this work hold repository admin, and both mechanisms need the same single toggle. That was flagged on #358 and is repeated here because it changes this issue's cost analysis specifically.
A team would need creating. Whatever pattern set is adopted names an owning team that does not exist today. Small, but it is a prerequisite nobody has listed and it needs someone with org permissions.