Skip to content

feat(security): wire SecretRedactHook.addPattern() and SkillSynthesizer ApprovalQueue - #165

Merged
ryaker merged 4 commits into
mainfrom
fix/winchester-security-wiring
Mar 24, 2026
Merged

feat(security): wire SecretRedactHook.addPattern() and SkillSynthesizer ApprovalQueue#165
ryaker merged 4 commits into
mainfrom
fix/winchester-security-wiring

Conversation

@ryaker

@ryaker ryaker commented Mar 24, 2026

Copy link
Copy Markdown
Owner

User description

Summary

Winchester Mystery House Audit — Stream F: Security Wiring

Resolves two 🟡 partially-wired audit items.

1. SecretRedactHook dynamic patterns (audit 🟡-4)

SecretRedactHook had static regex patterns (API keys, tokens, etc.) but addPattern() didn't exist — the orchestrator.ts comment at line 273 explicitly said so. Stored secrets from SecretsManager were never registered with the redact hook, meaning a secret named my_db_password stored via zora secret set my_db_password would not be redacted from tool arguments.

Changes:

  • Converted SecretRedactHook from plain const to a singleton class (SecretRedactHookImpl)
  • addPattern(keyPattern: RegExp, valuePattern?: RegExp) registers extra match arrays checked in run()
  • orchestrator.boot() now calls secretsManager.listSecrets() after init and registers each name as a case-insensitive key pattern
  • SecretRedactHook still exported as singleton — zero API change at call sites

2. SkillSynthesizer daemon approval (audit 🟡-3)

_confirmWithUser() silently dropped auto-generated skills in daemon mode (stdin not a TTY) with a TODO to wire ApprovalQueue. Skills generated during daemon runs were silently lost.

Changes:

  • Added approvalQueue?: ApprovalQueue to SkillSynthesizerOptions
  • Added setApprovalQueue(queue) method
  • Non-TTY path now checks queue first: routes to queue.request() with score=50 if queue is enabled
  • Falls back to fail-closed if no queue configured (preserves existing safety guarantee)
  • daemon.ts calls orchestrator.skillSynthesizer.setApprovalQueue(approvalQueue) after boot

Test plan

  • Run npm test — 0 failures (better than baseline of 18 failed tests / 3 files)
  • zora secret set my_api_key abc123 → verify my_api_key arg in any tool call is redacted to [REDACTED]
  • Daemon mode with approval.enabled: true → auto-generated skill routes through queue
  • Daemon mode with approval.enabled: false → skill generation fails closed with warning log

🤖 Generated with Claude Code


CodeAnt-AI Description

Keep stored secrets redacted and preserve skill approvals in daemon mode

What Changed

  • Secrets saved in the app are now hidden from tool arguments even when their names do not match the built-in secret patterns.
  • Daemon runs now send auto-generated skills through the approval flow instead of dropping them when there is no interactive prompt.
  • If no approval queue is available in daemon mode, skill generation still stays blocked rather than auto-saving.

Impact

✅ Fewer secret leaks in tool logs
✅ Safer daemon skill approvals
✅ Fewer lost skills in background runs

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Summary by CodeRabbit

  • New Features

    • Added approval-gated skill confirmation for non-interactive (daemon) environments.
    • Automatic redaction of stored secret names in tool call arguments.
    • Support for custom redaction patterns to mask sensitive data.
  • Tests

    • Added integration test coverage for security wiring behaviors and approval workflows.

@codeant-ai

codeant-ai Bot commented Mar 24, 2026

Copy link
Copy Markdown

CodeAnt AI is reviewing your PR.


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@coderabbitai

coderabbitai Bot commented Mar 24, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@ryaker has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 25 minutes and 33 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 015f9245-9394-4d75-9519-302a4f859108

📥 Commits

Reviewing files that changed from the base of the PR and between abf51a5 and cc76ab8.

📒 Files selected for processing (5)
  • src/cli/daemon.ts
  • src/hooks/built-in/secret-redact.ts
  • src/orchestrator/orchestrator.ts
  • src/skills/SkillSynthesizer.ts
  • tests/integration/security-wiring.test.ts
