feat: agent identity & attribution foundation

[nick-inkeep]

Summary

Agent identity & attribution foundation for Open Knowledge. Establishes a coherent (principal, agent_session) actor model at the CRDT origin layer and carries it through every downstream attribution surface — per-session undo, presence, timeline, shadow-repo commits, and main-git save-version. Unblocks V0-14 (per-agent undo) and closes the attribution gaps across 15 mutating POST handlers.

Also merges origin/main (PR #254 "Open in Agent Desktop" + the 2026-04-21-shadow-repo-single-mode spec) with an architectural reconciliation — the shadow-single-mode direction wins for the shadow-repo naming + layout; the identity foundation layers on top.

Implements spec at specs/2026-04-18-agent-identity-attribution-foundation/SPEC.md via 27 user stories (US-001 through US-030) + 3 docs commits + 2 review-fix commits + merge.

---

Key decisions

D2 — Per-session LocalTransactionOrigin objects (F1)

Each agent session creates a deep-frozen origin at session birth:

session.origin = Object.freeze({
  source: 'local',
  context: Object.freeze({
    origin: 'agent-write', paired: true,
    session_id: connectionId, agent_type, principal
  }),
});

Replaces the shared AGENT_WRITE_ORIGIN module-level constant. Identity-unique per session; matched by Set.has(origin) in Y.UndoManager.trackedOrigins. Flows through to observer short-circuits (via isPairedWriteOrigin(origin) === context.paired === true, structural not identity).

D3/D25 — Per-session Y.UndoManager

new Y.UndoManager([ytext, metaMap, flashMap], {
  trackedOrigins: new Set([session.origin]),
  captureTimeout: 500,
  captureTransaction: tr => tr.origin !== session.undoOrigin,
  ignoreRemoteMapChanges: true,
});

Scopes across Y.Text + Y.Map('metadata') + Y.Map('agent-flash') so one agent transaction touching all three is ONE undo step. ignoreRemoteMapChanges: true prevents partial-undo across concurrent sessions.

V0-14 — applyAgentUndo (XmlFragment-authoritative)

New paired origin AGENT_UNDO_ORIGIN. Handler wraps um.undo() + updateYFragment + applyFastDiff in one doc.transact(fn, session.undoOrigin). Never rebuilds XmlFragment from Y.Text (Bug-A/Bug-D anti-pattern). Fuzzer extended with agent-undo op; fidelity PBT bridge-observer-conversion.test.ts chain E covers composition.

Attribution sweep (D42 / FR-5)

15 mutating POST handlers thread extractAgentIdentity: handleAgentWrite, handleAgentWriteMd, handleAgentPatch, handleAgentUndo, handleSaveVersion, handleRollback, handleCreatePage, handleRename, handleRenamePath, handleDeletePath, handleUploadImage, plus sync handlers. Meta-test attribution-sweep-coverage.test.ts scans the route registry and enforces the contract.

The 4 sync/* handlers (handleSyncTrigger, handleSyncSetEnabled, handleSyncAbortMerge, handleSyncResolveConflict) are control-plane orchestrators — their commits land inside SyncEngine under classified writers (git-upstream, file-system) and are already correctly attributed. Exempt per EXEMPT_HANDLERS + corrigendum on SPEC.md §FR-5.

Per-writer shadow-repo fan-out (FR-7)

commitWip emits refs/wip/<branch>/<writer-id> — one commit per distinct writer in the L2 drain, all sharing the same tree SHA. Writer-ID taxonomy (D34):

• agent-<connectionId> — MCP session
• principal-<UUID> — browser tab
• file-system — file-watcher reconciled writes
• git-upstream — HEAD-move commit imports
• openknowledge-service — service fallback (park, rollback, etc.)

Legacy server, human-*, upstream classify as 'unknown' — allowlist-swept on first init post-upgrade.

Structured ok-actor: body (D13 / FR-8)

Every shadow-repo commit body carries:

ok-actor: {"v":1,"principal":"principal-...","agent_session":"conn-...","agent_type":"claude",
           "client_name":"claude-code","client_version":"1.5.2","label":null,
           "display_name":"Claude (a4f2)","color_seed":"claude-code","docs":["notes.md"]}

Subject-prefix action encoding: wip:, checkpoint:, reconcile:, park:, rollback:, rename:, import:.

Save-version Co-Authored-By (FR-9)

Author: Alice Smith <alice@inkeep.com>
checkpoint: feat: update section A
Co-Authored-By: Claude (a4f2) <agent-abc123@openknowledge.local>
Co-Authored-By: Cursor (9d2e) <agent-xyz789@openknowledge.local>

Rendered natively by GitHub/GitLab.

Activity log Y.Map ring-buffer (D49 / FR-11)

Y.Map('agent-effects') bounded at 50 entries per doc, oldest-by-timestamp eviction in the paired-write drain. Captures via YTextEvent.delta (Quill Delta ops — D22, not transaction.changed).

F2 session lifecycle (FR-14)

Server-side /collab/keepalive close handler reads connectionId from URL query, starts 30s grace timer, fires closeAllForAgent + clearFocus after grace. Reconnect during grace cancels the timer.

Merge reconciliation (2026-04-22)

Merge of origin/main brought in the 2026-04-21-shadow-repo-single-mode spec that took the opposite direction on naming + layout from this branch's D55/D56. User-directed resolution:

• Keep shadow terminology (revert D55). All internal files + symbols + log prefixes retain the shadow name.
• Shadow repo lives at <projectRoot>/.git/open-knowledge/ (single-mode; D56's <contentDir>/.open-knowledge/history/ reverts).
• Fail fast on missing .git/ via main's ensureProjectGit (revert D45 graceful-save).
• Adopt main's runDevShadowInit dev-plugin pipeline.
• principal.json stays at <contentDir>/.open-knowledge/principal.json (B1 — non-git state outside .git/, gitignored).
• D57 retained — Y.Map('activity') → Y.Map('agent-flash') rename is independent of the shadow-layout conflict.

Corrigendum breadcrumbs on SPEC.md §FR-5 document the merge-time adjudication per CLAUDE.md convention.

CORS allowlist (review-gate finding)

/review-local flagged the Access-Control-Allow-Origin: * header on /api/* as a CSRF vector for unauthenticated mutating endpoints. Replaced with a loopback Origin allowlist (localhost / 127.x.x.x / [::1] + "null" for packaged Electron). Non-loopback origins receive 403. Reflects the allowed Origin verbatim with Vary: Origin. isAllowedApiOrigin guards the gate.

---

Scope summary

Category	Count
User stories shipped (US-001 through US-030)	27
New precedents (AGENTS.md #24, #25)	2
POST handlers threading identity	15
New writer taxonomy classes	5
Origin types added	2 (AGENT_UNDO_ORIGIN, PARK_SNAPSHOT_ORIGIN)
Integration tests added	3 dedicated files (session-cleanup, persistence-fan-out, per-session-um-perf) + meta-test
Docs touched	AGENTS.md, PRECEDENTS.md, docs/content/internals/{service-topology,agent-write-path,lifecycle}.mdx, docs/content/guides/{timeline,github-sync,mcp-integration}.mdx, README files
Commits on branch (vs main)	84

---

Review setup

Standard bun run dev workflow. No new environment variables. First-run migration from legacy .git/openknowledge/ (no hyphen) to .git/open-knowledge/ (hyphenated) runs automatically in initShadowRepo.

The server now requires a project .git/ — ensureProjectGit fails fast on missing git. In a fresh clone, run git init in any standalone-mode project before starting the server.

To verify save-version's Co-Authored-By trailers after agent writes:

# In a dev session, trigger an agent write via MCP, then:
curl -X POST localhost:<port>/api/save-version \
  -H 'Content-Type: application/json' \
  -d '{"message":"test","principal":{"name":"Alice","email":"alice@example.com"}}'
git log -1 --pretty=%B  # should include Co-Authored-By trailers

---

Verification

Automated gate: bun run check — 15/15 tasks PASS (FULL TURBO warm replay). 1094 tests across 88 files, 0 fail.

Suite	Result
@inkeep/open-knowledge-core unit	722 pass / 54 files
@inkeep/open-knowledge-server unit	836+ pass / 55 files
@inkeep/open-knowledge-app unit	660 pass / 56 files
@inkeep/open-knowledge-app integration	204 pass, 2 skip / 37 files
@inkeep/open-knowledge-app fidelity	903 pass / 32 files
@inkeep/open-knowledge-app conversion	73 pass / 1 file
CLI unit	703 pass / 60 files
Desktop unit	78 pass / 9 files
E2E (test:e2e)	15/15 Playwright files PASS (via check:full:parallel)

Local review: /review-local dispatched 17 domain reviewers. 4 findings surfaced (1 Major CORS, 2 Minor, 1 Consider). All 4 resolved in commit a718c591 + merge adjustments. Final state clean.

QA coverage (tmp/ship/qa-progress.json)

Status	Count
Covered (automated tests)	18 / 25
Browser-only — deferred to post-PR manual verification	2 / 25
Needs verification (composition-covered; see below)	5 / 25

Needs human verification

• QA-007: Subprocess reconnect after grace → new session with fresh UM stack + new refs/wip/<branch>/agent-<newUUID>. Building blocks individually covered; no single end-to-end composition test.
• QA-022: 30-min soak for ghost-agent leak. CI runs 100-cycle NFR-5 soak (tier-1); 30-min version is ad-hoc/tier-2.
• QA-024: Two browser tabs share one principal-<UUID> ref. Dispatch logic unit-tested (resolveWriterFromOrigin); no integration test wiring two HocuspocusProvider clients with distinct tabSessionId.
• ux-flow QA-001: Presence-bar shows distinct avatars for two same-type agents — visual, needs browser.
• visual QA-019: Closed session disappears from presence after 30s grace — visual, needs browser.

---

Merge commit — what it changed

Commit 74ea33ce merges origin/main into this branch with architectural reconciliation. Summary of rename reversals and adjustments:

• history-*.ts files → shadow-*.ts across 8 file pairs (repo, lock, branch-gc, repo-layout in core + server)
• Symbol reversals: initHistoryRepo → initShadowRepo, HistoryHandle → ShadowHandle, historyGit → shadowGit, etc.
• Log prefixes [history] / [history-migration] → [shadow] / [shadow-migration]
• Shadow-repo .open-knowledge/history/ → .git/open-knowledge/ (main's single-mode)
• Save-version graceful [save-version] parent-git unavailable: warn → removed; fail fast via ensureProjectGit at boot
• installedAgentsProbe + JSON-body CORS error adopted from main

All other agent-identity work (F1 origins, per-session UM, attribution sweep, ok-actor body, save-version trailers, activity log, precedents #24/#25) retained.

---

Related issues

Implements the foundation described in specs/2026-04-18-agent-identity-attribution-foundation/SPEC.md. Adjacent specs on this branch path: 2026-04-21-shadow-repo-single-mode (via merge with main).

---

Future considerations

Deferred scope (from SPEC.md §15 Future Work)

• Identity attestation — cryptographic verification of claimed agent identity. Blocked on MCP protocol extension or in-repo trust substrate.
• Full headless-agent UX affordances — data model supports {null, agent_session} tuple; UI surfaces deferred until a concrete headless case ships.
• Cross-project identity aggregation — one connectionId spans N projects. Requires cloud-tier roll-up layer.
• v14 AttributionManager adoption — when Y.js v14 stable lands + TipTap/Hocuspocus upgrade, migrate y-lite capture to native DiffAttributionManager + IdMap.
• Character-level per-agent attribution (Option B) — infeasible on Y.js v13.6.30 without fork.

Open questions passed through to future work

• Q101 — handleApplyLinks existence + identity threading (audit during implementation; may be a no-op).
• Q102 — Agent-type registry extensibility (hardcoded → config-driven; monitor signal).
• Q103 — Nested agents-of-agents session model.
• Q104 — Cross-session UndoManager stack-item validity under interleaved writes (covered by FR-17 fuzzer; empirical monitoring).
• Q105 — Effect-diff Y.Map replication metadata scope (default: session_id + agent_type + color; revisit for richer inline rendering).

QA follow-ups

• Add integration test composition for QA-007 (reconnect-after-grace) and QA-024 (two-tab principal ref sharing) if they surface as regressions.
• Manual browser pass for QA-001 + QA-019 (visual presence-bar behavior) before/after any presence-layer refactor.

🤖 Generated with Claude Code

[changeset-bot]

⚠️ No Changeset found

Latest commit: 4e7e985

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[inkeep-internal-ci]

PR Review Summary

(5) Total Issues | Risk: Medium

🟠⚠️ Major (2) 🟠⚠️

Inline Comments:

• 🟠 Major: agent-sessions.ts:468 closeAllForAgent does not cancel in-flight pendingSessions
• 🟠 Major: api-extension.ts:1944 extractAgentIdentity result discarded — undo operations unattributed

🟡 Minor (3) 🟡

Inline Comments:

• 🟡 Minor: api-extension.ts:1991 No indication when undo stack was empty
• 🟡 Minor: agent-sessions.ts:196 scope='session' branch lacks test coverage
• 🟡 Minor: boot.ts:293 TOCTOU race between grace timer check and closeAllForAgent

💭 Consider (0) 💭

None.

🧹 While You're Here (0) 🧹

None.

🕐 Pending Recommendations (8)

Prior review findings that remain applicable:

• 🟡 activity-log.ts:98 One-shot observer may leak if no YTextEvent fires
• 🟡 persistence.ts:95 Defensive check for getPrincipal accessor
• 🟡 activity-log.test.ts:159-205 Error handling test incomplete — asserts metric is 0
• 🟡 agent-undo.test.ts Test uses hardcoded literal instead of exercising real session.undoOrigin
• 💭 contributor-tracker.ts:54 Module-level mutable state requires single-threaded execution guarantee
• 💭 agent-sessions.ts:425-430 UndoManager tracks agent-flash which is presence metadata
• 💭 shadow-repo.ts:349 Temporary index cleanup on error
• 🟢 boot.ts:395 Proper cleanup on destroy (positive note)

---

🚫 REQUEST CHANGES

Summary: This is a well-architected identity and attribution foundation with solid design decisions (per-session frozen origins, XmlFragment-authoritative undo, structured ok-actor: body). The two blocking issues are:

1. Session cleanup gap: closeAllForAgent only iterates this.sessions, not this.pendingSessions. A slow openDirectConnection during the 30s keepalive grace window can race against cleanup, leaving dangling sessions.

2. Attribution gap in undo handler: handleAgentUndo calls extractAgentIdentity but discards the result without calling recordContributor, so undo operations won't be properly attributed in the shadow repo.

Both are straightforward fixes. The Minor items (undo no-op feedback, scope='session' test coverage, grace timer race) are lower priority but worth addressing.

---

Discarded (3)

Location	Issue	Reason Discarded
api-extension.ts:1088	Self-asserted agent identity enables impersonation	Local-only server (loopback CORS), no auth model in scope — risk accepted for this iteration
agent-sessions.ts:325	Sessions Map unbounded memory	Low risk: sessions are cleaned up on keepalive close; would need sustained malicious reconnects to exploit
contributor-tracker.ts:54	pendingContributors memory leak	L2 drain fires on 2s debounce; would need complete persistence failure to accumulate

Reviewers (6)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
pr-review-standards	7	0	0	0	2	0	5
pr-review-security-iam	4	0	0	0	0	0	4
pr-review-tests	6	0	0	0	1	0	5
pr-review-sre	6	0	0	0	1	0	5
pr-review-errors	5	0	0	0	1	0	4
pr-review-consistency	0	0	0	0	0	0	0
Total	28	0	0	0	5	0	23

Note: High discard rate reflects that many findings duplicated prior review comments or were assessed as low-risk for this local-only iteration.

[inkeep-internal-ci]

🟠 MAJOR: Concurrent Map modification in closeAllForAgent

Issue: The method iterates over this.sessions with a for...of loop while calling await session.dc.disconnect() and this.sessions.delete(key) inside the loop. If disconnect() triggers any callback that causes another session operation, it could modify the sessions Map during iteration.

Why: While ES6 Maps tolerate deletion of the current key during iteration, other session operations triggered by the async disconnect could add or remove different keys, causing unpredictable iteration behavior.

Fix: Collect matching keys first, then iterate:

Suggested change

	for (const [key, session] of this.sessions) {
	async closeAllForAgent(agentId: string): Promise<void> {
	const suffix = `\0${agentId}`;
	const keysToClose = [...this.sessions.keys()].filter(k => k.endsWith(suffix));
	for (const key of keysToClose) {
	const session = this.sessions.get(key);
	if (!session) continue;

Refs:

• closeAll uses correct pattern

[inkeep-internal-ci]

🟠 MAJOR: Same concurrent modification issue in closeAllForDoc

Issue: This method also iterates directly over this.sessions while performing async operations and deleting entries.

Why: Same race condition as closeAllForAgent - cascading cleanup or event handlers could modify the sessions Map during iteration.

Fix: Apply the same pattern:

Suggested change

	/** Close all sessions for a given document (all agents). */
	async closeAllForDoc(docName: string): Promise<void> {
	const prefix = `${docName}\0`;
	const keysToClose = [...this.sessions.keys()].filter(k => k.startsWith(prefix));
	for (const key of keysToClose) {
	const session = this.sessions.get(key);
	if (!session) continue;

Refs:

• closeAll uses correct pattern

[inkeep-internal-ci]

🟡 Minor: Grace timer callback captures potentially reassignable variable

Issue: The setTimeout callback captures connectionId which could theoretically be reassigned between timer set and fire (unlikely but defensive coding should avoid).

Why: Using a const binding ensures the captured value cannot change, making the code more predictable and easier to reason about.

Fix: Capture in a const before setting the timer:

if (!connectionId) return;
const capturedId = connectionId; // const binding cannot be reassigned
const timer = setTimeout(async () => {
  keepaliveGraceTimers.delete(capturedId);
  // ... use capturedId instead of connectionId
}, KEEPALIVE_GRACE_MS);

Refs:

• JavaScript closure semantics

[inkeep-internal-ci]

🟡 Minor: Ref path construction does not validate writer.id format

Issue: The ref path is constructed as refs/wip/${branch}/${writer.id}. If a malformed writerId containing path separators (e.g., ../main/attacker) were to flow through, it could target arbitrary refs.

Why: While current code paths produce safe IDs, this is a defense-in-depth gap. The parseWriterId function validates IDs on read but not on write.

Fix: Add validation that writer.id matches the WRITER_ID_RE pattern:

import { WRITER_ID_RE } from '@inkeep/open-knowledge-core';

// Before constructing ref path:
if (!WRITER_ID_RE.test(writer.id)) {
  throw new Error(`Invalid writer.id format: ${writer.id}`);
}

Refs:

• WRITER_ID_RE in shadow-repo-layout.ts

[inkeep-internal-ci]

🟡 Minor: Silent fallback to SERVICE_WRITER may obscure attribution bugs

Issue: resolveWriterFromOrigin returns SERVICE_WRITER as a catch-all for unrecognized origins. Any new origin type added without updating this dispatch table will have its writes silently attributed to 'openknowledge-service'.

Why: This makes it easy to miss attribution bugs during development when adding new origin types.

Fix: Log a warning when falling back to SERVICE_WRITER for unexpected origin shapes:

// Before the final return SERVICE_WRITER:
console.warn(JSON.stringify({
  event: 'unknown-origin-fallback',
  origin: origin?.context?.origin ?? 'undefined',
  source: (origin as any)?.source ?? 'undefined'
}));
return SERVICE_WRITER;

Refs:

• attribution-sweep-coverage.test.ts

[inkeep-internal-ci]

🟠 MAJOR: closeAllForAgent does not cancel in-flight pendingSessions

Issue: This method iterates this.sessions but not this.pendingSessions (line 327). If the keepalive-grace timer fires while a getSession() call is in-flight (awaiting openDirectConnection), the pending session will complete and register itself after cleanup, creating a dangling session.

Why: The 30s keepalive grace window provides ample time for a slow openDirectConnection (network hiccup, server load) to race against cleanup. A dangling session leaks memory and may cause closeAllForAgent to appear successful while the session remains active, violating the MCP subprocess exit cleanup contract.

Fix: Also iterate and await/cancel pendingSessions entries for the agent:

// Cancel any pending sessions for this agent
for (const [key, pending] of this.pendingSessions) {
  if (key.endsWith(suffix)) {
    this.pendingSessions.delete(key);
    // Optionally await + close the session if it completes
  }
}

Refs:

• pendingSessions declaration:327
• F2 session lifecycle (FR-14) in SPEC

[inkeep-internal-ci]

🟠 MAJOR: extractAgentIdentity result discarded — undo operations unattributed

Issue: This line calls extractAgentIdentity(body) but discards the returned AgentIdentity. Unlike other handlers (e.g., handleAgentWrite at line 1088), there is no recordContributor() call to attribute the undo operation to the agent.

Why: Undo operations mutate Y.Doc state and flow through to L2 drain commits. Without recordContributor(), the undo will be attributed to the wrong writer (likely openknowledge-service fallback), breaking per-agent attribution in the shadow repo and causing the Timeline UI to show incorrect authorship for undo operations.

Fix: Thread identity through to recordContributor:

const identity = extractAgentIdentity(body);
// ... after successful applyAgentUndo ...
recordContributor(
  docName,
  `agent-${identity.connectionId}`,
  identity.displayName,
  identity.colorSeed,
  `undo: ${docName}`,
  { agentType: identity.agentType, clientName: identity.clientName, ... }
);

Refs:

• handleAgentWrite attribution pattern:1088-1095
• FR-5 attribution sweep requirement

[inkeep-internal-ci]

🟡 Minor: No indication when undo stack was empty

Issue: handleAgentUndo returns { ok: true } regardless of whether any undo operation actually occurred. If the undo stack was empty, the caller has no way to know their request was a no-op.

Why: MCP clients need feedback to decide whether to retry or report "nothing to undo" to the user. Silent success on no-op creates a confusing UX where the agent believes it undid something when nothing changed.

Fix: Return a discriminated response:

const result = await applyAgentUndo(session, scope);
return json({ 
  ok: true, 
  undone: result.undone, // boolean or count
  message: result.undone ? 'Undo applied' : 'Nothing to undo'
});

Refs:

• applyAgentUndo implementation:196

[inkeep-internal-ci]

🟡 Minor: scope='session' branch lacks test coverage

Issue: The applyAgentUndo function handles both scope='last' (undo one transaction) and scope='session' (clear entire session stack), but the test suite only exercises scope='last'. The while loop for scope='session' is untested.

Why: If the scope='session' loop has a bug (e.g., infinite loop if um.canUndo() returns stale state, or partial undo on error), it won't be caught until production. Given that scope='session' wipes all agent edits, bugs here could cause data loss.

Fix: Add test coverage for scope='session':

test('applyAgentUndo scope=session undoes entire stack', async () => {
  // Make 3 edits
  await agentWrite(port, docName, 'first');
  await agentWrite(port, docName, 'second');
  await agentWrite(port, docName, 'third');
  // Undo all
  await fetch(`/api/agent-undo`, {
    body: JSON.stringify({ docName, connectionId, scope: 'session' })
  });
  // Assert content is back to initial state
});

Refs:

• agent-undo.test.ts

[inkeep-internal-ci]

🟡 Minor: TOCTOU race between grace timer check and closeAllForAgent

Issue: If destroy() is called while a grace timer's callback is executing (between keepaliveGraceTimers.delete() at line 281 and closeAllForAgent() at line 284), the destroy() loop may miss the timer that already deleted itself from the Map, allowing closeAllForAgent to run against a partially-disposed sessionManager.

Why: While unlikely in practice (30s grace >> typical destroy timing), the race can cause intermittent errors during shutdown that are difficult to diagnose. The timer callback's async operations (closeAllForAgent, clearFocus) may throw or hang if the underlying resources are being torn down.

Fix: Add a "shutting down" guard:

let shuttingDown = false;
// In timer callback:
if (shuttingDown) return;
// In destroy():
shuttingDown = true;

Or use a mutex/AbortController pattern for coordinated shutdown.

Refs:

• destroy() timer cleanup:393-396

[inkeep-internal-ci]

PR Review Summary

(5) Total Issues | Risk: Medium

🟠⚠️ Major (2) 🟠⚠️

No new Major findings in this review cycle. See Pending Recommendations below for unresolved prior findings.

🟡 Minor (3) 🟡

No new Minor findings in this review cycle. See Pending Recommendations below for unresolved prior findings.

💭 Consider (0) 💭

None.

🧹 While You're Here (0) 🧹

None.

✅ Addressed Since Last Review

🟠 Concurrent Map modification in closeAllForAgent and closeAllForDoc — Both methods now use the snapshot pattern (const keys = [...this.sessions.keys()].filter(...)) before iterating, which prevents concurrent modification issues. This addresses the prior finding at agent-sessions.ts:485.

🕐 Pending Recommendations (5)

Prior review findings that remain applicable and unresolved:

• 🟠 agent-sessions.ts:468 closeAllForAgent does not cancel in-flight pendingSessions — if keepalive-grace timer fires while a getSession() call is in-flight, the pending session will complete and register itself after cleanup
• 🟠 api-extension.ts:1956 extractAgentIdentity result discarded in handleAgentUndo — no recordContributor() call means undo operations won't be attributed to the correct agent in shadow repo
• 🟡 agent-sessions.ts:196 scope='session' branch lacks test coverage — the while loop for clearing entire session stack is untested
• 🟡 api-extension.ts:2003 No indication when undo stack was empty — handleAgentUndo returns { ok: true } regardless of whether any undo occurred
• 🟡 boot.ts:293 TOCTOU race between grace timer check and closeAllForAgent — if destroy() is called while a grace timer callback is executing, the callback may run against partially-disposed resources

---

🚫 REQUEST CHANGES

Summary: Two Major issues from the prior review remain unaddressed: (1) closeAllForAgent does not handle in-flight pendingSessions, creating a race condition that can leak sessions; (2) handleAgentUndo discards the agent identity without recording the contributor, breaking attribution for undo operations. The concurrent Map modification issue was successfully fixed with the snapshot pattern. Please address the two Major findings before merge.

Discarded (1)

Location	Issue	Reason Discarded
agent-sessions.ts:485	Concurrent Map modification in closeAllForDoc	RESOLVED — Code now uses snapshot pattern at line 488: const keys = [...this.sessions.keys()].filter((k) => k.startsWith(prefix))

Reviewers (1)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
manual-review	6	0	0	0	0	5	1
Total	6	0	0	0	0	5	1

Note: Task subagents for domain-specific reviewers failed to spawn; findings are from manual verification of prior review issues.

[inkeep-internal-ci]

PR Review Summary

(0) Total Issues | Risk: Low

✅ Addressed Since Last Review

All Major and Minor issues from prior review cycles have been resolved:

🟠 closeAllForAgent does not cancel in-flight pendingSessions — RESOLVED. The method now awaits pending sessions at lines 484-493:

const pendingKeys = [...this.pendingSessions.keys()].filter((k) => k.endsWith(suffix));
if (pendingKeys.length > 0) {
  await Promise.allSettled(pendingKeys.map((k) => this.pendingSessions.get(k)));
}

🟠 extractAgentIdentity result discarded — undo operations unattributed — RESOLVED. handleAgentUndo now calls recordContributor when result.undone is true (lines 1996-2008).

🟠 Concurrent Map modification in closeAllForAgent and closeAllForDoc — RESOLVED. Both methods now use the snapshot pattern (const keys = [...this.sessions.keys()].filter(...)) before iterating.

🟡 scope='session' branch lacks test coverage — RESOLVED. New test at agent-undo.test.ts:113 exercises the while loop with 3 writes, full drain, and empty-stack edge case.

🟡 No indication when undo stack was empty — RESOLVED. applyAgentUndo now returns { undone: boolean } and the handler response includes this field.

🟡 TOCTOU race between grace timer check and closeAllForAgent — RESOLVED. Added shuttingDown guard at lines 286-292 with check in timer callback.

🟡 Missing explicit origin on doc.transact() — RESOLVED. EFFECT_CAPTURE_ORIGIN is now properly defined (lines 28-32) with paired: false and used in the transact call (line 85).

🕐 Pending Recommendations (6)

Prior review findings that remain applicable as minor/consider improvements:

• 🟡 persistence.ts:121 Silent fallback to SERVICE_WRITER may obscure attribution bugs — consider adding a warning log
• 🟡 shadow-repo.ts:364 Ref path construction does not validate writer.id format — defense-in-depth suggestion
• 🟡 activity-log.ts:98 One-shot observer may leak if no YTextEvent fires — cleanup consideration
• 🟡 activity-log.test.ts:159-205 Error handling test doesn't exercise the actual error path
• 💭 contributor-tracker.ts:54 Module-level mutable state — consider documenting single-threaded contract
• 💭 agent-sessions.ts:434-439 UndoManager tracks agent-flash which is presence metadata — UX consideration

---

💡 APPROVE WITH SUGGESTIONS

Summary: This is a well-executed implementation of the agent identity & attribution foundation. All 7 Major/Minor issues from the 3 prior review cycles have been addressed — the scope='session' test coverage was the last outstanding item and is now complete. The remaining 6 Pending Recommendations are minor defense-in-depth improvements and documentation suggestions that don't block merge. The code follows established precedents (#1, #10, #14, #24, #25) and the test coverage is comprehensive.

---

Reviewers (1)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
pr-review (orchestrator)	20	0	0	0	0	6	14
Total	20	0	0	0	0	6	14

Note: Manual review due to subagent dispatch failures. Verified all prior findings against current HEAD.

[inkeep-internal-ci]

PR Review Summary

(0) Total Issues | Risk: Low

✅ All Prior Issues Resolved

All Major and Minor issues from prior review cycles have been verified as resolved:

Prior Finding	Status	Evidence
🟠 closeAllForAgent does not cancel in-flight pendingSessions	✅ RESOLVED	Lines 484-493 now await pending sessions before draining
🟠 extractAgentIdentity result discarded in handleAgentUndo	✅ RESOLVED	Lines 1999-2008 now call recordContributor when undone=true
🟠 Concurrent Map modification in closeAllForAgent/closeAllForDoc	✅ RESOLVED	Both methods use snapshot pattern [...this.sessions.keys()].filter()
🟡 scope='session' branch lacks test coverage	✅ RESOLVED	New test at lines 113-158 exercises full drain
🟡 No indication when undo stack was empty	✅ RESOLVED	applyAgentUndo returns { undone: boolean }
🟡 TOCTOU race in grace timer	✅ RESOLVED	shuttingDown guard at line 292
🟡 Missing explicit origin on doc.transact()	✅ RESOLVED	EFFECT_CAPTURE_ORIGIN defined with paired: false (lines 28-32)

💭 Consider (2) 💭

💭 1) agent-undo.test.ts:86 Multi-client undo test doesn't assert undone field

Issue: The multi-client test asserts undoRes.ok but doesn't verify the undone field in the response body.
Why: The scope='session' test properly asserts body.undone === true (line 138), so this is only a minor coverage gap for the multi-client scenario.
Fix: Add const body = await undoRes.json(); expect(body.undone).toBe(true); after line 86.
Refs: agent-undo.test.ts:86

💭 2) agent-undo.test.ts:75,89 Fixed wait() calls instead of event-driven sync

Issue: Tests use hardcoded wait(600) delays for CRDT propagation instead of event-driven synchronization.
Why: While functional, this pattern can cause flakiness under load or slow CI runners. The awaitDocQuiescence primitive exists in the test harness for this purpose.
Fix: Consider replacing wait(600) with assertAllConverged or awaitDocQuiescence for more deterministic sync.
Refs: test-harness.ts:awaitDocQuiescence

🕐 Pending Recommendations (6)

Prior review findings that remain applicable as minor/consider improvements:

• 🟡 persistence.ts:121 Silent fallback to SERVICE_WRITER may obscure attribution bugs — consider adding a warning log
• 🟡 shadow-repo.ts:364 Ref path construction does not validate writer.id format
• 🟡 activity-log.ts:98 One-shot observer may leak if no YTextEvent fires
• 🟡 activity-log.test.ts:159-205 Error handling test incomplete — asserts metric is 0
• 💭 contributor-tracker.ts:54 Module-level mutable state requires single-threaded execution guarantee
• 💭 agent-sessions.ts:434-439 UndoManager tracks agent-flash which is presence metadata

---

✅ APPROVE

Summary: This PR establishes a coherent agent identity and attribution foundation with per-session origins, per-session UndoManager, classified writer IDs, and structured ok-actor: commit bodies. All Major and Minor issues from the 4 prior review cycles have been verified as resolved. The remaining Consider items are minor test coverage improvements that don't block merge. The architecture is sound, the test coverage is comprehensive (27 user stories, 15 POST handlers threaded, 3 new integration test files), and the documentation is thorough.

Discarded (8)

Location	Issue	Reason Discarded
api-extension.ts	Session ownership validation gap	Explicitly deferred per SPEC §15 "Identity attestation" — accepted scope
standalone.ts	Principal token mismatch during startup	Intentional design — fallback to SERVICE_WRITER is documented behavior (line 968-971)
api-extension.ts	Agent identity impersonation risk	Same as above — cryptographic verification deferred to future milestone
api-extension.ts:595-596	IPv6 loopback check redundancy	Both ::1 and [::1] forms are needed — browsers send either depending on URL construction
session-cleanup.test.ts	NFR-5 soak bypasses captureEffect path	NFR-5 tests session lifecycle, not activity log — different concern
agent-sessions.test.ts	closeSession UM destroy test weak	Single-session test is sufficient for UM destroy verification
Various	Architecture findings	pr-review-architecture returned no issues — noted architectural strengths
Various	Standards findings	pr-review-standards returned no issues

Reviewers (4)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
pr-review-standards	0	0	0	0	0	0	0
pr-review-security-iam	4	0	0	0	0	0	4
pr-review-tests	6	0	2	0	0	0	4
pr-review-architecture	0	0	0	0	0	0	0
Total	10	0	2	0	0	0	8

[inkeep-internal-ci]

PR Review Summary

(0) Total Issues | Risk: Low

✅ All Prior Issues Resolved

All Major and Minor issues from prior review cycles have been verified as resolved:

Prior Finding	Status	Evidence
🟠 closeAllForAgent does not cancel in-flight pendingSessions	✅ RESOLVED	Lines 499-508 now await pending sessions before draining
🟠 extractAgentIdentity result discarded in handleAgentUndo	✅ RESOLVED	Lines 2003-2052 now call recordContributor when undone=true
🟠 Concurrent Map modification in closeAllForAgent/closeAllForDoc	✅ RESOLVED	Both methods use snapshot pattern [...this.sessions.keys()].filter()
🟡 scope='session' branch lacks test coverage	✅ RESOLVED	Test at lines 113-158 exercises full drain
🟡 No indication when undo stack was empty	✅ RESOLVED	applyAgentUndo returns { undone: boolean }
🟡 TOCTOU race in grace timer	✅ RESOLVED	shuttingDown guard at line 245, checked at 292, set at 405

✅ Merge Changes Verified

The recent merge from origin/main introduced well-implemented fixes:

1. Double-prefix fix (agent-sessions.ts:393-409): Strips agent- prefix from agentId before creating session origins to avoid agent-agent-<id> phantom writers. Comprehensive comments explain the fix. Test coverage at agent-sessions.test.ts:238-250 verifies both prefixed and unprefixed cases.

2. git mv rename support (api-extension.ts:439-478): New renameTrackedPathInGit function preserves git history for tracked files with proper error handling and fallback to filesystem rename. Test coverage at api-file-ops.test.ts:463-522.

🕐 Pending Recommendations (6)

Prior review findings that remain applicable as minor/consider improvements:

• 🟡 persistence.ts:121 Silent fallback to SERVICE_WRITER may obscure attribution bugs — consider adding a warning log
• 🟡 shadow-repo.ts:364 Ref path construction does not validate writer.id format
• 🟡 activity-log.ts:98 One-shot observer may leak if no YTextEvent fires
• 🟡 persistence.ts:95 Defensive check for getPrincipal accessor
• 💭 contributor-tracker.ts:54 Module-level mutable state requires single-threaded execution guarantee
• 💭 agent-sessions.ts:449-454 UndoManager tracks agent-flash which is presence metadata

---

✅ APPROVE

Summary: This PR establishes a solid foundation for agent identity and attribution across the CRDT layer. All Major and Minor issues from prior reviews have been addressed. The recent merge from origin/main introduced well-tested fixes for the double-prefix bug and git history preservation. The 6 pending recommendations are all minor/consider-level improvements that don't block merge. Ship it! 🚀

Reviewers (2)

Reviewer	Returned	Main Findings	Consider	While You're Here	Inline Comments	Pending Recs	Discarded
pr-review-standards	0	0	0	0	0	0	0
pr-review-tests	0	0	0	0	0	0	0
Total	0	0	0	0	0	0	0

Note: Focused re-review after prior APPROVED cycle. Dispatched targeted reviewers to verify merge changes only.