Skip to content

feat(gardener): use the actual gardener login in @<bot> fix mentions - #351

Open
serenakeyitan wants to merge 2 commits into
devfrom
feat/dynamic-gardener-mention
Open

feat(gardener): use the actual gardener login in @<bot> fix mentions#351
serenakeyitan wants to merge 2 commits into
devfrom
feat/dynamic-gardener-mention

Conversation

@serenakeyitan

Copy link
Copy Markdown
Contributor

Summary

The PR-body footer that gardener-sync emits hard-codes @gardener fix. That mention only notifies a GitHub user named "gardener" — but the actual bot identity is whatever account runs gardener (resolved via gh api user or GARDENER_LOGIN). On any deployment where the bot is a maintainer's personal account, reviewers' @gardener fix comments notify nobody. The respond engine had the same issue with its static regex.

Changes

  • Export resolveGardenerLogin(shell) so sync.ts can reuse it.
  • Add buildFixCommandRegex(gardenerLogin) — builds the dynamic regex with regex-metachar escaping and a fallback to @gardener when the login is empty/whitespace (preserves prior behavior for default-named accounts).
  • Wire dynamic regex into respond's fix-detection (replaces GARDENER_FIX_RE).
  • Wire dynamic login into sync's PR-body template — resolve once at top of runSync (env override → gh api user), thread through prepareProposalGroup, emit \@${gardenerLogin || "gardener"} fix`` in the body.
  • Update skip diagnostic in respond to print the actual login.

Test plan

  • New buildFixCommandRegex unit tests: actual-login match, empty-login fallback, metachar escaping.
  • Existing self-loop guard test updated to use @serenakeyitan fix (matches its GARDENER_LOGIN=serenakeyitan setup) — confirms the literal @gardener fix no longer matches a custom login.
  • Sync golden snapshots regenerated to include the new gh api user --jq .login call at the top of runSync.
  • Full test suite: 1206 passed (3 new), 0 failed.
  • tsc --noEmit clean.

Discovered while

Caught while running gardener locally on serenakeyitan/paperclip-tree. The PR-body footer's @gardener fix was a dead mention since the bot is serenakeyitan — manual local patch worked but kept getting overwritten by npm i -g first-tree.

🤖 Generated with Claude Code

The PR-body footer that gardener-sync emits on every tree PR currently
hard-codes `@gardener fix`. That literal mention only notifies a
GitHub user named "gardener" — but the bot identity is whatever
account is running gardener (resolved via `gh api user` or
`GARDENER_LOGIN`). On any deployment where the bot is a maintainer's
personal account or a custom service account, reviewers' `@gardener
fix` comments notify nobody, and the loop silently breaks.

The respond engine had the same issue: the static regex
`/@gardener\s+fix/i` only matched the literal token, so `@<actual-bot>
fix` didn't trigger the fix path even though that's how reviewers
would actually mention the bot.

Changes:

- Export `resolveGardenerLogin(shell)` so sync.ts can reuse it.
- Add `buildFixCommandRegex(gardenerLogin)` — builds the
  `@<login>\s+fix` regex dynamically, with metachar escaping and a
  fallback to `@gardener` when the login is empty/whitespace.
- Use `buildFixCommandRegex` in the respond fix-detection path
  (replaces the static `GARDENER_FIX_RE`).
- Update the diagnostic skip log to print the actual login.
- In sync.ts, resolve `gardenerLogin` once at the top of `runSync`
  (env override → `gh api user`), thread it through
  `prepareProposalGroup`, and emit
  `comment \`@${gardenerLogin || "gardener"} fix\`` in the PR body.

Tests:

- New `buildFixCommandRegex` unit tests cover the actual-login,
  empty-login fallback, and metachar-escape cases.
- The existing self-loop guard test now uses `@serenakeyitan fix`
  (matching its `GARDENER_LOGIN=serenakeyitan` setup) since the
  literal `@gardener fix` no longer matches a custom login — that's
  the whole point of the change.
- Sync golden snapshots regenerated to include the new
  `gh api user --jq .login` call at the top of `runSync`.

All 1206 tests pass; typecheck clean.
@serenakeyitan

Copy link
Copy Markdown
Contributor Author

@bingran-you could you take a look? Caught locally on serenakeyitan/paperclip-tree — the @gardener fix mention in every sync PR's body is a dead mention since my bot identity is serenakeyitan, not gardener. Same issue in the respond engine's static regex. PR makes both dynamic from gardenerLogin (resolved from env or gh api user), with a fallback to the literal gardener for default-named accounts. 1206 tests pass.