📝 Walkthrough

Walkthrough

This PR implements security and approval wiring at daemon startup by: (1) injecting an initialized ApprovalQueue into SkillSynthesizer, (2) refactoring SecretRedactHook to a singleton with dynamic pattern registration, (3) registering stored secret names into the hook during orchestrator boot, and (4) adding integration tests validating the wiring.

Changes

Cohort / File(s) Summary
Approval Queue Integration
src/cli/daemon.ts, src/skills/SkillSynthesizer.ts
Daemon now calls orchestrator.skillSynthesizer.setApprovalQueue(approvalQueue) after boot. SkillSynthesizer adds optional approvalQueue field, setter method, and updated confirmation logic to request approval via queue when in non-TTY context with queue enabled.
Orchestrator Security Enhancements
src/orchestrator/orchestrator.ts
Added public skillSynthesizer getter. During boot, fetches stored secret names from SecretsManager and registers case-insensitive RegExp patterns into SecretRedactHook via addPattern(...) for each secret name. Removed prior TODO about wiring patterns.
Secret Redaction System
src/hooks/built-in/secret-redact.ts
Replaced inline object constant with singleton instance of SecretRedactHookImpl. Refactored redaction logic into instance methods using static patterns. Added public addPattern(keyPattern, valuePattern?) for runtime extensibility to store and apply additional key/value patterns alongside static ones.
Security Wiring Integration Tests
tests/integration/security-wiring.test.ts
New test suite validating: (1) secret name propagation from SecretsManager into redaction behavior, (2) dynamic value-pattern redaction in SecretRedactHook, (3) ApprovalQueue invocation in non-TTY SkillSynthesizer contexts and fail-closed behavior when queue unavailable. Uses module reset per test for singleton isolation.

Sequence Diagram(s)

sequenceDiagram
    participant Daemon as Daemon Process
    participant Orchestrator
    participant SkillSynthesizer
    participant SecretsManager
    participant SecretRedactHook
    participant ApprovalQueue

    Daemon->>Orchestrator: boot()
    activate Orchestrator
    
    Orchestrator->>SecretsManager: listSecrets()
    activate SecretsManager
    SecretsManager-->>Orchestrator: secret names []
    deactivate SecretsManager
    
    loop For each secret name
        Orchestrator->>SecretRedactHook: addPattern(^name$, i)
        activate SecretRedactHook
        SecretRedactHook->>SecretRedactHook: store pattern
        deactivate SecretRedactHook
    end
    
    Orchestrator-->>Daemon: boot complete
    deactivate Orchestrator
    
    Daemon->>SkillSynthesizer: setApprovalQueue(queue)
    activate SkillSynthesizer
    SkillSynthesizer->>SkillSynthesizer: _approvalQueue = queue
    deactivate SkillSynthesizer
    
    note over Daemon,ApprovalQueue: Runtime skill confirmation (non-TTY context)
    
    Daemon->>SkillSynthesizer: _confirmWithUser(skill)
    activate SkillSynthesizer
    
    alt process.stdin.isTTY is false & approvalQueue enabled
        SkillSynthesizer->>ApprovalQueue: request(skill)
        activate ApprovalQueue
        ApprovalQueue-->>SkillSynthesizer: { allow: true/false }
        deactivate ApprovalQueue
    else no queue or queue disabled
        SkillSynthesizer->>SkillSynthesizer: fail closed
    end
    
    deactivate SkillSynthesizer
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰 A queue for approval, a pattern so keen,
Secrets now redacted in every scene!
The daemon boots strong with HITL in sight,
Security wired just perfectly right. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly summarizes the main changes: wiring SecretRedactHook.addPattern() and SkillSynthesizer ApprovalQueue, which are the two core audit items addressed in this PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/winchester-security-wiring

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances security by enabling dynamic redaction of sensitive information and improves the reliability of skill generation in daemon mode. It addresses previous limitations where stored secrets were not always redacted and auto-generated skills were silently discarded in non-interactive environments, ensuring better control and transparency.

