feat(skills): autonomous skill generation

[ryaker]

User description

Summary

Adds autonomous skill generation to Zora. After a session completes, Zora checks complexity and — with HITL confirmation — synthesizes a reusable SKILL.md file to ~/.zora/skills/.

• Trigger: tool_calls >= 8 OR turns >= 8
• HITL gate: user confirms before anything is written to disk
• Integrity: SHA-256 hash stored in skills.lock.json, verified on every load
• Duplicate detection: word-overlap scan of existing skills (≥0.5 ratio = skip)
• Atomic writes: tmpfile + rename pattern

New files

• src/skills/SkillSynthesizer.ts — core synthesizer, LLM synthesis via existing provider interface
• src/skills/SkillsLock.ts — integrity manifest (load/save/verify SHA-256)
• tests/unit/skills/SkillSynthesizer.test.ts — 23 unit tests

Modified files

• src/skills/index.ts — barrel exports
• src/orchestrator/orchestrator.ts — event counting, post-task hook
• README.md — Autonomous Skill Generation section

Test plan

• 1279 unit tests pass (npm run test:unit)
• TypeScript strict — no errors (tsc --noEmit)
• Manual: run zora-agent ask on a complex task (8+ turns), confirm skill prompt appears
• Manual: verify ~/.zora/skills/<slug>/SKILL.md written correctly
• Manual: verify skills.lock.json SHA-256 matches file content

🤖 Generated with Claude Code

---

CodeAnt-AI Description

Add runtime safety layer, human approvals, per-project policy, agent cooldown, and autonomous skill generation

What Changed

• Daemon startup now runs a security audit and will block startup on critical FAILs (or can be bypassed with an explicit env var); new zora security CLI command scans and can auto-fix some issues.
• Each tool call is scored for irreversibility; actions above configured thresholds are logged, paused for human approval (sent to configured channels like Telegram), or auto-denied if critical.
• Session-level risk forecasting aggregates signals (drift, salami, commitment creep) and can cause approval gating or auto-deny based on composite score.
• Human-in-the-loop approval queue supports tokenized approvals (allow, deny, allow-30m, allow-session) delivered via messaging gateways; Telegram handlers added to send requests and accept replies.
• Per-project security policy (.zora/security-policy.toml) lets projects further restrict allowed tools and set a maximum irreversibility ceiling that cannot exceed the global policy.
• Subagents gain reputation tracking and cooldowns: repeated denied actions cause throttling, stricter restrictions, and eventual shutdown; admins can reset reputation.
• After complex sessions (tool_calls >= 8 OR turns >= 8), Zora can propose and save a SKILL.md (HITL confirmation required); written skills have SHA‑256 entries recorded in a skills.lock.json integrity manifest.
• One-shot "ask" mode avoids starting background channels/heartbeat so the process exits cleanly; skill synthesis is non-blocking after task completion. Unit tests added for the SkillSynthesizer and security docs updated.

Impact

✅ Clearer startup failure reasons (blocked on critical security findings)
✅ Fewer accidental destructive actions (action scoring + approvals + auto-deny)
✅ Easier task reuse via confirmed SKILL.md generation

💡 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

  • Runtime Safety Layer: action risk scoring with thresholds, human approval gating, startup security audit, per-project security policy, session risk forecasting, and agent reputation cooldowns
  • Autonomous post-session skill generation with on-disk skill locking
  • Persistent daemon improvements with Telegram approval workflow and new CLI security commands

• Documentation

  • New comprehensive Runtime Safety Layer guide, configuration reference, and troubleshooting docs; README updates

• Tests

  • Added skill synthesizer tests; relaxed a performance benchmark

• Chores

  • Bumped package version and SDK dependency

[codeant-ai]

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]

Warning

Rate limit exceeded

@ryaker has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 53 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: d6b966e3-eef3-4db9-ac6d-6c47f20967bd

📥 Commits

Reviewing files that changed from the base of the PR and between 4e10dce and 8db9777.

📒 Files selected for processing (1)

• src/cli/security-commands.ts

📝 Walkthrough

Walkthrough

Adds a Runtime Safety Layer: irreversibility scoring hook, human-in-the-loop approval queue, session risk forecaster, per-agent reputation cooldowns, per-project security policies, CLI security audit, daemon/orchestrator startup gating and wiring, Telegram approval transport, autonomous skill synthesis, docs, templates, and tests.

Changes

Cohort / File(s)	Summary
Documentation & Release README.md, CHANGELOG.md, docs/advanced/security-runtime.md	New Runtime Safety Layer documentation, README updates, detailed runtime security reference page, and changelog entry for 0.9.1.
Daemon / CLI src/cli/daemon.ts, src/cli/index.ts, src/cli/security-commands.ts	Daemon startup runs security audit and may abort on FAIL; initializes global ApprovalQueue, AgentCooldown, MemoryRiskForecaster; registers zora security audit command; ask adjusted to skip channels.
Core Safety Subsystems src/core/approval-queue.ts, src/core/agent-cooldown.ts, src/core/memory-risk-forecaster.ts, src/core/project-policy.ts	New approval queue (tokenized requests, reply handling), per-agent reputation persistence/enforcement, session risk forecaster (drift/salami/commitment creep), project policy loader/merge and enforcement helpers.
Tool / Hook Enforcement src/hooks/built-in/irreversibility-scorer.ts, src/hooks/index.ts, src/config/policy-loader.ts, src/types.ts	Added irreversibility scorer hook and export; policy parsing now supports actions.scores and actions.thresholds; types extended for heartbeat_provider, scores, thresholds.
Orchestrator & Tools src/orchestrator/orchestrator.ts, src/tools/subagent-tool.ts, src/steering/telegram-gateway.ts	Orchestrator gains skipChannels option, registers scorer hook, wires SkillSynthesizer post-session; subagent delegation consults cooldown and project policy; TelegramGateway supports /approve and broadcasting approval requests.
Skills System src/skills/SkillSynthesizer.ts, src/skills/SkillsLock.ts, src/skills/index.ts, tests/unit/skills/SkillSynthesizer.test.ts	Autonomous post-session skill synthesis, duplicate detection, HITL confirmation, atomic SKILL.md writes, skills.lock manifest, and unit tests.
Templates & Config src/templates/security-policy.toml.template, .claude/settings.json, .gitignore, package.json	Added project security-policy template; .claude/settings.json trimmed (hooks/env/workers removed); .ruvector entries removed from .gitignore; bumped package and Claude SDK version.
Tests & Benchmarks tests/unit/memory/performance-benchmarks.test.ts	Loosened SalienceScorer performance assertion (20ms → 50ms).

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant CLI
    participant Orchestrator
    participant Hook as IrreversibilityScorerHook
    participant Policy as ProjectPolicy
    participant Forecaster as MemoryRiskForecaster
    participant Queue as ApprovalQueue
    participant Telegram

    User->>CLI: submit task / ask
    CLI->>Orchestrator: start session / execute task
    loop for each tool call
        Orchestrator->>Hook: beforeToolCall(tool, jobId)
        Hook->>Policy: checkScoreLimit(tool, agentPolicy)
        alt Project policy denies
            Hook-->>Orchestrator: deny (project_policy)
        else
            Hook->>Forecaster: record(action, score)
            alt Forecaster suggests auto-deny
                Hook-->>Orchestrator: deny (session_risk_critical)
            else if score >= flag or Forecaster intercept
                Hook->>Queue: request(action, score, jobId, tool)
                Queue->>Telegram: send approval message
                Telegram->>User: notify /approve TOKEN decision
                User->>Telegram: /approve TOKEN allow
                Telegram->>Queue: handleReply(TOKEN, allow)
                Queue-->>Hook: approval result true
                Hook-->>Orchestrator: allow
            else
                Hook-->>Orchestrator: allow
            end
        end
        Orchestrator->>Orchestrator: execute tool
    end
    Orchestrator->>Orchestrator: maybeGenerateSkill()

Loading

sequenceDiagram
    participant Daemon as Daemon Startup
    participant Audit as SecurityAudit
    participant Config as ConfigLoader
    participant Cooldown as AgentCooldown
    participant Forecaster as MemoryRiskForecaster
    participant Queue as ApprovalQueue
    participant Orchestrator

    Daemon->>Audit: runSecurityAuditSilent()
    Audit->>Audit: validate permissions, secrets, binding, versions
    alt FAILs present
        Audit-->>Daemon: exitCode=1
        Daemon->>Daemon: process.exit(1)
    else PASS/WARN
        Audit-->>Daemon: continue
        Daemon->>Config: load policy.toml / safety config
        Daemon->>Cooldown: initGlobalCooldown(config.cooldown)
        Daemon->>Forecaster: initGlobalForecaster(config.risk_forecaster)
        Daemon->>Queue: new ApprovalQueue(config.approval)
        Daemon->>Orchestrator: boot orchestrator (hooks, channels)
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• feat(security): Zora Runtime Safety Layer — 6 enhancements (audit, scoring, approval, cooldown, forecaster, project policy) #153 — Implements largely the same Runtime Safety Layer modules and wiring (approval queue, forecaster, cooldown, hooks, CLI audit, skills).
• feat(skills): agent-callable skill tools (list_skills, invoke_skill) #132 — Overlaps on skills integration and orchestrator wiring relevant to SkillSynthesizer changes.
• Security hardening: action budgets, dry-run mode, intent capsules #38 — Related changes to policy parsing/types and policy-loader updates (actions.scores / actions.thresholds).

Suggested labels

size:XL

Poem

🐰 Hop-hop — I scored each risky tool,

