chore(project): address findings

Author: bgagent

agent/src/context.py

@@ -61,7 +61,10 @@ def fetch_github_issue(repo_url: str, issue_number: str, token: str) -> GitHubIs
         comments = [
             IssueComment(
                 id=int(c["id"]),
-                author=c["user"]["login"],
+                # GitHub returns "user": null for comments whose author
+                # account was deleted ("ghost" comments) — an unguarded
+                # c["user"]["login"] would abort the whole hydration.
+                author=(c.get("user") or {}).get("login", "(deleted user)"),
                 body=c["body"] or "",
             )
             for c in comments_resp.json()

agent/src/post_hooks.py

@@ -164,9 +164,15 @@ def _note_unpushed_commits(repo_dir: str, branch: str, config: TaskConfig) -> No
     latest work, so the reviewer must be told. Failure to post the comment is
     logged but not fatal — the WARN log line emitted by the caller is the
     fallback signal.
+
+    ``check=False`` means ``run_cmd`` does NOT raise on a non-zero ``gh``
+    exit, so the returncode is inspected explicitly — otherwise a failed
+    ``gh pr comment`` (missing scope, rate limit, not-a-PR) is a silent
+    no-op and the reviewer never learns the PR is stale. The ``except``
+    below only covers OS-level failures (gh missing, timeout).
     """
     try:
-        run_cmd(
+        result = run_cmd(
             [
                 "gh",
                 "pr",
@@ -181,6 +187,15 @@ def _note_unpushed_commits(repo_dir: str, branch: str, config: TaskConfig) -> No
             cwd=repo_dir,
             check=False,
         )
+        if result.returncode != 0:
+            stderr_msg = result.stderr.strip()[:200] if result.stderr else "(no stderr)"
+            log(
+                "WARN",
+                "Failed to post un-pushed-commits note "
+                f"(gh exit {result.returncode}): {stderr_msg} — the PR does not "
+                "reflect the agent's latest commits and the reviewer has NOT "
+                "been notified.",
+            )
     except Exception as e:
         log("WARN", f"Failed to post un-pushed-commits note: {type(e).__name__}: {e}")
 

agent/tests/test_context.py

@@ -106,6 +106,23 @@ def test_null_body_becomes_empty_string(self, monkeypatch):
 
         assert issue.body == ""
 
+    def test_comment_by_deleted_account_does_not_crash(self, monkeypatch):
+        # GitHub returns "user": null for comments whose author account was
+        # deleted ("ghost" comments). An unguarded c["user"]["login"] raised
+        # TypeError and aborted the whole issue hydration.
+        payload = {"title": "Title", "body": "body", "number": 12, "comments": 2}
+        comments = [
+            {"id": 1, "user": None, "body": "comment from a deleted account"},
+            {"id": 2, "user": {"login": "alice"}, "body": "still here"},
+        ]
+        monkeypatch.setattr(context, "requests", _fake_requests(payload, comments))
+
+        issue = fetch_github_issue("owner/repo", "12", "tok")
+
+        assert issue.comments[0].author == "(deleted user)"
+        assert issue.comments[0].body == "comment from a deleted account"
+        assert issue.comments[1].author == "alice"
+
 
 def _fetched_issue(monkeypatch, *, title, body, number, comments=None):
     """Fetch an issue from raw payloads, exercising the source sanitizer.

agent/tests/test_post_hooks.py

@@ -123,6 +123,30 @@ def test_push_failure_posts_unpushed_note_and_returns_url(self, monkeypatch):
         note_cmd = run_cmd.cmd_for("note-unpushed-commits")
         assert "comment" in note_cmd
 
+    def test_failed_note_post_warns_loudly(self, monkeypatch):
+        # check=False means run_cmd never raises on a non-zero gh exit, so
+        # _note_unpushed_commits must inspect the returncode itself — a
+        # failed `gh pr comment` (missing scope, rate limit) was previously
+        # a silent no-op while the PR quietly went stale.
+        monkeypatch.setattr(post_hooks, "ensure_pushed", lambda d, b: False)
+        sub = _pr_view("https://github.com/o/r/pull/9")
+        run_cmd = _RunCmdRecorder(returncodes={"note-unpushed-commits": 1})
+        monkeypatch.setattr(post_hooks.subprocess, "run", sub)
+        monkeypatch.setattr(post_hooks, "run_cmd", run_cmd)
+        warns: list[str] = []
+        monkeypatch.setattr(
+            post_hooks, "log", lambda lvl, msg: warns.append(msg) if lvl == "WARN" else None
+        )
+
+        url = post_hooks.ensure_pr(
+            _config(), _setup(), build_passed=True, lint_passed=True, strategy="push_resolve"
+        )
+
+        # The URL is still returned (PR exists), but the failure to notify
+        # the reviewer is surfaced as a WARN naming the consequence.
+        assert url == "https://github.com/o/r/pull/9"
+        assert any("reviewer has NOT been notified" in w for w in warns)
+
     def test_push_success_does_not_post_note(self, monkeypatch):
         monkeypatch.setattr(post_hooks, "ensure_pushed", lambda d, b: True)
         sub = _pr_view("https://github.com/o/r/pull/9")

cdk/package.json

@@ -55,7 +55,7 @@
     "@typescript-eslint/eslint-plugin": "^8",
     "@typescript-eslint/parser": "^8",
     "aws-cdk": "^2",
-    "esbuild": "^0.27.4",
+    "esbuild": "^0.28.1",
     "eslint": "^10",
     "eslint-import-resolver-typescript": "^4",
     "eslint-plugin-import-x": "^4",

cdk/src/constructs/github-screenshot-integration.ts

@@ -20,6 +20,7 @@
 import * as path from 'path';
 import { ArnFormat, Duration, RemovalPolicy, Stack } from 'aws-cdk-lib';
 import * as apigw from 'aws-cdk-lib/aws-apigateway';
+import * as cloudwatch from 'aws-cdk-lib/aws-cloudwatch';
 import * as dynamodb from 'aws-cdk-lib/aws-dynamodb';
 import * as iam from 'aws-cdk-lib/aws-iam';
 import { Architecture, Runtime } from 'aws-cdk-lib/aws-lambda';
@@ -110,6 +111,11 @@ export class GitHubScreenshotIntegration extends Construct {
   /** Async processor Lambda (browser + S3 + PR comment). */
   public readonly webhookProcessorFn: lambda.NodejsFunction;
 
+  /** Fires when a failed async invocation lands in the processor DLQ —
+   *  mirrors ``FanOutConsumer.dlqDepthAlarm``: without it the queue is
+   *  "for operator inspection" that no operator is ever told to make. */
+  public readonly processorDlqDepthAlarm: cloudwatch.Alarm;
+
   constructor(scope: Construct, id: string, props: GitHubScreenshotIntegrationProps) {
     super(scope, id);
 
@@ -185,6 +191,22 @@ export class GitHubScreenshotIntegration extends Construct {
       },
     ]);
 
+    // Alarm on any record landing in the DLQ. The processor handler
+    // swallows its own errors, so only init-time crashes reach this queue
+    // — rare, but each one is a screenshot pipeline silently down. Same
+    // threshold-1 shape as FanOutConsumer.dlqDepthAlarm.
+    this.processorDlqDepthAlarm = new cloudwatch.Alarm(this, 'WebhookProcessorDlqDepthAlarm', {
+      metric: processorDlq.metricApproximateNumberOfMessagesVisible({
+        period: Duration.minutes(5),
+        statistic: 'Maximum',
+      }),
+      threshold: 1,
+      evaluationPeriods: 1,
+      alarmDescription:
+        'Screenshot webhook processor DLQ has failed async invocations — the screenshot pipeline is crashing before handling events',
+      treatMissingData: cloudwatch.TreatMissingData.NOT_BREACHING,
+    });
+
     this.screenshotBucket.bucket.grantPut(this.webhookProcessorFn);
     props.githubTokenSecret.grantRead(this.webhookProcessorFn);
 

cdk/src/handlers/shared/validation.ts

@@ -39,7 +39,10 @@ export const MAX_MAX_TURNS = 500;
 /** Maximum allowed length for task_description. */
 export const MAX_TASK_DESCRIPTION_LENGTH = 10_000;
 
-const REPO_PATTERN = /^[a-zA-Z0-9._-]+\/[a-zA-Z0-9._-]+$/;
+// Dots are legal inside segments (`vercel/next.js`) but a segment of ONLY
+// dots (`owner/..`, `./repo`) is a path token, not a repo name — the
+// lookaheads reject those so URL/key interpolation never sees `.`/`..`.
+const REPO_PATTERN = /^(?!\.+\/)[a-zA-Z0-9._-]+\/(?!\.+$)[a-zA-Z0-9._-]+$/;
 const IDEMPOTENCY_KEY_PATTERN = /^[a-zA-Z0-9_-]{1,128}$/;
 const WEBHOOK_NAME_PATTERN = /^[a-zA-Z0-9][a-zA-Z0-9 _-]{0,62}[a-zA-Z0-9]$/;
 // ULID format: 26 chars, Crockford Base32 alphabet (0-9, A-Z excluding I, L, O, U).

cdk/test/constructs/github-screenshot-integration.test.ts

@@ -65,6 +65,31 @@ describe('GitHubScreenshotIntegration construct', () => {
     });
   });
 
