Greptile Summary

This PR replaces console.warn in resolveCurrentUserSid with a structured subsystem logger, correctly importing createSubsystemLogger and initialising a module-level logger. However, the new call site passes String(err) as the second argument to logger.warn, which expects meta?: Record<string, unknown> — this is a TypeScript compile error, and at runtime the error detail is silently discarded rather than included in the log output.

Confidence Score: 3/5

Not safe to merge as-is — the logger call has a type mismatch that drops the error detail and will fail TypeScript compilation.

A single P1 finding (type error + runtime data loss at the logger call site) caps the score. The structural change itself is correct; only the argument to logger.warn needs fixing.

src/security/windows-acl.ts — line 302, the logger.warn call

This is a comment left during a code review.
Path: src/security/windows-acl.ts
Line: 302

Comment:
**Wrong type passed as `meta` — error detail silently dropped**

`SubsystemLogger.warn` is typed as `(message: string, meta?: Record<string, unknown>) => void`, so the second argument must be a `Record<string, unknown>`, not a `string`. Passing `String(err)` here is a TypeScript compile error, and at runtime the error details are lost: `emitLog` calls `Object.keys(meta)` on the string, which returns character-index keys, so the actual error text never reaches the log sink.

Combine the error into the message string or pass it as a structured meta object:

```suggestion
    logger.warn(`resolveCurrentUserSid failed: ${String(err)}`);
```

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

_{Reviews (1): Last reviewed commit: "fix(security): replace console.warn with..." | Re-trigger Greptile}

"CI failure in checks-node-agentic-commands (qr-cli.test.ts vitest mock hoisting) is a pre-existing infrastructure issue unrelated to this change. checks-node-core-src-security passes."

CI failures in check, check-additional, checks-node-channels, checks-node-core-fast-support, checks-node-core-support-boundary, and checks-node-extensions are pre-existing issues on main and unrelated to this change. checks-node-core-src-security (covering the modified windows-acl.ts) passes, and checks-node-agentic-commands (previously blocked by #73177) is now green after #73206.

Codex review: needs maintainer review before merge.

Keep open. Current main still has the raw console.warn and TODO in resolveCurrentUserSid, while this PR is now a focused one-file change that replaces it with createSubsystemLogger("security/windows-acl") and uses the corrected single-string logger.warn call. The earlier bot finding about passing a string as logger meta appears addressed in the latest PR patch, and the related qr-cli hoisting issue was handled separately by merged #73206.

Maintainer follow-up before merge:

Keep this PR open for normal maintainer review. If CI remains green, the best path is to land the focused one-file logger change, preserving resolveCurrentUserSid's best-effort null fallback and avoiding any unrelated security, dependency, or workflow changes. Run the Windows ACL test plus the qr-cli hoisting regression path and changed gate before merge.

Best possible solution:

Keep this PR open for normal maintainer review. If CI remains green, the best path is to land the focused one-file logger change, preserving resolveCurrentUserSid's best-effort null fallback and avoiding any unrelated security, dependency, or workflow changes. Run the Windows ACL test plus the qr-cli hoisting regression path and changed gate before merge.

Acceptance criteria:

• pnpm test src/security/windows-acl.test.ts
• pnpm test src/cli/qr-cli.test.ts
• pnpm exec oxfmt --check --threads=1 src/security/windows-acl.ts
• pnpm check:changed

What I checked:

• Current main still has the requested cleanup target: src/security/windows-acl.ts still logs resolveCurrentUserSid failures with console.warn and keeps the TODO saying to replace it with a structured logger once available. (src/security/windows-acl.ts:299, bd5afadc5c7a)
• PR patch is focused and uses the corrected logger.warn form: Provided pullFiles context shows only src/security/windows-acl.ts changed: it imports createSubsystemLogger, creates createSubsystemLogger("security/windows-acl"), and calls logger.warn(resolveCurrentUserSid failed: ${String(err)}), matching the bot review suggestion. (src/security/windows-acl.ts:302, 85176cb42ade)
• Logger API supports the corrected call shape: SubsystemLogger.warn is typed as warn(message: string, meta?: Record<string, unknown>), so the latest template-string-only call avoids the earlier type mismatch and preserves the error text in the message. (src/logging/subsystem.ts:26, bd5afadc5c7a)
• Existing test covers best-effort failure behavior: windows-acl.test.ts verifies inspectWindowsAcl continues successfully when whoami throws and the unknown SID remains untrusted, so the logging-only change should preserve the existing fallback behavior. (src/security/windows-acl.test.ts:515, bd5afadc5c7a)
• Related import-graph test issue appears resolved separately: Current main's qr-cli.test.ts uses vi.hoisted for mocks; the provided context says merged fix(test): resolve vitest mock hoisting in qr-cli.test.ts #73206 fixed test(cli): qr-cli.test.ts fails with ReferenceError due to vitest mock hoisting #73177, which was the qr-cli hoisting failure exposed by importing logging/subsystem through windows-acl. (src/cli/qr-cli.test.ts:6, bd5afadc5c7a)
• Security review pass: The provided PR metadata shows one changed file with no dependency, workflow, permission, install, release, lockfile, generated, or secret-handling changes. The only security-sensitive aspect is a logging-path change in src/security/windows-acl.ts, which should still receive normal security/core validation before merge. (src/security/windows-acl.ts:299, bd5afadc5c7a)

Likely related people:

• Peter Steinberger: Local blame and release/current-main history attribute the current windows-acl SID-resolution catch block and subsystem logger implementation to Peter in the available repository history. (role: introduced/current-main maintainer in visible history; confidence: medium; commits: be8c24633aaa, ade863e08f93; files: src/security/windows-acl.ts, src/security/windows-acl.test.ts, src/logging/subsystem.ts)
• DoncicX: Beyond authoring this PR, provided related context shows DoncicX authored merged fix(test): resolve vitest mock hoisting in qr-cli.test.ts #73206, which fixed the qr-cli hoisting issue exposed by adding logging/subsystem to the windows-acl import graph. (role: adjacent validation owner; confidence: medium; commits: 6e1789b21d2a; files: src/cli/qr-cli.test.ts)

Remaining risk / open question:

• The PR touches a security-adjacent Windows ACL logging path, so it should not be merged without the targeted security test lane and changed-gate validation.
• Adding a logging/subsystem import changes the import graph; test(cli): qr-cli.test.ts fails with ReferenceError due to vitest mock hoisting #73177/fix(test): resolve vitest mock hoisting in qr-cli.test.ts #73206 suggest the qr-cli hoisting path was already exposed once, so that regression path should stay in validation.
• This is a logging cleanup rather than a demonstrated vulnerability fix, so maintainers may want to adjust the title/category before merge.

Codex review notes: model gpt-5.5, reasoning high; reviewed against bd5afadc5c7a.

same as #74471