warned the humans and queued each rule.
Tokens danced, approvals flew, so sly,
cooldowns nibbled mischief passing by.
Skills bloom from sessions — hop, hooray, hi!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 63.83% 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 PR title 'feat(skills): autonomous skill generation' directly and clearly describes the main change: implementation of autonomous skill generation functionality.

_{✏️ 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 feature/autonomous-skill-generation

📝 Coding Plan

• Generate coding plan for human review comments

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 significantly enhances Zora's capabilities by introducing autonomous skill generation, allowing the agent to learn and codify complex task sequences into reusable skills. Concurrently, a robust Runtime Safety Layer has been implemented to provide advanced security controls, ensuring that Zora operates within defined boundaries and that risky actions are subject to human oversight. These changes aim to make Zora more intelligent, adaptable, and secure in its operations.

Highlights

• Autonomous Skill Generation: Zora can now autonomously generate reusable SKILL.md files after complex sessions (triggered by 8+ tool calls or turns), with human-in-the-loop confirmation before writing to disk.
• Runtime Safety Layer: A comprehensive Runtime Safety Layer has been introduced, featuring irreversibility scoring for actions, human-in-the-loop approval queues, session risk forecasting (drift, salami, commitment creep), agent reputation management, per-project security policies, and a startup security audit.
• Integrity and Security: Skills are managed with SHA-256 hash verification via skills.lock.json to ensure integrity, and a new zora security CLI command provides an audit of the Zora installation for misconfigurations.
• Dependency Updates: The @anthropic-ai/claude-agent-sdk and various sharp native modules have been updated to their latest versions.

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]

Sequence Diagram

This PR adds a post-session flow where the orchestrator triggers skill synthesis when a task is complex enough. A generated skill is deduplicated, confirmed by the user, then saved atomically with an integrity hash update.

sequenceDiagram
    participant User
    participant Orchestrator
    participant SkillSynthesizer
    participant LLMProvider
    participant SkillsStorage

    User->>Orchestrator: Complete ask session
    Orchestrator->>SkillSynthesizer: Submit task summary with tool calls and turns
    SkillSynthesizer->>SkillSynthesizer: Check threshold and duplicate skills
    SkillSynthesizer->>LLMProvider: Generate reusable skill document
    LLMProvider-->>SkillSynthesizer: Return proposed skill content
    SkillSynthesizer-->>User: Ask for save confirmation
    User->>SkillSynthesizer: Approve save
    SkillSynthesizer->>SkillsStorage: Atomic write skill file and update lock hash

Loading

---

Generated by CodeAnt AI

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[codeant-ai]

Suggestion: Forecaster numeric settings are unchecked casts, so invalid values can become NaN; this silently disables intercept/auto-deny comparisons (score >= NaN is always false), weakening runtime risk controls. [logic error]

Severity Level: Critical 🚨

- ❌ Session risk interception can silently never trigger.
- ⚠️ Session auto-deny path may be bypassed by misconfiguration.

Suggested change

	interceptThreshold: (forecasterConfig['intercept_threshold'] as number) ?? 72,
	autoDenyThreshold: (forecasterConfig['auto_deny_threshold'] as number) ?? 88,
	maxEvents: (forecasterConfig['max_events'] as number) ?? 50,
	interceptThreshold: (typeof forecasterConfig['intercept_threshold'] === 'number' && Number.isFinite(forecasterConfig['intercept_threshold']))
	? forecasterConfig['intercept_threshold']
	: 72,
	autoDenyThreshold: (typeof forecasterConfig['auto_deny_threshold'] === 'number' && Number.isFinite(forecasterConfig['auto_deny_threshold']))
	? forecasterConfig['auto_deny_threshold']
	: 88,
	maxEvents: (typeof forecasterConfig['max_events'] === 'number' && Number.isFinite(forecasterConfig['max_events']))
	? forecasterConfig['max_events']
	: 50,

Steps of Reproduction ✅

1. Add malformed `risk_forecaster` values in config (e.g., strings for
`intercept_threshold`/`auto_deny_threshold`); this section is not type-validated by
`validateConfig` (`src/config/defaults.ts:173-231`).

2. Start daemon; `initGlobalForecaster` receives unchecked values from
`src/cli/daemon.ts:156-163`.

3. Run a task that executes tools: `IrreversibilityScorerHook` records session risk and
checks forecaster decisions at `src/hooks/built-in/irreversibility-scorer.ts:102-119`.

4. `MemoryRiskForecaster.shouldIntercept/shouldAutoDeny` compare composite score with
thresholds (`src/core/memory-risk-forecaster.ts:176-185`); invalid threshold values coerce
to `NaN`, making comparisons false and suppressing intended session-risk blocking.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/cli/daemon.ts
**Line:** 161:163
**Comment:**
	*Logic Error: Forecaster numeric settings are unchecked casts, so invalid values can become `NaN`; this silently disables intercept/auto-deny comparisons (`score >= NaN` is always false), weakening runtime risk controls.

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]

Suggestion: checkPlaintextSecrets calls fs.readdirSync without error handling, so unreadable or invalid audit directories throw and crash the audit flow instead of returning a structured check result. Wrap directory listing in try/catch and return a FAIL check when the directory cannot be scanned. [possible bug]

Severity Level: Major ⚠️

- ❌ Daemon startup aborts before structured security audit output.
- ⚠️ `zora security` can terminate on unreadable directories.

Suggested change

	const tomlFiles = fs.readdirSync(zoraDir).filter((f: string) => f.endsWith('.toml'));
	let tomlFiles: string[];
	try {
	tomlFiles = fs.readdirSync(zoraDir).filter((f: string) => f.endsWith('.toml'));
	} catch {
	return [{
	id: 'SECRET-PLAINTEXT-READDIR',
	label: 'No plaintext secrets in *.toml',
	severity: 'FAIL',
	message: `Cannot read ${zoraDir} to scan for plaintext secrets`,
	fixable: false,
	}];
	}

Steps of Reproduction ✅

1. Start daemon flow (`zora-agent start`), which runs `main()` in `src/cli/daemon.ts` and
calls `runSecurityAuditSilent({ zoraDir: configDir })` at line 114.

2. Ensure `configDir` exists but is unreadable (daemon chooses project/global at
`src/cli/daemon.ts:107`; e.g., restrictive permissions on that directory).

3. `buildReport()` in `src/cli/security-commands.ts` calls `checkPlaintextSecrets()` at
line 362, which executes `fs.readdirSync(zoraDir)` at line 163 without try/catch.

4. `readdirSync` throws (EACCES), promise rejects, and daemon exits via `main().catch`
fatal path at `src/cli/daemon.ts:327-329` instead of returning structured check results.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/cli/security-commands.ts
**Line:** 163:163
**Comment:**
	*Possible Bug: `checkPlaintextSecrets` calls `fs.readdirSync` without error handling, so unreadable or invalid audit directories throw and crash the audit flow instead of returning a structured check result. Wrap directory listing in `try/catch` and return a FAIL check when the directory cannot be scanned.

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]

Suggestion: The Signal policy file path is checked at the wrong location (<zoraDir>/channel-policy.toml), but runtime code loads it from <zoraDir>/config/channel-policy.toml; this causes false warnings and inconsistent behavior with the daemon. Point the audit check to the same config/ path used by runtime. [logic error]

Severity Level: Major ⚠️

- ❌ Signal policy warnings can be false positives.
- ⚠️ Daemon security logs become misleading for operators.

Suggested change

	const policyFile = path.join(zoraDir, 'channel-policy.toml');
	const policyFile = path.join(zoraDir, 'config', 'channel-policy.toml');

Steps of Reproduction ✅

1. Configure Signal per docs: `docs/SIGNAL_CHANNEL_SETUP.md:104` copies to
`config/channel-policy.toml`; docs also state boot checks this path at line 141.

2. Runtime code confirms same path: daemon uses `path.join(configDir, 'config',
'channel-policy.toml')` at `src/cli/daemon.ts:218`; orchestrator uses it at
`src/orchestrator/orchestrator.ts:480`.

3. Run `zora-agent security` (registered in `src/cli/index.ts:502`, command action in
`src/cli/security-commands.ts:453-460`).

4. `checkSignalPolicyFile()` tests `path.join(zoraDir, 'channel-policy.toml')` at line
327, so it can warn missing even when runtime-valid `config/channel-policy.toml` exists.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/cli/security-commands.ts
**Line:** 327:327
**Comment:**
	*Logic Error: The Signal policy file path is checked at the wrong location (`<zoraDir>/channel-policy.toml`), but runtime code loads it from `<zoraDir>/config/channel-policy.toml`; this causes false warnings and inconsistent behavior with the daemon. Point the audit check to the same `config/` path used by runtime.

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]

Suggestion: In project mode, the audit only checks policy.toml under the selected zoraDir, but policy resolution always depends on global ~/.zora/policy.toml too; this leaves the required global policy file unaudited. Add a separate permission check for the global policy when it differs from the local path. [security]

Severity Level: Critical 🚨

- ❌ Global required policy file can bypass permission audit.
- ⚠️ Security gate may pass with insecure global policy.

Suggested change

	checks.push(checkFilePerm(path.join(zoraDir, 'policy.toml'), 0o600, fix));
	const localPolicyPath = path.join(zoraDir, 'policy.toml');
	checks.push(checkFilePerm(localPolicyPath, 0o600, fix));
	const globalPolicyPath = path.join(os.homedir(), '.zora', 'policy.toml');
	if (path.resolve(globalPolicyPath) !== path.resolve(localPolicyPath)) {
	checks.push(checkFilePerm(globalPolicyPath, 0o600, fix));
	}

