review: drip-182 batch 3 — aaif-goose/goose#8916+8904

- aaif-goose/goose#8916 fix(bedrock): cache trailing message for stable prefix across agent turns (merge-as-is)
- aaif-goose/goose#8904 fix(oidc-proxy): validate exp independently of MAX_TOKEN_AGE_SECONDS (merge-as-is — security fix with test inversion in same commit)

Author: Bojun-Vvibe

reviews/2026-W18/drip-182/block-goose-pr-8904.md

@@ -0,0 +1,67 @@
+---
+pr: block/goose#8904
+sha: 2eb4a5d9966f72ea23c67a24f780110c4c5a01f4
+verdict: merge-as-is
+reviewed_at: 2026-04-29T18:31:00Z
+---
+
+# fix(oidc-proxy): validate exp independently of MAX_TOKEN_AGE_SECONDS (#8832)
+
+## Context
+
+In `oidc-proxy/src/index.js` `verifyOidcToken`, the previous code
+had:
+
+```js
+if (env.MAX_TOKEN_AGE_SECONDS) {
+  const age = ...;
+  if (age > parseInt(env.MAX_TOKEN_AGE_SECONDS, 10)) {
+    return { valid: false, reason: "Token too old" };
+  }
+} else if (!payload.exp || payload.exp < Date.now() / 1000) {
+  return { valid: false, reason: "Token expired" };
+}
+```
+
+The `else if` is the bug: when `MAX_TOKEN_AGE_SECONDS` is set, the
+`exp` check is *unreachable*. A signed token with `exp` 5 minutes
+in the past but `iat` within the configured window is accepted as
+valid. The proxy still validates signature and issuer earlier in
+the flow, so this isn't an "unsigned token wins" bug, but it is a
+clean spec violation: RFC 7519 says `exp` is a hard upper bound on
+acceptable token lifetime.
+
+## What's good
+
+- Fix is the minimal one-liner: replace `else if` with `if` so both
+  checks run independently. Author resisted the temptation to
+  refactor the surrounding validation pipeline, which would have
+  blurred the diff.
+- The test update at `oidc-proxy/test/index.test.js:232` is the
+  most valuable part of the PR. The previous test was literally
+  asserting the buggy behavior:
+  `it("accepts recently-expired token within MAX_TOKEN_AGE_SECONDS", ...) → expect(response.status).toBe(200)`.
+  The new test inverts both the name and the assertion:
+  `it("rejects expired token even when within MAX_TOKEN_AGE_SECONDS", ...) → expect(response.status).toBe(401); expect((await response.json()).error).toBe("Token expired")`.
+  That's a textbook example of "the test was documenting the bug" —
+  having the new test land in the same commit forecloses regression.
+- Author calls out that the test file change was *intentional* in
+  the PR body ("Updated the existing test that inadvertently
+  documented the bypass behavior"). That kind of explicit reasoning
+  in a security-adjacent PR is the right operator hygiene — saves
+  the next reviewer from wondering why a test went from passing to
+  inverted.
+- DCO sign-off mentioned in checklist; matches contributor
+  conventions for `block/goose`.
+
+## Concerns / nits
+
+- None substantive. A nit: the `reason: "Token expired"` string is
+  reused from the old branch. Consumers parsing reasons will not
+  see a behavior change, which is good.
+
+## Verdict
+
+`merge-as-is` — security fix, smallest possible diff, test is
+inverted in the same commit so the regression can't quietly come
+back. This should land on the next patch release.

reviews/2026-W18/drip-182/block-goose-pr-8916.md

@@ -0,0 +1,66 @@
+---
+pr: block/goose#8916
+sha: 00c2141debc4eff86146ed4450ba2249a20ceec2
+verdict: merge-as-is
+reviewed_at: 2026-04-29T18:31:00Z
+---
+
+# fix(bedrock): cache trailing message for stable prefix across agent turns
+
+## Context
+
+In `crates/goose/src/providers/bedrock.rs` (`BedrockProvider::converse`,
+around line 232), the previous code placed prompt-cache breakpoints on
+the first three visible messages
+(`const MESSAGE_CACHE_BUDGET: usize = 3; let cache_count = … visible_messages.len().min(MESSAGE_CACHE_BUDGET)`)
+and then iterated `enumerate()` setting cache=true for `idx < cache_count`.
+This PR replaces that with a single trailing-message cache point.
+
+The author's reasoning matches Anthropic's prompt caching contract:
+cache reads walk *backward* from the breakpoint, hashing the prefix.
+A cache point pinned to position 0..3 means everything appended after
+position 3 has to be reprocessed every turn — linear growth in turn
+count. A trailing breakpoint means the next turn's lookback (≤20
+blocks) finds the previous turn's write, and only the new content
+between turns gets fresh processing.
+
+## What's good
+
+- The diff is exactly the change described in the comment — no
+  drive-by refactors. The new `last_idx = visible_messages.len().checked_sub(1)`
+  + `cache_last && Some(idx) == last_idx` pattern is the cleanest
+  Rust expression of "set the flag for exactly the last element,
+  or none if the list is empty."
+- The misleading old comment ("caching recent messages would shift
+  positions each turn, causing misses") is replaced with an accurate
+  description of the lookup model and a documentation link. Future
+  maintainers won't re-introduce the original mistake based on that
+  comment.
+- The author correctly identified that the existing test surface
+  (`providers::formats::bedrock` per-message helpers and
+  `providers::bedrock::test_caching_*` enable-flag tests) doesn't
+  actually exercise the *placement* of the cache point — it exercises
+  whether `to_bedrock_message_with_caching` honors the boolean.
+  Adding a placement test would be valuable but is not strictly
+  required for correctness; the change itself is mechanically obvious.
+- The system-prompt cache point is left untouched, which is correct
+  — system prompts are stable across turns, so a head-anchored
+  breakpoint is the right policy there.
+
+## Concerns / nits
+
+- For agent loops that add >20 blocks per turn (large multi-step
+  tool batches), the trailing-breakpoint strategy degrades to
+  full-prefix reprocessing because the lookback window is exceeded.
+  This is documented behavior, not a bug, but worth a comment in
+  the code or a follow-up that emits a debug log when this
+  threshold is approached.
+- `enable_caching && last_idx.is_some()` could be folded into a
+  `let cache_last = enable_caching && !visible_messages.is_empty();`
+  for one less `Option` ceremony. Style nit only.
+
+## Verdict
+
+`merge-as-is` — the analysis is correct, the diff is minimal, the
+old comment was wrong and is now right. The 20-block-window
+caveat is a follow-up consideration.