[maxwellpeterson]

Why

We're moving towards a world where observer verification runs independently for every chat thread and gadget in a workspace, so users aren't locked out of an entire workspace just because they don't have access to data observed in a single chat thread or gadget. This PR lays the groundwork for that model.

Summary of Changes

This PR adds a new observation tracking API for gatekeepers to use, which moves most observation bookkeeping out of the gatekeeper and into the overseer. Instead of gatekeepers tracking observed resources internally, they can now report a unique list of subresource IDs that were read when recording an observation. These resource IDs are are stored by the overseer as part of the observation's entry in the workspace activity log and verified in bulk by calling GatekeeperUserVerifier.verifyObserverAccess(subresources) for each user's connected account.

To facilitate migration from the old observation tracking API, gatekeepers that opt in to the new API must also implement a bulk import method for reading previously observed subresource IDs out of the gatekeeper facet's state and into the overseer. This bulk import method is only called when a gatekeeper upgrades to the new API after being added to a workspace.

Limitations / Security Holes

Observations made indirectly using capabilities passed between chat threads or gadgets are not correctly attributed to the caller. Observations made using external capabilities are always attributed to the context (chat thread or gadget) that the capability originated from, regardless of the context in which the capability is called. This gap could be closed in the future by wrapping each gadget and executeCode run in a membrane that tracks cross-context capability transfer and method calls, if such a membrane API existed.

Deleting a gatekeeper connection causes observations recorded from that connection to start being ignored, and unblocks users who can't access the observed data from joining the workspace. This matches the current behavior on main. However, because observation history is now owned by the overseer instead of the gatekeeper facet, it would be much easier to continue verifying these observations after the connection is deleted if we wanted to. I stuck with the current behavior but wanted to flag that this as something we could change if we wanted to.

Observations made by chat threads do not transfer to gadgets when the chat thread calls the gadget or edits its code. Of course, chat threads may leak data to gadgets that is not caught by observer verification, but at the same time a full observation history transfer feels too conservative here.

Conversely, observations made by a gadget do not transfer to chat threads that call that gadget. As implemented, the only users who can see chat threads can also see (and pass observer verification for) all gadgets in the workspace. Transferring observation history in this case adds no sharing security benefit, since access to chat threads implies access to all gadgets. In a future world where users can join any shared workspace and only see chat threads and gadgets they have access to, gadget to chat thread observation inheritance will need to be implemented. For now it's left out of scope.

No gatekeepers currently implement this API. Every gatekeeper we own will need to be migrated, which shouldn't be too hard.

[github-actions]

Preview: pr881-mpeterson-gra-5c24a6fa

https://pr881-mpeterson-gra-5c24a6fa-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

[ask-bonk]

Review: 1 finding.

Posted one P2 inline comment, verified with a focused Node reproduction.

github run

[ask-bonk]

Review: 2 findings.

Posted two inline comments—one P1 and one P2—both verified with focused Node reproductions.

github run

[ask-bonk]

Review: 1 finding.

Posted one new P1 inline comment, verified with a focused Node reproduction. Existing findings were not reposted.

github run

[ask-bonk]

Review: 3 findings.

The three existing open inline findings still apply, confirmed with focused reproductions against the current ledger implementation. No additional actionable issues found.

github run

[ask-bonk]

LGTM!

github run

[ask-bonk]

Review: 1 finding.

Posted one P1 inline comment, verified with a focused reproduction using the actual admission and ledger code.

github run

[ask-bonk]

LGTM!

github run

[clanky-the-clanker]

💬 Approved with comments — The ledger design holds up and no security problems were found. One WARNING: every legacy connection now makes an uncached describe() call on each use, and if that call fails the connection is refused. Two INFO notes: a regression test was deleted without a replacement, and admission cost has no upper bound.

Findings: 1 warning · 2 info