Steps of Reproduction ✅

1. In project mode, daemon chooses project `.zora` as audit root (`src/cli/daemon.ts:107`)
and passes it into `runSecurityAuditSilent` (`src/cli/daemon.ts:114`).

2. `buildReport()` only checks `${zoraDir}/policy.toml` permissions at
`src/cli/security-commands.ts:357-358`.

3. Policy resolution always requires and loads global `~/.zora/policy.toml` at
`src/config/policy-loader.ts:102` and `:107`, optionally overlaying project policy
(`:104`).

4. Therefore, when project `.zora` exists, global required policy file permissions are not
audited, creating a verified security coverage gap.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/cli/security-commands.ts
**Line:** 358:358
**Comment:**
	*Security: In project mode, the audit only checks `policy.toml` under the selected `zoraDir`, but policy resolution always depends on global `~/.zora/policy.toml` too; this leaves the required global policy file unaudited. Add a separate permission check for the global policy when it differs from the local path.

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]

Suggestion: thresholds is set to undefined when the global policy omits [actions.thresholds], but resolvePolicy() deep-merges raw project policy over this parsed object. If a project then defines only one threshold field (for example warn), the merged object becomes partial and flag/auto_deny stay undefined, so high-risk checks in the scorer silently stop triggering. Always materialize threshold defaults during parsing so deep-merge keeps a complete threshold object. [security]

Severity Level: Critical 🚨

- ❌ Irreversibility scorer can skip approval_required decisions.
- ❌ Auto-deny path may not trigger risky tool calls.
- ⚠️ Affects both `ask` and daemon policy resolution.

Suggested change

	thresholds: actPol?.['thresholds'] ? {
	warn: (actPol['thresholds'] as Record<string, number>)['warn'] ?? 40,
	flag: (actPol['thresholds'] as Record<string, number>)['flag'] ?? 65,
	auto_deny: (actPol['thresholds'] as Record<string, number>)['auto_deny'] ?? 95,
	} : undefined,
	thresholds: {
	warn: (actPol?.['thresholds'] as Record<string, number> | undefined)?.['warn'] ?? 40,
	flag: (actPol?.['thresholds'] as Record<string, number> | undefined)?.['flag'] ?? 65,
	auto_deny: (actPol?.['thresholds'] as Record<string, number> | undefined)?.['auto_deny'] ?? 95,
	},

Steps of Reproduction ✅

1. Initialize normal policy setup via `zora-agent init`; generated presets in
`src/cli/presets.ts:27-31,64-68,101-105,141-145` define `[actions]` but no `thresholds`,
so global `~/.zora/policy.toml` typically omits thresholds.

2. Run `zora-agent ask ...` (CLI entrypoint at `src/cli/index.ts:46-70`), which calls
`resolvePolicy()` (`src/cli/index.ts:23`, `src/config/policy-loader.ts:98-129`) and
deep-merges raw project policy over parsed global policy
(`src/config/policy-loader.ts:121-124`).

3. Add project override `.zora/policy.toml` with partial `[actions.thresholds]` (e.g.,
only `warn`); because parser currently returns `thresholds: undefined` when missing
globally (`src/config/policy-loader.ts:59-63`), merge keeps a partial object.

4. During boot, orchestrator passes that partial object directly
(`src/orchestrator/orchestrator.ts:459-460`) into `IrreversibilityScorerHook`; checks at
`src/hooks/built-in/irreversibility-scorer.ts:74` and `:90` compare against `undefined`,
so `auto_deny`/`flag` branches never trigger for high scores.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/config/policy-loader.ts
**Line:** 59:63
**Comment:**
	*Security: `thresholds` is set to `undefined` when the global policy omits `[actions.thresholds]`, but `resolvePolicy()` deep-merges raw project policy over this parsed object. If a project then defines only one threshold field (for example `warn`), the merged object becomes partial and `flag`/`auto_deny` stay `undefined`, so high-risk checks in the scorer silently stop triggering. Always materialize threshold defaults during parsing so deep-merge keeps a complete threshold object.

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]

Suggestion: The synthesis loop overwrites streamed text with done.content.text, but some providers emit a generic done message instead of final content. This causes valid generated markdown to be replaced with a placeholder string and then rejected as invalid frontmatter. Only use done text as a fallback when no streamed text was collected. [logic error]

Severity Level: Critical 🚨

- ❌ Gemini-backed skill synthesis drops valid generated SKILL.md.
- ⚠️ Autonomous skill generation silently skips after threshold hit.

Suggested change

	if (content.text) accumulated = content.text;
	if (content.text && accumulated.length === 0) accumulated = content.text;

Steps of Reproduction ✅

1. Run the `ask` flow (`src/cli/index.ts:178`) which creates `Orchestrator` and calls
`submitTask` (`src/cli/index.ts:188`).

2. In `submitTask`, selected provider is injected into skill synthesis
(`src/orchestrator/orchestrator.ts:831`) and post-task generation is triggered
(`src/orchestrator/orchestrator.ts:843`), once threshold is met.

3. During `synthesize()`, streamed text is appended
(`src/skills/SkillSynthesizer.ts:167`), but Gemini emits a generic done payload `content:
{ text: 'Gemini task complete' }` (`src/providers/gemini-provider.ts:300-304`).

4. Current done-handler overwrites accumulated markdown
(`src/skills/SkillSynthesizer.ts:171`), then frontmatter parse/slug validation fails and
generation is skipped (`src/skills/SkillSynthesizer.ts:248-251`).

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/skills/SkillSynthesizer.ts
**Line:** 171:171
**Comment:**
	*Logic Error: The synthesis loop overwrites streamed text with `done.content.text`, but some providers emit a generic done message instead of final content. This causes valid generated markdown to be replaced with a placeholder string and then rejected as invalid frontmatter. Only use done text as a fallback when no streamed text was collected.

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]

Suggestion: Using a fixed temp filename (SKILL.md.tmp) creates a race condition when two sessions write the same skill concurrently, causing collisions, rename failures, or cross-session clobbering. Use a unique temp file name per write operation. [race condition]

Severity Level: Major ⚠️

- ❌ Concurrent identical-skill saves can fail unpredictably.
- ⚠️ Final SKILL.md may reflect wrong racing session.

Suggested change

	const tmp = `${dest}.tmp`;
	await fs.writeFile(tmp, content, 'utf-8');
	const tmp = `${dest}.${process.pid}.${Date.now()}.tmp`;
	await fs.writeFile(tmp, content, { encoding: 'utf-8', flag: 'wx' });

Steps of Reproduction ✅

1. Use daemon/dashboard task path where concurrent jobs are allowed: `POST /api/task`
(`src/dashboard/server.ts:346`) and background submit without await
(`src/cli/daemon.ts:198-202`).

2. Trigger two complex sessions concurrently so both hit skill generation
(`src/orchestrator/orchestrator.ts:843`) and produce the same slug.

3. Both `writeSkill()` calls target the same destination and identical tmp path
(`src/skills/SkillSynthesizer.ts:191-193`).

4. One write/rename can clobber or race the other (`src/skills/SkillSynthesizer.ts:194`),
causing nondeterministic failures or wrong final content for that skill.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/skills/SkillSynthesizer.ts
**Line:** 192:193
**Comment:**
	*Race Condition: Using a fixed temp filename (`SKILL.md.tmp`) creates a race condition when two sessions write the same skill concurrently, causing collisions, rename failures, or cross-session clobbering. Use a unique temp file name per write operation.

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]

Suggestion: Auto-confirming when stdin is not a TTY bypasses the human-in-the-loop gate and writes files without explicit user consent. In non-interactive environments, this should default to not saving unless skipConfirmation is explicitly enabled. [security]

Severity Level: Critical 🚨

- ❌ HITL confirmation policy bypassed in daemon/background execution.
- ⚠️ Skills may persist from API tasks without consent.

Suggested change

	if (this._skipConfirmation || !process.stdin.isTTY) {
	return true;
	if (this._skipConfirmation) {
	return true;
	}
	if (!process.stdin.isTTY) {
	log.warn({ skill: name }, 'Non-interactive session; skipping skill save without explicit confirmation');
	return false;

Steps of Reproduction ✅

1. Start daemon mode (`src/cli/daemon.ts:20`) and submit tasks through dashboard endpoint
`POST /api/task` (`src/dashboard/server.ts:346`).

2. Daemon submits jobs in background via `orchestrator.submitTask(...)`
(`src/cli/daemon.ts:200`) without interactive CLI ask loop.

3. After task completion, orchestrator calls `maybeGenerateSkill(...)`
(`src/orchestrator/orchestrator.ts:843`), which reaches `_confirmWithUser(...)`
(`src/skills/SkillSynthesizer.ts:275`).

4. Current gate auto-returns `true` whenever stdin is non-TTY
(`src/skills/SkillSynthesizer.ts:276-278`), so `writeSkill` proceeds
(`src/skills/SkillSynthesizer.ts:185-195`) without explicit user approval.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/skills/SkillSynthesizer.ts
**Line:** 276:277
**Comment:**
	*Security: Auto-confirming when stdin is not a TTY bypasses the human-in-the-loop gate and writes files without explicit user consent. In non-interactive environments, this should default to not saving unless `skipConfirmation` is explicitly enabled.

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]

Suggestion: save always uses the same temporary filename, so concurrent saves can collide (.tmp overwritten/renamed by another writer), causing intermittent write failures or corrupted update flow. Use a unique temp filename per save operation before rename. [race condition]

Severity Level: Major ⚠️

- ⚠️ Concurrent lock saves can intermittently fail rename.
- ⚠️ Manifest writes become nondeterministic under parallel tasks.
- ⚠️ Skill integrity tracking reliability degrades in daemon mode.

Suggested change

	const tmp = `${this._lockPath}.tmp`;
	const tmp = `${this._lockPath}.${process.pid}.${Date.now()}.${crypto.randomUUID()}.tmp`;

Steps of Reproduction ✅

1. Trigger two concurrent autonomous skill writes through the same daemon flow: `POST
/api/task` (`src/dashboard/server.ts:346`), background `orchestrator.submitTask()`
(`src/cli/daemon.ts:200`), then `maybeGenerateSkill()`
(`src/orchestrator/orchestrator.ts:842-847`).

2. Both sessions call `SkillsLock.save()` via `update()`
(`src/skills/SkillsLock.ts:78-81`), entering save logic at lines 55-62.

3. Each writer uses identical temp path `skills.lock.json.tmp`
(`src/skills/SkillsLock.ts:59`), so writes/renames race on one temp file.

4. One writer can rename the shared tmp first; the other rename then sees missing tmp (or
overwrites prior data), producing intermittent lock update failure or lost manifest
content.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/skills/SkillsLock.ts
**Line:** 59:59
**Comment:**
	*Race Condition: `save` always uses the same temporary filename, so concurrent saves can collide (`.tmp` overwritten/renamed by another writer), causing intermittent write failures or corrupted update flow. Use a unique temp filename per save operation before rename.

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]

