fix(project): resolve audit findings, review findings, and code-scanning alerts

[krokoko]

Summary

Follow-up to #324 (high-severity audit batch). This PR closes out the remaining medium/low findings from the 2026-06-11 full-codebase audit, the confirmed findings from a high-effort multi-agent code review of that work, the PR #324 review follow-up (whitespace-secret guards), and all three open CodeQL code-scanning alerts.

Two commits: ea4c94a (audit findings) and bc9d593 (review findings + code-scanning alerts).

Security hardening

• All four webhook verifiers fail closed on empty/whitespace signing secrets. The GitHub and Linear verifiers were hardened in fix(project): fix multiple issues identified #324; this PR mirrors the same guard into slack-verify.ts and webhook-create-task.ts (PR fix(project): fix multiple issues identified #324 review follow-up), then consolidates all eight hand-copied checks into one shared chokepoint — isUsableHmacSecret() in cdk/src/handlers/shared/hmac-secret.ts — so a future webhook source can't forget the invariant. HMAC('', body) is computable by anyone; an empty secret must never verify. Forgery tests added per verifier.
• GitHub issue content sanitized at the source (agent/src/context.py): fetch_github_issue now routes title, body, and every comment through sanitize_external_content as the GitHubIssue model is built, and assemble_prompt wraps the block in explicit untrusted-content delimiters. Previously the local/dry-run path injected raw issue text into the prompt (the orchestrator only sanitizes the deployed path), and the model carried unsanitized strings any future consumer would trust.
• github-deployment-status.ts validates shape, not just presence: repoFullName must match owner/repo and sha must be hex before either is interpolated into the GitHub API URL / S3 key (defense-in-depth; payload is already HMAC-verified).
• Non-dict tool_input is now an explicit fail-closed deny in agent/src/hooks.py instead of an accidental AttributeError caught by the engine's broad except.

Code-scanning alerts (CodeQL, all 3 open alerts)

• Alert 29 — agent/src/shell.py (py/clear-text-logging, high): log() now emits via an os.write(1, ...) sink after redact_secrets, matching the pattern server._debug_cw already used. Output-observing tests switched capsys → capfd.
• Alert 31 — agent/src/server.py (py/clear-text-logging, high): _warn_cw previously printed messages unredacted; it now routes through _redact_cached_credentials plus a shared _emit_stdout_line helper (extracted from _debug_cw, removing that duplication).
• Alert 30 — cli/src/commands/admin.ts (js/clear-text-logging, high): admin invite-user no longer prints the Cognito password to stdout. The share-block is written to a 0600 file under ~/.bgagent/invites/ and the command prints the path with instructions to share over a secure channel and delete. Scrollback and CI capture outlive the "share once" intent.

Correctness & resilience

• Linear final-status comments are idempotent across partial-batch retries: a linear_final_comment_event_id post-once marker on TaskRecord (conditional write) prevents duplicate comments when a sibling channel's infra rejection re-runs all dispatchers — Linear has no comment-edit API. A shared saveDispatchMarker() helper owns the never-throw / benign-conditional-failure semantics for current and future channels.
• Transient backoff curve restored: extracting the retry helpers to cli/src/retry.ts had silently halved every delay (2**(attempt-1) vs the original 2**attempt), doubling retry pressure on a degraded backend. Restored, and a new test pins the exact per-attempt jitter window so the curve can't drift undetected again.
• events --all --limit N caps total events, not page size — it previously returned the entire stream in N-event pages.
• CLI token refresh: concurrent callers share one in-flight Cognito refresh (fixes a last-writer-wins race that could clobber rotated refresh tokens), and only genuine auth rejections (NotAuthorizedException/UserNotFoundException) map to "Session expired — run bgagent login"; transient network blips now say "Token refresh failed — retry."
• waitForTask bounded and script-friendly: tolerates up to 5 consecutive transient failures with jittered backoff, enforces a 24h wall-clock ceiling checked at loop top, and exits with code 2 on timeout/exhaustion (CliError now carries exitCode) so wrappers can distinguish "CLI gave up waiting" from a genuinely FAILED task (exit 1).
• Corrupt ~/.bgagent/credentials.json now throws a friendly "run bgagent login" CliError instead of a raw SyntaxError.
• validateMaxBudgetUsd rejects NaN/Infinity (was accepted as a valid budget); full test block added for a previously untested validator.
• detect_default_branch falls back to main on OSError/SubprocessError (e.g. missing gh), as its docstring always promised; push_resolve push failures now surface as a PR comment instead of silently returning the stale PR URL as success.

