Repository navigation
refactor: apply scoped logger context to interim PRs - #3078
amikofalvy wants to merge 2 commits into
Conversation
These files were modified by PRs #3062 and #3064 which merged after the logger migration in #3067 but were not updated to use the scoped context pattern. Wraps tool-approval.ts and tool-wrapper.ts execute paths in runWithLogContext({ toolCallId, toolName }), strips repeated ambient fields from individual logger calls, and adds logger scoped context guidance to AGENTS.md and the api-logging-guidelines skill. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 57130f6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Migrates the tool-execution layer ( Key changes
Summary ｜ 5 files ｜ 2 commits ｜ base: Scoped logger context in tool execution
In
Developer guidelines for scoped logger context
The SKILL.md rewrite replaces the former "Avoid Redundant Logging" principle with "Use Scoped Logger Context (ALS)" and provides a concrete 4-step migration strategy: wrap in
|
There was a problem hiding this comment.
Clean migration — the ALS context changes are functionally correct and the documentation additions are well-structured. One numbering bug in the skill file and one architectural note about triple-nested scopes.
Claude Opus ｜ 𝕏
| - ALS middleware provides tenant/project/agent IDs on every log line automatically | ||
| - Adding duplicate logs creates noise without value | ||
|
|
||
| ### 2. Log Level Guidelines |
There was a problem hiding this comment.
Section numbering is off: the new section 1 ("Use Scoped Logger Context") pushed everything down, but this header kept its old number. Sequence reads 1, 2, 2, 3 instead of 1, 2, 3, 4.
| ### 2. Log Level Guidelines | |
| ### 3. Log Level Guidelines |
| { approved: false; deniedResult: unknown } | { approved: true } | { approved: 'pending' } | ||
| > { | ||
| logger.info({ toolName, toolCallId, args }, 'Tool requires approval - waiting for user response'); | ||
| return runWithLogContext({ toolCallId, toolName }, () => |
There was a problem hiding this comment.
Both parseAndCheckApproval (line 249) and this function wrap in runWithLogContext({ toolCallId, toolName }). Since both callers (function-tools.ts:98, mcp-tools.ts:99) already go through wrapToolWithStreaming which sets the same scope at tool-wrapper.ts:108, tool executions end up with three nested pino child loggers carrying identical bindings.
This is functionally harmless (pino child nesting is cheap and ALS composes correctly), and keeping the wrappers here is defensively correct — if these functions are ever called outside wrapToolWithStreaming, they still get context. Just flagging for awareness; no change needed unless you want to eliminate the redundancy.
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
.agents/skills/api-logging-guidelines/SKILL.md:50Duplicate section numbering (### 2. appears twice)
💡 APPROVE WITH SUGGESTIONS
Summary: This is a clean refactoring PR that correctly migrates tool-approval.ts and tool-wrapper.ts to the scoped logger context pattern (ALS). The runWithLogContext wrappers appropriately set toolName and toolCallId context, and all logger calls correctly strip these now-ambient fields. The AGENTS.md documentation addition is clear and comprehensive. The only issue is a minor section numbering typo in the skill file that can be addressed in a follow-up.
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-docs |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 1 | 0 | 0 | 0 | 1 | 0 | 0 |
| - ALS middleware provides tenant/project/agent IDs on every log line automatically | ||
| - Adding duplicate logs creates noise without value | ||
|
|
||
| ### 2. Log Level Guidelines |
There was a problem hiding this comment.
🟡 Minor: Duplicate section numbering
Issue: This section is numbered "### 2." but line 43 already has "### 2. Avoid Redundant Logging". The sections should be numbered sequentially (1, 2, 3, 4, 5) but currently read 1, 2, 2, 3, 4.
Why: Contributors following this skill will be confused by duplicate numbering. When referencing "principle 2" it's ambiguous which section is meant.
Fix: (1-click apply)
| ### 2. Log Level Guidelines | |
| ### 3. Log Level Guidelines |
Then renumber the subsequent sections accordingly (3→4, 4→5).
Preview URLsUse these stable preview aliases for testing this PR:
These point to the same Vercel preview deployment as the bot comment, but they stay stable and easier to find. Raw Vercel deployment URLs
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | Risk: Low
Delta Analysis
The only change since the last automated review is the addition of a changeset file (.changeset/back-white-warbler.md). The changeset is correctly formatted:
- ✅ Targets
@inkeep/agents-api(the package containingtool-approval.tsandtool-wrapper.ts) - ✅ Uses
patchlevel (appropriate for internal refactoring with no API changes) - ✅ Message follows changelog guidelines: "Migrate tool-approval and tool-wrapper logger calls to scoped context pattern"
No new issues found in the delta.
🕐 Pending Recommendations (1)
- 🟡
.agents/skills/api-logging-guidelines/SKILL.md:50Section numbering bug —### 2.appears twice (lines 43 and 50). Sequence should be 1, 2, 3, 4, 5 but currently reads 1, 2, 2, 3, 4.
💡 APPROVE WITH SUGGESTIONS
Summary: The delta (changeset addition) is correctly formatted. The PR correctly migrates tool-approval.ts and tool-wrapper.ts to the scoped logger context pattern. One minor section numbering issue from the prior review remains unaddressed — see Pending Recommendations above for the inline comment link.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Delta-only review — no sub-reviewers dispatched for changeset-only changes.
Ito Test Report ✅8 test cases ran. 8 passed. Overall, this unified local non-production QA run passed with 8/8 executed test cases and 0 failures (plus one code-first check that produced no verifiable feature outcome), and both runtime validation and route/path code review found no code-supported production defect in scope. The most important findings were that baseline chat streaming remained functional (with simulated model execution for deterministic local validation), approval flows behaved correctly and safely (mixed per-toolCallId approve/deny mapping, idempotent handling of replayed/unknown/forged IDs, and single-resolution under rapid repeated actions), and the UI/security behaviors were stable across mobile usability, script-injection rendering as inert text, and cross-tab conversation isolation. ✅ Passed (8)
Commit: Tell us how we did: Give Ito Feedback |
|
This pull request has been automatically marked as stale because it has not had recent activity. If this PR is still relevant:
Thank you for your contributions! |
|
This pull request has been automatically closed due to inactivity. If you'd like to continue working on this, please:
Thank you for your understanding! |
Summary
tool-approval.tsandtool-wrapper.tsto the scoped logger context pattern (ALS) introduced in refactor: migrate logger calls to scoped context patterns #3067runWithLogContextapi-logging-guidelinesskillChanges
tool-approval.ts: WrapswaitForToolApprovalandparseAndCheckApprovalinrunWithLogContext({ toolCallId, toolName }), strips repeatedtoolName/toolCallIdfrom 8 logger callstool-wrapper.ts: Wraps theexecuteclosure inrunWithLogContext({ toolName, toolCallId }), strips repeated fields from 3 logger callsAGENTS.md: New "Logger: Scoped Context Pattern" section documentinggetLogger,runWithLogContext, and.with()patternsapi-logging-guidelinesskill: Updated to teach the ALS scoped context pattern as the primary approachTest plan
pnpm typecheckpassespnpm lintpassespnpm formatpassespending-approvals.test.ts🤖 Generated with Claude Code