_{The decision is advisory: this comment never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #881.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (1)

### [Code Quality] Legacy connections are never cached in initialize(), so every session open, slash command and hook makes a fresh describe() RPC. A transient describe() failure now refuses a connection that isn't even tracked. The forget() comment about class replacement also doesn't match its only caller.
- **File:** packages/workshop-backend/src/observer-ledger.ts:251

## INFO (2)

### [Code Quality] The exports-binding test that covered the {rpc, exports} env shape, skipping of deleted gadgets and getEnvForLoader was deleted rather than updated for executionId.
- **File:** packages/workshop-backend/__tests__/exports-binding.test.ts

### [Performance] Admission walks and verifies every distinct subresource ever observed, with no cap, so latency and RPC load on the vendor grow over the workspace's lifetime.
- **File:** packages/workshop-backend/src/observer-ledger.ts:488

---

📋 Code Quality: ⚠️ 1 warning · 1 info

The ledger design is mostly sound. Legacy connections now re-describe on every use, and a regression test was deleted without a replacement.

Recommendation: 📝 Minor Comments

• [WARNING] Legacy connections are never cached in initialize(), so every session open, slash command and hook makes a fresh describe() RPC. A transient describe() failure now refuses a connection that isn't even tracked. The forget() comment about class replacement also doesn't match its only caller. (packages/workshop-backend/src/observer-ledger.ts:251)
• [INFO] The exports-binding test that covered the {rpc, exports} env shape, skipping of deleted gadgets and getEnvForLoader was deleted rather than updated for executionId. (packages/workshop-backend/__tests__/exports-binding.test.ts)

View full review

Most of the ledger holds up: the commit/revalidate ordering, the scope-recheck loop in admission, and the sparse requirement index.

Every legacy connection now makes an uncached describe() call on each use, and a failed call blocks the connection (packages/workshop-backend/src/observer-ledger.ts:200-254)

• initialize() caches only two outcomes: tracked connections and vendorless ones. For a legacy connection, #initialize returns false at line 251, so the entry is removed from #initialized.
• getReadyGatekeeperFacet runs on every session open and every slash command. startHook and #ensureObserver also go through initialize(). Each of these now makes a fresh describe() RPC to the facet.
• According to the description, no gatekeeper implements the new API yet. That means every existing connection takes this path and pays an extra round trip on every use.
• It also adds a new way to fail. A transient describe() error throws "could not confirm its observer tracking protocol" (lines 230-236). That makes a connection that isn't even tracked unusable.
• Possible fixes:
  • Cache the negative result for a short TTL.
  • Re-describe only when the facet or its class changes.
  • Fall back to the legacy path when describe() fails for a connection that was never tracked.
• The forget() doc comment (line 219) says it runs when "its class was replaced". Its only caller is removeGatekeeper, and nothing resets the cache on a class change, so the comment does not match the code.

The {rpc, exports} env-shape regression test was deleted without a replacement (packages/workshop-backend/__tests__/exports-binding.test.ts)

• The deleted test covered three behaviours:
  • getEnvForAgent pairs each gadget's rpc stub with its exports.
  • Deleted gadgets are skipped.
  • getEnvForLoader provides direct stubs and no exports.
• The new executionId field is covered by git-binding.test.ts, but nothing else now covers those three behaviours.
• Updating the test for executionId would keep that coverage, rather than deleting it.

---

⚡ Performance: 💬 1 info

Admission cost grows with the observation history, which has no upper bound.

Recommendation: 💬 Optional Nits

• [INFO] Admission walks and verifies every distinct subresource ever observed, with no cap, so latency and RPC load on the vendor grow over the workspace's lifetime. (packages/workshop-backend/src/observer-ledger.ts:488)

View full review

Admission cost grows with the number of distinct subresources ever observed through a tracked connection (packages/workshop-backend/src/observer-ledger.ts:488-541)

• requiredSubresources() walks every distinct requirement key for the role's contexts.
• checkAccess() then sends all of those keys to verifyObserverAccess, in sequential batches of 10,000.
• The accumulated history has no count cap, so each admission costs O(distinct IDs ever read). That cost is paid again on every re-run of the scope pass.
• In a busy workspace, admission latency and the RPC load on the vendor will rise over time.
• This is not a regression today, because no gatekeeper uses the new API yet.
• Either of these would bound the cost:
  • a cap on distinct IDs per connection;
  • a per-account "verified up to action N" watermark. This one would need to be invalidated whenever the account choice changes.

---

✅ All other checks passed

🔒 Security: ✅ LGTM

• The observer ledger checks fail closed. They cover the audience, bulk verifier results, account removal and accounts from the wrong vendor.
• The authorization commit point is synchronous. It re-checks the live execution and the hook before the audit record is written.
• Subresource IDs are validated for length, count and well-formed Unicode.
• Subresource IDs are removed from action-log entries before they are sent to clients.
• The description already documents its own limitations: capabilities passed across contexts, and the "use" scope. Nothing in the code makes them worse than described.

---

📖 AGENTS.md: ✅ LGTM

No issues found in the write-gatekeeper skill updates.

---

📝 Documentation: ✅ LGTM

No issues found in docs/observers.md or the plans files.

---

_{Reviewed commit: ed41090f}

[deregtd]

Completed the full 17-file review at 89a5d575cff0eb1f483c08263dbe08db6f9d06c0 (base 835de9e8776429fc16781edbe66367d879a64b87). No additional blocking finding. The five earlier inline findings are verified resolved; the exact-head observer ledger and verification regression suites pass all 82 tests in real SQLite Durable Objects, including oversized persistence and rollback cases.

Follow-up to the inconclusive Worker Logs eval: I replayed trial 9's saved final server through the real Workshop draft GadgetClient.connectToGadget(chatId) path, with frozen dependencies. The first reset() RPC fails with an internal error on both this head and its exact base, before any ingestion batch. The same saved server's reset/ingest/summary succeeds directly in a SQLite Gadget DO. This localizes the failing call and demonstrates that this failure already exists before this PR; it does not establish an observation-tracking regression. The underlying Workshop/runtime cause remains unlocalized.

Current required build/tests, lint and CLA checks pass. Validation used local Workers with external requests intercepted; no live provider/model calls, forced-eviction acceptance run or remote CI rerun.

[ask-bonk]

LGTM!

github run

[ask-bonk]

LGTM!

github run

[ask-bonk]

LGTM!

github run

[ask-bonk]

LGTM!

github run

[github-actions]

Eval results

Verdict: ⚪ Unchanged. No task moved beyond what these runs can tell apart from noise.

Task	Score	Δ score	Fisher test	Cache hits	Avg min	Avg steps
change-calendar (gpt-6-luna)	100% → 90%	−10 pp	p = 1.00	95% 0 pp	4.1 → 3.4	27.6 → 26.1
chess (gpt-6-luna)	70% → 90%	+20 pp	p = 0.58	97% → 98% +1 pp	7.3 → 10.1	45.3 → 65.1
compaction (claude-sonnet-5-5)	100%	0 pp	p = 1.00	89% → 90% +1 pp	2.0 → 2.4	43.0
compaction (gpt-6-luna)	100%	0 pp	p = 1.00	93% 0 pp	1.7 → 1.8	43.0
incident-desk (gpt-6-luna)	80% → 100%	+20 pp	p = 0.47	96% 0 pp	4.2 → 4.1	32.2 → 30.2
ticket-triage-agent (gpt-6-luna)	100%	0 pp	p = 1.00	97% → 96% −1 pp	0.1	2.0 → 1.9
worker-logs (gpt-6-luna)	80% → 100%	+20 pp	p = 0.47	95% 0 pp	3.7 → 4.1	21.8 → 25.2

Failed checks

Task	Check	Failed
change-calendar (gpt-6-luna)	t3 existing-windows-survive-the-rule-change	0/10 → 1/10
change-calendar (gpt-6-luna)	t3 same-service-windows-must-be-a-day-apart	0/10 → 1/10
change-calendar (gpt-6-luna)	t3 billing-is-capped-at-four-hours-others-are-not	0/10 → 1/10
chess (gpt-6-luna)	t1 agrees-with-the-oracle-on-the-hard-positions	3/10 → 1/10
chess (gpt-6-luna)	t1 agrees-with-the-oracle-on-perft-positions	3/10 → 1/10
chess (gpt-6-luna)	t1 agrees-with-the-oracle-through-random-games	1/10
chess (gpt-6-luna)	t1 starts-from-the-standard-position	0/10 → 1/10
chess (gpt-6-luna)	t1 the-game-is-shared-across-connections	0/10 → 1/10
incident-desk (gpt-6-luna)	t1 opens-acknowledges-and-resolves-in-order	2/10 → 0/10
incident-desk (gpt-6-luna)	t1 simultaneous-acknowledges-yield-exactly-one-owner	2/10 → 0/10
incident-desk (gpt-6-luna)	t1 simultaneous-opens-of-one-id-admit-exactly-one	2/10 → 0/10
incident-desk (gpt-6-luna)	t1 board-lists-by-severity-then-age	2/10 → 0/10
worker-logs (gpt-6-luna)	t1 hourly-buckets-cover-every-hour-including-empty-ones	2/10 → 0/10
worker-logs (gpt-6-luna)	t1 ranges-are-half-open-and-filter-by-worker	2/10 → 0/10
worker-logs (gpt-6-luna)	t1 ingests-resets-and-summarises-per-worker	1/10 → 0/10

Run · trajectories and raw results

[ask-bonk]

🔬 Eval runs review

Performance

Against main, change-calendar passed 10/10 → 9/10, chess 7/10 → 9/10, and incident-desk and worker-logs each 8/10 → 10/10; ticket-triage-agent and both compaction cases stayed fully passing, with no pass-rate or prompt-cache change beyond noise. Costs were mixed: change-calendar averaged $0.0241 → $0.0207 per run, while chess averaged $0.0430 → $0.0638. Chess accounted for the largest step increase, 45.3 → 65.1, and took 7.3 → 10.1 minutes: nine rather than seven runs reached the PGN/draw extensions, with repeated edits and lengthy implementation probes in successful trajectories.

⚪ VERDICT: NO REGRESSION FROM THIS PR

The diff adds observer attribution and verification, but the candidate’s two failed runs contain identifiable generated-code mistakes, and neither the pass-rate changes nor the mixed cost and timing changes establish a PR-caused effect.

Triage

Failure modes

• Syntax-breaking UI edit · change-calendar 1/10 · model error · this PR: no — Trial 3, turn 3’s editFile(client.js) added TOO_CLOSE without a comma before UNKNOWN_SERVICE, preventing Worker startup and blocking all three checks; the agent reread only server.js before declaring completion.
• Mixed chess square formats · chess 1/10 · model error · this PR: no — Trial 4, turn 1’s writeFile(server.js) generated numeric pseudoMoves() coordinates but made applyMove() interpret them as algebraic strings; the status calculation reached undefined.toLowerCase(), breaking all five checks without an RPC smoke check before the final reply.

Tool errors

• editFile: Multiple matches were found · change-calendar 1 → 2, chess 3 → 7, incident-desk 6 → 1 · model error — Agents supplied fragments shared by several methods; candidate chess trial 6 even retried the identical return this.exclusive(...) edit before adding method-specific context.
• editFile: No matching text was found · change-calendar 0 → 2, chess 6 → 5, incident-desk 1 → 2, worker-logs 0 → 1 · model error — Replacement targets did not match the current text; candidate calendar trial 6 repeated an overescaped regex target twice, then succeeded after using the file’s actual escaping.
• editFile: Validation failed · change-calendar 2 → 2, chess 1 → 1, incident-desk 4 → 1, worker-logs 3 → 2 · model error — Calls omitted required fields, misspelled replacement, or put a tool_uses batch inside a single edit request; corrected flat arguments succeeded.
• grep: Invalid regular expression, unterminated group · chess 0 → 1, incident-desk 1 → 0 · model error — Candidate chess searched for subscribe(new and main incident-desk for for (const without escaping the literal parentheses; simpler patterns recovered.
• executeCode: Failed to start Worker · chess 1 → 0 · model error — Main trial 9, turn 2 submitted a module beginning with a stray .; the next call removed it and executed successfully.

What to do

• No eval-driven change to this PR is indicated. Optional follow-up in packages/workshop-backend/src/agent.ts: remind agents to run a minimal executeCode RPC smoke check after their final gadget edits, before claiming completion.

github run