Highlights

  • Dynamic Secret Redaction: The SecretRedactHook has been refactored into a singleton class (SecretRedactHookImpl) and now includes an addPattern() method. This allows Orchestrator to dynamically register secret names from SecretsManager as redaction patterns, ensuring that stored secrets are redacted from tool arguments.
  • Daemon Mode Skill Approval: The SkillSynthesizer now supports an ApprovalQueue for human-in-the-loop (HITL) confirmation in daemon mode. Previously, auto-generated skills were silently dropped in non-TTY environments. With this change, if an ApprovalQueue is configured, skill generation requests are routed for out-of-band approval, otherwise, it falls back to failing closed.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@codeant-ai codeant-ai Bot added the size:L This PR changes 100-499 lines, ignoring generated files label Mar 24, 2026
@codeant-ai

codeant-ai Bot commented Mar 24, 2026

Copy link
Copy Markdown

Sequence Diagram

This PR wires two previously partial security paths: stored secret names are now registered for runtime argument redaction, and daemon skill generation now uses the approval queue instead of silently dropping skills. The flow ensures both safer tool inputs and preserved human approval in non interactive runs.

sequenceDiagram
    participant Daemon
    participant Orchestrator
    participant SecretsManager
    participant SecretRedactHook
    participant ToolRunner
    participant SkillSynthesizer
    participant ApprovalQueue

    Daemon->>Orchestrator: Boot system
    Orchestrator->>SecretsManager: Initialize and list secret names
    SecretsManager-->>Orchestrator: Return stored secret names
    Orchestrator->>SecretRedactHook: Register secret name patterns

    ToolRunner->>SecretRedactHook: Run before tool execution
    SecretRedactHook-->>ToolRunner: Return redacted arguments

    Daemon->>SkillSynthesizer: Set approval queue via orchestrator
    SkillSynthesizer->>ApprovalQueue: Request skill save approval in daemon mode
    ApprovalQueue-->>SkillSynthesizer: Approve or deny
    SkillSynthesizer->>SkillSynthesizer: Save skill only if approved
Loading

Generated by CodeAnt AI

@codeant-ai

codeant-ai Bot commented Mar 24, 2026

Copy link
Copy Markdown

Nitpicks 🔍

🔒 No security issues identified
⚡ Recommended areas for review

  • Regex Injection
    Secret names are interpolated directly into a regular expression. If a stored name contains regex metacharacters, the redaction pattern can become broader than intended or fail to register, so this path should be validated with escaped input.

  • Possible Bug
    Dynamically added RegExp patterns are tested repeatedly with .test(). If a caller passes a global or sticky regex, its lastIndex state can advance between checks and cause later secret matches to be skipped intermittently. Please verify the runtime patterns are normalized or reset before each match.

  • Error Handling
    The approval-queue path awaits request() without guarding against queue backend failures. A transient transport or timeout error would bubble out of _confirmWithUser() and abort skill generation instead of cleanly failing closed, so this path should be validated.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces two security-related features: dynamic secret redaction and a human-in-the-loop approval flow for skill synthesis in daemon mode. The changes are well-structured, but I've found a critical issue in orchestrator.ts that will cause a runtime error and prevent the secret redaction from working as intended. Specifically, an incorrect method name is called, and secret names are not properly escaped when constructing regular expressions, leading to a Regex injection vulnerability. I've provided a code suggestion to fix both issues. I also have a suggestion to improve code conciseness in secret-redact.ts.