+  test('alarms on the first record landing in the processor DLQ', () => {
+    // The processor handler swallows its own errors, so only init-time
+    // crashes reach the queue — each one means the screenshot pipeline is
+    // silently down. Without the alarm the queue is "for operator
+    // inspection" that no operator is ever told to make.
+    template.resourceCountIs('AWS::CloudWatch::Alarm', 1);
+    template.hasResourceProperties('AWS::CloudWatch::Alarm', {
+      MetricName: 'ApproximateNumberOfMessagesVisible',
+      Namespace: 'AWS/SQS',
+      Statistic: 'Maximum',
+      Period: 300,
+      Threshold: 1,
+      EvaluationPeriods: 1,
+      TreatMissingData: 'notBreaching',
+      Dimensions: Match.arrayWith([
+        Match.objectLike({
+          Name: 'QueueName',
+          Value: Match.objectLike({
+            'Fn::GetAtt': Match.arrayWith([Match.stringLikeRegexp('WebhookProcessorDlq')]),
+          }),
+        }),
+      ]),
+    });
+  });
+
   test('wires the DLQ as the processor Lambda async-invoke dead-letter target', () => {
     // The queue existing is not enough — it must be bound to the
     // processor function's DeadLetterConfig or failed async invokes

cdk/test/handlers/shared/validation.test.ts

@@ -77,6 +77,19 @@ describe('isValidRepo', () => {
     expect(isValidRepo('trailing-slash/')).toBe(false);
     expect(isValidRepo('has spaces/repo')).toBe(false);
   });
+
+  test('rejects pure-dot path segments while keeping dotted names', () => {
+    // `owner/..` is a path token, not a repo name — the char class allows
+    // dots so the multi-slash rule alone never caught the single-segment
+    // traversal shapes. Dotted REAL names (next.js) must keep working.
+    expect(isValidRepo('owner/..')).toBe(false);
+    expect(isValidRepo('owner/.')).toBe(false);
+    expect(isValidRepo('../repo')).toBe(false);
+    expect(isValidRepo('./repo')).toBe(false);
+    expect(isValidRepo('vercel/next.js')).toBe(true);
+    expect(isValidRepo('owner/.github')).toBe(true);
+    expect(isValidRepo('owner/repo.')).toBe(true);
+  });
 });
 
 describe('hasTaskSpec', () => {

cli/src/auth.ts

@@ -115,6 +115,12 @@ async function ensureFreshCredentials(): Promise<Credentials> {
 
 function isExpired(creds: Credentials): boolean {
   const expiryMs = new Date(creds.token_expiry).getTime();
+  // A corrupt token_expiry parses to NaN, and every comparison against NaN
+  // is false — the token would be classified as never-expiring and surface
+  // as an opaque 401 instead of a refresh. Treat unparseable as expired.
+  if (!Number.isFinite(expiryMs)) {
+    return true;
+  }
   return Date.now() >= expiryMs - TOKEN_REFRESH_BUFFER_MS;
 }