@bingran-you bingran-you added the breeze:wip breeze is actively working on it label Apr 28, 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.

Requesting changes because the main bug looks only half-fixed on the actual breeze-runner path. runRespond() still skips gh api user in snapshot mode and only consults env.GARDENER_LOGIN (src/products/gardener/engine/respond.ts:876-881), but breeze-runner buildAgentEnv() does not export GARDENER_LOGIN at all (src/products/breeze/engine/daemon/runner.ts:364-374). In the real review-request flow that means gardenerLogin == "", buildFixCommandRegex() falls back to @gardener, and @ fix comments from the sync PR body still will not trigger respond unless someone manually injects that env var. The new tests only cover snapshot mode with GARDENER_LOGIN already present, so they miss the deployed path. Please wire the login through the runner environment, or reuse the existing GARDENER_USER plumbing instead of introducing a second env contract, and add a snapshot-mode test that exercises a custom bot login without manual gh api user lookup. 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 Apr 28, 2026
Reviewer feedback: the previous PR fixed the static @gardener regex
in respond.ts but only covered snapshot-mode tests where
GARDENER_LOGIN was already set. The deployed breeze-runner path
never exported the daemon login at all, so in production
gardenerLogin == "" and buildFixCommandRegex() fell back to the
literal "gardener" — reviewers' @<actual-bot> fix mentions
silently no-op'd.

Changes:
- buildAgentEnv() in runner.ts now exports GARDENER_USER from
  request.identity.login (the daemon's resolved DaemonIdentity.login
  from gh auth status).
- respond.ts and sync.ts read GARDENER_USER as the canonical env
  contract — same name already used by comment.ts and
  install-workflow.ts. GARDENER_LOGIN is honored as a back-compat
  alias so older deployments don't break on upgrade.
- Two new snapshot-mode tests: one with GARDENER_USER driving a
  custom bot login (asserts no `gh api user` call fires and that
  @<custom-login> fix mentions are recognized), one for the
  back-compat GARDENER_LOGIN fallback.
- One new buildAgentEnv test asserting GARDENER_USER is exported
  from request.identity.login.

All 1209 tests pass; typecheck clean.

Addresses review on #351.
@serenakeyitan

Copy link
Copy Markdown
Contributor Author

Pushed fix addressing the runner-path bug:

  • Root cause confirmed. runRespond() only consulted env.GARDENER_LOGIN, but buildAgentEnv() in daemon/runner.ts:364-374 never exported it — so on the real breeze-runner path gardenerLogin == "" and buildFixCommandRegex() fell back to the literal gardener. The previous tests passed only because they pre-set the env manually.

  • Fix per your suggestion (reuse existing contract, not a second one). buildAgentEnv() now exports GARDENER_USER from request.identity.login — the daemon's already-resolved login from DaemonIdentity (which is what gh auth status returns). respond.ts and sync.ts now read GARDENER_USER as the canonical name (matching comment.ts:1719 and install-workflow.ts:242). GARDENER_LOGIN is honored as a back-compat alias so older deployments keep working.

  • New tests covering the deployed path (your specific ask):

    • tests/gardener/gardener-respond.test.ts — snapshot mode with GARDENER_USER=custom-bot-acct, asserts no gh api user call fires AND the @custom-bot-acct fix mention is recognized (hard-fails on gh api user).
    • Same file — back-compat test confirming GARDENER_LOGIN still works when GARDENER_USER is unset.
    • tests/breeze/breeze-daemon-runner-agent.test.ts — asserts buildAgentEnv() exports GARDENER_USER from request.identity.login.

All 1209 tests pass; typecheck clean.

Ready for re-review. cc @bingran-you

@serenakeyitan serenakeyitan added the github-scan:human github-scan: needs human input label May 2, 2026

@serenakeyitan serenakeyitan left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please proceed with this approach — confirmed via #366 E2E test.

@serenakeyitan serenakeyitan added github-scan:human github-scan: needs human input and removed github-scan:human github-scan: needs human input labels May 2, 2026
@serenakeyitan

Copy link
Copy Markdown
Contributor Author

Note: the previous PR review at first-tree-ai/first-tree#351 (review) was generated by an interrupted automated end-to-end test for #366, NOT a real review. GitHub's API does not allow dismissing COMMENTED reviews (only APPROVED/CHANGES_REQUESTED are dismissible). Apologies for the noise. cc @bingran-you

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