Comment thread src/orchestrator/orchestrator.ts Outdated
Comment on lines +275 to +280
const secretNames = await this._secretsManager.listSecrets();
for (const name of secretNames) {
// Match as a key name (exact, case-insensitive) so any arg key matching
// the stored secret name is redacted.
SecretRedactHook.addPattern(new RegExp(`^${name}$`, 'i'));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

There are two issues in this block that will prevent secret redaction from working correctly:

  1. Incorrect method name: The method to retrieve secret names from SecretsManager is listSecretNames(), but listSecrets() is being called. This will result in a runtime error.
  2. Regex injection vulnerability: The secret name is used directly to construct a regular expression. If a secret name contains characters with special meaning in regex (e.g., ., *, +), it can lead to incorrect matching. The secret name must be escaped before being used in the RegExp constructor.

These issues are critical as they break the intended security feature.

      const secretNames = await this._secretsManager.listSecretNames();
      for (const name of secretNames) {
        // Match as a key name (exact, case-insensitive) so any arg key matching
        // the stored secret name is redacted.
        const escapedName = name.replace(/[.*+?^${}()|[\]]/g, '\\$&');
        SecretRedactHook.addPattern(new RegExp(`^${escapedName}$`, 'i'));
      }

Comment on lines +37 to 42
private _shouldRedact(key: string, value: string): boolean {
if (STATIC_KEY_PATTERN.test(key) || STATIC_VALUE_PATTERN.test(value)) return true;
if (this._extraKeyPatterns.some(p => p.test(key))) return true;
if (this._extraValuePatterns.some(p => p.test(value))) return true;
return false;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The _shouldRedact method can be simplified by combining the conditions into a single return statement. This improves readability and makes the logic more concise.

  private _shouldRedact(key: string, value: string): boolean {
    return (
      STATIC_KEY_PATTERN.test(key) ||
      STATIC_VALUE_PATTERN.test(value) ||
      this._extraKeyPatterns.some(p => p.test(key)) ||
      this._extraValuePatterns.some(p => p.test(value))
    );
  }

Comment on lines +45 to +49
if (typeof value === 'string') {
return this._shouldRedact(key, value) ? '[REDACTED]' : value;
}
if (Array.isArray(value)) {
return value.map((item, i) => this._redactDeep(String(i), item));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Redaction is only applied when the value is a string, and array recursion replaces the original key with numeric indexes. This leaks secrets when a sensitive key (including dynamically registered secret names) holds non-string or array/object values, because nested values are checked against 0, 1, etc. instead of the sensitive parent key. Redact immediately when the key matches a sensitive pattern and preserve parent-key context for arrays. [security]

Severity Level: Critical 🚨
- ❌ Secret array values can bypass redaction.
- ⚠️ Tool-call logs may retain sensitive argument data.
- ⚠️ Affects all tools using structured argument payloads.
Suggested change
if (typeof value === 'string') {
return this._shouldRedact(key, value) ? '[REDACTED]' : value;
}
if (Array.isArray(value)) {
return value.map((item, i) => this._redactDeep(String(i), item));
const keyIsSensitive =
STATIC_KEY_PATTERN.test(key) || this._extraKeyPatterns.some(p => p.test(key));
if (keyIsSensitive) {
return '[REDACTED]';
}
if (typeof value === 'string') {
return this._shouldRedact(key, value) ? '[REDACTED]' : value;
}
if (Array.isArray(value)) {
return value.map(item => this._redactDeep(key, item));
Steps of Reproduction ✅
1. Start normal boot path where secrets are loaded in
`src/orchestrator/orchestrator.ts:271-280` (`Orchestrator.boot`), which calls
`SecretRedactHook.addPattern(new RegExp(\`^\${name}$\`, 'i'))` for each stored secret
name.

2. Trigger any tool call event with nested args like `{ prodcred: ["plainsecret"] }`;
tool-call processing enters `src/orchestrator/orchestrator.ts:1017-1021` and executes
`ToolHookRunner.runBefore(...)`.

3. In `src/hooks/built-in/secret-redact.ts:65-67`, `SecretRedactHook.run` calls
`_redactObj`, which invokes `_redactDeep("prodcred", value)` for each argument.

4. In `_redactDeep` at `src/hooks/built-in/secret-redact.ts:48-50`, array handling
replaces the key with indices via `this._redactDeep(String(i), item)`, so `"prodcred"`
context is lost.

5. For each string item, `_shouldRedact("0", "plainsecret")` at
`src/hooks/built-in/secret-redact.ts:37-41` returns false unless value matches static
token regex, so value remains unredacted; then orchestrator logs hooked args at
`src/orchestrator/orchestrator.ts:1028-1040`.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/hooks/built-in/secret-redact.ts
**Line:** 45:49
**Comment:**
	*Security: Redaction is only applied when the value is a string, and array recursion replaces the original key with numeric indexes. This leaks secrets when a sensitive key (including dynamically registered secret names) holds non-string or array/object values, because nested values are checked against `0`, `1`, etc. instead of the sensitive parent key. Redact immediately when the key matches a sensitive pattern and preserve parent-key context for arrays.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎

Comment thread src/orchestrator/orchestrator.ts Outdated
// method is added to the hook's interface (SecretRedactHook has no addPattern yet)
// Wire stored secret names into SecretRedactHook so their values are
// redacted from tool arguments even if they don't match static patterns.
const secretNames = await this._secretsManager.listSecrets();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The secrets API call uses listSecrets(), but SecretsManager exposes listSecretNames(). This will break the boot path when secrets are enabled, so switch to the correct method to preserve the existing manager contract. [logic error]

Severity Level: Critical 🚨
- ❌ Orchestrator boot crashes when secrets are enabled.
- ❌ CLI ask/daemon startup fails before handling tasks.
- ⚠️ Type-check/lint fails on invalid method call.
Suggested change
const secretNames = await this._secretsManager.listSecrets();
const secretNames = await this._secretsManager.listSecretNames();
Steps of Reproduction ✅
1. Set `ZORA_MASTER_PASSWORD` and run any entrypoint that boots the orchestrator, e.g.
`ask` command at `src/cli/index.ts:178-180` or daemon at `src/cli/daemon.ts:190-191`.

2. `Orchestrator.boot()` enters the secrets branch at
`src/orchestrator/orchestrator.ts:268-275` and executes
`this._secretsManager.listSecrets()`.

3. `this._secretsManager` is `SecretsManager`, whose implemented list API is
`listSecretNames()` at `src/security/secrets-manager.ts:136-139`; no `listSecrets()`
exists in that class.

4. Runtime throws `TypeError: this._secretsManager.listSecrets is not a function`, so boot
aborts before task execution.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/orchestrator/orchestrator.ts
**Line:** 275:275
**Comment:**
	*Logic Error: The secrets API call uses `listSecrets()`, but `SecretsManager` exposes `listSecretNames()`. This will break the boot path when secrets are enabled, so switch to the correct method to preserve the existing manager contract.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎

Comment thread src/orchestrator/orchestrator.ts Outdated
for (const name of secretNames) {
// Match as a key name (exact, case-insensitive) so any arg key matching
// the stored secret name is redacted.
SecretRedactHook.addPattern(new RegExp(`^${name}$`, 'i'));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Secret names are interpolated directly into RegExp, so names containing regex metacharacters can throw at runtime or create unintended broad matches. Escape the secret name before building the regex to ensure safe, exact-key matching. [security]

Severity Level: Major ⚠️
- ❌ Boot can crash on malformed secret names.
- ⚠️ Secret redaction can match unintended argument keys.
- ⚠️ Tool argument logging may be over-redacted unexpectedly.
Suggested change
SecretRedactHook.addPattern(new RegExp(`^${name}$`, 'i'));
const escapedName = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
SecretRedactHook.addPattern(new RegExp(`^${escapedName}$`, 'i'));
Steps of Reproduction ✅
1. Store a secret name containing regex metacharacters via CLI `secret set <name> <value>`
at `src/cli/secret-commands.ts:39-45` (name is passed directly; no validation in
`SecretsManager.storeSecret()` at `src/security/secrets-manager.ts:66-93`).

2. Start a flow that calls `Orchestrator.boot()` (`src/cli/index.ts:178-180` or
`src/cli/daemon.ts:190-191`) with `ZORA_MASTER_PASSWORD` set.

3. Boot loads names and builds a regex from raw name at
`src/orchestrator/orchestrator.ts:275-279` using `new RegExp(\`^${name}$\`, 'i')`.

4. If name is invalid regex text (e.g. `[`), `RegExp` throws and boot fails; if name is
valid-but-special (e.g. `a.*`), pattern matches unintended keys, causing incorrect
redaction scope.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/orchestrator/orchestrator.ts
**Line:** 279:279
**Comment:**
	*Security: Secret names are interpolated directly into `RegExp`, so names containing regex metacharacters can throw at runtime or create unintended broad matches. Escape the secret name before building the regex to ensure safe, exact-key matching.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎

@codeant-ai

codeant-ai Bot commented Mar 24, 2026

Copy link
Copy Markdown

CodeAnt AI finished reviewing your PR.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
tests/integration/security-wiring.test.ts (1)

63-67: Exercise the real orchestrator wiring in at least one test.

These cases reimplement the production wiring with listSecretNames() + hook.addPattern(...), so they won’t catch regressions at the actual call site in src/orchestrator/orchestrator.ts—the current listSecrets() typo slipped through for exactly that reason. One boot-path test that goes through Orchestrator.boot() would make this coverage much more meaningful.

Also applies to: 84-87, 106-109, 127-130

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/integration/security-wiring.test.ts` around lines 63 - 67, The tests
currently mirror orchestration wiring by calling sm.listSecretNames() and
hook.addPattern(...) instead of exercising the real code path, so add at least
one integration test that creates an Orchestrator instance and calls
Orchestrator.boot() (or the exported boot helper) to exercise the actual wiring
in src/orchestrator/orchestrator.ts and catch issues like the
listSecrets/listSecretNames typo; update the tests that currently call
sm.listSecretNames() + hook.addPattern (locations referenced around the current
blocks) to instead initialize the real Orchestrator, invoke boot(), and assert
the expected hook patterns/registrations so the production call site is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/cli/daemon.ts`:
- Around line 193-195: The approval queue is being set after Orchestrator.boot()
starts background work, causing SkillSynthesizer._approvalQueue to be unset
during startup; move the call to
orchestrator.skillSynthesizer.setApprovalQueue(approvalQueue) so it runs before
orchestrator.boot() is invoked (or else inject approvalQueue into Orchestrator
at construction/boot time), ensuring SkillSynthesizer sees a non-null
_approvalQueue during boot and preventing synthesized skills from being dropped.

In `@src/orchestrator/orchestrator.ts`:
- Line 275: Replace the incorrect call to this._secretsManager.listSecrets()
with the existing API this._secretsManager.listSecretNames(); update the call in
the orchestrator (where secretNames is assigned) so it awaits listSecretNames()
(preserving the secretNames variable) and ensure any downstream code consuming
secretNames matches the returned type from listSecretNames().
- Around line 276-280: The loop that builds redact patterns uses raw secret
names to construct RegExp objects (see secretNames iteration and
SecretRedactHook.addPattern(new RegExp(`^${name}$`, 'i'))), which breaks when
names contain regex metacharacters; fix by escaping regex-special characters in
each name before interpolation (use a function to replace characters like
-/\\^$*+?.()|[]{} with escaped versions) and then create the RegExp from the
escaped string with the same anchors and 'i' flag so secrets are matched
literally and safely.

---

Nitpick comments:
In `@tests/integration/security-wiring.test.ts`:
- Around line 63-67: The tests currently mirror orchestration wiring by calling
sm.listSecretNames() and hook.addPattern(...) instead of exercising the real
code path, so add at least one integration test that creates an Orchestrator
instance and calls Orchestrator.boot() (or the exported boot helper) to exercise
the actual wiring in src/orchestrator/orchestrator.ts and catch issues like the
listSecrets/listSecretNames typo; update the tests that currently call
sm.listSecretNames() + hook.addPattern (locations referenced around the current
blocks) to instead initialize the real Orchestrator, invoke boot(), and assert
the expected hook patterns/registrations so the production call site is covered.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ce83a63c-7808-48e6-a107-b62210f87843

📥 Commits

Reviewing files that changed from the base of the PR and between 006a85e and abf51a5.

📒 Files selected for processing (5)
  • src/cli/daemon.ts
  • src/hooks/built-in/secret-redact.ts
  • src/orchestrator/orchestrator.ts
  • src/skills/SkillSynthesizer.ts
  • tests/integration/security-wiring.test.ts

Comment thread src/cli/daemon.ts Outdated
Comment thread src/orchestrator/orchestrator.ts Outdated
Comment thread src/orchestrator/orchestrator.ts
ryaker pushed a commit that referenced this pull request Mar 24, 2026
… redaction, setApprovalQueue race, lastIndex drift, error handling

- Escape secret names before constructing RegExp to prevent regex injection (BLOCKER 1)
- Redact entire array when parent key is sensitive, preventing bypass via numeric indices (BLOCKER 2)
- Move setApprovalQueue() before orchestrator.boot() to eliminate race window (BLOCKER 3)
- Strip g/y flags from patterns in addPattern() to prevent lastIndex drift on global/sticky regexes (MUST-FIX 4)
- Wrap ApprovalQueue.request() in try/catch, fail closed on transport error (MUST-FIX 5)

Co-Authored-By: Claude <noreply@anthropic.com>
ryaker-LG and others added 4 commits March 23, 2026 22:53
…er ApprovalQueue

Two Winchester audit items resolved:

## SecretRedactHook dynamic patterns (🟡-4)
SecretRedactHook was a plain const with static regex patterns and no way
to register new ones at runtime. The orchestrator.ts TODO at line 273-274
noted addPattern() didn't exist yet.

- Converted SecretRedactHook to a class (SecretRedactHookImpl) implementing
  ToolHook; exports singleton instance
- addPattern(keyPattern, valuePattern?) registers extra regex arrays
- After SecretsManager.init(), orchestrator iterates listSecrets() and
  calls addPattern() for each stored secret name (exact case-insensitive match)
- Stored secrets now guaranteed to be redacted even if their names don't
  match the static pattern (key|token|secret|password|auth|bearer|credential)

## SkillSynthesizer daemon approval (🟡-3)
_confirmWithUser() silently dropped skills in daemon mode (stdin not a TTY)
with a TODO to wire ApprovalQueue.

- Added approvalQueue? option to SkillSynthesizerOptions
- Added setApprovalQueue(queue) method called by daemon after boot
- When stdin is not TTY and queue is enabled: routes to queue.request()
  with score=50 (medium risk) instead of failing closed
- Falls back to fail-closed if queue not configured (preserves safety)
- daemon.ts calls orchestrator.skillSynthesizer.setApprovalQueue(approvalQueue)
  immediately after orchestrator.boot()

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sizer approval queue wiring

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Wiring tests caught a TypeError: SecretsManager exposes listSecretNames()
but orchestrator.ts called the non-existent listSecrets(). Would have
thrown at runtime whenever ZORA_MASTER_PASSWORD was set, silently
preventing any stored secret names from reaching SecretRedactHook.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… redaction, setApprovalQueue race, lastIndex drift, error handling

- Escape secret names before constructing RegExp to prevent regex injection (BLOCKER 1)
- Redact entire array when parent key is sensitive, preventing bypass via numeric indices (BLOCKER 2)
- Move setApprovalQueue() before orchestrator.boot() to eliminate race window (BLOCKER 3)
- Strip g/y flags from patterns in addPattern() to prevent lastIndex drift on global/sticky regexes (MUST-FIX 4)
- Wrap ApprovalQueue.request() in try/catch, fail closed on transport error (MUST-FIX 5)

Co-Authored-By: Claude <noreply@anthropic.com>
@ryaker
ryaker force-pushed the fix/winchester-security-wiring branch from e4a32c3 to cc76ab8 Compare March 24, 2026 05:54
@ryaker
ryaker merged commit fe8016e into main Mar 24, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants