Skip to content

fix(github-scan): auto-revert github-scan:human on new human comment (#358) - #359

Merged
serenakeyitan merged 1 commit into
mainfrom
fix/358-auto-revert-human-on-comment
May 2, 2026
Merged

fix(github-scan): auto-revert github-scan:human on new human comment (#358)#359
serenakeyitan merged 1 commit into
mainfrom
fix/358-auto-revert-human-on-comment

Conversation

@serenakeyitan

Copy link
Copy Markdown
Contributor

Refs #358.

Summary

When the daemon polls an item classified human and a qualifying human comment is observed after the github-scan:human label was applied, the daemon strips the label so the (unchanged) classifier naturally re-derives new and the dispatcher picks it up on the next cycle.

This closes the silent-failure handoff documented in #358 (e.g. agent-team-foundation/first-tree-website#10) where a human replies in the issue and reasonably expects the agent to read the decision and continue working — instead of having to also manually strip the label.

Implementation

  • New module packages/github-scan/src/github-scan/engine/runtime/auto-revert.ts exporting:
    • shouldAutoRevertHuman(input) — pure decision function applying the four guards.
    • autoRevertHumanLabels(entries, deps) — driver that fetches the timeline + comments, calls the existing gh.removeLabel, and mutates entry.labels in place.
  • Wired into pollOnce (packages/github-scan/src/github-scan/engine/daemon/poller.ts) between enrichWithLabels and classifyEntries — so when guards pass, classification on the same cycle re-derives new (no extra round trip needed).
  • runner-skeleton.ts passes the daemon's resolved identity.login as agentLogin. When identity resolution fails (degraded mode), auto-revert is skipped — fail-safe.
  • The classifier itself is intentionally untouched. Auto-revert is purely an action layer.
  • Spurious-fire prevention: we mutate entry.labels in the same cycle, and on the next cycle the live GraphQL label-set no longer contains github-scan:human, so the entry skips the auto-revert branch entirely. No per-poll tombstone needed.
  • Updated agent prompt in daemon/runner.ts to document the human → new transition so agents know they can stop on github-scan:human and trust the daemon to re-queue on reply.

Guards (from issue #358)

# Guard How tested
1 Author MUST NOT be the agent (a) own-comment ignored test
2 Body length > 20 chars (b) short ack ignored test ("ok thx", "👍")
3 Reactions alone do NOT count (c) reaction ignored test (empty body shape)
4 created_at strictly after label timestamp (e) pre-label comment test + a separate == strict-inequality test
All guards pass → revert fires (d) genuine reply triggers revert test

Acceptance-criteria coverage

packages/github-scan/tests/github-scan/github-scan-auto-revert.test.ts — 11 tests, 5 acceptance + 2 extra guard edges + 4 driver tests:

  • (a) own-comment ignored
  • (b) short ack ignored
  • (c) reaction ignored
  • (d) genuine reply triggers revert
  • (e) pre-label comment does NOT trigger revert
  • driver: end-to-end gh.removeLabel is called and entry.labels is mutated when guards pass
  • driver: skips items not labeled github-scan:human
  • driver: degrades safely when label-event timestamp can't be fetched

Validation

  • pnpm -r test — 522 tests pass across 38 files
  • pnpm lint
  • pnpm typecheck

cc @bingran-you for review.

When the daemon polls an item classified `human` and observes a
qualifying human comment posted strictly after the
`github-scan:human` label was applied, strip the label so the
classifier naturally re-derives `new` on the next cycle and the
dispatcher picks the item up.

Guards (issue #358):
  1. Comment author MUST NOT be the agent itself.
  2. Comment body length > 20 chars (filters thumbs-up / "ok" acks).
  3. Reactions alone do NOT count as a comment.
  4. Comment created_at MUST be strictly after the label-event timestamp.

The fix is a new `runtime/auto-revert.ts` module wired into
`pollOnce` between `enrichWithLabels` and `classifyEntries`. The
classifier itself is unchanged. Production passes the daemon's
resolved `identity.login` as `agentLogin` so the own-comment guard
is exact; when identity resolution failed (degraded mode), the
auto-revert is skipped entirely.

Updated the agent prompt in `daemon/runner.ts` to document the new
human → new transition so agents know they can safely stop on
`github-scan:human` and trust the daemon to re-queue on reply.

Refs #358

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@serenakeyitan
serenakeyitan requested a review from bingran-you May 2, 2026 00:59
@bingran-you bingran-you added the breeze:wip breeze is actively working on it label May 2, 2026

@bingran-you bingran-you 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.

LGTM overall — clean separation between the pure decision function and the I/O-bound driver, all five acceptance cases from #358 are covered, and the in-place entry.labels mutation neatly avoids the need for a per-cycle tombstone. A few non-blocking observations:

Pagination cap (worth a follow-up, not a blocker)

fetchHumanLabelAppliedAt and fetchIssueComments both fetch only the first page (per_page=100) of the timeline / comments respectively (packages/github-scan/src/github-scan/engine/runtime/auto-revert.ts:99,145). For a long-lived issue with > 100 timeline events between the labeled event and the head of the timeline, the latest github-scan:human labeled event could fall off the page and fetchHumanLabelAppliedAt would return null — auto-revert would then be silently skipped (a warning fires, but the human handoff stays stuck). Same shape for the comments fetch. Realistic for typical issues; could bite for noisy long-running threads. Could either page or — for the timeline specifically — query in reverse via ?page=N&per_page=100 only when the first page doesn't contain the event.

Silent removeLabel failure

gh.removeLabel swallows non-zero exits (runtime/gh.ts:174). If the REST call fails after we've mutated entry.labels in place, the same cycle will (correctly) re-classify as new and dispatch — but on the next cycle the GraphQL re-read will still show github-scan:human, the auto-revert path will re-fire, and the agent gets re-dispatched. Probably tolerable (the agent will idempotently respond to the same comment), but worth thinking about whether the in-memory mutation should be conditional on removeLabel actually succeeding. A quick check on removeLabel's return + early-exit on failure would close this.

Body length threshold

comment.body.trim().length <= AUTO_REVERT_MIN_BODY_CHARS (≤ 20) means strictly more than 20 characters qualifies, which matches the PR description and the issue spec. Just noting in case anyone reads the constant name and assumes "at least 20".

Nit — agent-prompt wording

The new prompt line in runner.ts:200 says "anyone other than {login}" but the actual guard is case-insensitive on the login and requires > 20 chars and requires post-label timestamp. If an agent ever reasons about whether to set github-scan:human based on this prompt, the simplification is fine; if they ever debug why a revert didn't fire, the abbreviated description could mislead. Minor.

Approving — none of the above blocks merge. The pagination one is the only one I'd consider opening a follow-up issue for once you've seen this in production for a bit.

This reply was drafted by breeze, an autonomous agent running on behalf of the account owner.

@bingran-you bingran-you added breeze:done breeze has finished handling it and removed breeze:wip breeze is actively working on it labels May 2, 2026
@serenakeyitan

Copy link
Copy Markdown
Contributor Author

End-to-end verification

Ran a real-GitHub smoke test of this PR against a separate daemon on :7879 (production daemon untouched on :7878), fresh GITHUB_SCAN_DIR=/tmp/scan-test-pr359, --dry-run to avoid spawning competing agents.

Test target: agent-team-foundation/first-tree#309 — PR present in the daemon's notifications inbox, no github-scan:human at start.

Test setup note: my only gh identity is serenakeyitan, so a real human-vs-agent split was simulated by setting agentLogin to a non-serenakeyitan value at the runPoller call site (one-line patch reverted after the test). Everything downstream of agentLoginfetchHumanLabelAppliedAt, fetchIssueComments, the four guards in shouldAutoRevertHuman, and the gh.removeLabel call — ran live against real GitHub.

Positive case (auto-revert fires)

Negative case (guards hold)

Real-world surprise worth noting

The poller candidate set comes only from /notifications. Items the operator isn't subscribed to (e.g. first-tree-context#197, which I tried first) never enter the inbox and so never reach auto-revert, even when they carry github-scan:human. Not a regression introduced by this PR — but worth knowing: auto-revert reaches exactly what the daemon polls. Pure unit logic in auto-revert.ts is correct; downstream visibility is bounded by the notifications feed.

Test pollution cleaned: github-scan:human removed from #309, daemon stopped, /tmp/scan-test-pr359* deleted, source patch reverted (git diff clean).

cc @bingran-you

@serenakeyitan serenakeyitan added the github-scan:done github-scan: handled label May 2, 2026
@serenakeyitan
serenakeyitan merged commit 5692d4b into main May 2, 2026
2 checks passed
@serenakeyitan
serenakeyitan deleted the fix/358-auto-revert-human-on-comment branch May 2, 2026 01:38
serenakeyitan added a commit that referenced this pull request May 3, 2026
…rator (#360) (#361)

Refs #360. Surfaced from the live #359 smoke ([test
report](first-tree-ai/first-tree#359 (comment)))
where the operator's `gh auth` user (`serenakeyitan`) was the same as
the daemon's agent identity, so every operator comment was filtered as
own-comment and auto-revert never fired.

## Summary

Adds a `--agent-login <login>` CLI flag (with env and config-file
fallbacks) so the operator's GitHub identity can be declared
independently of the daemon's `gh auth` user. The auto-revert
own-comment guard from #358/#359 now uses this resolved identity.

## Resolution order

| # | Source | Wins over |
|---|---|---|
| 1 | CLI flag `--agent-login <login>` | env, yaml, gh auth |
| 2 | Env var `GITHUB_SCAN_AGENT_LOGIN` | yaml, gh auth |
| 3 | Yaml `agent_login` / `agentLogin` (in
`~/.first-tree/github-scan/config.yaml`) | gh auth |
| 4 | Daemon's `gh auth` identity | (final fallback — preserves
zero-config dogfood) |

## Implementation

- `runtime/config.ts`: adds `agentLogin` to `DaemonConfig`,
`DaemonCliOverrides`, the yaml schema, and the existing 4-tier
`loadGitHubScanDaemonConfig` resolver.
- `daemon/runner-skeleton.ts`: parses `--agent-login` flag (both
`--agent-login alt-bot` and `--agent-login=alt-bot`), passes
`config.agentLogin ?? identity?.login` into `runPoller` /
`runPollerOnce`. Logs an explicit override line when the resolved
identity differs from `gh auth`.
- `cli.ts`: surfaces `--agent-login <login>` in `run`, `daemon`, and
`start` help text.
- `auto-revert.ts` already takes `agentLogin` as a parameter (PR #359),
so no change needed there — the resolved value just flows through.

## Tests

- 6 new config tests (all 4 resolution tiers + camelCase yaml +
empty-CLI-no-clobber)
- 2 new `parseDaemonArgs` tests (both `--agent-login` forms +
empty-value rejection)
- 1 new auto-revert test verifying guard 1 (own-comment) uses the
*resolved* identity, NOT the `gh auth` user — this is the exact #360
scenario
- All existing 11 auto-revert tests + existing config tests still pass
- `pnpm -r test` (531 tests), `pnpm lint`, `pnpm typecheck` all pass

## Live smoke (3 cases — all green)

Tested against `first-tree-ai/first-tree-website#12` on a
separate daemon (port 7879, separate `GITHUB_SCAN_DIR`) without
disturbing the running prod daemon on 7878. Full transcript posted as a
follow-up comment.

| Case | Setup | Auto-revert expected | Result |
|---|---|---|---|
| 1 | `--agent-login some-other-login` | YES (operator comment is
"human" now) | YES — label stripped, status=`new` |
| 2 | `GITHUB_SCAN_AGENT_LOGIN=some-other-login`, no flag | YES | YES —
label stripped |
| 3 | No flag, no env | NO (gh auth fallback → `serenakeyitan` IS the
agent → own-comment filter) | NO — label retained |

cc @bingran-you

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breeze:done breeze has finished handling it github-scan:done github-scan: handled

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants