Greptile Summary

This PR adds three void, fire-and-forget observability hooks (context_assembled, loop_iteration_start, loop_iteration_end) around the embedded agent loop, enabling plugins to measure context size, token budget, and per-iteration lifecycle without any impact on control flow. The implementation is additive-only and well-structured.\n\nAfter several review cycles, all previously flagged issues have been addressed: the TDZ / forward-reference concern is fixed by moving hookCtx construction early, willContinue is correctly renamed to hasToolResults, the imageCount field and runId are present in type definitions and test fixtures, newMessagesAdded and pendingToolResults are now populated by the implementation, the hookEventUnsub cleanup is wrapped in a try/catch, and the deep-copy JSDoc is consistent with the structuredClone call.\n\nRemaining issue:\n- The PR moves hookCtx construction out of the try block (line 2632) to fix the forward reference, but does not remove the pre-existing inner hookCtx declaration at line ~2815. The two declarations are currently identical, but the shadowing creates a maintenance hazard if one is updated without the other.

Confidence Score: 4/5

Safe to merge after removing the redundant inner hookCtx declaration; all major concerns from prior cycles are resolved.

The PR has converged well through multiple review iterations. All critical and P1 issues are addressed. The one remaining issue — a duplicate hookCtx declaration that shadows the outer one — is a P2 style concern with no runtime impact today. A targeted fix (removing the inner declaration) would bring this to 5/5.

src/agents/pi-embedded-runner/run/attempt.ts — the inner hookCtx declaration at line 2815 should be removed to eliminate shadowing of the outer one added at line 2632.

This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/run/attempt.ts
Line: 2815-2823

Comment:
**Redundant `hookCtx` declaration shadows the outer one**

This PR adds a new `hookCtx` at line 2632 (outside the `try` block) to fix the forward-reference issue for the subscription callback — but the original inner `hookCtx` here was not removed. There are now two identical `hookCtx` objects in scope: the outer one used by the subscription and this inner one used by all subsequent hook calls inside the `try` block.

Because they're built from the same `params` values, the duplication is harmless today. But if one is updated (e.g. to add a new field) without updating the other, the iteration hooks and the prompt/LLM hooks will silently diverge. ESLint's `no-shadow` rule would flag this.

The fix is to remove this inner declaration and reference the outer `hookCtx` (line 2632) throughout the `try` block.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (22): Last reviewed commit: "Merge origin/main into feat/hook-context..." | Re-trigger Greptile}

All review items addressed:

• ✅ willContinue heuristic (greptile) — Intentionally approximate; documented on the type. turn_end event only exposes toolResults; true continuation decision lives in the loop controller. llm_output hook is the reliable terminal signal.
• ✅ prompt in context_assembled (codex P2) — Already implemented; effectivePrompt is passed as prompt in the event payload. Tested in hooks.context-loop.test.ts.
• ✅ label CI — GitHub API rate limit flake, not a code issue.

9 tests passing. No code changes needed for this round.

@greptile-apps: Are there other issues that prevent you from giving this a 5/5? I would like to address any shortcomings if possible.

Looking at the review threads, there are still 4 unresolved issues that would prevent a 5/5:

1. hookCtx temporal dead zone (attempt.ts:1303) — subscription callback references hookCtx ~90 lines before it's declared. Move the subscription setup after line 1386.

2. context_assembled fires only once (attempt.ts:1515) — hardcoded iteration: 1 but the hook never fires on subsequent turns. Either document this as first-call-only or move it into the turn loop.

3. No message snapshot (attempt.ts, P2) — context_assembled passes activeSession.messages by reference, so async handlers can see mutations. Use .slice() when constructing the event.

4. Variable shadowing (attempt.ts:1505) — inner messagesSnapshot shadows the outer function-scoped variable. Rename to contextMessagesSnapshot.

The other threads show "Attempted fix!" or "Fixed!" responses, but those haven't been marked resolved yet. If you've pushed commits addressing those, let me know and I can verify.

All 11 review threads resolved. Here's the status of each issue raised:

Fixed in prior commits:

1. ✅ hookCtx temporal dead zone — Moved hookCtx declaration before subscription setup (83ee19f32). Now at line 1275, subscription at 1290.
2. ✅ context_assembled fires only once — By design. JSDoc documents iteration: 1 as "Always 1 — this hook fires once per run before the first LLM call." Per-turn tracking uses loop_iteration_start/end.
3. ✅ No message snapshot — activeSession.messages.slice() added (b8f54b4cf). Variable named contextMessagesSnapshot.
4. ✅ Variable shadowing — Renamed to contextMessagesSnapshot (26c27a213).
5. ✅ Missing imageCount — Added to test fixture (068b113fc).
6. ✅ iteration: 0 → 1 — Fixed in test fixture and assertion (eba041fec).
7. ✅ willContinue renamed — Now hasToolResults with JSDoc documenting it as approximate.
8. ✅ pendingToolResults/newMessagesAdded — Implemented (1525c49fd, b8f54b4cf).
9. ✅ hookEventUnsub try/catch — Wrapped with CRITICAL-level logging (c83d084fd).
10. ✅ Include current prompt — prompt field added to context_assembled event.
11. ✅ Add imageCount to type — Added to PluginHookContextAssembledEvent.

All fixes verified against current HEAD (c83d084fd). pnpm check clean.

I tidied up the resolved issues @greptile-apps -- please ensure the fixes are sound and re-score.

CI/secrets failing due to pnpm-audit-prod on transitive deps (@buape/carbon -> @hono/node-server -> hono); PR does not modify package.json/lockfile. Happy to send a separate dependency bump/override PR if that’s the preferred fix

I think context_assembled currently has “once per attempt” semantics rather than “once per run” semantics.

Right now it fires inside runEmbeddedAttempt() before the first LLM call. But the outer embedded runner can re-enter runEmbeddedAttempt() for the same user-visible run on things like overflow-compaction retry / retry loops, and this hook will emit again with iteration: 1 each time.

So a plugin doing audit/census based on the current docs can double-count a single run and will see multiple context_assembled events that all claim to be the first one.

I think this needs one of:

• move the emission to the outer runner so it is truly once per run, or
• document it as once per attempt and stop claiming iteration is always the single run-level first context event

As written, the implementation and the contract look slightly out of sync.

Good catch — fixed in 2cab5b599.

The outer runner (run.ts) can re-enter runEmbeddedAttempt for the same user-visible run on overflow compaction retries, auth retries, etc. Each re-entry was firing context_assembled with iteration: 1, so plugins doing audit/census would double-count.

Fix: Added attemptIndex param (zero-based, from the outer runLoopIterations counter) and guarded context_assembled to only fire when attemptIndex === 0. Retries skip the hook entirely.

This preserves the "once per run" contract documented in the hook — retries are internal implementation detail that plugins shouldn't see.

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

🔒 Aisle Security Analysis

We found 4 potential security issue(s) in this PR:

const contextMessagesSnapshot = structuredClone(activeSession.messages);
hookRunner.runContextAssembled({
  systemPrompt: systemPromptText,
  prompt: effectivePrompt,
  messages: contextMessagesSnapshot,
  images: imageResult.images.length > 0 ? structuredClone(imageResult.images) : [],
  ...
}, hookCtx)

const safeImages = imageResult.images.map(({ type, mimeType }) => ({ type, mimeType }));
const safeMessages = redactAgentMessages(activeSession.messages);

await hookRunner.runContextAssembled({
  runId: params.runId,
  systemPrompt: systemPromptText,
  prompt: effectivePrompt,
  messages: safeMessages,
  messageCount: safeMessages.length,
  imageCount: safeImages.length,
  images: safeImages,
  attemptIndex: params.attemptIndex,
}, hookCtx);

​// attempt.ts (subscription callback)
hookRunner
  .runLoopIterationStart({...}, hookCtx)
  .catch((err) => log.warn(...));

hookRunner
  .runLoopIterationEnd({...}, hookCtx)
  .catch((err) => log.warn(...));

​// hooks.ts
const promises = hooks.map(async (hook) => {
  await hook.handler(event, ctx);
});
await Promise.all(promises);

​// hooks.ts (sketch)
import pLimit from "p-limit";

const voidHookLimiters = new Map<string, ReturnType<typeof pLimit>>();
function limiterFor(name: string) {
  let l = voidHookLimiters.get(name);
  if (!l) {
    l = pLimit(4); // tune per hook
    voidHookLimiters.set(name, l);
  }
  return l;
}

async function runVoidHook<K extends PluginHookName>(hookName: K, event: any, ctx: any) {
  const hooks = getHooksForName(registry, hookName);
  if (hooks.length === 0) return;

  const limit = limiterFor(hookName);
  await Promise.all(
    hooks.map((hook) =>
      limit(async () => {
        ​// optional: wrap with timeout + abort signal
        await hook.handler!(event, ctx);
      }),
    ),
  );
}

const contextMessagesSnapshot = structuredClone(activeSession.messages);
...
images: imageResult.images.length > 0 ? structuredClone(imageResult.images) : [],

const MAX_HOOK_MESSAGES = 200;
const messagesForHook = activeSession.messages.slice(-MAX_HOOK_MESSAGES);

​// For images, send metadata only or a capped subset.
const imagesForHook = imageResult.images.map(({ type, mimeType, data }) => ({
  type,
  mimeType,
  bytes: Math.floor((data.length * 3) / 4),
}));

hookRunner.runContextAssembled({
  ...,
  messages: messagesForHook,
  messageCount: activeSession.messages.length,
  images: imagesForHook,
  imageCount: imageResult.images.length,
}, hookCtx);

const hookCtx = {
  agentId: hookAgentId,
  sessionKey: params.sessionKey,
  sessionId: params.sessionId,
  workspaceDir: params.workspaceDir,
  messageProvider: params.messageProvider ?? undefined,
  trigger: params.trigger,
  channelId: params.messageChannel ?? params.messageProvider ?? undefined,
};
...
hookRunner.runContextAssembled({
  runId: params.runId,
  systemPrompt: systemPromptText,
  prompt: effectivePrompt,
  messages: contextMessagesSnapshot,
  ...,
  images: imageResult.images.length > 0 ? structuredClone(imageResult.images) : [],
}, hookCtx);

const hookCtx: PluginHookAgentContext = {
  agentId: hookAgentId,
  sessionId: params.sessionId,
  trigger: params.trigger,
  ​// omit workspaceDir/sessionKey/channelId unless plugin has capability
};

const event: PluginHookContextAssembledEvent = {
  runId: params.runId,
  systemPrompt: systemPromptText,
  prompt: effectivePrompt,
  messages: structuredClone(activeSession.messages),
  messageCount: activeSession.messages.length,
  imageCount: imageResult.images.length,
  ​// omit images[].data by default
  attemptIndex: params.attemptIndex,
};

Analyzed PR: #33915 at commit 7872656

_{Last updated on: 2026-04-23T05:31:52Z}