Skip to content

fix(babysit-prs): casefold owner/repo/login compares in babysit scripts - #1329

Merged
kyle-sexton merged 3 commits into
mainfrom
fix/815-casefold-owner-repo-login-babysit
Jul 25, 2026
Merged

fix(babysit-prs): casefold owner/repo/login compares in babysit scripts#1329
kyle-sexton merged 3 commits into
mainfrom
fix/815-casefold-owner-repo-login-babysit

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • babysit_delta.py's head_repository_scope used .lower() for the base-owner, head-owner,
    same-repository, and configured-owners comparisons; babysit_feedback.py's
    latest_reviews_by_author used .lower() on the per-reviewer login key. The sibling
    pr_queue_snapshot.py already casefolds the identical owner/repo/login identity concept
    (.casefold()), so these two were the last stragglers against that ratified convention.
  • Converted both files' identity comparisons from .lower() to .casefold(). Functionally
    equivalent today (GitHub owner/repo/login values are ASCII-only), but keeps every babysit
    script agreeing on one normalization primitive for this concept.
  • Added regression tests: head_repository_scope now covers a differently-cased base repo,
    headRepository.nameWithOwner, and allowlisted owner all still resolving as a match (same-repo
    and cross-repo paths); latest_reviews_by_author covers a differently-cased login (Rev vs
    rev) collapsing to one latest review instead of two.
  • Bumped source-control to 0.26.4 with a matching CHANGELOG entry.

Closes #815

Test plan

  • python -m pytest tests/test_babysit_delta.py tests/test_babysit_feedback.py -q — 89
    passed, 16 subtests passed
  • python -m pytest tests/ -q (full babysit-prs suite) — 351 passed, 58 subtests passed
  • grep -n "\.lower(" babysit_delta.py babysit_feedback.py — no matches remain
  • scripts/check-changelog-parity.sh --check and --check-bump origin/main — both pass after
    the origin/main merge

Related

This was generated by AI during work-loop execution.

@kyle-sexton @claude
fix(babysit-prs): casefold owner/repo/login compares in babysit scripts
90111f8 
babysit_delta.py's head_repository_scope and babysit_feedback.py's
latest_reviews_by_author used .lower() for owner/repo/login identity
comparisons where the sibling pr_queue_snapshot.py already casefolds
the identical concept. Converts both to .casefold() and adds
case-insensitivity regression tests locking in the behavior.

Closes #815

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 32s —— View job

PR Review

  • Gather context and understand the request
  • Read changed files
  • Review code for correctness, security, and alignment
  • Post findings

Summary: Minimal, targeted consistency fix. The two stragglers (babysit_delta.py and babysit_feedback.py) now agree with pr_queue_snapshot.py on .casefold() as the normalization primitive for GitHub identity comparisons. Functionally equivalent for GitHub's ASCII-only namespace, but correct by convention and future-safe.

Correctness

All five changed call sites are correct:

No other .lower() calls on identity values remain in either file, matching the grep confirmation in the test plan.

Tests

The three new tests cover all the targeted paths:

  • test_owner_repo_login_comparisons_are_case_insensitive exercises the same-repo path with mixed-case repo, nameWithOwner, and login — the same_repository casefold comparison is implicitly verified because branch_write_allowed can only be True when same_repository is True (given the cross-repo flag is False by default in make_pr).
  • test_cross_repo_head_owner_case_insensitive_allowlist_match exercises the cross-repo allowlist lookup with an upper-cased login, cleanly isolating the head_owner in configured_owners path.
  • test_differently_cased_login_still_collapses_to_one_latest_review is the key regression for babysit_feedback.py — two reviews from "Rev" and "rev" must collapse to one, with the later timestamp winning.

Minor nit

test_cross_repo_head_owner_case_insensitive_allowlist_match has no docstring, unlike the first new delta test. Not a blocker — the name is self-explanatory — but the sibling test set a precedent with an intent-statement docstring and consistency would be nice. Fix this →

Version bump and changelog

0.26.2 → 0.26.3 is the right semver level (patch: bug fix / consistency). The CHANGELOG entry is clear and links the issue. ✓

