fix: address PR review findings and scope reviewer to diff only (#23)

* fix: address PR review findings and scope reviewer to diff only

* test: update conclusion-job failure test to expect proceed-on-error behavior

* fix: retry conclusion job check once then fail closed; blank workflow name over misattributed companion

* fix: restore findExistingComment error handling — fall through to create on transient list failure

Author: adamhenson

.github/workflows/agent-review.lock.yml

@@ -19,7 +19,6 @@ permissions:
   pull-requests: read
 
 tools:
-  bash: ["cat", "head", "tail", "grep", "wc", "ls", "find"]
   github:
     toolsets: [repos, pull_requests]
 
@@ -41,19 +40,21 @@ Review PR #{{ github.event.pull_request.number }}: "{{ github.event.pull_request
 
 ## Review Process
 
-1. **Fetch the PR diff** using the GitHub tool.
-2. **Review for:**
+1. **Fetch the PR diff** using the GitHub tool (`get-pull-request-diff` or equivalent).
+2. **Review only the changed lines** (`+` lines in the diff). Do not fetch or read full file contents — your entire review must be scoped to lines present in the diff.
+3. **Review for:**
    - TypeScript type safety issues (strict mode — no `any`, no unchecked nulls)
    - Code style: JSDoc block above every function, alphabetical params, explicit return types
    - Logic errors or bugs
    - Missing test coverage for new logic
-3. **Post inline review comments** on specific lines where you find issues.
-4. **Submit your review:**
+4. **Post inline review comments** only on lines that appear in the diff.
+5. **Submit your review:**
    - No issues: use `noop` with a brief summary.
    - Issues found: COMMENT with specific, actionable feedback.
 
 ## Important
 
+- **Only comment on lines in the diff.** Do not flag pre-existing code that was not touched by this PR.
 - Be concise and constructive.
 - Don't nitpick formatting — Biome handles that.
 - Focus on correctness and type safety.

.github/workflows/agent-review.md

@@ -290,8 +290,12 @@ If you're not using gh-aw, add these two steps to your agent job to write and up
 - name: Write token counts
   if: always()
   run: |
-    printf '{"input_tokens":%s,"output_tokens":%s,"cache_read_tokens":%s,"cache_write_tokens":%s,"turns":%s}\n' \
-      "$INPUT_TOKENS" "$OUTPUT_TOKENS" "$CACHE_READ_TOKENS" "$CACHE_WRITE_TOKENS" "$TURNS" \
+    turns_field=""
+    if [ -n "$TURNS" ]; then
+      turns_field=",\"turns\":$TURNS"
+    fi
+    printf '{"input_tokens":%s,"output_tokens":%s,"cache_read_tokens":%s,"cache_write_tokens":%s%s}\n' \
+      "$INPUT_TOKENS" "$OUTPUT_TOKENS" "$CACHE_READ_TOKENS" "$CACHE_WRITE_TOKENS" "$turns_field" \
       > /tmp/agent-tokens.json
 
 - uses: actions/upload-artifact@v4

README.md

@@ -379,7 +379,7 @@ describe('resolveWorkflowRun', () => {
     expect(result.tokens?.cacheWriteTokens).toBe(0);
   });
 
-  it('skips when listJobsForWorkflowRun fails (fail closed to prevent double-ingest)', async () => {
+  it('retries once then skips when listJobsForWorkflowRun fails twice (fail closed)', async () => {
     const octokit = makeOctokit({});
     octokit.rest.actions.listJobsForWorkflowRun = vi
       .fn()
@@ -389,11 +389,28 @@ describe('resolveWorkflowRun', () => {
     const result = await resolveWorkflowRun(baseArgs);
 
     expect(result.shouldProceed).toBe(false);
+    expect(octokit.rest.actions.listJobsForWorkflowRun).toHaveBeenCalledTimes(2);
     expect(vi.mocked(core.warning)).toHaveBeenCalledWith(
       expect.stringContaining('could not check conclusion job status')
     );
   });
 
+  it('proceeds when listJobsForWorkflowRun succeeds on retry after transient failure', async () => {
+    const octokit = makeOctokit({ conclusionJobStatus: 'completed' });
+    octokit.rest.actions.listJobsForWorkflowRun = vi
+      .fn()
+      .mockRejectedValueOnce(new Error('500 server error'))
+      .mockResolvedValueOnce({
+        data: { jobs: [{ name: 'conclusion', status: 'completed', conclusion: 'success' }] },
+      } as never);
+    mockGetOctokit.mockReturnValue(octokit as never);
+
+    const result = await resolveWorkflowRun(baseArgs);
+
+    expect(result.shouldProceed).toBe(true);
+    expect(octokit.rest.actions.listJobsForWorkflowRun).toHaveBeenCalledTimes(2);
+  });
+
   it('warns and returns partial data when workflow run API fails', async () => {
     const octokit = makeOctokit({});
     octokit.rest.actions.getWorkflowRun = vi.fn().mockRejectedValue(new Error('API error'));

__tests__/workflow-run.test.ts

@@ -314,7 +314,12 @@ async function findExistingComment({
     const existing = comments.find((c) => c.body?.includes(COMMENT_MARKER));
     if (!existing) return null;
     return { id: existing.id, body: existing.body ?? '' };
-  } catch {
+  } catch (error) {
+    // Log and return null so upsertComment falls through to createComment.
+    // A possible duplicate is preferable to silently dropping the comment entirely.
+    core.warning(
+      `AgentMeter: could not list comments to find existing — will create new: ${error}`
+    );
     return null;
   }
 }

dist/index.js

@@ -99,8 +99,9 @@ export async function run(): Promise<void> {
       if (!inputs.triggerEvent) resolvedTriggerEvent = runData.triggerEvent;
       resolvedTriggerRef = runData.triggerRef;
       resolvedTriggerType = runData.triggerType;
-      // Always use the agent workflow's name from the run data — ctx.workflowName is the
-      // companion workflow and would misattribute if used as a fallback here.
+      // Always use the agent workflow's name from run data, even if empty. A blank name is
+      // preferable to silently inheriting ctx.workflowName (the companion workflow), which would
+      // misattribute ingests in the dashboard.
       resolvedWorkflowName = runData.workflowName;
       workflowRunTokens = runData.tokens;
       artifactTurns = runData.artifactTurns;
@@ -138,11 +139,17 @@ export async function run(): Promise<void> {
   // Fall back to resolvedTriggerRef from resolveTrigger (companion workflow_run mode — it knows
   // whether a PR was found regardless of the triggering event name, e.g. issue_comment on a PR).
   // Last resort: buildTriggerRef from the event name and number.
+  // Prefer resolvedTriggerType (set by resolveTrigger in workflow_run mode — already knows
+  // PR vs issue) over resolvedTriggerEvent when building the ref label, so a manual
+  // trigger_number without trigger_event doesn't default to the generic '#N' form for PRs.
   const triggerRef =
     ctx.triggerRef ??
     resolvedTriggerRef ??
     (resolvedTriggerNumber !== null
-      ? buildTriggerRef({ eventName: resolvedTriggerEvent, number: resolvedTriggerNumber })
+      ? buildTriggerRef({
+          eventName: resolvedTriggerType ?? resolvedTriggerEvent,
+          number: resolvedTriggerNumber,
+        })
       : null);
 
   // resolvedTriggerType is only set in companion workflow_run mode (from resolveTrigger).

src/comment.ts

@@ -180,7 +180,7 @@ async function checkConclusionJobCompleted({
   /** Workflow run ID */
   workflowRunId: number;
 }): Promise<boolean> {
-  try {
+  const attemptCheck = async (): Promise<boolean> => {
     const { data } = await octokit.rest.actions.listJobsForWorkflowRun({
       owner,
       repo,
@@ -198,12 +198,24 @@ async function checkConclusionJobCompleted({
     }
     core.info(`AgentMeter: conclusion job completed (${conclusionJob.conclusion}) — proceeding.`);
     return true;
-  } catch (error) {
-    // Fail closed on API errors — proceeding on a failed gate check risks double-ingest.
-    // A missing conclusion job (non-gh-aw workflow) is handled above as a successful
-    // API call that returns no matching job, not as an exception.
-    core.warning(`AgentMeter: could not check conclusion job status: ${error}. Skipping.`);
-    return false;
+  };
+
+  try {
+    return await attemptCheck();
+  } catch (firstError) {
+    core.warning(
+      `AgentMeter: could not check conclusion job status (attempt 1): ${firstError}. Retrying…`
+    );
+    try {
+      return await attemptCheck();
+    } catch (secondError) {
+      // Both attempts failed — fail closed to prevent duplicate ingest on persistent API errors
+      // (e.g. under-scoped token). The retry above already handled transient one-shot failures.
+      core.warning(
+        `AgentMeter: could not check conclusion job status (attempt 2): ${secondError}. Skipping.`
+      );
+      return false;
+    }
   }
 }
 
@@ -334,9 +346,9 @@ async function resolveTrigger({
     }
   }
 
-  // gh-aw issue branches are named agent/issue-N — require the full prefix to
-  // avoid mismatching unrelated branches like feature/fix-issue-12-auth.
-  const issueMatch = headBranch.match(/\bagent\/issue-(\d+)\b/);
+  // gh-aw issue branches are named agent/issue-N — anchor to the full branch name to
+  // avoid substring matches in longer branch names like feature/agent/issue-12-fix.
+  const issueMatch = headBranch.match(/^agent\/issue-(\d+)$/);
   if (issueMatch?.[1]) {
     const num = parseInt(issueMatch[1], 10);
     return {