Observability

• CloudWatch alarm on the fan-out DLQ depth — a silent notification outage (every Slack/GitHub/Linear delivery failing) previously accumulated unseen for the queue's 14-day retention.
• Async-invoke DLQ for the screenshot webhook processor — backstop for init-time crashes that Lambda's built-in retries would otherwise drop.
• bgagent watch renders structured approval-milestone metadata (severity, request_id, timeout, matching rules) in text mode instead of a bare ★ approval_requested.
• Hot-path efficiency: verbose-log redaction (deep copy + stringify of every request/response body) is now gated behind isVerbose().

Contracts, tests, hygiene

• constants-parity.test.ts pins the CLI's mirrored limits to contracts/constants.json, turning silent drift into a CI failure.
• New hermetic tests for the agent's highest-side-effect functions (repo.py clone/branch/credential-helper argv, post_hooks.py push/PR flows) — these were previously only ever mocked out; shared FakeRunCmd/make_task_config fixtures in conftest.py.
• New CLI test suites: wait.test.ts (timer-mocked poll/retry/ceiling/exit codes), debug.test.ts (redaction), plus extended auth/events/watch coverage.
• Removed two unused @aws-sdk/*-agentcore runtime deps from the CLI (yarn.lock pruned); added files/engines to cli and docs manifests.
• Docs: ORCHESTRATOR/ARCHITECTURE/AGENTS.md/agent README migrated off the retired task_type vocabulary, ADR-014 marked accepted, curly-quote and mise run // command-form fixes, .gitignore .DS_Store case fix; Starlight mirrors regenerated.

Verification

Full monorepo build (mise run build) green: agent 1051 tests (78.8% coverage), cdk 2029 tests (111 suites), cli 344 tests (28 suites), types-sync drift check OK, cdk synth + cdk-nag clean. The three code-scanning alerts should auto-close on the post-merge default-branch scan.

[isadeks]

Thanks for the thorough hardening pass — the empty-HMAC-secret consolidation, the fail-closed hooks, and the new resilience around waitForTask/token-refresh are all solid, well-documented work. I ran a high-effort multi-angle review plus verification. Below: blockers first, then nits, then what's done well.

---

🔴 Blockers

1. events --all --limit N returns stale pagination → silent data loss

cli/src/commands/events.ts:96

drainAllEvents slices the events array to limit but returns the raw last-page pagination (set at line 94 before the slice). So --output json emits has_more: true and a next_token that points past the events that were just dropped.

events.push(...result.data);
pagination = result.pagination;          // full last page's cursor
if (limit !== undefined && events.length >= limit) {
  return { events: events.slice(0, limit), pagination };  // events sliced, pagination is NOT
}

Trigger: bgagent events <id> --all --limit 75 --output json on a task with ≥101 events → returns events 1–75 plus next_token pointing after event 100. A script that consumes the 75 and follows the token resumes at event 101, silently skipping 76–100.

The regression test --all --limit N caps the TOTAL asserts only parsed.data and never inspects parsed.pagination, so the bug ships green. Fix: when slicing, clear/recompute the returned cursor (e.g. pagination: { has_more: false, next_token: null } once the client-side cap is hit), and extend the test to assert on pagination.

2. status.test.ts leaks process.exitCode = 1 → CLI test command exits non-zero despite green assertions

cli/test/commands/status.test.ts:47,158

The last test (--wait with --output json …) asserts expect(process.exitCode).toBe(1) but the suite's beforeEach/afterEach never reset it — afterEach only does consoleSpy.mockRestore(). Jest reports all 344 CLI tests passing, but the process exits 1, so the CLI test step can fail CI even with green assertions. (Confirmed by a second reviewer's run: assertions pass, command exits 1.)

watch.test.ts:244 already does this correctly — process.exitCode = undefined; in beforeEach. Mirror that here (add process.exitCode = undefined to beforeEach, or reset in afterEach).

3. _note_unpushed_commits silently swallows a failed reviewer notification

agent/src/post_hooks.py:235

The push-failure path runs run_cmd(['gh','pr','comment', …], check=False). With check=False, run_cmd logs but does not raise on a non-zero exit, and the return code is never inspected — so the surrounding except Exception is dead code for the gh-failure case. A failed gh pr comment (missing scope, rate limit, not-a-PR) reports success while posting nothing.

This compounds the situation: the caller is already returning a stale PR URL as success because the push failed, and the one mechanism meant to warn the reviewer is itself a silent no-op. Inspect the return code and emit a distinct WARN when the comment doesn't post (mirror ensure_pushed's return push_result.returncode == 0 pattern).

4. fetch_github_issue crashes on comments by deleted accounts (local/dry-run paths)

agent/src/context.py:62

author=sanitize_external_content(c["user"]["login"]),   # c["user"] unguarded
body=sanitize_external_content(c["body"] or ""),         # body IS guarded

GitHub returns "user": null for comments authored by since-deleted accounts → c["user"] is None → None["login"] raises TypeError: 'NoneType' object is not subscriptable, aborting issue hydration. The body is defended with or "" but the author is not. The sanitize wrapper this PR added sits on this exact line and didn't pick up the guard.

Reachable on local batch (pipeline.py:731) and DRY_RUN=1 (pipeline.py:1194) only — the AgentCore production path uses pre-hydrated context and is safe. Guard with (c.get("user") or {}).get("login", "") or similar.

---

🟡 Nits / discuss

• events --limit means two different things (cli/src/commands/events.ts:35). Help text says "Max total number of events to return", but on the non---all path opts.limit is forwarded as the server's per-page limit (which the server caps at 100), while --all treats it as a true total. So bgagent events <id> --limit 500 (no --all) silently returns ≤100. Align the help text with the behavior, or make both paths enforce a total.

• events --all MAX_PAGES=100 cap is silent in text mode (events.ts:104). When the drain stops at the page cap with has_more still true, the --all text branch never prints the (More events available) hint that the non---all path does — a capped drain looks identical to a complete one. Print a truncation notice when the cap trips.

• waitForTask now has a non-overridable 24h ceiling (cli/src/wait.ts:70). Pre-PR this was an unbounded while(true) poll; it now throws CliError (exit 2) at 24h with no --timeout flag or env var to restore the old behavior. Likely intentional and recoverable (re-run resumes), but it's a default-behavior flip — worth a CHANGELOG line and ideally a --timeout/--max-wait flag.

• isTransientError is brittle (cli/src/retry.ts:56). Network transients are matched only via err instanceof TypeError && /fetch failed|network/i.test(err.message), with no err.cause.code check. The common ECONNRESET/ENOTFOUND cases happen to wrap as 'fetch failed' (so they match), and the 5xx-via-ApiError path is robust — but an undici 'terminated' (mid-stream socket close) or any non-TypeError transient is misclassified as fatal, defeating the retry budget the PR is trying to strengthen. Consider checking err.cause?.code.

• isExpired NaN propagation (cli/src/auth.ts:116). A corrupt-but-valid-JSON credentials.json with a non-date token_expiry makes new Date(...).getTime() → NaN, and Date.now() >= NaN - BUFFER is always false → token treated as never-expiring → opaque 401 instead of a refresh. Not introduced by this PR and unreachable on the happy path (token_expiry is always written via .toISOString()), but a one-line Number.isFinite guard (treat NaN as expired) would harden it.

• 0o600 mode not re-applied on overwrite (cli/src/config.ts:96, cli/src/commands/admin.ts). fs.writeFileSync(path, data, { mode: 0o600 }) only honors mode when creating a file; on overwrite of an existing file the permission bits are left untouched. Low severity (the happy path creates 0o700 dirs + 0o600 files), but a re-login / re-invite into a pre-existing loose-perms file won't re-tighten it. A trailing fs.chmodSync(path, 0o600) makes the intent durable.

• isValidRepo regex allows owner/.. (cdk/src/handlers/shared/github-deployment-status.ts:114). ^[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$ permits . in segments, so owner/.. passes; the new "path traversal" test only exercises the double-slash case (trivially rejected by the single-slash rule), giving false confidence. Impact is bounded (HMAC-gated, URL normalizes, replaceAll('/','_') neutralizes the S3 key), but the regex doesn't enforce the owner/repo shape the comment claims.

• renderMilestoneSuffix drops unrecognized metadata (cli/src/commands/watch.ts:160). It returns as soon as any known field (severity/request_id/scope/timeout_s/matching_rule_ids) is present, so the JSON-dump fallback for unknown keys (guarded by parts.length === 0) never runs — contradicting the docstring's "so nothing is lost". Text-mode only; JSON mode serializes verbatim.

• Screenshot DLQ has no alarm (cdk/src/constructs/github-screenshot-integration.ts:154). The new WebhookProcessorDlq is a local const with no CloudWatch alarm and isn't exposed, while the sibling fan-out DLQ in this same PR got a public dlqDepthAlarm (threshold 1). The diff comment promises "operator inspection," but nothing notifies an operator — the exact silent-outage gap the fan-out alarm closes. Consider mirroring the alarm here.

• wait.ts off-by-one in the abort message (cli/src/wait.ts:89). The guard is consecutiveTransientFailures > MAX_TRANSIENT_FAILURES (=5) after a pre-increment, so it aborts on the 6th failure, but the error message says "after 5 consecutive transient failures." Cosmetic — use >= or say "after 6."

---

✅ Done well

• HMAC-secret consolidation is the right altitude. Collapsing eight hand-copied empty-secret checks into one isUsableHmacSecret() type guard, applied at both fetch and verify points, turns "a future webhook source forgets the invariant" into a structural impossibility. All 8 call sites route correctly and Slack's cache-delete path is preserved.
• Linear idempotency marker correctly fixes a real pre-PR double-post and brings Linear in line with Slack's collective-terminal dedup and GitHub's edit-in-place. The saveDispatchMarker never-throw / benign-CCF classification is exactly the right contract (a marker write must never turn a successful post into a retry).
• Retry-curve regression caught and pinned. Noticing the extraction had silently halved the backoff (2**(attempt-1) vs 2**attempt) and then pinning the exact per-attempt jitter window with a test so it can't drift again is excellent defensive work.
• Verbose-redaction gating behind isVerbose() correctly short-circuits the deep-copy + stringify on the watch poll hot path.
• Fail-closed hooks: non-dict tool_input now denies explicitly instead of relying on an AttributeError swallowed by the engine's broad except — the right instinct.
• CodeQL alerts handled at the source (os.write sink after redaction; password written to a 0600 file instead of stdout) rather than suppressed.
• Test investment: hermetic tests for the highest-side-effect agent functions (repo.py/post_hooks.py) that were previously only ever mocked, the constants-parity drift guard, and the new wait.test.ts/debug.test.ts suites are real coverage wins.

---

Verification summary

• Linear / HMAC / webhook / fanout targeted tests pass; no PR-diff blocker in the Linear changes.
• Agent: 1051 passed. CDK targeted changed tests: 303 passed (--coverage=false; full CDK needs yarn install --frozen-lockfile first — node_modules was stale/missing @aws-sdk/s3-presigned-post). CLI: 344 assertions pass but the command exits 1 due to blocker #2.

[krokoko]

Thanks @isadeks for the thorough multi-angle review — every one of the four blockers was real, and the verification detail (especially confirming the exit-1-with-green-assertions behavior empirically) made them fast to act on. All four blockers and all nits are now fixed and pushed.

Fixed in response to your review (6681645, 50bcc6c)

Blockers

1. events --all --limit N stale pagination — drainAllEvents now returns { has_more: false, next_token: null } when the client-side cap trips, so a JSON consumer can no longer follow a cursor that skips the sliced-off events. The regression test now asserts on parsed.pagination, and a new test pins the MAX_PAGES truncation notice (which now prints to stderr in text mode instead of silently truncating).
2. status.test.ts exitCode leak — process.exitCode = undefined is reset in both beforeEach and afterEach (the afterEach matters: it's what saves the last test's value from leaking into the Jest worker's exit). Same fix applied to watch.test.ts, which had the identical leak via its SIGINT test (exited 130 with green assertions). Both suites verified to exit 0.
3. _note_unpushed_commits silent no-op — the returncode from gh pr comment is now inspected explicitly (since check=False never raises), and a non-zero exit logs a WARN naming the consequence ("the reviewer has NOT been notified"). Test added with a scripted gh failure.
4. Ghost-account crash in fetch_github_issue — (c.get("user") or {}).get("login", "(deleted user)") guards the deleted-account "user": null case. Test added with a null-user comment.

Nits — all taken

• --limit help text is now path-specific (total with --all, page size without).
• status --wait gained a --max-wait <seconds> flag overriding the 24h ceiling (with validation + tests); the wait-abort message now reports the actual failure count instead of the off-by-one "after 5".
• isTransientError now checks err.cause.code against a whitelist of socket-level transients (ECONNRESET, UND_ERR_SOCKET, etc.), covering undici mid-stream terminations; new retry.test.ts covers the classifier and abortableSleep's abort contract.
• isExpired treats an unparseable token_expiry (NaN) as expired — refresh instead of an opaque 401.
• fs.chmodSync(0o600) after the credential/invite writes makes the mode durable on overwrite (hoisted into a shared SECRET_FILE_MODE constant).
• isValidRepo regex now rejects pure-dot segments (owner/.., ./repo) via lookaheads while keeping dotted real names (vercel/next.js) — tests cover both directions.
• The screenshot processor DLQ got its own processorDlqDepthAlarm (threshold 1, same shape as the fan-out alarm), exposed on the construct and pinned by a construct test.
• renderMilestoneSuffix docstring corrected to match the actual behavior (JSON fallback only when no salient fields are present).

Already fixed before your review landed (d53675b)

A self-review pass on this branch had flagged three items that overlap with your themes:

• Transient Linear post failures are no longer dropped: postIssueComment/addIssueReaction return a classified LinearPostResult ({ ok } | { ok: false, retryable }), and dispatchToLinear throws on retryable failures (network/timeout/5xx/429) so the record lands in batchItemFailures for a Lambda retry — idempotent because the post-once marker is only persisted after a successful post. Terminal failures (auth, GraphQL errors) stay log-only.
• Construct-test gaps: the fan-out dlqDepthAlarm metric binding/threshold is pinned in fanout-consumer.test.ts, and a new github-screenshot-integration.test.ts asserts the processor DLQ exists and is wired as the Lambda's DeadLetterConfig.
• GitHubIssue/IssueComment sanitization is structural: sanitize_external_content moved into Pydantic field_validators, so every construction path (including model_validate from a cache) yields a sanitized instance — the "consumers must not re-sanitize" docstring is now enforced by the type rather than one caller's discipline.

Also in this batch: the security:deps CI failure is resolved (esbuild 0.27.7 → 0.28.1 via a root resolution + cdk devDependency bump; OSV scan clean).

Full monorepo build green: agent 1058 / cdk 2039 / cli 355 tests, synth + cdk-nag clean, types-sync OK.

[krokoko]

Also deployed and tested live the changes