Overall: clean, well-scoped change. Ready to merge pending the optional docstring nit.

@claude

claude Bot commented Jul 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 6s —— View job

Security Review

  • Gather context and understand the request
  • Review PR diff
  • Read changed files
  • Analyze for security vulnerabilities
  • Post security review findings

No security issues found.

This PR makes a targeted normalization change (.lower() → .casefold()) in two trust-boundary functions. Here's the security-relevant analysis:

Scope of change

The two changed call sites are both in security-critical paths:

  • head_repository_scope — gates branch_write_allowed and base_repo_allowed, the access-control signals that determine whether the engine is permitted to perform automated mutations (merges, branch writes) on a given PR.
  • latest_reviews_by_author — deduplicates per-author review state, used by collect_feedback to classify human blocking signals.

Why this is safe

The PR description correctly states that GitHub owner/repo/login values are restricted to ASCII. For all-ASCII input, str.casefold() and str.lower() produce identical output — there is no character in the ASCII range for which the two functions differ. The change therefore cannot alter any access-control decision for any real GitHub-sourced value.

Allowlist input (allowed_owners)

The configured_owners set is built from caller-supplied configuration, not from PR data — so an external actor cannot influence what gets put into the allowlist. Even if a configuration author supplied a non-ASCII owner name (which GitHub's identifier rules prevent), .casefold() could only make the comparison more restrictive (e.g., folding ß → ss would require an exact match on the folded form), not less.

No new attack surface introduced

  • No new code paths, branches, or logic changes — all four changed expressions are drop-in replacements in existing conditional checks.
  • No new data flows, external inputs, or outputs.
  • No GitHub Actions workflow changes.
  • No credential or secret handling.
  • No new dependencies or supply-chain exposure.

This was referenced Jul 25, 2026
Closed
Merged
Closed
@kyle-sexton
Merge remote-tracking branch 'origin/main' into fix/815-casefold-owne…
a049bce 
…r-repo-login-babysit

# Conflicts:
#	plugins/source-control/CHANGELOG.md
@kyle-sexton
Merge origin/main into fix/815-casefold-owner-repo-login-babysit
654d9be 
Compose both CHANGELOG sides: main's 0.26.4 gate-separation entry keeps
0.26.4; this branch's casefold entry moves to a new 0.26.5 section, and
the plugin manifest bumps to 0.26.5.
@claude

claude Bot commented Jul 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 32s —— View job

PR Review

  • Gather context and understand the request
  • Read changed files
  • Review code for correctness, security, and alignment
  • Post findings

Summary: This is a clean, minimal consistency fix. The five .lower() → .casefold() substitutions in babysit_delta.py and babysit_feedback.py now match the convention already in pr_queue_snapshot.py. Functionally equivalent for GitHub's ASCII-only namespace; the fix is the right call for long-term coherence.

Version bump

Main is at 0.26.4; this PR introduces 0.26.5. Correct patch-level bump for a bug fix / consistency change. The CHANGELOG entry at CHANGELOG.md:6 is clear and links the issue. ✓

Correctness

All five changed call sites are correct:

No .lower() calls on identity values remain in either file. ✓

Tests

Three new tests, all well-targeted:

Security

No concerns — str.casefold() and str.lower() are identical for all ASCII input, so no access-control decision can change for real GitHub-sourced values. The configured_owners allowlist is caller-supplied, not attacker-controlled.

Overall: clean, well-scoped change. Ready to merge. The missing docstring nit from the previous review round is the only outstanding item, and it's not a blocker.

@claude

claude Bot commented Jul 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 26s —— View job

Security Review

  • Gather context and understand the request
  • Fetch PR diff
  • Read changed files
  • Analyze for security vulnerabilities
  • Post security review findings

No security issues found.

This PR makes a single normalization change (.lower() → .casefold()) across two security-critical trust-boundary functions and their tests. Here is the full security analysis.

Changed attack surface

head_repository_scope — the primary access-control gate. Its output fields base_repo_allowed, review_trigger_allowed, and branch_write_allowed determine whether the babysit engine may perform automated mutations (merges, thread resolution) on a given PR. Four call sites changed:

  • L172 — base_owner from GitHub API repo field
  • L182 — head_owner from GitHub API headRepositoryOwner.login
  • L185 — same_repository comparison between nameWithOwner and repo
  • L189 — configured_owners set from caller-supplied allowed_owners frozenset

latest_reviews_by_author — review-state deduplication. The login key controls whether two reviews from differently-cased logins collapse to one actor's latest state. One call site changed.

Why .casefold() is safe at each site

Functional equivalence for GitHub identifiers. GitHub owner names, repository names, and user logins are restricted to ASCII alphanumeric characters and hyphens by GitHub's identifier rules. For any string composed exclusively of ASCII characters, str.casefold() and str.lower() produce identical output — there is no ASCII character for which the two functions differ. No access-control decision can change for any real GitHub-sourced value.

Allowlist input is not attacker-controlled. configured_owners is built from allowed_owners, which is the caller-supplied frozenset resolved from administrator configuration, not from PR data. An external actor contributing a PR cannot influence what owners appear in the allowlist.

Theoretical non-ASCII edge case (academic). For hypothetical non-ASCII input (which GitHub's own identifier validation would reject before reaching this code), .casefold() can produce broader folding than .lower() (e.g., "ß" → "ss"). In the allowlist path this could theoretically make a comparison more permissive, not less. However: (a) GitHub enforces ASCII-only identifiers server-side, (b) the data arrives from the GitHub GraphQL API where the server has already validated the identifier, and (c) even if such a value somehow arrived, it would not match any legitimate allowlist entry (which would also have been subject to the same casefold on the allowlist side, requiring an exact casefold match).

No injection surface. All five changed expressions are pure string method calls (str.casefold()). No shell execution, subprocess invocation, SQL, template rendering, or file-path construction touches these values. The computed strings are used only as dictionary keys and set membership tests in Python.

No new data flows or code paths. Every changed line is a drop-in replacement within an existing conditional check. No branches, inputs, outputs, or trust boundaries were added.

No GitHub Actions changes. No workflow files were modified. No pull_request_target, workflow_run, github context interpolation, permissions: widening, or action-pin changes are present.

No dependency changes. No new packages, loosened pins, or supply-chain exposure.

@kyle-sexton
kyle-sexton merged commit 0d88839 into main Jul 25, 2026
26 checks passed
@kyle-sexton
kyle-sexton deleted the fix/815-casefold-owner-repo-login-babysit branch July 25, 2026 21:04
kyle-sexton added a commit that referenced this pull request Jul 26, 2026
@kyle-sexton @claude
fix(source-control): harden prune_babysit_worktrees.py against orphan…
8a56fd3 
…ed state (#1331)

*This was generated by AI during work-loop execution.*

Closes #816

## Summary

`prune_babysit_worktrees.py` had two related robustness gaps, both
observed at queue-start prune
(#657, sweep-converted into #816, operator-ratified 2026-07-23):

1. A worktree directory left behind by a lock-blocked `git worktree
remove` -- git's administrative
record dropped from `.git/worktrees/`, the directory itself surviving on
disk, most commonly a
Windows file lock -- made every subsequent prune run error `fatal: not a
git repository` on that
   entry instead of self-healing.
2. The lock-blocked removal itself gave no signal: the residual
directory was left with no report
   and no cleanup attempt.

`git_status` failures now go through `is_missing_repo_error`, which
distinguishes "this path is no
longer a valid git repository" from every other git failure (permission
errors, `gh` network
issues, etc. still surface as real errors). An orphaned entry is
self-healed by
`drop_orphaned_worktree`: it drops the stale worker-lease record for
that key (by the time this
runs, `main`'s active-lease check has already established it isn't a
live/unexpired hold -- the
lease-only case with *no* matching directory stays
`manage_babysit_lease.py reap`'s job, unchanged)
and removes the residual directory only when it is empty and
root-contained
(`remove_empty_orphan_directory`) -- an orphan's `.git` pointer could be
corrupted or gone while real
uncommitted work still sits there, so a non-empty orphan is reported,
never force-deleted. This is
reported via a new `orphan_dropped` row action, and does **not** flip
the run's exit code, so the
same orphan stops re-erroring every cycle.

`remove_worktree` now verifies the directory actually left disk after a
*successful*
`git worktree remove` (`attempt_directory_removal` -- safe to fully
`rmtree` here, since git already
confirmed the directory was removable) and reports a still-locked
directory via `residual_directory`
in the JSON row plus a stderr `WARNING`, instead of leaving a silent
orphan for the next run to
stumble over (which the `orphan_dropped` path above then self-heals).

`source-control` `0.26.2` → `0.26.3` with the matching CHANGELOG entry.

## Test plan

Every command below was run from the branch worktree; all passed.

- `python -m pytest tests/` in
`plugins/source-control/skills/babysit-prs/scripts/` -- **364 passed,
58 subtests passed** (was 358 before this change; 6 new test classes
covering the orphan-detection,
directory-removal, and self-heal paths, plus 2 new cases added to the
existing
  `RemoveWorktreeIsHermetic` class).
- `MissingRepoErrorDetectionTests` -- `is_missing_repo_error` fires on a
real `git status` failure
against a plain (non-git) directory, and not on an unrelated failure
string.
- `AttemptDirectoryRemovalTests` -- already-gone path reports `True`; a
surviving directory (with
content) is fully removed; a removal blocked by a patched
`shutil.rmtree` OSError (simulating the
Windows-lock scenario portably, since CI runs on `ubuntu-24.04`) reports
`False` without raising.
- `RemoveEmptyOrphanDirectoryTests` -- removes an empty directory under
root; **leaves a non-empty
directory's content untouched**; refuses a directory outside root
(defense in depth, matching
    `remove_worktree`'s own containment guard).
- `DropOrphanedWorktreeTests` -- drops the lease record and removes an
empty orphan directory; is a
no-op when no lease record exists; never deletes content from a
non-empty orphan directory;
    refuses to remove an orphan directory outside root.
- `RemoveWorktreeIsHermetic` (extended) -- a clean removal reports
`residual_directory: False`; a
cleanup that stays blocked (via a patched `attempt_directory_removal`,
since a real
`git worktree remove` already clears the directory in this hermetic test
environment) reports
    `residual_directory: True`.
- `MainSelfHealsAnOrphanedWorktreeEntry` -- end-to-end `main()` run
against an orphaned directory
plus its stale lease record: exit code `0`, `orphan_dropped` action,
both the directory and the
    lease record gone.
- `python -m py_compile prune_babysit_worktrees.py
tests/test_prune_babysit_worktrees.py` -- exit 0.

An advisor review (this repo's own reviewer-in-the-loop discipline)
caught two real gaps in the
first pass before this PR was opened: (1) an unconditional `rmtree` on
the orphan path would have
destroyed uncommitted work in a directory git never confirmed removable
-- the issue text says
*empty* directory, and I'd drifted from that while implementing; fixed
by adding
`remove_empty_orphan_directory`, gated on emptiness and
root-containment, distinct from the
unconditional `attempt_directory_removal` used only after a
git-confirmed removal. (2) whether a
lease-only orphan (no matching directory) needed its own sweep in this
script -- confirmed via
`grep` that `manage_babysit_lease.py reap` already handles that case
independently and already runs
adjacent to this script at queue start (`SKILL.md` runbook step 3), so
no parallel mechanism was
added here.

## Related

- Sweep-converted from #657; ratified as `work-class: scoped` (C3)
2026-07-23.
- **Version note:** open PR #1329 (`fix(babysit-prs): casefold
owner/repo/login compares in babysit
scripts`) also bumps `source-control` `0.26.2` → `0.26.3`. Whichever of
these two merges second
will conflict on the manifest version line and the CHANGELOG insert
point -- resolve by rebase,
  not force-merge.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

source-control/babysit-prs: casefold owner/repo/login compares in babysit scripts

1 participant

@kyle-sexton