feat: add scoped logger context via AsyncLocalStorage#3054
Conversation
Add runWithLogContext() and PinoLogger.with() for automatic context propagation. Middleware in createApp.ts sets tenantId/projectId/agentId for run and manage routes. Convert AgentSession, TriggerService, and github routes as proof-of-concept adoption. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: e299804 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.
1 Skipped Deployment
|
|
TL;DR — Introduces Key changes
Summary ｜ 24 files ｜ 8 commits ｜ base:
|
| Middleware path | Bindings seeded |
|---|---|
/run/* |
tenantId, projectId, agentId (from executionContext) |
/manage/tenants/* |
tenantId, projectId (from Hono context / route param) |
Adoption in AgentSession, GitHub routes, and TriggerService
Before: ~50 log calls across these files manually threaded
sessionId,tenantId,installationId, etc.
After:AgentSessionusesthis.logger = getLogger('AgentSession').with({ sessionId })in the constructor; GitHub routes wrap per-installation handlers inrunWithLogContext({ installationId });TriggerServicewrapsprocessWebhookanddispatchExecutionto scopetriggerId/invocationId/conversationId.
The net effect is significantly less boilerplate at call sites — log statements pass only event-specific data (e.g. { repositoryCount }) while identity/scope fields come from context automatically.
agents-api/src/domains/run/session/AgentSession.ts · agents-api/src/domains/manage/routes/github.ts · agents-api/src/domains/run/services/TriggerService.ts
Claude Opus ｜ 𝕏
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid foundation for automatic log context propagation. The ALS + WeakMap caching approach is well-designed, the snapshot semantics on .with() are correct, and the test coverage for the core primitives is thorough. Three issues worth addressing before merge — one medium (type-safety of the middleware context lookup), one medium (dead code), and a few minor items in the proof-of-concept conversions.
Claude Opus ｜ 𝕏
| export function getLogContext(): Record<string, unknown> { | ||
| const store = loggerStorage.getStore(); | ||
| if (!store) return {}; | ||
| const raw = (store as any).bindings?.() ?? {}; | ||
| return raw; | ||
| } |
There was a problem hiding this comment.
Medium: getLogContext() is exported but has zero callers.
This function uses pino's internal .bindings() API via (store as any).bindings?.(), which is undocumented and could break across pino major versions. If this is intentional future API, add a test for it. If it's speculative, remove it — it can always be added later when there's a concrete use case.
| if (!this.isSnapshot) { | ||
| setBasePinoInstance(this.pinoInstance); | ||
| } |
There was a problem hiding this comment.
Minor: setBasePinoInstance called from fromInstance path may be surprising.
When a PinoLogger is constructed with fromInstance and snapshot: false (the default for non-.with() callers), it overwrites the global basePinoInstance. This means any code that creates a PinoLogger via fromInstance without realizing the side effect will silently change the ALS fallback for all loggers.
Currently only the LoggerFactory and tests use fromInstance, but this is an implicit global mutation that could bite future callers. Consider documenting this in PinoLoggerConfig.fromInstance's JSDoc.
|
|
||
| const execStartedAt = Date.now(); | ||
| const dispatchDelayMs = dispatchedAt ? execStartedAt - dispatchedAt : undefined; | ||
| return runWithLogContext({ triggerId, invocationId, conversationId }, async () => { |
There was a problem hiding this comment.
Minor: redundant nested runWithLogContext in executeAgentAsync.
dispatchExecution (line 528) already calls runWithLogContext({ triggerId, invocationId, conversationId }, ...) around the call to executeAgentAsync. Then executeAgentAsync itself wraps its body in another runWithLogContext({ triggerId, invocationId, conversationId }, ...) with the exact same bindings.
The nesting composes correctly (inner inherits parent), but the duplication is pointless — the inner scope re-binds the same keys with the same values. Since executeAgentAsync is also exported and called independently (e.g. from eval workflows), keeping its own scope is fine, but you could at least drop triggerId from the inner call since the outer already binds it. Alternatively, document this is intentional for standalone-call safety.
| }; | ||
| return { | ||
| getLogger: vi.fn(() => mockLogger), | ||
| runWithLogContext: vi.fn((_bindings: any, fn: any) => fn()), |
There was a problem hiding this comment.
Nit: runWithLogContext mock is correct but withRequestContext mock (line 27) appears to be a leftover. No code in the PR imports withRequestContext from ../logger. If it existed before and was removed, the mock is harmless but dead. If it's needed for other tests not in this PR, ignore.
| { sessionId, messageId, agentId: executionContext.agentId }, | ||
| 'AgentSession created' | ||
| ); | ||
| this.logger = getLogger('AgentSession').with({ sessionId }); |
There was a problem hiding this comment.
Good pattern. Using .with({ sessionId }) to create a session-scoped logger instance that automatically includes sessionId in every log line is the right approach — eliminates the repetitive { sessionId: this.sessionId } spread across ~30 call sites.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(6) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
🟠 1) logger.ts:136-163 recreateInstance() doesn't update basePinoInstance
Issue: When recreateInstance() is called after adding transports or updating options, it creates a new pino instance but does not call setBasePinoInstance(). This means the module-level basePinoInstance used by runWithLogContext as the default parent remains stale.
Why: Creates a split-brain scenario where logs from new ALS scopes (via runWithLogContext) use the old instance, while direct logger method calls use the new instance with updated transports. If a team adds a new transport for observability, ALS-scoped logs silently won't reach it.
Fix: Add setBasePinoInstance(this.pinoInstance) at the end of recreateInstance().
Refs:
🟠 2) TriggerService.ts:566-583 ALS context lost in .catch() handler
Issue: The error handler attached via .catch() to executionPromise runs after the runWithLogContext scope has exited. The ALS store is empty when this code executes, so logger.error falls back to the base logger without tenant/project/agent/trigger/invocation context.
Why: Production incident investigation becomes significantly harder when error logs from background trigger failures can't be correlated with the originating request.
Fix: Capture bindings before the .catch() and wrap the error handler in runWithLogContext.
Refs:
Inline Comments:
- 🟠 Major:
logger.ts:163recreateInstance doesn't update basePinoInstance - 🟠 Major:
TriggerService.ts:569ALS context lost in catch handler
🟡 Minor (3) 🟡
🟡 1) setup.ts (agents-work-apps) Missing runWithLogContext mock export
Issue: The test setup mocks the logger module but doesn't export runWithLogContext, while agents-api's setup.ts does.
Why: If agents-work-apps code adopts the runWithLogContext pattern, tests will fail with undefined function error.
Fix: Add runWithLogContext: vi.fn((_bindings: any, fn: any) => fn()) to both mock blocks.
Refs:
🟡 2) logger.test.ts No tests for async function behavior with runWithLogContext
Issue: All current tests use synchronous callbacks, but production code wraps async functions. ALS context propagation across await boundaries is untested.
Why: If context is lost after an await (which shouldn't happen with proper ALS usage, but is a critical invariant), logs would silently lose scoped fields in production.
Fix: Add a test like:
it('preserves context across await boundaries', async () => {
await runWithLogContext({ tenantId: 't1' }, async () => {
logger.info({}, 'before');
await Promise.resolve();
logger.info({}, 'after await');
});
// verify both logs have tenantId
});🟡 3) logger.test.ts No tests for error propagation in runWithLogContext
Issue: No tests verify that thrown errors (sync) or rejected promises (async) correctly bubble up from within runWithLogContext.
Why: If errors are swallowed or transformed, callers won't receive expected exceptions. TriggerService relies on errors propagating for status updates.
Fix: Add tests for both sync throws and async rejections, verifying the exact error instance propagates.
Inline Comments:
- 🟡 Minor:
setup.ts:26Missing runWithLogContext mock (first block) - 🟡 Minor:
setup.ts:48Missing runWithLogContext mock (second block)
💭 Consider (1) 💭
💭 1) AgentSession.ts:238 .with() snapshot semantics may miss runtime context updates
Issue: The session logger is created via .with({ sessionId }) at construction time, using snapshot semantics. If the AgentSession is constructed before middleware sets ALS context, or if context changes during the session's lifecycle, logs will have stale/missing context.
Why: This is the intended design (frozen context at construction), but differs from module-scope loggers that continue proxying ALS. For long-running sessions, this is a subtle contract.
Fix: Document that AgentSession must be constructed within a runWithLogContext scope, or consider whether session-level loggers need dynamic context.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
logger.ts:43 |
Module-level mutable basePinoInstance shared across requests |
By design — only one logger config per process is expected. Racing writes are an edge case. |
logger.ts:36-41 |
getLogContext() uses undocumented pino internals |
Function is exported but not used in converted files. Low risk. |
logger.ts:71 |
WeakMap cache per PinoLogger creates unbounded memory growth | WeakMap allows GC when ALS instance is unreachable. Memory is bounded by concurrent requests × modules. |
createApp.ts:273 |
Middleware depends on upstream setting executionContext | Correct behavior — fallback to next() without context is intentional for excluded routes. |
github.ts |
Inconsistent pattern — some handlers don't use runWithLogContext | Intentional — handlers without installationId rely on middleware-provided tenantId. |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | Inline Comments | Discarded |
|---|---|---|---|---|---|
pr-review-sre |
6 | 2 | 0 | 2 | 2 |
pr-review-architecture |
8 | 0 | 1 | 0 | 5 |
pr-review-tests |
5 | 2 | 0 | 0 | 0 |
pr-review-consistency |
5 | 1 | 0 | 2 | 2 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 |
| Total | 24 | 5 | 1 | 4 | 9 |
Note: pr-review-standards returned no findings — the core implementation is solid.
💡 APPROVE WITH SUGGESTIONS
Summary: The AsyncLocalStorage-based scoped logger context is a well-designed feature with good test coverage. The core implementation is solid. However, there are two issues worth addressing:
recreateInstance()bug — ALS-scoped logs won't use updated transports afteraddTransport()calls (1-line fix)- Context loss in error handlers — Background trigger failure logs lose correlation IDs, hurting incident investigation
Both fixes are straightforward. The test mock inconsistency in agents-work-apps is minor but worth fixing for consistency. Nice work on this foundational infrastructure change! 🎉
| const safeExecutionPromise = executionPromise.catch(async (error) => { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| const errorStack = error instanceof Error ? error.stack : undefined; | ||
| logger.error({ err: errorMessage, errorStack }, 'Background trigger execution failed'); |
There was a problem hiding this comment.
🟠 MAJOR: ALS context lost in .catch() handler — logs will lack correlation IDs
Issue: The safeExecutionPromise.catch() handler executes after the runWithLogContext scope has exited (via waitUntil). At this point, the ALS store is empty, so logger.error falls back to the base logger without tenantId, projectId, agentId, triggerId, or invocationId.
Why: When investigating production incidents, error logs from background trigger failures won't correlate with the triggering request. This makes debugging significantly harder in multi-tenant environments.
Fix: Capture the bindings before the catch and wrap the error handler:
// Before the .catch():
const errorBindings = { tenantId, projectId, agentId, triggerId, invocationId };
const safeExecutionPromise = executionPromise.catch(async (error) => {
return runWithLogContext(errorBindings, async () => {
const errorMessage = error instanceof Error ? error.message : String(error);
// ... rest of error handling
});
});Refs:
- Node.js ALS docs — context only propagates within the callback
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
|
…cs, async test - Add JSDoc to .with() documenting snapshot semantics (captures ALS context at call time, returned logger does not continue proxying) - Add async propagation test to verify ALS context survives await boundaries Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
Re-review scope: This review covers only the 3 commits since the prior automated review. Two major issues from that review remain unresolved.
✅ Delta Changes Addressed Prior Feedback
The following items from prior reviews have been addressed in this delta:
- ✅
getLogContext()dead code — Removed (pullfrog feedback) - ✅ Missing async await boundary test — Added comprehensive test at
logger.test.ts:203-214 - ✅ JSDoc for
.with()snapshot semantics — Added clear documentation explaining the behavior - ✅ Type safety in
createApp.ts— Improved from(c as any).get()to typed key access
🟡 Minor (1) 🟡
🟡 1) logger.test.ts:203-214 Async test uses standalone PinoLogger instead of factory pattern
Issue: The new async await boundary test creates a direct PinoLogger via createTestLogger(), but production code obtains loggers through loggerFactory.getLogger().
Why: The test proves ALS propagation works for direct instances but doesn't verify factory-cached loggers also correctly resolve ALS context across await boundaries. Low risk since other tests validate factory loggers with ALS, but coverage would be more representative with a factory-based async test.
Fix: Consider adding a companion test using loggerFactory.configure() and loggerFactory.getLogger() with async/await.
Refs:
💭 Consider (1) 💭
💭 1) logger.test.ts:207 Test only verifies single await boundary
Issue: The async test validates context survives one setTimeout await but doesn't exercise multiple sequential awaits or nested async calls.
Why: AsyncLocalStorage is designed to handle this correctly by specification, so risk is low. Adding multiple awaits would marginally increase confidence.
Fix: Optionally enhance with multiple awaits in sequence.
🕐 Pending Recommendations (2)
These issues were raised in the prior review and remain unresolved:
- 🟠
logger.ts:129-156—recreateInstance()doesn't callsetBasePinoInstance(), causing split-brain where ALS-scoped logs miss transport updates - 🟠
TriggerService.ts:568-585— ALS context lost in.catch()handler afterrunWithLogContextscope exits viawaitUntil
💡 APPROVE WITH SUGGESTIONS
Summary: The delta commits addressed several feedback items nicely — the dead code removal, JSDoc additions, and async test are all good improvements. However, the two major issues from the prior review (recreateInstance() not updating basePinoInstance and ALS context loss in the .catch() handler) remain unresolved. These should be addressed before merge to ensure:
- Transport changes (e.g., adding observability transports) propagate to ALS-scoped logs
- Error logs from background trigger failures can be correlated with the originating request
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
setup.ts (agents-work-apps) |
Missing runWithLogContext mock |
Not blocking — agents-work-apps doesn't currently use the pattern. Can be added when adoption spreads. |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | Pending Recs | Discarded |
|---|---|---|---|---|---|
pr-review-sre |
1 | 0 | 0 | 1 | 0 |
pr-review-standards |
1 | 0 | 0 | 1 | 0 |
pr-review-tests |
2 | 1 | 1 | 0 | 0 |
| Total | 4 | 1 | 1 | 2 | 0 |
Note: Major findings routed to Pending Recommendations as they were raised in prior review.
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 | (2) Pending | Risk: Medium
Re-review scope: This review covers only the 1 commit since the prior automated review.
✅ Delta Analysis
The delta since the last review consists of a single commit:
47481b4e0—chore: remove accidentally committed pr-context skill
This commit removes .agents/skills/pr-context/SKILL.md (385 lines deleted), which was accidentally committed and shouldn't have been part of the PR. No code changes. The core logger implementation and adoption files remain unchanged.
🕐 Pending Recommendations (2)
These major issues were raised in prior reviews and remain unresolved. They should be addressed before merge:
- 🟠
logger.ts:129-156—recreateInstance()doesn't callsetBasePinoInstance()after creating a new pino instance, causing split-brain where ALS-scoped logs miss transport updates - 🟠
TriggerService.ts:567-585— ALS context lost in.catch()handler afterrunWithLogContextscope exits viawaitUntil; error logs won't have correlation IDs
🚫 REQUEST CHANGES
Summary: The delta commit is a trivial cleanup (removing an accidentally committed file). However, two major issues identified in prior reviews remain unaddressed:
-
recreateInstance()bug — When transports are added viaaddTransport()or options updated viaupdateOptions(), the new pino instance is created butbasePinoInstanceisn't updated. This means ALS-scoped logs will continue using the old instance, missing new transports. Fix: AddsetBasePinoInstance(this.pinoInstance)at the end ofrecreateInstance(). -
Context loss in error handlers — The
.catch()handler indispatchExecutionruns after therunWithLogContextscope exits. Error logs will lacktenantId,projectId,agentId,triggerId, andinvocationId, making incident investigation significantly harder. Fix: Capture bindings before the.catch()and wrap the error handler inrunWithLogContext.
Both fixes are straightforward 1-5 line changes. The core ALS implementation is solid — just these two edge cases need attention before this is ready to merge.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | Pending Recs | Discarded |
|---|---|---|---|---|---|
| N/A | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 |
Note: No sub-reviewers dispatched — delta was trivial (skill file deletion only).
Allow logger.info('message') without an empty data object, alongside the
existing logger.info({ data }, 'message') form. Converts 41 empty-object
calls in AgentSession, TriggerService, and github.ts to the cleaner form.
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 | (2) Pending | Risk: Medium
Re-review scope: This review covers only the 1 commit since the prior automated review.
✅ Delta Analysis
The delta since the last review consists of a single commit:
fc7e62b94—feat(agents-core): add string-only overloads to PinoLogger methods
What changed:
- Added function overloads to
error(),warn(),info(),debug()methods allowinglogger.info('message')without requiring an empty data object - Updated ~60 call sites across
github.ts,TriggerService.ts, andAgentSession.tsto use the cleaner syntax
Assessment: This is a clean DX improvement. The implementation correctly distinguishes between single-argument (message-only) and two-argument (data + message) calls. TypeScript overloads provide type safety. The changes are mechanical and low-risk.
💭 Consider (1) 💭
💭 1) logger.test.ts No tests for string-only overloads
Issue: The new function overloads (logger.info('message') without data object) are exercised by production code but not by any unit tests. All existing tests use the two-argument form.
Why: Low risk since CI passes and TypeScript provides type safety, but explicit test coverage would document the expected behavior and catch regressions if the overload logic changes.
Fix: Consider adding a test case:
it('supports string-only log calls', () => {
runWithLogContext({ tenantId: 'tenant-1' }, () => {
logger.info('message only');
});
expect(lines[0].tenantId).toBe('tenant-1');
expect(lines[0].msg).toBe('message only');
});Refs:
🕐 Pending Recommendations (2)
These major issues were raised in prior reviews and remain unresolved. They should be addressed before merge:
- 🟠
logger.ts:129-156—recreateInstance()doesn't callsetBasePinoInstance()after creating a new pino instance, causing split-brain where ALS-scoped logs miss transport updates - 🟠
TriggerService.ts:567-585— ALS context lost in.catch()handler afterrunWithLogContextscope exits viawaitUntil; error logs won't have correlation IDs
🚫 REQUEST CHANGES
Summary: The delta commit (fc7e62b94) is a clean DX improvement — adding string-only overloads to logger methods simplifies call sites from logger.info({}, 'message') to logger.info('message'). No new issues introduced.
However, two major issues identified in prior reviews remain unaddressed:
-
recreateInstance()bug — When transports are added viaaddTransport()or options updated viaupdateOptions(), the new pino instance is created butbasePinoInstanceisn't updated. This means ALS-scoped logs will continue using the old instance, missing new transports. Fix: AddsetBasePinoInstance(this.pinoInstance)at the end ofrecreateInstance(). -
Context loss in error handlers — The
.catch()handler indispatchExecutionruns after therunWithLogContextscope exits. Error logs will lacktenantId,projectId,agentId,triggerId, andinvocationId, making incident investigation significantly harder. Fix: Capture bindings before the.catch()and wrap the error handler inrunWithLogContext.
Both fixes are straightforward 1-5 line changes. The core ALS implementation is solid — just these two edge cases need attention before this is ready to merge.
Discarded (0)
No findings discarded in this review cycle.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | Pending Recs | Discarded |
|---|---|---|---|---|---|
| N/A | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 |
Note: No sub-reviewers dispatched — delta was mechanical (method overloads + call site simplifications).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Acknowledged the author's reply on the
|
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (2) Pending | Risk: Medium
Re-review scope: This review covers only the 1 commit since the prior automated review.
✅ Delta Analysis
The delta since the last review consists of a single commit:
e29980463—fix: remove remaining empty object arg from multiline logger.warn call
What changed:
- logger.warn(
- {},
- 'waitUntil is NOT available — background execution will be abandoned on serverless'
- );
+ logger.warn(
+ 'waitUntil is NOT available — background execution will be abandoned on serverless'
+ );Assessment: This is a correct cleanup using the new string-only overload added in fc7e62b94. The change is mechanical, low-risk, and consistent with the pattern established across other converted call sites. ✅
🕐 Pending Recommendations (2)
These major issues were raised in prior reviews and remain unresolved. They should be addressed before merge:
- 🟠
logger.ts:129-156—recreateInstance()doesn't callsetBasePinoInstance()after creating a new pino instance, causing split-brain where ALS-scoped logs miss transport updates - 🟠
TriggerService.ts:568-585— ALS context lost in.catch()handler afterrunWithLogContextscope exits viawaitUntil; error logs won't have correlation IDs
🚫 REQUEST CHANGES
Summary: The delta commit (e29980463) is a trivial 1-line cleanup removing an empty object argument from logger.warn — correctly using the new string-only overload. No issues with this change.
However, two major issues identified in prior reviews remain unaddressed:
-
recreateInstance()bug (logger.ts:129-156) — When transports are added viaaddTransport()or options updated viaupdateOptions(), the new pino instance is created butbasePinoInstanceisn't updated. This means ALS-scoped logs will continue using the old instance, missing new transports.Fix: Add
setBasePinoInstance(this.pinoInstance)at the end ofrecreateInstance(). -
Context loss in error handlers (TriggerService.ts:568-585) — The
.catch()handler runs after therunWithLogContextscope exits viawaitUntil. Error logs will lacktenantId,projectId,agentId,triggerId, andinvocationId, making incident investigation significantly harder.Fix: Capture bindings before the
.catch()and wrap the error handler inrunWithLogContext:const errorBindings = { tenantId, projectId, agentId, triggerId, invocationId, conversationId }; const safeExecutionPromise = executionPromise.catch(async (error) => { return runWithLogContext(errorBindings, async () => { // ... existing error handling }); });
Both fixes are straightforward 1-5 line changes. The core ALS implementation is solid — just these two edge cases need attention before this is ready to merge.
Discarded (0)
No findings discarded in this review cycle.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| N/A | 0 | 0 | 0 | 0 | 0 | 2 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: No sub-reviewers dispatched — delta was trivial (1-line cleanup to use string-only logger overload).
Ito Test Report ✅17 test cases ran. 1 additional finding, 16 passed. The unified run executed 17 test cases with 16 passed and 1 failed, showing strong stability across GitHub manage routes, installation lifecycle actions, callback/connect flows, webhook/run-auth paths, and adversarial scenarios with expected controlled contracts (200/202/4xx/404/503) and no middleware crashes or data leaks. The single confirmed issue is a minor docs regression where API reference pages initialize and call PostHog even when NEXT_PUBLIC_POSTHOG_KEY is unset, causing repeated console errors on load/reload that add debugging noise but do not indicate a backend failure. ✅ Passed (16)
ℹ️ Additional Findings (1)
🟡 API docs pages render generated operation lists
Relevant code:
if (typeof window !== 'undefined') {
posthog.init(process.env.NEXT_PUBLIC_POSTHOG_KEY || '', {
api_host: process.env.NEXT_PUBLIC_POSTHOG_HOST || 'https://us.i.posthog.com',
capture_pageview: false,
person_profiles: 'always',
disable_surveys: true,
disable_external_dependency_loading: true,
<Suspense>
<PostHogPageview />
</Suspense>
<PostHogProvider>
<RootProvider search={{ SearchDialog }}>
posthog.capture('$pageview', {
$current_url: url,
});Commit: Tell us how we did: Give Ito Feedback |
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs' |
…ull and projectFull Replace the pre-ALS logger DI pattern with module-scope getLogger() calls. In agentFull.ts, the default logger was a no-op, silently swallowing all 167 logger calls in production. This change turns on real logging for agent CRUD operations and aligns both files with the scoped logger context pattern established in PR #3054. - Delete AgentLogger interface and no-op defaultLogger - Delete ProjectLogger type export - Add module-scope getLogger('agentFull') / getLogger('projectFull') - Remove logger parameter from all exported curried functions - Update all internal and external call sites Closes PRD-6465 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
#3072) * refactor(agents-core): remove logger dependency injection from agentFull and projectFull Replace the pre-ALS logger DI pattern with module-scope getLogger() calls. In agentFull.ts, the default logger was a no-op, silently swallowing all 167 logger calls in production. This change turns on real logging for agent CRUD operations and aligns both files with the scoped logger context pattern established in PR #3054. - Delete AgentLogger interface and no-op defaultLogger - Delete ProjectLogger type export - Add module-scope getLogger('agentFull') / getLogger('projectFull') - Remove logger parameter from all exported curried functions - Update all internal and external call sites Closes PRD-6465 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: update skipped SDK test and clarify changeset for logger DI removal - Fix type error in agent-refactor.test.ts (skipped test) that destructured 2 args from the outer curried function which now only takes 1 (db). The test was already broken before this change — it named the params (tenantId, agentData) but the outer function takes (db, logger). - Clarify in changeset that removed types had zero external consumers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: remove unused tenantId variable in skipped SDK test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
runWithLogContext(bindings, fn)andPinoLogger.with(bindings)to@inkeep/agents-corefor automatic request-scoped log context propagation via AsyncLocalStoragecreateApp.tsthat setstenantId/projectId/agentIdfor all/run/*and/manage/tenants/*routes — every downstream log call automatically includes these fieldsAgentSession(class member pattern),TriggerService(runWithLogContextat function entry),github.tsroutes (runWithLogContextper handler)How it works
AsyncLocalStorage<PinoLoggerInstance>stores pino child loggers with pre-serialized bindingsconst logger = getLogger('X')works unchanged across ~177 files —PinoLoggermethods resolve the active ALS instance at call time via a WeakMap-cached proxy (~10ns overhead).with()uses snapshot semantics: captures current ALS context at call time, does not continue proxyingrecreateInstance()clears WeakMap cache to ensure transport/option changes propagaterunWithLogContextcalls compose: inner scope inherits parent bindings + adds new onesTest plan
.with()snapshot semanticsagents-apiandagents-work-appssetup filespnpm typecheckpasses (16/16 tasks)agents-apitests: 2,237 passedagents-coretests: 2,099 passedFollow-up (PR 2)
Broad sweep of remaining ~220 files to adopt the pattern — mechanical but large, deferred intentionally.
📋 Spec:
specs/2026-04-06-logger-scoped-context/SPEC.md🤖 Generated with Claude Code