Revert "fix: address PR #20 review findings and scope reviewer to diff only"

This reverts commit dec1e33.

Author: adamhenson

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

@@ -19,6 +19,7 @@ permissions:
   pull-requests: read
 
 tools:
+  bash: ["cat", "head", "tail", "grep", "wc", "ls", "find"]
   github:
     toolsets: [repos, pull_requests]
 
@@ -40,21 +41,19 @@ Review PR #{{ github.event.pull_request.number }}: "{{ github.event.pull_request
 
 ## Review Process
 
-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:**
+1. **Fetch the PR diff** using the GitHub tool.
+2. **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
-4. **Post inline review comments** only on lines that appear in the diff.
-5. **Submit your review:**
+3. **Post inline review comments** on specific lines where you find issues.
+4. **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

@@ -288,12 +288,8 @@ 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: |
-    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" \
+    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" \
       > /tmp/agent-tokens.json
 
 - uses: actions/upload-artifact@v4

README.md

@@ -302,17 +302,21 @@ async function findExistingComment({
   repo: string;
   issueOrPrNumber: number;
 }): Promise<{ id: number; body: string } | null> {
-  const gh = octokit as ReturnType<typeof import('@actions/github').getOctokit>;
-  const comments = await gh.paginate(gh.rest.issues.listComments, {
-    owner,
-    repo,
-    issue_number: issueOrPrNumber,
-    per_page: 100,
-  });
+  try {
+    const gh = octokit as ReturnType<typeof import('@actions/github').getOctokit>;
+    const comments = await gh.paginate(gh.rest.issues.listComments, {
+      owner,
+      repo,
+      issue_number: issueOrPrNumber,
+      per_page: 100,
+    });
 
-  const existing = comments.find((c) => c.body?.includes(COMMENT_MARKER));
-  if (!existing) return null;
-  return { id: existing.id, body: existing.body ?? '' };
+    const existing = comments.find((c) => c.body?.includes(COMMENT_MARKER));
+    if (!existing) return null;
+    return { id: existing.id, body: existing.body ?? '' };
+  } catch {
+    return null;
+  }
 }
 
 /**

dist/index.js

@@ -99,10 +99,9 @@ export async function run(): Promise<void> {
       if (!inputs.triggerEvent) resolvedTriggerEvent = runData.triggerEvent;
       resolvedTriggerRef = runData.triggerRef;
       resolvedTriggerType = runData.triggerType;
-      // Prefer the agent workflow's name from the run data over ctx.workflowName (which is the
-      // companion workflow). Only overwrite when non-empty so a failed getWorkflowRun() doesn't
-      // blank out the name entirely.
-      if (runData.workflowName) resolvedWorkflowName = runData.workflowName;
+      // 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.
+      resolvedWorkflowName = runData.workflowName;
       workflowRunTokens = runData.tokens;
       artifactTurns = runData.artifactTurns;
     }
@@ -139,17 +138,11 @@ 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: resolvedTriggerType ?? resolvedTriggerEvent,
-          number: resolvedTriggerNumber,
-        })
+      ? buildTriggerRef({ eventName: 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> {
-  const attemptCheck = async (): Promise<boolean> => {
+  try {
     const { data } = await octokit.rest.actions.listJobsForWorkflowRun({
       owner,
       repo,
@@ -198,24 +198,12 @@ async function checkConclusionJobCompleted({
     }
     core.info(`AgentMeter: conclusion job completed (${conclusionJob.conclusion}) — proceeding.`);
     return true;
-  };
-
-  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 — proceed rather than silently dropping the ingest. A transient
-      // API hiccup or token-scope issue should not cause permanent data loss.
-      core.warning(
-        `AgentMeter: could not check conclusion job status (attempt 2): ${secondError}. 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;
   }
 }
 
@@ -346,9 +334,9 @@ async function resolveTrigger({
     }
   }
 
-  // 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+)$/);
+  // 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/);
   if (issueMatch?.[1]) {
     const num = parseInt(issueMatch[1], 10);
     return {