Suggestion: update does a read-modify-write without any synchronization, so concurrent updates can overwrite each other and silently drop previously written skill hashes. Serialize updates per lock file path so each write sees the latest manifest state before saving. [race condition]

Severity Level: Major ⚠️

- ⚠️ skills.lock.json can lose hashes for generated skills.
- ⚠️ Integrity manifest becomes incomplete after concurrent skill saves.
- ⚠️ Future hash verification may fail for missing entries.

Suggested change

	const data = await this.load();
	data[name] = hashContent(content);
	await this.save(data);
	const ctor = this.constructor as typeof SkillsLock & {
	_writeQueues?: Map<string, Promise<void>>;
	};
	ctor._writeQueues ??= new Map<string, Promise<void>>();
	const previous = ctor._writeQueues.get(this._lockPath) ?? Promise.resolve();
	const next = previous.then(async () => {
	const data = await this.load();
	data[name] = hashContent(content);
	await this.save(data);
	});
	ctor._writeQueues.set(this._lockPath, next.catch(() => {}));
	await next;

Steps of Reproduction ✅

1. Start daemon mode where tasks are accepted via `POST /api/task`
(`src/dashboard/server.ts:346`, calls `submitTask(prompt.trim())` at line 359).

2. Send two complex task requests quickly; daemon submits both in background without
awaiting completion (`src/cli/daemon.ts:200`), so `orchestrator.submitTask()` runs
concurrently.

3. For both jobs, post-task hook triggers autonomous synthesis
(`src/orchestrator/orchestrator.ts:842-847`) and each path reaches
`SkillSynthesizer.updateLockFile()` (`src/skills/SkillSynthesizer.ts:204-205`) →
`SkillsLock.update()` (`src/skills/SkillsLock.ts:78-82`).

4. Both `update()` calls read same old manifest (`load()` line 79), each mutates different
key, then last `save()` wins, dropping the other key; resulting `skills.lock.json` misses
one generated skill hash.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** src/skills/SkillsLock.ts
**Line:** 79:81
**Comment:**
	*Race Condition: `update` does a read-modify-write without any synchronization, so concurrent updates can overwrite each other and silently drop previously written skill hashes. Serialize updates per lock file path so each write sees the latest manifest state before saving.

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 finished reviewing your PR.

[coderabbitai]

Actionable comments posted: 19

🧹 Nitpick comments (8)

src/core/approval-queue.ts (2)

190-202: Unbounded recursion risk in token generation.

If the _pending map becomes large (approaching the token space of 30^4 = 810,000 combinations), the recursive call at Line 199 could cause a stack overflow. While unlikely in practice, adding a retry limit would be defensive.

🛡️ Suggested bounded retry

