From 542a3b6a896a1db8a0cdc7832ef231b5e53dfeee Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Fri, 17 Apr 2026 22:01:52 +0530 Subject: [PATCH] fix: address PR review feedback (#131) --- .changeset/late-lions-share.md | 2 +- .../core/src/__tests__/agent-report.test.ts | 47 ++++++++++++++ packages/core/src/agent-report.ts | 62 ++++++++++--------- 3 files changed, 82 insertions(+), 29 deletions(-) diff --git a/.changeset/late-lions-share.md b/.changeset/late-lions-share.md index b02745029..3a7a0db3a 100644 --- a/.changeset/late-lions-share.md +++ b/.changeset/late-lions-share.md @@ -1,6 +1,6 @@ +--- "@aoagents/ao-core": minor "@aoagents/ao-cli": minor - --- Allow workers to report non-terminal PR workflow events like `pr-created`, `draft-pr-created`, and `ready-for-review` with optional PR URL/number metadata, while keeping merged and closed PR state SCM-owned. diff --git a/packages/core/src/__tests__/agent-report.test.ts b/packages/core/src/__tests__/agent-report.test.ts index e3604d3d4..94d1309ad 100644 --- a/packages/core/src/__tests__/agent-report.test.ts +++ b/packages/core/src/__tests__/agent-report.test.ts @@ -310,6 +310,53 @@ describe("applyAgentReport", () => { expect(payload.pr.number).toBe(44); }); + it("preserves prior PR URL when ready_for_review only supplies a PR number", () => { + applyAgentReport(dataDir, sessionId, { + state: "draft_pr_created", + prUrl: "https://github.com/test/repo/pull/45", + now: new Date("2025-01-02T10:00:00.000Z"), + }); + + applyAgentReport(dataDir, sessionId, { + state: "ready_for_review", + prNumber: 45, + now: new Date("2025-01-02T10:05:00.000Z"), + }); + + const meta = readMetadataRaw(dataDir, sessionId)!; + expect(meta[AGENT_REPORT_METADATA_KEYS.PR_URL]).toBe("https://github.com/test/repo/pull/45"); + expect(meta[AGENT_REPORT_METADATA_KEYS.PR_NUMBER]).toBe("45"); + }); + + it("does not clear PR metadata on later non-PR workflow reports", () => { + applyAgentReport(dataDir, sessionId, { + state: "pr_created", + prUrl: "https://github.com/test/repo/pull/46", + now: new Date("2025-01-02T10:00:00.000Z"), + }); + + applyAgentReport(dataDir, sessionId, { + state: "working", + now: new Date("2025-01-02T10:05:00.000Z"), + }); + + const meta = readMetadataRaw(dataDir, sessionId)!; + expect(meta[AGENT_REPORT_METADATA_KEYS.PR_URL]).toBe("https://github.com/test/repo/pull/46"); + expect(meta[AGENT_REPORT_METADATA_KEYS.PR_NUMBER]).toBe("46"); + expect(meta[AGENT_REPORT_METADATA_KEYS.PR_IS_DRAFT]).toBe("false"); + }); + + it("rejects conflicting PR URL and PR number inputs", () => { + expect(() => + applyAgentReport(dataDir, sessionId, { + state: "pr_created", + prUrl: "https://github.com/test/repo/pull/47", + prNumber: 99, + now: new Date("2025-01-02T10:00:00.000Z"), + }), + ).toThrow(/does not match PR URL/); + }); + it("sets startedAt on the first working transition", () => { const now = new Date("2025-01-01T12:00:00.000Z"); // Re-seed with startedAt explicitly null so we exercise the first-start diff --git a/packages/core/src/agent-report.ts b/packages/core/src/agent-report.ts index 756e7f33b..961ece001 100644 --- a/packages/core/src/agent-report.ts +++ b/packages/core/src/agent-report.ts @@ -371,13 +371,17 @@ export function applyAgentReport( typeof input.prNumber === "number" && Number.isInteger(input.prNumber) && input.prNumber > 0 ? input.prNumber : undefined; - const inferredPrNumber = - trimmedPrUrl && parsedPrNumber === undefined - ? (() => { - const parsed = parsePrFromUrl(trimmedPrUrl); - return parsed?.number; - })() - : undefined; + const parsedPrFromUrl = trimmedPrUrl ? parsePrFromUrl(trimmedPrUrl) : null; + const urlPrNumber = parsedPrFromUrl?.number; + if ( + trimmedPrUrl && + parsedPrNumber !== undefined && + urlPrNumber !== undefined && + parsedPrNumber !== urlPrNumber + ) { + throw new Error(`PR number ${parsedPrNumber} does not match PR URL ${trimmedPrUrl}`); + } + const inferredPrNumber = urlPrNumber; const prNumber = parsedPrNumber ?? inferredPrNumber; const prIsDraft = input.state === "draft_pr_created" @@ -385,6 +389,12 @@ export function applyAgentReport( : input.state === "pr_created" || input.state === "ready_for_review" ? false : undefined; + const existingPrUrl = raw[AGENT_REPORT_METADATA_KEYS.PR_URL]?.trim() || undefined; + const existingPrNumberRaw = raw[AGENT_REPORT_METADATA_KEYS.PR_NUMBER]; + const existingPrNumber = + existingPrNumberRaw && /^\d+$/.test(existingPrNumberRaw) + ? Number.parseInt(existingPrNumberRaw, 10) + : undefined; let before: AgentReportAuditSnapshot | null = null; let previousState: CanonicalSessionState | null = null; let nextState: CanonicalSessionState | null = null; @@ -419,17 +429,17 @@ export function applyAgentReport( current.session.reason = mapped.sessionReason; current.session.lastTransitionAt = now; if (isPRWorkflowReport(input.state)) { + const effectivePrUrl = trimmedPrUrl ?? current.pr.url ?? existingPrUrl; + const effectivePrNumber = + prNumber ?? current.pr.number ?? existingPrNumber ?? parsedPrFromUrl?.number; current.pr.state = "open"; current.pr.reason = input.state === "ready_for_review" ? "review_pending" : "in_progress"; current.pr.lastObservedAt = now; - if (trimmedPrUrl) { - current.pr.url = trimmedPrUrl; + if (effectivePrUrl) { + current.pr.url = effectivePrUrl; } - if (prNumber !== undefined) { - current.pr.number = prNumber; - } else if (trimmedPrUrl) { - const parsed = parsePrFromUrl(trimmedPrUrl); - current.pr.number = parsed?.number ?? current.pr.number; + if (effectivePrNumber !== undefined) { + current.pr.number = effectivePrNumber; } } if (mapped.sessionState === "working" && current.session.startedAt === null) { @@ -454,20 +464,16 @@ export function applyAgentReport( // Clear stale notes from previous reports so they don't mislead humans. metadataUpdates[AGENT_REPORT_METADATA_KEYS.NOTE] = ""; } - if (trimmedPrUrl) { - metadataUpdates[AGENT_REPORT_METADATA_KEYS.PR_URL] = trimmedPrUrl; - } else { - metadataUpdates[AGENT_REPORT_METADATA_KEYS.PR_URL] = ""; - } - if (prNumber !== undefined) { - metadataUpdates[AGENT_REPORT_METADATA_KEYS.PR_NUMBER] = String(prNumber); - } else { - metadataUpdates[AGENT_REPORT_METADATA_KEYS.PR_NUMBER] = ""; - } - if (prIsDraft !== undefined) { - metadataUpdates[AGENT_REPORT_METADATA_KEYS.PR_IS_DRAFT] = prIsDraft ? "true" : "false"; - } else { - metadataUpdates[AGENT_REPORT_METADATA_KEYS.PR_IS_DRAFT] = ""; + if (isPRWorkflowReport(input.state)) { + if (trimmedPrUrl) { + metadataUpdates[AGENT_REPORT_METADATA_KEYS.PR_URL] = trimmedPrUrl; + } + if (prNumber !== undefined) { + metadataUpdates[AGENT_REPORT_METADATA_KEYS.PR_NUMBER] = String(prNumber); + } + if (prIsDraft !== undefined) { + metadataUpdates[AGENT_REPORT_METADATA_KEYS.PR_IS_DRAFT] = prIsDraft ? "true" : "false"; + } } updateMetadata(dataDir, sessionId, metadataUpdates);