feat(source-control): surface babysit bot-to-personal identity fallback as an attribution-drift finding (#450)

[kyle-sexton]

Summary

On a safe-tier worker run against melodic-software/medley, the consumer's identity wrapper reported "Bot token generation failed, using personal identity" and every write silently landed authored as the personal user. The fallback was local-log-only: nothing reached the babysit orchestrator or the PR, so attribution drifted with no signal unless a human read the worker transcript.

Per the maintainer decision on #450, the token machinery itself is sound where creds are bound — the medley-session failure is environment/scope-specific (unbound creds or the App not installed on that repo), owned by medley (tools/github-auth/gh-bot.sh) and tracked separately. This repo's actionable fix is observability: make the silent bot→personal degrade a first-class, orchestrator-visible finding. No gh-bot.sh change is required for the finding to fire.

Change

Adds an attribution_drift reconciliation arm to the babysit-prs snapshot engine, extending the existing ledger↔timeline reconciliation with an intended-write-identity dimension:

• New concept. babysit_self_logins defines acceptable-as-mine, not intended-as-author. This introduces the intended-write-identity notion via a new babysit_intended_write_identity userConfig key (threaded as --intended-write-identity to the snapshot).
• Detection. For each write the mutation ledger recorded performing, the arm verifies the landed timeline author is the intended write-identity, not merely some accepted self-login. A recorded write that landed under a different self-login — the canonical bot-token-fallback-to-personal case — is attribution drift.
• Surface. It rides the existing material-finding / cycle-status channel (the same surface foreign_activity uses), naming the drift and the affected PR on that PR's status line. No new silent-log-only path.
• Complement of foreign_activity, not a change to it. That arm reconciles same-login timeline events the ledger cannot account for; this reconciles the writes it did record. They are mutually exclusive per comment (in-ledger vs not-in-ledger), so the new dimension is purely additive. Unlike foreign_activity, drift reports without suppressing dispatch — the PR is still ours to babysit; only a past write's authorship is wrong.
• Degrade behavior. Dormant when no intended write-identity is configured, so an unconfigured classifier never fires false positives.

Coverage is bounded to the write class the ledger records with a recoverable author (review-trigger comments); drift on reactions, classification replies, and branch pushes is out of reach until the ledger records their identifiers with authorship (see deferred follow-up below).

Docs updated: SKILL.md (config table row + snapshot invocation threading), reference/orchestration.md (the arm, and why it reports rather than suppresses), CHANGELOG.md (0.11.0). Version bumped 0.10.0 → 0.11.0.

Verification

• New unit + full-classify regression tests in test_babysit_delta.py (AttributionDriftTests): dormant-without-intended, detects-drift, no-drift-when-intended, unledgered-write-is-not-drift, and the acceptance path — the finding lands in material_findings via classify_pr with no gh-bot.sh in the loop (trivially true; there is no gh-bot.sh in this repo).
• Full engine suite green (224 tests, up from 217), ruff clean, guarded-wrapper behavior checks pass.
• markdownlint-cli2 and typos clean on all changed markdown; git diff --check clean.

Deferred follow-up (not filed)

• Broaden the babysit mutation ledger to record reaction, classification-reply, and branch-push identifiers with authorship, then extend attribution_drift detection to those write classes — closing the coverage gap this PR's review-trigger-only reconciliation leaves.

Closes #450.

Related

• source-control:babysit-prs: gh-bot.sh bot token generation fails, worker writes fall back to personal identity #450 — this PR.
• feat(source-control): babysit autopilot merge tier — bot-account review + criteria-gated merge #476 — autopilot merge tier; lists source-control:babysit-prs: gh-bot.sh bot token generation fails, worker writes fall back to personal identity #450 as a prerequisite. Unblocked by this PR (stronger write autonomy makes silent attribution drift materially worse, so drift must be orchestrator-visible first); not addressed here.
• source-control:babysit-prs: claude[bot] review job commits a fix locally but cannot push it (GitHub App write denied) #454 — sibling issue in the same bot-identity / permissions cluster. Not addressed here.
• medley tools/github-auth/gh-bot.sh — the token-generation root cause (bot token mint failing and falling back). Cross-repo, owned by and tracked against medley; no change here — this PR only makes the drift visible.

🤖 Generated with a Claude Code implementation subagent (issue #450)

[claude]

Claude finished @kyle-sexton's task in 2m 44s —— View job

---

PR Review

• Gather context and understand request
• Read changed files and helper functions
• Review all 7 changed files

---

The approach is well-designed and the implementation is correct. detect_attribution_drift is a clean complement to detect_foreign_activity — the inverted ledger-membership guard (comment_id not in recorded_comment_ids vs comment_id in recorded_comment_ids) correctly partitions the two arms with no overlap. Three observations follow, ordered by consequence.

---

request_attempt_history path is untested

test_babysit_delta.py — _ledger_prev only populates request_history. The loop at babysit_delta.py:277 iterates both request_history and request_attempt_history, but no test puts a comment_id in the request_attempt_history sub-dict to verify the ledger-read covers that branch. The detection code is correct (both keys feed the same recorded_comment_ids set), but the coverage gap means a future refactor could accidentally break the retry-ledger path silently.

# Missing test case — a ledger entry in request_attempt_history should also trigger drift:
def _ledger_prev_attempt(self) -> dict[str, object]:
    return {"review_trigger": {"request_attempt_history": {HEAD: {"comment_id": "100"}}}}

Fix this →

---

Case-insensitive path exercised in production but not in tests

babysit_delta.py:274,290 applies .casefold() to both intended and self_logins, matching the existing pattern in detect_foreign_activity. Since GitHub treats logins case-insensitively and users may enter babysit_intended_write_identity with any casing, this is the right call. However, all five new tests use lowercase-only identifiers ("bot", "personal"), so a typo that dropped the .casefold() on either side would pass every test.

Fix this →

---

Silent false-positive risk when intended_write_identity is not in self_logins

babysit_delta.py:285 skips a comment only when landed_cf == intended (correct author) OR landed_cf not in self_logins (not our write, irrelevant). If a user configures babysit_intended_write_identity to a login that isn't actually in babysit_self_logins — say a typo, or setting the bot identity without adding it to the self set — then every write landing under any accepted self-login passes the second condition and fires as drift. The plugin.json description already warns about this, but the runtime produces no diagnostic: the finding fires with the wrong intended_author value and no indication the config is inconsistent.

A one-line guard at the top of the function — or a log warning — would make misconfiguration diagnosable without silent false positives:

# After building self_logins, before the early-return:
if intended and intended not in self_logins:
    # intended identity is not itself a self-login; every write would be flagged
    return {"detected": False, "evidence": [], "config_warning": "intended_write_identity not in self_logins"}

The exact mechanism (early return vs. warning field) is a judgment call, but the current silent behavior makes misconfiguration hard to debug.

Fix this →

---

Everything else looks correct

• Type safety. issue_comment_database_id always returns str, matching str(entry["comment_id"]) in the ledger-read — no type mismatch.
• Empty-author safety. author_login returns "" for ghost/deleted users; landed_cf not in self_logins is then True (since we strip empty strings from self_logins), so continue fires and no spurious evidence is produced.
• Empty comment_id safety. issue_comment_database_id returns "" on failure; if not comment_id skips it before the set-membership check.
• Dispatch suppression. Correctly omitted — attribution_drift appends to material without touching any suppression state, consistent with the design documented in orchestration.md.
• prev=None guard. json_object((previous or {}).get("review_trigger")) safely yields {} and the arm returns detected: False — verified by test_unledgered_write_is_not_drift.
• pr_queue_snapshot.py. The getattr(..., None) or "" pattern is consistent with how other optional args are threaded into ClassifyConfig.
• Docs. plugin.json description, SKILL.md config table and invocation text, and orchestration.md accurately describe the arm, its coverage boundary, and the reporter-not-suppressor distinction.

---

The two test gaps are worth closing before merge; the misconfiguration guard is lower priority (it's documented) but would improve operability. The core logic is sound.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e6a967be3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 2m 53s —— View job

---

PR Review

• Gather context and understand request
• Read changed files
• Assess prior review findings (Claude + Codex) against current HEAD
• Review new commit (resolve @me for single-PR scope)
• Produce final review

---

New commit assessment — fix(source-control): resolve @me into self-logins for single-PR babysit scope

The Codex finding was correct and the fix is correct. Before this commit, build_snapshot resolved @me only inside the else (discovery) branch, so a --pr owner/repo#N run derived self_logins from raw --author @me,... — dropping the literal string @me without resolving it to a login. Both detect_foreign_activity and detect_attribution_drift gate on landed_cf in self_logins, so the personal fallback identity was silently absent and the checks were dead letters for every single-PR run.

The fix hoists gh.resolve_authors + resolve_self_logins above the scope branch so resolution is unconditional. The regression test at test_pr_queue_snapshot.py:186 correctly verifies the post-fix behavior: it mocks resolve_author to return "kyle-sexton", stubs view_pr to raise (so the loop is a no-op), and asserts "kyle-sexton" in args.resolved_self_logins — directly confirming the authenticated login lands in the self-login set for a --pr run.

One observation: gh.resolve_author("@me") is now called twice when --author includes @me — once inside gh.resolve_authors and once inside resolve_self_logins. Both calls return the same value, and the dedup guard in resolve_self_logins prevents duplication in the result. Not a bug, just worth noting for future maintainers if the gh call is ever measured as slow.

The fix is minimal, focused, and safe to merge.

---

Status of prior Claude review findings

Three findings from the previous review cycle remain unaddressed:

1. request_attempt_history path untested — test_babysit_delta.py:191

_ledger_prev() still only populates request_history. The loop at babysit_delta.py:277 iterates both request_history and request_attempt_history, but no test puts a comment_id in request_attempt_history. The detection code is correct, but a future refactor that accidentally drops the second key would pass every AttributionDriftTests test silently.

Fix this →

2. Case-insensitive comparison untested — test_babysit_delta.py:184

All five AttributionDriftTests use lowercase-only identifiers ("bot", "personal"). The .casefold() calls at babysit_delta.py:271-272 are correct, but a typo that dropped .casefold() on either side would pass every existing test. A single mixed-case case (e.g. intended="Bot", self_logins={"Bot", "Personal"}, comment authored as "personal") would lock in the invariant.

Fix this →

3. Silent misconfiguration when intended_write_identity is not in self_logins — babysit_delta.py:271

If a user sets babysit_intended_write_identity to a login that is not in babysit_self_logins (e.g. a typo, or adding the bot identity without also adding it to the self-login set), the skip condition at babysit_delta.py:296 — landed_cf not in self_logins — is True for every comment the operator posted (since none of their logins match intended), and every recorded write fires as drift. The finding then surfaces with a wrong intended_author value and no indication that the config is the cause. A one-line guard after building self_logins would make this diagnosable:

if intended and intended not in self_logins:
    return {"detected": False, "evidence": [], "config_warning": "intended_write_identity not in self_logins"}

Fix this →

---

Summary

The new fix commit correctly resolves the Codex-flagged single-PR @me scope regression, with a focused refactor and a targeted regression test. The core attribution-drift logic (first commit) remains sound as previously reviewed.

Three test-coverage and operational robustness gaps from the prior review cycle carry forward. Items 1 and 2 are low-risk but worth closing before merge to prevent silent future regressions. Item 3 is an operational issue with no in-code diagnostic that could be addressed with minimal effort.