-  private _generateToken(): string {
+  private _generateToken(attempt = 0): string {
+    if (attempt > 10) {
+      throw new Error('Failed to generate unique approval token after 10 attempts');
+    }
     const chars = 'ABCDEFGHJKLMNPQRSTUVWXYZ23456789';
     const bytes = crypto.randomBytes(4);
     let token = 'ZORA-';
     for (let i = 0; i < 4; i++) {
       token += chars[bytes[i]! % chars.length];
     }
     if (this._pending.has(token)) {
-      return this._generateToken();
+      return this._generateToken(attempt + 1);
     }
     return token;
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/approval-queue.ts` around lines 190 - 202, The _generateToken method
currently uses unbounded recursion when a generated token collides with
this._pending, risking stack overflow; change it to an iterative retry loop with
a configurable maxAttempts (e.g., 10_000 or a smaller sensible default) inside
_generateToken to attempt fresh tokens up to that limit and, if exhausted, throw
a clear error; keep the same token format and randomness logic but remove the
recursive call to this._generateToken(), reference the _generateToken function
and the _pending map when implementing the retry loop and error path.

---

126-141: Magic number for blanket threshold default.

Line 127 uses flagThreshold ?? 80 as a fallback, but this should ideally align with the policy's default flag threshold to maintain consistency. Consider exporting this as a constant or documenting the relationship.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/approval-queue.ts` around lines 126 - 141, Replace the magic literal
80 used as the fallback for flag threshold by introducing and
importing/exporting a named constant (e.g., DEFAULT_FLAG_THRESHOLD) and use this
constant in the expression where blanketMaxScore is assigned (currently using
this._config.flagThreshold ?? 80); update references to _blanketMaxScore and log
messages that mention the threshold to use the constant or its value, and ensure
the constant’s name/definition documents that it matches the policy default so
the relationship is explicit.

src/cli/security-commands.ts (2)

178-199: Confusing label for FAIL results in plaintext secret detection.

When a plaintext secret is found (FAIL), the label is set to "No plaintext secrets in ${file}" (Line 180), which contradicts the actual finding. This makes the audit output confusing.

♻️ Suggested fix

           results.push({
             id: `SECRET-PLAINTEXT-${file.replace(/\./g, '-').toUpperCase()}-L${lineNum}`,
-            label: `No plaintext secrets in ${file}`,
+            label: `Plaintext secret in ${file}`,
             severity: 'FAIL',
             message: `Plaintext ${keyName} found — move to env var ${envVar}`,
             location: `${file}:${lineNum}`,
             fixable: false,
           });

Then update the PASS condition at Line 191:

-    if (!results.some(r => r.label === `No plaintext secrets in ${file}`)) {
+    if (!results.some(r => r.id.startsWith(`SECRET-PLAINTEXT-${file.replace(/\./g, '-').toUpperCase()}`))) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cli/security-commands.ts` around lines 178 - 199, The FAIL case pushes a
result object into results with a contradictory label "No plaintext secrets in
${file}" when a plaintext secret is detected; update the label for the failing
branch (the object created when you push id
`SECRET-PLAINTEXT-${file.replace(/\./g, '-').toUpperCase()}-L${lineNum}`) to
reflect the failure (e.g., "Plaintext secret in ${file}") and leave the PASS
branch (the block that pushes id `SECRET-PLAINTEXT-${file.replace(/\./g,
'-').toUpperCase()}`) unchanged so that results.some(...) correctly finds PASS
entries by the original "No plaintext secrets in ${file}" label.

---

153-158: Minor: Comment doesn't match regex groups.

The comment states "group 2 = quote char, group 3 = value", but the patterns only have two capturing groups: group 1 is the key name, group 2 is the value. The quote characters are matched but not captured.

📝 Suggested comment fix

 // Patterns that indicate a plaintext secret. Each pattern captures:
-//   group 1 = key name, group 2 = quote char, group 3 = value
+//   group 1 = key name, group 2 = value
 const SECRET_PATTERNS: RegExp[] = [

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cli/security-commands.ts` around lines 153 - 158, The comment above
SECRET_PATTERNS is inaccurate: the regexes currently define two capturing groups
(group 1 = key name, group 2 = value) and do not capture a quote character;
update the comment to reflect the actual groups or alter the regexes to add a
capture for the quote if you intended to capture it. Specifically, edit the
comment near SECRET_PATTERNS (and/or adjust the RegExp literals) so it correctly
documents that group 1 is the key name and group 2 is the value (or, if you
prefer capturing the quote, add a third capture around the quote char in both
patterns and then document group 2 = quote char, group 3 = value).

src/skills/SkillsLock.ts (2)

78-82: Potential race condition in concurrent update() calls.

The load → modify → save pattern in update() isn't atomic. If two skill syntheses complete simultaneously, one hash entry could be lost. Given that skill synthesis is relatively rare and the impact is minor (the hash would be added on the next write), this is low priority but worth noting.

Optional: File locking for concurrent safety

Consider using a file lock (e.g., proper-lockfile package) if concurrent skill generation becomes more common:

import lockfile from 'proper-lockfile';

async update(name: string, content: string): Promise<void> {
  const release = await lockfile.lock(this._lockPath, { retries: 3 });
  try {
    const data = await this.load();
    data[name] = hashContent(content);
    await this.save(data);
  } finally {
    await release();
  }
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/skills/SkillsLock.ts` around lines 78 - 82, The update() method uses a
non-atomic load → modify → save sequence causing a race when concurrent updates
occur; fix by adding file locking around the critical section: import lockfile
from 'proper-lockfile', acquire a lock on the lock path (use this._lockPath)
with retries before calling load(), then set data[name] = hashContent(content)
and call save(), and finally always release the lock in a finally block to
ensure the lock is freed even on error.

---

68-73: verify() method exists but is never called.

The verify() method provides skill integrity checking, but per the codebase analysis, it's not invoked anywhere—only update() is called from SkillSynthesizer.updateLockFile(). This means tampered skills would be loaded without verification.

Consider either:

1. Adding verification during skill loading (e.g., in SkillLoader)
2. Documenting this as a planned future enhancement
3. Removing the method if it's not part of the current scope

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/skills/SkillsLock.ts` around lines 68 - 73, The verify() method in
SkillsLock is never used; update the skill-loading path to perform integrity
checks by calling SkillsLock.verify(name, content) whenever a skill's content is
loaded (e.g., inside the SkillLoader loadSkill method or at the start of
SkillSynthesizer.updateLockFile before accepting content), and if verify returns
false, reject the load/update with a clear error or log and do not load the
tampered skill; ensure you reference SkillsLock.verify and the existing
SkillsLock.load()/update() usage so the change integrates with current lockfile
handling.

src/tools/subagent-tool.ts (1)

71-82: Cooldown enforcement silently skipped when no cooldown is configured.

When cooldown parameter is undefined and getGlobalCooldown() returns null (which happens in CLI ask mode before initGlobalCooldown() is called), the entire cooldown check is bypassed. This is likely intentional for one-shot CLI usage, but consider adding a debug log to make this behavior observable:

Optional: Add debug logging

       const activeCooldown = cooldown ?? getGlobalCooldown();
-      if (activeCooldown) {
+      if (!activeCooldown) {
+        log.debug({ subagent: name }, 'No cooldown configured — skipping enforcement');
+      } else {
         const check = await activeCooldown.checkAndEnforce(name);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tools/subagent-tool.ts` around lines 71 - 82, When no cooldown is
provided and getGlobalCooldown() returns null the cooldown branch is skipped
silently; update the logic around activeCooldown (the cooldown ??
getGlobalCooldown() assignment) to emit a debug-level log when activeCooldown is
null (e.g., "no cooldown configured — skipping check for agent {name}") so the
behavior is observable in CLI ask mode before initGlobalCooldown() is called;
use the module's existing logger (e.g., processLogger.debug or logger.debug) if
available, otherwise console.debug, and keep the call next to where
checkAndEnforce(name) would be invoked.

tests/unit/skills/SkillSynthesizer.test.ts (1)

235-251: Add one happy-path maybeGenerateSkill test.

This suite only exercises the two early returns. A stub provider plus a confirmation spy here would cover the branch that synthesizes content, hits the confirmation gate, and writes SKILL.md—the part most likely to regress.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/skills/SkillSynthesizer.test.ts` around lines 235 - 251, Add a
happy-path unit test that exercises the branch which actually synthesizes a
skill: create a stub implementation of the provider expected by SkillSynthesizer
(implementing the same interface the synthesizer uses), instantiate
SkillSynthesizer with that stub and skipConfirmation disabled (or with a spyable
confirmation handler), call maybeGenerateSkill with values that meet the
threshold, assert the confirmation prompt was invoked (spy called), and assert
that a SKILL.md file was created with the synthesized content (or that the
synthesizer's write method was called) to validate successful synthesis and
write-out; reference SkillSynthesizer, maybeGenerateSkill, and SKILL.md when
locating where to hook the stub and assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@CHANGELOG.md`:
- Line 5: Update the release header "## [0.9.1] — 2026-03-12" in CHANGELOG.md so
the date matches the actual PR/release timing: either change the date to the
PR/release date (e.g., 2026-03-19) or replace the date with a placeholder like
"[Unreleased]" until the release is finalized; ensure the header text "##
[0.9.1] — 2026-03-12" is the one you edit.

In `@docs/advanced/security-runtime.md`:
- Around line 168-182: The TOML example lists knobs that the runtime doesn't
read; update docs/advanced/security-runtime.md to only document the actual
forecaster settings exposed by src/core/memory-risk-forecaster.ts:
interceptThreshold, autoDenyThreshold, maxEvents, and stateDir (remove drift_*,
salami_*, creep_*, and composite_flag entries), and provide the corresponding
TOML keys and brief descriptions for those four settings so the example matches
the runtime behavior.
- Around line 243-250: The docs show a per-project "flag" setting under
[policy.actions] but src/core/project-policy.ts only parses
max_irreversibility_score, so the "flag = 45" example is a no-op; either
remove/replace the unsupported "flag" example in docs or implement support for
it by adding parsing and handling in the ProjectPolicy loader
(src/core/project-policy.ts) so it reads "flag" from [policy.actions] and
applies the per-project flag threshold consistently with existing logic for
max_irreversibility_score; update any related tests or documentation to reflect
the chosen approach.
- Around line 194-213: Docs and config keys are inconsistent with the
implementation in src/core/agent-cooldown.ts: the code uses level1Threshold,
level2Threshold, shutdownThreshold and only applies a delay at level1 (defaults
3,6,10), while the docs show 0/3/5/8 and different TOML keys (throttle_after,
restrict_after, suspend_after, reset_hours); update the documentation (or the
implementation) so they match: either change the markdown table and TOML keys to
use level1Threshold/level2Threshold/shutdownThreshold and document that only
level1 applies a delay, or change src/core/agent-cooldown.ts to read the
documented keys (throttle_after, restrict_after, suspend_after, reset_hours) and
implement thresholds 3,5,8 and a delay at the throttled level; reference
src/core/agent-cooldown.ts and the config keys (level1Threshold,
level2Threshold, shutdownThreshold, throttle_after, restrict_after,
suspend_after, reset_hours) when making the edit so the runtime and docs are
aligned.

In `@README.md`:
- Around line 145-152: The README has unlabeled fenced code blocks (e.g., the
block containing "⚠️ Zora Action Approval Required" and similar blocks at the
other noted occurrences) which trigger MD040; fix this by adding a language tag
to each fence (use "text" for plain output blocks or "bash" for command
snippets) so the fences become ```text or ```bash as appropriate, leaving the
inner content unchanged.

In `@src/cli/daemon.ts`:
- Around line 167-176: ApprovalQueue is being enabled from config but may never
get a send handler (setSendHandler) which causes silent auto-denies; after
reading approvalConfig (and before creating/using ApprovalQueue) detect whether
the channel/transport layer can actually wire an approval send handler (via
ChannelManager or equivalent check) and if it cannot, override the config to
force enabled:false when instantiating ApprovalQueue (i.e., merge in enabled:
false into the options from DEFAULT_APPROVAL_CONFIG/approvalConfig) and emit a
WARN-level log explaining approval was disabled due to missing transport;
alternatively, block startup if you prefer—ensure any code that sets up
ApprovalQueue (the ApprovalQueue constructor usage and
DEFAULT_APPROVAL_CONFIG/approvalConfig handling) consults ChannelManager ability
to register setSendHandler and acts accordingly.

In `@src/cli/security-commands.ts`:
- Around line 243-250: The ZORA_BIND_HOST validation in the block checking
bindHost (variable bindHost) omits IPv6 localhost (::1), causing a false FAIL;
update the condition in the ZORA_BIND_HOST check so it treats '::1' as a
localhost equivalent (same as 'localhost' and '127.0.0.1')—mirror the logic used
in checkAgentBusUrl or add '::1' to the allowed values, ensuring the returned
failure is only for non-localhost addresses while keeping id, label, severity,
message, and fixable fields unchanged.

In `@src/core/agent-cooldown.ts`:
- Around line 166-169: The current _reputationPath loses uniqueness by
sanitizing agentId; replace the lossy sanitization with a collision-free
encoding: compute a stable hash (e.g., SHA-256 hex or base64url) of agentId
using Node's crypto and use that hash as (or prefix/suffix to) the filename so
distinct agentIds always map to distinct files; update _reputationPath to derive
the hashedName from agentId and join it with this._reputationDir (add a crypto
import if missing) and keep an optional short sanitized label for readability if
desired.
- Around line 153-163: The save/load are vulnerable to partial writes resetting
AgentReputation; change _save(agentId, rep) to perform an atomic replace: write
JSON to a temp file in the same directory (e.g., this._reputationPath(agentId) +
".tmp" + process.pid), fsync the file descriptor, then rename (fs.renameSync)
the temp file to the real path to atomically replace the file; ensure the target
directory exists and set file mode as needed. Update _load(agentId) so
JSON.parse failures are not silently treated as a fresh reputation: on parse
error, attempt to read a fallback (the temp or .bak file) and if that also
fails, log and rethrow or return the last known safe state rather than always
returning this._defaultReputation(agentId). Use the existing method names _save,
_load, _reputationPath and the AgentReputation type to locate the changes.

In `@src/core/memory-risk-forecaster.ts`:
- Around line 218-231: The current _save(sessionId, state) writes directly to
filePath which can produce torn writes and _loadFromDisk(sessionId) returns null
on any parse error, allowing sessions to "fail open"; fix by making writes
atomic and making loads return a safe fallback: change _save to write JSON to a
temporary file (e.g., filePath + '.tmp'), fsync/close it and then fs.renameSync
to atomically replace the real file (so _statePath-produced files are never
half-written), and change _loadFromDisk to, on JSON parse error, attempt to
recover by returning this._cache.get(sessionId) if present (or a validated
minimal SessionRiskState) instead of null; also validate the parsed object
conforms to SessionRiskState before returning. Ensure you reference and update
the methods _save, _loadFromDisk, _statePath, and the in-memory _cache and
SessionRiskState checks.
- Around line 81-89: computeSalami currently treats each dangerous sequence as
an unordered set (using recentCategories.includes) which causes out-of-order
matches; change it to require an ordered, contiguous match. In computeSalami,
for each seq in DANGEROUS_SEQUENCES, slide a window of length seq.length over
recentCategories and check equality element-by-element (e.g., for i from 0 to
recentCategories.length - seq.length, verify seq.every((cat, j) =>
recentCategories[i + j] === cat)); only increment score when a contiguous
ordered match is found; keep the final Math.min(100, score) behavior.
- Around line 141-144: The baseline is being set prematurely after the first
event; change the condition so baselineCategories is initialized only once after
at least three events are recorded. In the block that currently checks
state.baselineCategories.length === 0 && state.events.length >= 1 (inside the
record()/event-processing logic), update it to require state.events.length >= 3
and then set state.baselineCategories = state.events.slice(0, 3).map(e =>
e.actionCategory) so the baseline truly reflects the first three actions and
remains frozen thereafter.

In `@src/core/project-policy.ts`:
- Around line 113-119: The current logic lets a child widen the parent's
allowlist by setting tools.allowed to childAllowed or dropping it; change this
so the resulting allowed list is the parent's allowlist further restricted by
the child's allowed (if present) instead of replaced. Specifically, compute
allowed as: if child.tools.allowed is defined then parent.tools.allowed.filter(t
=> child.tools.allowed.includes(t)) else parent.tools.allowed; keep mergedDenied
as [...new Set([...parent.tools.denied, ...child.tools.denied])] and return
tools.allowed using this intersection to ensure a child can only narrow the
parent's allowed set.

In `@src/hooks/built-in/irreversibility-scorer.ts`:
- Around line 63-70: The code calls getAgentPolicy(ctx.jobId) but project
policies are keyed by agent name, so replace the lookup to use the agent
identifier that matches registration (e.g., ctx.agentName or ctx.agentId)
instead of ctx.jobId; update the call in the irreversibility scorer to
getAgentPolicy(ctx.agentName || ctx.agentId) (or thread an agentName through
ToolCallContext if missing) and keep the subsequent checkScoreLimit(...) and
logging logic unchanged.
- Around line 25-50: toolToAction() is missing mappings for destructive shell
and specific messaging tools so keys like shell_exec_destructive,
send_signal_message, and send_telegram_message never get scored correctly;
update the mapping inside toolToAction to return "shell_exec_destructive" for
destructive shell variants (e.g., any destructive bash/execute_bash/run_command
tool name used in your codebase) and add explicit entries mapping
"send_signal_message" and "send_telegram_message" to "send_message" (or the
correct messaging action key used by DEFAULT_IRREVERSIBILITY_SCORES), then
run/adjust any tests that rely on those action keys so the flag/auto_deny gates
use the intended scores.

In `@src/orchestrator/orchestrator.ts`:
- Around line 816-828: The _skillTurns counter is being incremented on
AgentEvent.type === 'done' inside the _skillOnEvent wrapper, but 'done' fires
only once; change the event check to increment _skillTurns when event.type ===
'turn.end' (update both branches of the ternary where _skillOnEvent is defined)
so actual turns are counted; keep tracking of tool calls on event.type ===
'tool_call' and ensure options.onEvent(event) is still forwarded after updating
the conditional.

In `@src/skills/SkillSynthesizer.ts`:
- Around line 275-278: The _confirmWithUser method currently auto-approves when
process.stdin is not a TTY, which allows daemon/background runs to bypass
human-in-the-loop; change the logic in _confirmWithUser to "fail closed": if
this._skipConfirmation is true return true, otherwise if process.stdin.isTTY
proceed with interactive confirmation, but if not a TTY return false (do not
auto-approve); additionally add a clear TODO/comment and a hook or parameter for
injecting an out-of-band confirmer for daemon runs so callers (e.g.,
orchestrator) must explicitly provide a confirmer instead of relying on stdin
fallback.
- Around line 191-196: Before renaming the temp SKILL.md into place in
maybeGenerateSkill (where dest and tmp are used and updateLockFile(name,
content) is called), capture the existing SKILL.md (if present) by reading it
into a backup (e.g., readFile(dest) or rename to dest + '.bak'), then perform
the rename(tmp -> dest) and call updateLockFile; if updateLockFile throws, catch
the error and roll back by restoring the original SKILL.md from the backup (or
deleting the new dest and renaming the backup back), ensure any temporary files
(tmp and backup) are cleaned up, and rethrow or re-log the original error so
behavior remains consistent.

In `@src/tools/subagent-tool.ts`:
- Around line 84-91: The registered agent policy is never cleared after
delegation: after calling registerAgentPolicy(name, subagentPolicy) you must
ensure clearAgentPolicy(name) runs when the subagent finishes; wrap the
submitTask(...) invocation (the delegation/await that uses the policy) in a
try/finally and call clearAgentPolicy(name) in the finally block so the entry in
the global _policyRegistry is removed regardless of success or error; keep the
existing loadProjectPolicy(...) and its catch (no-op) intact and only register
the policy if loadProjectPolicy returned one.

---

Nitpick comments:
In `@src/cli/security-commands.ts`:
- Around line 178-199: The FAIL case pushes a result object into results with a
contradictory label "No plaintext secrets in ${file}" when a plaintext secret is
detected; update the label for the failing branch (the object created when you
push id `SECRET-PLAINTEXT-${file.replace(/\./g,
'-').toUpperCase()}-L${lineNum}`) to reflect the failure (e.g., "Plaintext
secret in ${file}") and leave the PASS branch (the block that pushes id
`SECRET-PLAINTEXT-${file.replace(/\./g, '-').toUpperCase()}`) unchanged so that
results.some(...) correctly finds PASS entries by the original "No plaintext
secrets in ${file}" label.
- Around line 153-158: The comment above SECRET_PATTERNS is inaccurate: the
regexes currently define two capturing groups (group 1 = key name, group 2 =
value) and do not capture a quote character; update the comment to reflect the
actual groups or alter the regexes to add a capture for the quote if you
intended to capture it. Specifically, edit the comment near SECRET_PATTERNS
(and/or adjust the RegExp literals) so it correctly documents that group 1 is
the key name and group 2 is the value (or, if you prefer capturing the quote,
add a third capture around the quote char in both patterns and then document
group 2 = quote char, group 3 = value).

In `@src/core/approval-queue.ts`:
- Around line 190-202: The _generateToken method currently uses unbounded
recursion when a generated token collides with this._pending, risking stack
overflow; change it to an iterative retry loop with a configurable maxAttempts
(e.g., 10_000 or a smaller sensible default) inside _generateToken to attempt
fresh tokens up to that limit and, if exhausted, throw a clear error; keep the
same token format and randomness logic but remove the recursive call to
this._generateToken(), reference the _generateToken function and the _pending
map when implementing the retry loop and error path.
- Around line 126-141: Replace the magic literal 80 used as the fallback for
flag threshold by introducing and importing/exporting a named constant (e.g.,
DEFAULT_FLAG_THRESHOLD) and use this constant in the expression where
blanketMaxScore is assigned (currently using this._config.flagThreshold ?? 80);
update references to _blanketMaxScore and log messages that mention the
threshold to use the constant or its value, and ensure the constant’s
name/definition documents that it matches the policy default so the relationship
is explicit.

In `@src/skills/SkillsLock.ts`:
- Around line 78-82: The update() method uses a non-atomic load → modify → save
sequence causing a race when concurrent updates occur; fix by adding file
locking around the critical section: import lockfile from 'proper-lockfile',
acquire a lock on the lock path (use this._lockPath) with retries before calling
load(), then set data[name] = hashContent(content) and call save(), and finally
always release the lock in a finally block to ensure the lock is freed even on
error.
- Around line 68-73: The verify() method in SkillsLock is never used; update the
skill-loading path to perform integrity checks by calling
SkillsLock.verify(name, content) whenever a skill's content is loaded (e.g.,
inside the SkillLoader loadSkill method or at the start of
SkillSynthesizer.updateLockFile before accepting content), and if verify returns
false, reject the load/update with a clear error or log and do not load the
tampered skill; ensure you reference SkillsLock.verify and the existing
SkillsLock.load()/update() usage so the change integrates with current lockfile
handling.

In `@src/tools/subagent-tool.ts`:
- Around line 71-82: When no cooldown is provided and getGlobalCooldown()
returns null the cooldown branch is skipped silently; update the logic around
activeCooldown (the cooldown ?? getGlobalCooldown() assignment) to emit a
debug-level log when activeCooldown is null (e.g., "no cooldown configured —
skipping check for agent {name}") so the behavior is observable in CLI ask mode
before initGlobalCooldown() is called; use the module's existing logger (e.g.,
processLogger.debug or logger.debug) if available, otherwise console.debug, and
keep the call next to where checkAndEnforce(name) would be invoked.

In `@tests/unit/skills/SkillSynthesizer.test.ts`:
- Around line 235-251: Add a happy-path unit test that exercises the branch
which actually synthesizes a skill: create a stub implementation of the provider
expected by SkillSynthesizer (implementing the same interface the synthesizer
uses), instantiate SkillSynthesizer with that stub and skipConfirmation disabled
(or with a spyable confirmation handler), call maybeGenerateSkill with values
that meet the threshold, assert the confirmation prompt was invoked (spy
called), and assert that a SKILL.md file was created with the synthesized
content (or that the synthesizer's write method was called) to validate
successful synthesis and write-out; reference SkillSynthesizer,
maybeGenerateSkill, and SKILL.md when locating where to hook the stub and
assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 29afc1c8-dbc0-4004-8a17-5cca7be6e7ab

📥 Commits

Reviewing files that changed from the base of the PR and between 26524e6 and 898265c.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (27)

• .claude/settings.json
• .gitignore
• CHANGELOG.md
• README.md
• docs/advanced/security-runtime.md
• node_modules
• package.json
• src/cli/daemon.ts
• src/cli/index.ts
• src/cli/security-commands.ts
• src/config/policy-loader.ts
• src/core/agent-cooldown.ts
• src/core/approval-queue.ts
• src/core/memory-risk-forecaster.ts
• src/core/project-policy.ts
• src/hooks/built-in/irreversibility-scorer.ts
• src/hooks/index.ts
• src/orchestrator/orchestrator.ts
• src/skills/SkillSynthesizer.ts
• src/skills/SkillsLock.ts
• src/skills/index.ts
• src/steering/telegram-gateway.ts
• src/templates/security-policy.toml.template
• src/tools/subagent-tool.ts
• src/types.ts
• tests/unit/memory/performance-benchmarks.test.ts
• tests/unit/skills/SkillSynthesizer.test.ts

💤 Files with no reviewable changes (2)

• .gitignore
• node_modules

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Changelog date precedes PR creation date.

The changelog entry is dated 2026-03-12, but the PR was created on 2026-03-19. Consider updating the date to reflect when this release actually ships, or use a placeholder like [Unreleased] until the release is finalized.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@CHANGELOG.md` at line 5, Update the release header "## [0.9.1] — 2026-03-12"
in CHANGELOG.md so the date matches the actual PR/release timing: either change
the date to the PR/release date (e.g., 2026-03-19) or replace the date with a
placeholder like "[Unreleased]" until the release is finalized; ensure the
header text "## [0.9.1] — 2026-03-12" is the one you edit.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Document only the forecaster knobs the runtime actually reads.

src/core/memory-risk-forecaster.ts only exposes interceptThreshold, autoDenyThreshold, maxEvents, and stateDir. The drift_*, salami_*, creep_*, and composite_flag settings shown here are currently ignored, so this example gives users a false sense that they tuned the forecaster.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/advanced/security-runtime.md` around lines 168 - 182, The TOML example
lists knobs that the runtime doesn't read; update
docs/advanced/security-runtime.md to only document the actual forecaster
settings exposed by src/core/memory-risk-forecaster.ts: interceptThreshold,
autoDenyThreshold, maxEvents, and stateDir (remove drift_*, salami_*, creep_*,
and composite_flag entries), and provide the corresponding TOML keys and brief
descriptions for those four settings so the example matches the runtime
behavior.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

The cooldown table and config keys are out of sync with src/core/agent-cooldown.ts.

The implementation defaults to level1Threshold: 3, level2Threshold: 6, shutdownThreshold: 10, and only applies a delay at level 1. The 3/5/8 behavior and throttle_after / restrict_after / suspend_after / reset_hours keys documented here do not match the code, so readers will configure fields the runtime never consumes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/advanced/security-runtime.md` around lines 194 - 213, Docs and config
keys are inconsistent with the implementation in src/core/agent-cooldown.ts: the
code uses level1Threshold, level2Threshold, shutdownThreshold and only applies a
delay at level1 (defaults 3,6,10), while the docs show 0/3/5/8 and different
TOML keys (throttle_after, restrict_after, suspend_after, reset_hours); update
the documentation (or the implementation) so they match: either change the
markdown table and TOML keys to use
level1Threshold/level2Threshold/shutdownThreshold and document that only level1
applies a delay, or change src/core/agent-cooldown.ts to read the documented
keys (throttle_after, restrict_after, suspend_after, reset_hours) and implement
thresholds 3,5,8 and a delay at the throttled level; reference
src/core/agent-cooldown.ts and the config keys (level1Threshold,
level2Threshold, shutdownThreshold, throttle_after, restrict_after,
suspend_after, reset_hours) when making the edit so the runtime and docs are
aligned.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

flag is not a supported per-project action setting.

src/core/project-policy.ts only parses max_irreversibility_score from [policy.actions]. The flag = 45 example here is a no-op today, so the docs are promising a control users cannot actually enforce.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/advanced/security-runtime.md` around lines 243 - 250, The docs show a
per-project "flag" setting under [policy.actions] but src/core/project-policy.ts
only parses max_irreversibility_score, so the "flag = 45" example is a no-op;
either remove/replace the unsupported "flag" example in docs or implement
support for it by adding parsing and handling in the ProjectPolicy loader
(src/core/project-policy.ts) so it reads "flag" from [policy.actions] and
applies the per-project flag threshold consistently with existing logic for
max_irreversibility_score; update any related tests or documentation to reflect
the chosen approach.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add languages to the new fenced blocks.

MD040 is already firing on these fences. Tag them as text (or bash where appropriate) so the README stays lint-clean.

Also applies to: 344-351, 389-391

🧰 Tools

🪛 markdownlint-cli2 (0.21.0)

[warning] 145-145: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 145 - 152, The README has unlabeled fenced code
blocks (e.g., the block containing "⚠️ Zora Action Approval Required" and
similar blocks at the other noted occurrences) which trigger MD040; fix this by
adding a language tag to each fence (use "text" for plain output blocks or
"bash" for command snippets) so the fences become ```text or ```bash as
appropriate, leaving the inner content unchanged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Several high-risk defaults are unreachable from toolToAction().

DEFAULT_IRREVERSIBILITY_SCORES defines shell_exec_destructive: 90, but nothing ever returns that key, so destructive bash commands are scored as generic shell 50. The map also misses send_signal_message and send_telegram_message, so those tools fall back to the 50-point default instead of the intended messaging score. That weakens both the flag and auto_deny gates.

Also applies to: 127-143

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/hooks/built-in/irreversibility-scorer.ts` around lines 25 - 50,
toolToAction() is missing mappings for destructive shell and specific messaging
tools so keys like shell_exec_destructive, send_signal_message, and
send_telegram_message never get scored correctly; update the mapping inside
toolToAction to return "shell_exec_destructive" for destructive shell variants
(e.g., any destructive bash/execute_bash/run_command tool name used in your
codebase) and add explicit entries mapping "send_signal_message" and
"send_telegram_message" to "send_message" (or the correct messaging action key
used by DEFAULT_IRREVERSIBILITY_SCORES), then run/adjust any tests that rely on
those action keys so the flag/auto_deny gates use the intended scores.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Project policy lookup uses the wrong identifier.

src/core/project-policy.ts registers policies by agent name, but ToolCallContext only provides jobId, and this code queries getAgentPolicy(ctx.jobId). Unless those happen to be the same string, per-project score ceilings never apply.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/hooks/built-in/irreversibility-scorer.ts` around lines 63 - 70, The code
calls getAgentPolicy(ctx.jobId) but project policies are keyed by agent name, so
replace the lookup to use the agent identifier that matches registration (e.g.,
ctx.agentName or ctx.agentId) instead of ctx.jobId; update the call in the
irreversibility scorer to getAgentPolicy(ctx.agentName || ctx.agentId) (or
thread an agentName through ToolCallContext if missing) and keep the subsequent
checkScoreLimit(...) and logging logic unchanged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Rollback SKILL.md if the lockfile update fails.

The skill file is renamed into place before skills.lock.json is updated. If updateLockFile() throws, maybeGenerateSkill() only logs the error, leaving an untracked skill on disk even though integrity checks depend on the lockfile.

Suggested fix

     const dest = path.join(skillDir, 'SKILL.md');
     const tmp = `${dest}.tmp`;
     await fs.writeFile(tmp, content, 'utf-8');
     await fs.rename(tmp, dest);
-
-    await this.updateLockFile(name, content);
+    try {
+      await this.updateLockFile(name, content);
+    } catch (err) {
+      await fs.rm(dest, { force: true });
+      throw err;
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/skills/SkillSynthesizer.ts` around lines 191 - 196, Before renaming the
temp SKILL.md into place in maybeGenerateSkill (where dest and tmp are used and
updateLockFile(name, content) is called), capture the existing SKILL.md (if
present) by reading it into a backup (e.g., readFile(dest) or rename to dest +
'.bak'), then perform the rename(tmp -> dest) and call updateLockFile; if
updateLockFile throws, catch the error and roll back by restoring the original
SKILL.md from the backup (or deleting the new dest and renaming the backup
back), ensure any temporary files (tmp and backup) are cleaned up, and rethrow
or re-log the original error so behavior remains consistent.

[coderabbitai]

Actionable comments posted: 1

🤖 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 141-153: The cooldown init uses raw casts so NaN can propagate;
update the initGlobalCooldown call to validate numeric fields the same way
forecaster does: for each cooldownConfig property (enabled, level1_threshold,
level2_threshold, shutdown_threshold, reset_after_hours, level1_delay_ms) check
typeof value === 'number' && Number.isFinite(value) before using it, otherwise
fall back to DEFAULT_COOLDOWN_CONFIG values; keep the same property mapping and
pass the validated values into initGlobalCooldown (symbols to change:
cooldownConfig, initGlobalCooldown, DEFAULT_COOLDOWN_CONFIG, and the specific
keys
level1Threshold/level2Threshold/shutdownThreshold/resetAfterHours/level1DelayMs).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 54b6fd8c-4e37-4886-a52a-f04172076c93

📥 Commits

Reviewing files that changed from the base of the PR and between 898265c and 21a8ca4.

📒 Files selected for processing (4)

• src/cli/daemon.ts
• src/cli/security-commands.ts
• src/config/policy-loader.ts
• src/core/agent-cooldown.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/config/policy-loader.ts

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

src/core/project-policy.ts (1)

54-54: Redundant spread in default policy returns.

The spread { ...DEFAULT_PROJECT_POLICY, tools: {...}, ... } overwrites every property from DEFAULT_PROJECT_POLICY, making the spread effectively unused. Either return { ...DEFAULT_PROJECT_POLICY } directly or remove the spread.

Suggested simplification

-    return { ...DEFAULT_PROJECT_POLICY, tools: { denied: [] }, filesystem: { allowedPaths: [], deniedPaths: [] }, network: { allowedDomains: [] }, actions: { maxIrreversibilityScore: 100 } };
+    return { ...DEFAULT_PROJECT_POLICY };

Also applies to: 63-63

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/core/project-policy.ts` at line 54, The return statement currently does {
...DEFAULT_PROJECT_POLICY, tools: {...}, filesystem: {...}, network: {...},
actions: {...} } which entirely overwrites DEFAULT_PROJECT_POLICY and makes the
spread redundant; update the function to either return a shallow clone of
DEFAULT_PROJECT_POLICY directly (return { ...DEFAULT_PROJECT_POLICY }) or remove
the spread and return only the explicit object literal if you intend to replace
defaults, and apply the same change to the similar return at the other
occurrence; reference DEFAULT_PROJECT_POLICY and the return statements in
src/core/project-policy.ts to locate and adjust the code.

🤖 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/security-commands.ts`:
- Around line 202-210: The PASS entry is always appended because the code checks
for the PASS label instead of whether a FAIL already exists; update the guard in
the block that pushes the PASS result (the results.push that creates id
`SECRET-PLAINTEXT-...` and label `No plaintext secrets in ${file}`) to check for
the FAIL label (e.g. `Plaintext secret in ${file}`) or for any existing
secret-related id/label (e.g. use results.some(r => r.label === \`Plaintext
secret in ${file}\` || r.label === \`No plaintext secrets in ${file}\` ) or
check ids with r.id.startsWith('SECRET-PLAINTEXT-')) so the PASS is only added
when no FAIL exists for that file.
- Around line 284-296: The IPv6 localhost check in the AgentBus URL validation
(the isLocal regex) fails to recognize bracketed IPv6 hosts like [::1]; update
the regex used where isLocal is defined to also accept bracketed IPv6 literals
(e.g., match either plain localhost/127.0.0.1 or \[::1\] and variants with
optional port) so URLs like "http://[::1]:8080" are treated as local; modify the
regex in the block that computes isLocal (the constant named isLocal next to
match and url) accordingly and keep the existing logic that treats non-local
http:// as WARN.

In `@src/core/project-policy.ts`:
- Around line 131-137: The allowedPaths intersection logic in project-policy.ts
incorrectly handles a parent allowed path of "/" because the test
cp.startsWith(pp + '/') turns into cp.startsWith("//") and fails to match;
update the filter used when both parent.filesystem.allowedPaths and
child.filesystem.allowedPaths are non-empty (the cp =>
parent.filesystem.allowedPaths.some(pp => ... ) predicate) so that if pp === '/'
it treats that as matching any child path (or simply checks
cp.startsWith('/')/cp === '/' accordingly), otherwise keep the existing equality
or cp.startsWith(pp + '/') check; adjust the predicate to handle the root case
explicitly so child paths like "/home" are included when parent has "/".

In `@src/tools/subagent-tool.ts`:
- Around line 84-91: The subagent policy is being registered directly via
registerAgentPolicy(name, subagentPolicy) without enforcing the parent
inheritance rule; instead, after loading subagentPolicy with
loadProjectPolicy(subagentDir) call mergeParentChild(parentPolicy,
subagentPolicy) to produce an effectivePolicy and then register that
effectivePolicy. Locate the parent policy (e.g., loadProjectPolicy for the
parent/global project or retrieve the already-registered parent policy) and pass
it as the first argument to mergeParentChild, handle the case where parent is
missing by treating it as the current global/default policy, and use the merged
result when calling registerAgentPolicy(name, effectivePolicy).

---

Nitpick comments:
In `@src/core/project-policy.ts`:
- Line 54: The return statement currently does { ...DEFAULT_PROJECT_POLICY,
tools: {...}, filesystem: {...}, network: {...}, actions: {...} } which entirely
overwrites DEFAULT_PROJECT_POLICY and makes the spread redundant; update the
function to either return a shallow clone of DEFAULT_PROJECT_POLICY directly
(return { ...DEFAULT_PROJECT_POLICY }) or remove the spread and return only the
explicit object literal if you intend to replace defaults, and apply the same
change to the similar return at the other occurrence; reference
DEFAULT_PROJECT_POLICY and the return statements in src/core/project-policy.ts
to locate and adjust the code.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f5152465-c380-456e-af5b-96a933696ba9

📥 Commits

Reviewing files that changed from the base of the PR and between 21a8ca4 and 0a4a105.

📒 Files selected for processing (8)

• src/cli/daemon.ts
• src/cli/security-commands.ts
• src/core/approval-queue.ts
• src/core/project-policy.ts
• src/orchestrator/orchestrator.ts
• src/skills/SkillSynthesizer.ts
• src/skills/SkillsLock.ts
• src/tools/subagent-tool.ts

🚧 Files skipped from review as they are similar to previous changes (5)

• src/cli/daemon.ts
• src/skills/SkillsLock.ts
• src/skills/SkillSynthesizer.ts
• src/core/approval-queue.ts
• src/orchestrator/orchestrator.ts

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/cli/security-commands.ts (1)

409-430: Consider TTY detection for colored output.

The ANSI escape codes in formatText will produce garbled output when piped to a file or non-TTY environment. This is a common CLI issue and not critical, but could be improved for better automation support.

♻️ Optional: Add TTY detection

 function formatText(report: AuditReport): string {
+  const useColor = process.stdout.isTTY ?? false;
+  const colorize = (code: string, text: string) => useColor ? `\x1b[${code}m${text}\x1b[0m` : text;
   const date = report.timestamp.replace('T', ' ').replace(/\.\d+Z$/, '');
   const lines: string[] = [`Zora Security Audit — ${date}`, ''];

   for (const check of report.checks) {
     const icon = check.severity === 'PASS' ? '✓' : check.severity === 'FAIL' ? '✗' : '⚠';
-    const label = check.severity === 'PASS' ? '\x1b[32mPASS\x1b[0m'
-                : check.severity === 'FAIL' ? '\x1b[31mFAIL\x1b[0m'
-                : '\x1b[33mWARN\x1b[0m';
+    const label = check.severity === 'PASS' ? colorize('32', 'PASS')
+                : check.severity === 'FAIL' ? colorize('31', 'FAIL')
+                : colorize('33', 'WARN');

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cli/security-commands.ts` around lines 409 - 430, The formatted text
currently always injects ANSI color codes in formatText which pollutes non-TTY
output; change formatText to accept a boolean (e.g., useColors) or detect
process.stdout.isTTY and only add the escape sequences to label/icon/summary
when true, leaving plain text otherwise; update callers of formatText (wherever
it's invoked) to pass through the TTY flag or rely on process.stdout.isTTY so
piping to files produces clean output while interactive terminals still show
colors.

🤖 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/security-commands.ts`:
- Around line 117-127: The checkFilePerm function generates non-unique IDs by
using only the basename (id = `PERM-${rel.replace(/\./g, '-').toUpperCase()`)
which causes collisions when buildReport audits same-named files in different
directories; update checkFilePerm to incorporate path context (e.g., include a
sanitized directory fragment or path.relative value) when constructing the id so
it becomes unique across locations (keep the existing basename and mode in label
but change id generation to use path.dirname or a short sanitized path segment),
ensuring references from buildReport to checkFilePerm continue to work.
- Around line 161-174: checkPlaintextSecrets only reads top-level .toml files so
it misses files in subfolders like config/channel-policy.toml (which
checkSignalPolicyFile expects); update checkPlaintextSecrets to recursively
traverse zoraDir (e.g., using fs.readdirSync with { withFileTypes: true } and a
recursive helper or a glob) to collect all .toml files under zoraDir before
scanning, ensuring files in subdirectories such as config/ are included in the
results.

---

Nitpick comments:
In `@src/cli/security-commands.ts`:
- Around line 409-430: The formatted text currently always injects ANSI color
codes in formatText which pollutes non-TTY output; change formatText to accept a
boolean (e.g., useColors) or detect process.stdout.isTTY and only add the escape
sequences to label/icon/summary when true, leaving plain text otherwise; update
callers of formatText (wherever it's invoked) to pass through the TTY flag or
rely on process.stdout.isTTY so piping to files produces clean output while
interactive terminals still show colors.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 846e5a37-391a-484e-b802-c35e5ee537e8

📥 Commits

Reviewing files that changed from the base of the PR and between 0a4a105 and 4e10dce.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (4)

• package.json
• src/cli/security-commands.ts
• src/core/project-policy.ts
• src/tools/subagent-tool.ts

🚧 Files skipped from review as they are similar to previous changes (3)

• package.json
• src/tools/subagent-tool.ts
• src/core/project-policy.ts