From 243218d9e316793faf628d87d3fea78433f09835 Mon Sep 17 00:00:00 2001 From: mathuraditya724 Date: Fri, 7 Aug 2026 19:39:51 +0530 Subject: [PATCH 1/2] =?UTF-8?q?feat(agent):=20=F0=9F=91=80/=F0=9F=8E=89=20?= =?UTF-8?q?signals,=20leaner=20diffs,=20friendlier=20tone,=20own=20the=20P?= =?UTF-8?q?R=20E2E?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server now drops an instant πŸ‘€ "eyes" reaction on the comment/issue that triggers Jared, straight from the webhook handler β€” the human sees it's picked up in seconds, independent of sandbox warmup. The agent leaves a single πŸŽ‰ "hooray" reaction on that same trigger once the work is actually done. Reaction target selection is unit-tested. Instruction + skill changes: - Keep diffs minimal: never commit AGENTS.md / .agents / harness overlays or unrelated files (this is exactly what polluted craft#864). git status / git diff --staged before every commit. - deslop is now mandatory before every commit, not "as needed". - Friendlier, humbler, shorter tone for anything a human reads. - Own your PR end-to-end: fix actionable review feedback and push it; don't defer it to "the author of a later commit" and stop. Confirm the push landed (HEAD == @{u}) before claiming a fix. - fix-ci: stop busy-polling CI in a loop β€” push, confirm, comment, stop; the CI-completion webhook wakes you when the run settles. Co-authored-by: Cursor --- .../container/.agents/skills/fix-ci/SKILL.md | 21 ++++- .../.agents/skills/repo-setup/SKILL.md | 4 + .../skills/respond-to-comment/SKILL.md | 32 ++++++- .../container/flue/src/agents/instructions.ts | 43 ++++++++- apps/server/container/skills/fix-ci/SKILL.md | 21 ++++- .../container/skills/repo-setup/SKILL.md | 4 + .../skills/respond-to-comment/SKILL.md | 32 ++++++- apps/server/src/agents/instructions.ts | 43 ++++++++- .../lib/github/__tests__/reactions.test.ts | 42 +++++++++ apps/server/src/lib/github/reactions.ts | 90 +++++++++++++++++++ apps/server/src/routes/webhooks/github.ts | 6 ++ 11 files changed, 318 insertions(+), 20 deletions(-) create mode 100644 apps/server/src/lib/github/__tests__/reactions.test.ts create mode 100644 apps/server/src/lib/github/reactions.ts diff --git a/apps/server/container/.agents/skills/fix-ci/SKILL.md b/apps/server/container/.agents/skills/fix-ci/SKILL.md index 8abb156..175380d 100644 --- a/apps/server/container/.agents/skills/fix-ci/SKILL.md +++ b/apps/server/container/.agents/skills/fix-ci/SKILL.md @@ -36,10 +36,23 @@ but the rest should read naturally. 5. Infra/dependency issue? BLOCKED. 6. Otherwise: make the smallest fix. Reproduce locally if possible. 7. Load `deslop` and `review` skills. -8. Commit, push, and post a comment summarizing what you fixed and - how. Write it like a teammate explaining the fix, not a status - report. +8. Commit and push, then **confirm the push landed** before you claim + anything: + + ```sh + git push origin HEAD + git rev-parse HEAD @{u} # the two SHAs must match β€” if not, the push failed + ``` + + Then post one comment summarizing what you fixed and how. Write it + like a teammate explaining the fix, not a status report. +9. **Stop β€” do not busy-wait on CI.** After pushing, end the turn. Don't + sit in a `for`/`while` loop polling `gh pr checks` / `gh run watch` + waiting for the new run to go green: the CI-completion webhook wakes + you again once the run settles, and looping just burns the sandbox and + floods the transcript. Make the fix, push, confirm, comment, stop. Avoid modifying CI config unless the failure is specifically in it. Avoid bumping dependency versions β€” the fix should target the code, -not the toolchain. Don't force-push. Don't merge. +not the toolchain. Never stage unrelated files (`AGENTS.md`, harness +config) β€” `git status` before committing. Don't force-push. Don't merge. diff --git a/apps/server/container/.agents/skills/repo-setup/SKILL.md b/apps/server/container/.agents/skills/repo-setup/SKILL.md index 6e4bbde..059f6fa 100644 --- a/apps/server/container/.agents/skills/repo-setup/SKILL.md +++ b/apps/server/container/.agents/skills/repo-setup/SKILL.md @@ -74,3 +74,7 @@ own container, so `/workspace/repo` is already isolated. disposable. - Multi-repo investigation may clone **other** repositories under `~/dev/...`, but the target repo for this issue/PR stays `/workspace/repo`. +- The runtime bakes an `AGENTS.md` / `.agents/` skills overlay into the working + tree. Those are harness files, **not** part of the repo β€” never stage or commit + them. `git status` before every commit and unstage anything you didn't + deliberately change; keep the diff limited to the fix. diff --git a/apps/server/container/.agents/skills/respond-to-comment/SKILL.md b/apps/server/container/.agents/skills/respond-to-comment/SKILL.md index 340391f..e17c1e3 100644 --- a/apps/server/container/.agents/skills/respond-to-comment/SKILL.md +++ b/apps/server/container/.agents/skills/respond-to-comment/SKILL.md @@ -33,12 +33,38 @@ Before triaging, confirm the comment is actually for you: - **Not actionable**: style preference, out of scope, already handled β†’ reply with reason - **Approval thumbs-up** (short body, no code refs): don't reply, stop +## Own your PR end-to-end + +If the bot authored the PR, the bot owns getting it merge-ready β€” that doesn't +change just because a maintainer later pushed a follow-up commit onto the branch. +When a review comment on your PR is actionable, **fix it and push**; don't defer it +("leaving this for the author of that commit") and stop. Deferring strands the PR. + +If a comment genuinely isn't yours to act on β€” it's out of scope, or it's about a +change only a human should make β€” say that plainly on the thread AND `@`-mention a +human maintainer so someone picks it up. Silently leaving it open with no owner is +the one thing to avoid. + ## Workflow 1. Check PR authorship β€” only push to your own PR's branch. -2. If actionable on your own PR: implement the fix, load `deslop`, commit, - push, then reply on the thread with the commit SHA and resolve the thread - (see below). After all fixes, re-request review. +2. If actionable on your own PR: implement the fix, load `deslop`, commit and + push, then **confirm the push landed** before you reply: + + ```sh + git push origin HEAD + git rev-parse HEAD @{u} # the two SHAs must match β€” if not, the push failed + ``` + + Only once it's really on the branch, reply on the thread with the commit SHA, + resolve the thread (see below), and drop a πŸŽ‰ reaction on the comment as the + "done" signal: + + ```sh + gh api -X POST repos///pulls/comments//reactions -f content=hooray + ``` + + After all fixes, re-request review. 3. If actionable on someone else's PR: reply with a `suggestion` block or description. Don't push. 4. If not actionable: reply on the thread with the reason and leave it open diff --git a/apps/server/container/flue/src/agents/instructions.ts b/apps/server/container/flue/src/agents/instructions.ts index 1dc8c4a..c182905 100644 --- a/apps/server/container/flue/src/agents/instructions.ts +++ b/apps/server/container/flue/src/agents/instructions.ts @@ -98,7 +98,9 @@ then load the situation skill for the task at hand. 1. **Always first**: load \`repo-setup\` 2. **Then the situation skill**: \`resolve-issue\`, \`review-pr\`, \`fix-ci\`, or \`respond-to-comment\` -3. **Utility skills** as needed: \`deslop\`, \`review\`, \`pr\`, \`mark-pr-ready\`, \`apply-fixes\`, \`auto-merge\` +3. **Utility skills**: \`review\`, \`pr\`, \`mark-pr-ready\`, \`apply-fixes\`, \`auto-merge\` as + needed β€” and \`deslop\` **always right before you commit** (clean the diff every + time, not only when it looks messy) ### Model tiering β€” spend the premium model on judgment only @@ -221,11 +223,46 @@ for routine best-effort calls you can and should make yourself. - On webhook runs no human is watching β€” do not ask clarifying questions; make a best-effort call. Operator turns are the exception (see above) - Work in \`/workspace/repo\` β€” \`repo-setup\` puts it on the right branch +- **Keep the diff minimal and on-topic.** Only touch files needed for THIS task. + Never commit \`AGENTS.md\`, \`.agents/\`, \`.lore.md\`, editor/harness config, or + anything unrelated to the fix β€” the sandbox may leave harness overlays in the + working tree, so \`git status\` and \`git diff --staged\` before every commit and + unstage anything stray. Unrelated file churn just bloats the PR and slows review. +- **Confirm your push landed.** After pushing, verify the local branch HEAD equals + \`origin/\` before you tell anyone it's done. A commit that never left the + sandbox is not a fix β€” replying "fixed in " when the push failed is worse + than saying nothing. +- **Clean the diff with \`deslop\` before every commit** β€” no AI noise (narration + comments, needless try/catch, \`as any\`, leftover debug logs). Every code change, + not just the messy-looking ones. + +## Signaling progress with reactions + +The server already drops an πŸ‘€ reaction on the comment/issue that triggered you, so +the human knows you picked it up β€” you don't need to add that. When you FINISH what +they asked for (posted the review, pushed the fix, opened/updated the PR), leave a +single πŸŽ‰ reaction on that same trigger as the "done" signal: + +\`\`\`sh +# a top-level issue/PR comment +gh api -X POST repos///issues/comments//reactions -f content=hooray +# an inline PR review comment +gh api -X POST repos///pulls/comments//reactions -f content=hooray +# the issue or PR itself +gh api -X POST repos///issues//reactions -f content=hooray +\`\`\` + +One reaction, once, when you're actually done β€” not on every step, and never when +you ended in \`SKIPPED\` or \`BLOCKED\`. ## Tone & voice -Write like a competent teammate: concise, no filler, lowercase natural language -in PR comments, show don't narrate, no emoji unless the project already uses them. +Anything a human will read β€” PR/issue comments, review replies, chat answers β€” should +sound like a friendly, humble teammate: warm, plain, and short. Say the useful thing +and stop. No corporate filler, no status-report voice, no hedging walls of text, no +emoji in prose unless the thread already uses them (the πŸŽ‰ reaction above is the one +exception). If you were wrong or unsure, just say so plainly. Prefer two clear +sentences over a paragraph. ## Output diff --git a/apps/server/container/skills/fix-ci/SKILL.md b/apps/server/container/skills/fix-ci/SKILL.md index 8abb156..175380d 100644 --- a/apps/server/container/skills/fix-ci/SKILL.md +++ b/apps/server/container/skills/fix-ci/SKILL.md @@ -36,10 +36,23 @@ but the rest should read naturally. 5. Infra/dependency issue? BLOCKED. 6. Otherwise: make the smallest fix. Reproduce locally if possible. 7. Load `deslop` and `review` skills. -8. Commit, push, and post a comment summarizing what you fixed and - how. Write it like a teammate explaining the fix, not a status - report. +8. Commit and push, then **confirm the push landed** before you claim + anything: + + ```sh + git push origin HEAD + git rev-parse HEAD @{u} # the two SHAs must match β€” if not, the push failed + ``` + + Then post one comment summarizing what you fixed and how. Write it + like a teammate explaining the fix, not a status report. +9. **Stop β€” do not busy-wait on CI.** After pushing, end the turn. Don't + sit in a `for`/`while` loop polling `gh pr checks` / `gh run watch` + waiting for the new run to go green: the CI-completion webhook wakes + you again once the run settles, and looping just burns the sandbox and + floods the transcript. Make the fix, push, confirm, comment, stop. Avoid modifying CI config unless the failure is specifically in it. Avoid bumping dependency versions β€” the fix should target the code, -not the toolchain. Don't force-push. Don't merge. +not the toolchain. Never stage unrelated files (`AGENTS.md`, harness +config) β€” `git status` before committing. Don't force-push. Don't merge. diff --git a/apps/server/container/skills/repo-setup/SKILL.md b/apps/server/container/skills/repo-setup/SKILL.md index 6e4bbde..059f6fa 100644 --- a/apps/server/container/skills/repo-setup/SKILL.md +++ b/apps/server/container/skills/repo-setup/SKILL.md @@ -74,3 +74,7 @@ own container, so `/workspace/repo` is already isolated. disposable. - Multi-repo investigation may clone **other** repositories under `~/dev/...`, but the target repo for this issue/PR stays `/workspace/repo`. +- The runtime bakes an `AGENTS.md` / `.agents/` skills overlay into the working + tree. Those are harness files, **not** part of the repo β€” never stage or commit + them. `git status` before every commit and unstage anything you didn't + deliberately change; keep the diff limited to the fix. diff --git a/apps/server/container/skills/respond-to-comment/SKILL.md b/apps/server/container/skills/respond-to-comment/SKILL.md index 340391f..e17c1e3 100644 --- a/apps/server/container/skills/respond-to-comment/SKILL.md +++ b/apps/server/container/skills/respond-to-comment/SKILL.md @@ -33,12 +33,38 @@ Before triaging, confirm the comment is actually for you: - **Not actionable**: style preference, out of scope, already handled β†’ reply with reason - **Approval thumbs-up** (short body, no code refs): don't reply, stop +## Own your PR end-to-end + +If the bot authored the PR, the bot owns getting it merge-ready β€” that doesn't +change just because a maintainer later pushed a follow-up commit onto the branch. +When a review comment on your PR is actionable, **fix it and push**; don't defer it +("leaving this for the author of that commit") and stop. Deferring strands the PR. + +If a comment genuinely isn't yours to act on β€” it's out of scope, or it's about a +change only a human should make β€” say that plainly on the thread AND `@`-mention a +human maintainer so someone picks it up. Silently leaving it open with no owner is +the one thing to avoid. + ## Workflow 1. Check PR authorship β€” only push to your own PR's branch. -2. If actionable on your own PR: implement the fix, load `deslop`, commit, - push, then reply on the thread with the commit SHA and resolve the thread - (see below). After all fixes, re-request review. +2. If actionable on your own PR: implement the fix, load `deslop`, commit and + push, then **confirm the push landed** before you reply: + + ```sh + git push origin HEAD + git rev-parse HEAD @{u} # the two SHAs must match β€” if not, the push failed + ``` + + Only once it's really on the branch, reply on the thread with the commit SHA, + resolve the thread (see below), and drop a πŸŽ‰ reaction on the comment as the + "done" signal: + + ```sh + gh api -X POST repos///pulls/comments//reactions -f content=hooray + ``` + + After all fixes, re-request review. 3. If actionable on someone else's PR: reply with a `suggestion` block or description. Don't push. 4. If not actionable: reply on the thread with the reason and leave it open diff --git a/apps/server/src/agents/instructions.ts b/apps/server/src/agents/instructions.ts index 5159d89..a00f3e3 100644 --- a/apps/server/src/agents/instructions.ts +++ b/apps/server/src/agents/instructions.ts @@ -98,7 +98,9 @@ then load the situation skill for the task at hand. 1. **Always first**: load \`repo-setup\` 2. **Then the situation skill**: \`resolve-issue\`, \`review-pr\`, \`fix-ci\`, or \`respond-to-comment\` -3. **Utility skills** as needed: \`deslop\`, \`review\`, \`pr\`, \`mark-pr-ready\`, \`apply-fixes\`, \`auto-merge\` +3. **Utility skills**: \`review\`, \`pr\`, \`mark-pr-ready\`, \`apply-fixes\`, \`auto-merge\` as + needed β€” and \`deslop\` **always right before you commit** (clean the diff every + time, not only when it looks messy) ### Model tiering β€” spend the premium model on judgment only @@ -221,11 +223,46 @@ for routine best-effort calls you can and should make yourself. - On webhook runs no human is watching β€” do not ask clarifying questions; make a best-effort call. Operator turns are the exception (see above) - Work in \`/workspace/repo\` β€” \`repo-setup\` puts it on the right branch +- **Keep the diff minimal and on-topic.** Only touch files needed for THIS task. + Never commit \`AGENTS.md\`, \`.agents/\`, \`.lore.md\`, editor/harness config, or + anything unrelated to the fix β€” the sandbox may leave harness overlays in the + working tree, so \`git status\` and \`git diff --staged\` before every commit and + unstage anything stray. Unrelated file churn just bloats the PR and slows review. +- **Confirm your push landed.** After pushing, verify the local branch HEAD equals + \`origin/\` before you tell anyone it's done. A commit that never left the + sandbox is not a fix β€” replying "fixed in " when the push failed is worse + than saying nothing. +- **Clean the diff with \`deslop\` before every commit** β€” no AI noise (narration + comments, needless try/catch, \`as any\`, leftover debug logs). Every code change, + not just the messy-looking ones. + +## Signaling progress with reactions + +The server already drops an πŸ‘€ reaction on the comment/issue that triggered you, so +the human knows you picked it up β€” you don't need to add that. When you FINISH what +they asked for (posted the review, pushed the fix, opened/updated the PR), leave a +single πŸŽ‰ reaction on that same trigger as the "done" signal: + +\`\`\`sh +# a top-level issue/PR comment +gh api -X POST repos///issues/comments//reactions -f content=hooray +# an inline PR review comment +gh api -X POST repos///pulls/comments//reactions -f content=hooray +# the issue or PR itself +gh api -X POST repos///issues//reactions -f content=hooray +\`\`\` + +One reaction, once, when you're actually done β€” not on every step, and never when +you ended in \`SKIPPED\` or \`BLOCKED\`. ## Tone & voice -Write like a competent teammate: concise, no filler, lowercase natural language -in PR comments, show don't narrate, no emoji unless the project already uses them. +Anything a human will read β€” PR/issue comments, review replies, chat answers β€” should +sound like a friendly, humble teammate: warm, plain, and short. Say the useful thing +and stop. No corporate filler, no status-report voice, no hedging walls of text, no +emoji in prose unless the thread already uses them (the πŸŽ‰ reaction above is the one +exception). If you were wrong or unsure, just say so plainly. Prefer two clear +sentences over a paragraph. ## Output diff --git a/apps/server/src/lib/github/__tests__/reactions.test.ts b/apps/server/src/lib/github/__tests__/reactions.test.ts new file mode 100644 index 0000000..76fb2e5 --- /dev/null +++ b/apps/server/src/lib/github/__tests__/reactions.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from "vitest" +import { ackTarget } from "../reactions" + +describe("ackTarget", () => { + it("targets the comment on a brand-new issue comment", () => { + expect(ackTarget("issue_comment", "created", { comment: { id: 42 } })).toEqual({ kind: "issueComment", id: 42 }) + }) + + it("targets the review comment on a new PR review comment", () => { + expect(ackTarget("pull_request_review_comment", "created", { comment: { id: 7 } })).toEqual({ + kind: "reviewComment", + id: 7, + }) + }) + + it("targets the issue when it is labeled / opened / reopened", () => { + for (const action of ["labeled", "opened", "reopened"]) { + expect(ackTarget("issues", action, { issue: { number: 5 } })).toEqual({ kind: "issue", number: 5 }) + } + }) + + it("targets the PR when a review is submitted", () => { + expect(ackTarget("pull_request_review", "submitted", { pull_request: { number: 9 } })).toEqual({ + kind: "issue", + number: 9, + }) + }) + + it("ignores CI, pushes, edits and deletions", () => { + expect(ackTarget("workflow_run", "completed", {})).toBeNull() + expect(ackTarget("push", null, {})).toBeNull() + expect(ackTarget("issue_comment", "edited", { comment: { id: 1 } })).toBeNull() + expect(ackTarget("issue_comment", "deleted", { comment: { id: 1 } })).toBeNull() + expect(ackTarget("issues", "closed", { issue: { number: 1 } })).toBeNull() + }) + + it("is null when the id / number is missing", () => { + expect(ackTarget("issue_comment", "created", {})).toBeNull() + expect(ackTarget("issues", "labeled", {})).toBeNull() + expect(ackTarget("pull_request_review", "submitted", {})).toBeNull() + }) +}) diff --git a/apps/server/src/lib/github/reactions.ts b/apps/server/src/lib/github/reactions.ts new file mode 100644 index 0000000..de1594e --- /dev/null +++ b/apps/server/src/lib/github/reactions.ts @@ -0,0 +1,90 @@ +// Immediate emoji acknowledgment for user-facing GitHub events. +// +// When a human triggers Jared β€” labels an issue, comments on a PR/issue, or +// leaves a review comment β€” we drop an πŸ‘€ "eyes" reaction on the exact thing +// they touched, synchronously at webhook time and independent of the agent +// actually booting a sandbox. It's the cheap "seen it, on it" signal a teammate +// gives, and it lands in seconds even when the real run takes a minute to warm +// up. Best-effort: a failed reaction never blocks dispatch. +// +// The matching "done" signal (πŸŽ‰) is left by the agent itself once it has +// finished the work (see the "Signaling progress" section of the instructions), +// because only the agent knows when the review/fix/PR is actually complete. + +import { formatError } from "@jared/utils" +import type { GitHubApp } from "./app" +import { lookup } from "./entity" + +type AckTarget = + | { kind: "issue"; number: number } + | { kind: "issueComment"; id: number } + | { kind: "reviewComment"; id: number } + +function toNumber(v: unknown): number | null { + return typeof v === "number" && Number.isFinite(v) ? v : null +} + +/** + * The thing to react on for a given event, or null when the event has no + * natural "you asked, I'm looking" target (CI runs, pushes, edits, deletions). + */ +export function ackTarget(event: string, action: string | null, payload: Record): AckTarget | null { + if (event === "issue_comment" && action === "created") { + const id = toNumber(lookup(payload, "comment.id")) + return id ? { kind: "issueComment", id } : null + } + if (event === "pull_request_review_comment" && action === "created") { + const id = toNumber(lookup(payload, "comment.id")) + return id ? { kind: "reviewComment", id } : null + } + if (event === "issues" && (action === "labeled" || action === "opened" || action === "reopened")) { + const number = toNumber(lookup(payload, "issue.number")) + return number ? { kind: "issue", number } : null + } + if (event === "pull_request_review" && action === "submitted") { + const number = toNumber(lookup(payload, "pull_request.number")) + return number ? { kind: "issue", number } : null + } + return null +} + +/** + * Drop an πŸ‘€ reaction on the triggering comment/issue so the human sees Jared + * picked the event up right away. Never throws β€” a reaction is a nicety, not a + * gate on dispatch. + */ +export async function acknowledgeGitHubEvent(opts: { + app: GitHubApp + installationId: number | null + repo: string | null + event: string + action: string | null + payload: Record + logger?: { warn: (obj: unknown, msg: string) => void } +}): Promise { + const { app, installationId, repo, event, action, payload, logger } = opts + if (!installationId || !repo) return + const [owner, name] = repo.split("/") + if (!owner || !name) return + + const target = ackTarget(event, action, payload) + if (!target) return + + try { + const octokit = app.getInstallationOctokit(installationId) + if (target.kind === "issue") { + await octokit.reactions.createForIssue({ owner, repo: name, issue_number: target.number, content: "eyes" }) + } else if (target.kind === "issueComment") { + await octokit.reactions.createForIssueComment({ owner, repo: name, comment_id: target.id, content: "eyes" }) + } else { + await octokit.reactions.createForPullRequestReviewComment({ + owner, + repo: name, + comment_id: target.id, + content: "eyes", + }) + } + } catch (err) { + logger?.warn({ error: formatError(err) }, "ack reaction failed") + } +} diff --git a/apps/server/src/routes/webhooks/github.ts b/apps/server/src/routes/webhooks/github.ts index f67eeb8..6c7434c 100644 --- a/apps/server/src/routes/webhooks/github.ts +++ b/apps/server/src/routes/webhooks/github.ts @@ -15,6 +15,7 @@ import { createGitHubApp, type GitHubApp } from "@/lib/github/app" import { TRIGGER_LABEL } from "@/lib/github/constants" import { dispatchGitHubEvent } from "@/lib/github/dispatch" import { extractEntityKey, lookup, lookupString } from "@/lib/github/entity" +import { acknowledgeGitHubEvent } from "@/lib/github/reactions" import type { BaseEnv } from "@/types" // A CI burst (many workflow_run/check_suite completions for one push) lands @@ -260,6 +261,11 @@ const router = new Hono().post("/", async (c) => { }) } + // Instant "seen it, on it" β€” drop an πŸ‘€ reaction on the comment/issue the + // human just touched. Fire-and-forget so it lands in seconds regardless of + // how long the sandbox takes to warm up (the agent leaves the πŸŽ‰ "done" one). + c.executionCtx.waitUntil(acknowledgeGitHubEvent({ app, installationId, repo, event, action, payload, logger })) + // --- Dispatch to sandbox in waitUntil (shared with the manual resend path) --- c.executionCtx.waitUntil( dispatchGitHubEvent(c.env, db, logger, { From d1c0d7ca058afc82046d1a4c5fab401892e393cb Mon Sep 17 00:00:00 2001 From: mathuraditya724 Date: Fri, 7 Aug 2026 19:42:29 +0530 Subject: [PATCH 2/2] =?UTF-8?q?fix(agent):=20stop=20pushing=20back=20on=20?= =?UTF-8?q?review=20comments=20=E2=80=94=20just=20do=20them?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BYK on craft#865: "the new model is lazy AF pushing back on all review comments, like that comment on AGENTS.md". The thread shows the pattern β€” told to revert AGENTS.md, Jared argued twice ("harness artifact… leaving open for the maintainers", "reverting would just be overwritten") before complying on the third ask, and separately never actioned a "drop this paragraph" comment until a human pinged "you missed this one". respond-to-comment now biases hard to action: a concrete reviewer request is an instruction to carry out, not a debate. Explicitly bans the "it'll be overwritten / harness artifact / not my responsibility / leaving for the maintainers" deflections, bans arguing the same point twice, and adds a step to enumerate open review threads so none are missed. Same "act on feedback, don't defend your choices" line added to the tone section. Co-authored-by: Cursor --- .../skills/respond-to-comment/SKILL.md | 51 +++++++++++++++---- .../container/flue/src/agents/instructions.ts | 5 ++ .../skills/respond-to-comment/SKILL.md | 51 +++++++++++++++---- apps/server/src/agents/instructions.ts | 5 ++ 4 files changed, 90 insertions(+), 22 deletions(-) diff --git a/apps/server/container/.agents/skills/respond-to-comment/SKILL.md b/apps/server/container/.agents/skills/respond-to-comment/SKILL.md index e17c1e3..ed1c8b1 100644 --- a/apps/server/container/.agents/skills/respond-to-comment/SKILL.md +++ b/apps/server/container/.agents/skills/respond-to-comment/SKILL.md @@ -29,21 +29,50 @@ Before triaging, confirm the comment is actually for you: ## Triage -- **Actionable**: real bug, missing test, valid concern β†’ fix it -- **Not actionable**: style preference, out of scope, already handled β†’ reply with reason -- **Approval thumbs-up** (short body, no code refs): don't reply, stop +Default to **doing what the reviewer asked.** A concrete request on your own PR β€” +"drop this paragraph", "revert this file", "rename X", "remove the try/catch" β€” is an +instruction to carry out, not a proposal to debate. Make the change. + +- **Actionable** (the overwhelming majority): a real bug, missing test, valid + concern, or any direct change request β†’ fix it and push. +- **Genuinely not actionable**: only when doing it would clearly break something, + contradicts an explicit project rule, or is factually wrong. Reply with ONE short, + specific reason β€” and just do it if the reviewer says it again. +- **Approval thumbs-up** (short body, no code refs): don't reply, stop. + +## Don't push back β€” do the work + +The failure mode to avoid: writing a paragraph about why a comment doesn't apply +instead of addressing it. Reviewers read that as lazy, and they're right. Concretely: + +- **Never refuse with "it'll be overwritten" / "it's a harness artifact" / "not my + responsibility" / "leaving this for the maintainers".** If it's in your PR's diff, + it's yours β€” fix it. (The real fix for a stray harness file like `AGENTS.md` is to + never commit it in the first place β€” see `repo-setup` β€” but if a reviewer asks you + to revert it, just revert it now.) +- **Never argue the same point twice.** If a reviewer restates their request, that's + your cue to do it, not to re-explain your reasoning. +- **Don't defer actionable feedback** to "the author of a later commit". You authored + the PR; you own it to merge-ready. + +If a comment truly isn't yours to act on (out of scope, or a call only a human should +make), say so in one plain sentence AND `@`-mention a human so it has an owner β€” never +silently leave it open. ## Own your PR end-to-end -If the bot authored the PR, the bot owns getting it merge-ready β€” that doesn't -change just because a maintainer later pushed a follow-up commit onto the branch. -When a review comment on your PR is actionable, **fix it and push**; don't defer it -("leaving this for the author of that commit") and stop. Deferring strands the PR. +The bot owns getting its own PR merge-ready β€” that doesn't change because a maintainer +pushed a follow-up commit onto the branch. Before you start, list the open review +threads so none slip through (a missed comment is as bad as a refused one): -If a comment genuinely isn't yours to act on β€” it's out of scope, or it's about a -change only a human should make β€” say that plainly on the thread AND `@`-mention a -human maintainer so someone picks it up. Silently leaving it open with no owner is -the one thing to avoid. +```sh +gh api graphql -f query='query($o:String!,$r:String!,$n:Int!){repository(owner:$o,name:$r){pullRequest(number:$n){reviewThreads(first:100){nodes{isResolved path comments(first:1){nodes{databaseId author{login} body}}}}}}}' \ + -f o= -f r= -F n= \ + --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false) | {path,body:.comments.nodes[0].body[0:80]}' +``` + +Work through every unresolved thread; don't stop until each is either fixed-and-pushed +or has a one-line reason plus a human owner. ## Workflow diff --git a/apps/server/container/flue/src/agents/instructions.ts b/apps/server/container/flue/src/agents/instructions.ts index c182905..93f5d22 100644 --- a/apps/server/container/flue/src/agents/instructions.ts +++ b/apps/server/container/flue/src/agents/instructions.ts @@ -264,6 +264,11 @@ emoji in prose unless the thread already uses them (the πŸŽ‰ reaction above is t exception). If you were wrong or unsure, just say so plainly. Prefer two clear sentences over a paragraph. +When someone reviews your work, humility means **acting on the feedback, not +defending your choices**. If a reviewer asks for a change, make it β€” don't write a +paragraph on why you won't, and never argue the same point twice. Pushing back on +review comments instead of just doing them reads as lazy (see \`respond-to-comment\`). + ## Output For each webhook event: the URL produced (PR, review, commit, or comment), diff --git a/apps/server/container/skills/respond-to-comment/SKILL.md b/apps/server/container/skills/respond-to-comment/SKILL.md index e17c1e3..ed1c8b1 100644 --- a/apps/server/container/skills/respond-to-comment/SKILL.md +++ b/apps/server/container/skills/respond-to-comment/SKILL.md @@ -29,21 +29,50 @@ Before triaging, confirm the comment is actually for you: ## Triage -- **Actionable**: real bug, missing test, valid concern β†’ fix it -- **Not actionable**: style preference, out of scope, already handled β†’ reply with reason -- **Approval thumbs-up** (short body, no code refs): don't reply, stop +Default to **doing what the reviewer asked.** A concrete request on your own PR β€” +"drop this paragraph", "revert this file", "rename X", "remove the try/catch" β€” is an +instruction to carry out, not a proposal to debate. Make the change. + +- **Actionable** (the overwhelming majority): a real bug, missing test, valid + concern, or any direct change request β†’ fix it and push. +- **Genuinely not actionable**: only when doing it would clearly break something, + contradicts an explicit project rule, or is factually wrong. Reply with ONE short, + specific reason β€” and just do it if the reviewer says it again. +- **Approval thumbs-up** (short body, no code refs): don't reply, stop. + +## Don't push back β€” do the work + +The failure mode to avoid: writing a paragraph about why a comment doesn't apply +instead of addressing it. Reviewers read that as lazy, and they're right. Concretely: + +- **Never refuse with "it'll be overwritten" / "it's a harness artifact" / "not my + responsibility" / "leaving this for the maintainers".** If it's in your PR's diff, + it's yours β€” fix it. (The real fix for a stray harness file like `AGENTS.md` is to + never commit it in the first place β€” see `repo-setup` β€” but if a reviewer asks you + to revert it, just revert it now.) +- **Never argue the same point twice.** If a reviewer restates their request, that's + your cue to do it, not to re-explain your reasoning. +- **Don't defer actionable feedback** to "the author of a later commit". You authored + the PR; you own it to merge-ready. + +If a comment truly isn't yours to act on (out of scope, or a call only a human should +make), say so in one plain sentence AND `@`-mention a human so it has an owner β€” never +silently leave it open. ## Own your PR end-to-end -If the bot authored the PR, the bot owns getting it merge-ready β€” that doesn't -change just because a maintainer later pushed a follow-up commit onto the branch. -When a review comment on your PR is actionable, **fix it and push**; don't defer it -("leaving this for the author of that commit") and stop. Deferring strands the PR. +The bot owns getting its own PR merge-ready β€” that doesn't change because a maintainer +pushed a follow-up commit onto the branch. Before you start, list the open review +threads so none slip through (a missed comment is as bad as a refused one): -If a comment genuinely isn't yours to act on β€” it's out of scope, or it's about a -change only a human should make β€” say that plainly on the thread AND `@`-mention a -human maintainer so someone picks it up. Silently leaving it open with no owner is -the one thing to avoid. +```sh +gh api graphql -f query='query($o:String!,$r:String!,$n:Int!){repository(owner:$o,name:$r){pullRequest(number:$n){reviewThreads(first:100){nodes{isResolved path comments(first:1){nodes{databaseId author{login} body}}}}}}}' \ + -f o= -f r= -F n= \ + --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false) | {path,body:.comments.nodes[0].body[0:80]}' +``` + +Work through every unresolved thread; don't stop until each is either fixed-and-pushed +or has a one-line reason plus a human owner. ## Workflow diff --git a/apps/server/src/agents/instructions.ts b/apps/server/src/agents/instructions.ts index a00f3e3..c323ff2 100644 --- a/apps/server/src/agents/instructions.ts +++ b/apps/server/src/agents/instructions.ts @@ -264,6 +264,11 @@ emoji in prose unless the thread already uses them (the πŸŽ‰ reaction above is t exception). If you were wrong or unsure, just say so plainly. Prefer two clear sentences over a paragraph. +When someone reviews your work, humility means **acting on the feedback, not +defending your choices**. If a reviewer asks for a change, make it β€” don't write a +paragraph on why you won't, and never argue the same point twice. Pushing back on +review comments instead of just doing them reads as lazy (see \`respond-to-comment\`). + ## Output For each webhook event: the URL produced (PR, review, commit, or comment),