feat: add an advisor that reviews each turn with a second model

[elkaix]

Related Issue

No linked issue — directly requested feature; problem explained below. Based on the model-roles branch (#56) because the advisor consumes the advisor model role; retarget to main after #56 merges.

Problem

There is no way to get a continuous second opinion on an agent session. A user who wants a stronger or different model to sanity-check the working agent's decisions has to interrupt and paste context manually.

What changed

An opt-in advisor reviews the session with a second model and feeds notes back to the working agent:

• After each completed main-agent turn, a session-owned coordinator runs an ephemeral in-memory agent (no persistence, no tools) over a projected copy of the conversation and asks it for structured notes ({note, severity: nit|concern|blocker}).
• Notes are buffered and injected as one <advisory> block ("weigh, don't blindly obey") when the next turn starts — or immediately if a turn is already running. Advisories never launch a turn on their own and never block or slow the primary turn (single-flight, 120s timeout, fire-and-forget).
• The advisor model resolves from [advisor] model or the advisor model role; it never falls back to the session model. Privacy default: it runs only when its model uses the same provider entry as the session model — a cross-provider advisor stays configured but inactive with one warning (transcript redaction / explicit cross-provider opt-in is future work).
• Config: [advisor] table (enabled, default false; model; instructions). The advisor disables itself for the session after three consecutive failures.
• Mid-turn interruption on blocker severity is deliberately deferred; blockers arrive as the first advisory of the next turn.

Checklist

• I have read the CONTRIBUTING document.
• I have linked a related issue, or explained the problem above.
• I have added tests that prove my feature works.
• Ran gen-changesets skill, or this PR needs no changeset.
• Ran gen-docs skill, or this PR needs no doc update.

Summary by CodeRabbit

• New Features

  • Added an optional AI advisor that reviews completed user-driven turns and provides advisory notes to the main agent.
  • Supports configurable advisor models and instructions, with reviews delivered to the next active turn.
  • Advisor activity is limited to compatible model providers and automatically pauses after repeated failures.
  • Added configuration support for enabling the advisor and preserving advisor settings when configurations are updated.

• Documentation

  • Added configuration guidance, supported settings, restrictions, behavior details, and an example setup for the advisor feature.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds an opt-in SessionAdvisor that reviews completed user-driven main-agent turns with a configured same-provider model. It validates and buffers advisory notes, then injects them into the next main-agent turn.

Changes

Advisor runtime

Layer / File(s)	Summary
Advisor configuration and persistence packages/agent-core/src/config/schema.ts, packages/agent-core/src/config/toml.ts, packages/agent-core/test/config/configs.test.ts, docs/configuration/config-files.md, .changeset/advisor-runtime.md	Adds advisor.enabled, advisor.model, and advisor.instructions to configuration schemas and TOML handling. Documents configuration and adds serialization and patch-merge tests.
Session event wiring packages/agent-core/src/agent/index.ts, packages/agent-core/src/session/index.ts	Adds optional agent event callbacks. The main agent reports turn start and successful turn completion events to SessionAdvisor.
Advisor review and delivery packages/agent-core/src/session/session-advisor.ts, packages/agent-core/test/session/session-advisor.test.ts	Reviews eligible turns with a temporary same-provider model, validates and truncates notes, buffers delivery, prevents concurrent reviews, handles failures, and disables after three consecutive failures. Tests cover configuration, model selection, delivery, concurrency, errors, and aborts.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MainAgent
  participant SessionAdvisor
  participant AdvisorAgent
  MainAgent->>SessionAdvisor: report completed user turn
  SessionAdvisor->>AdvisorAgent: request structured review
  AdvisorAgent-->>SessionAdvisor: return advisory notes
  SessionAdvisor->>MainAgent: inject notes into next turn

Loading

Possibly related PRs

• Pythoughts-labs/pythinker-code#56: Modifies advisor model configuration and resolution used by this runtime feature.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title uses the feat prefix, imperative mood, clearly describes the advisor feature, and is 63 characters long.
Description check	✅ Passed	The description includes all required sections, explains the problem and implementation, and marks the checklist items complete.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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

[pkg-pr-new]

pnpm dlx https://pkg.pr.new/@pythoughts/pythinker-code@32c3701

npx https://pkg.pr.new/@pythoughts/pythinker-code@32c3701

commit: 32c3701

[greptile-apps]

Greptile Summary

This change adds an opt-in session advisor, advisor configuration persistence, documentation, and coverage for bounded review-note delivery. One blocking behavior remains: a delayed advisor result can be inserted into a later user turn that is already in progress.

Confidence Score: 4/5

Not merge-safe until delayed advisor notes are prevented from entering an already-active later turn.

One verified non-security P1 finding remains. Under the scoring table, one non-security P1 results in a confidence score of 4.

Files Needing Attention: packages/agent-core/src/session/session-advisor.ts

T-Rex Logs

What T-Rex did

• T-Rex produced a proof for the posted P1 finding.
• T-Rex prepared the advisor turn-isolation reproduction source to show how the isolation could be reproduced.
• T-Rex captured the advisor turn-isolation runtime output log to document the execution trace.
• The reproduction source and runtime log artifacts were attached to the pull request summary.

_{Ran code and verified through T-Rex}

Prompt To Fix All With AI

### Issue 1
packages/agent-core/src/session/session-advisor.ts:160-168
**Delayed advisor notes enter the wrong turn**

An advisor run is asynchronous, but pending notes are delivered whenever any main turn is active. If a review of user turn 1 finishes after user turn 2 has already started, `#deliverPending()` steers the turn-1 advice into the in-progress turn-2 conversation. This can redirect work already underway and breaks the intended next-turn delivery behavior. Associate pending advice with the reviewed turn and only inject it at the start of the next eligible user turn; retain it when that delivery boundary has already passed.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{Reviews (3): Last reviewed commit: "test: build the surrogate-pair fixture f..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (3)

packages/agent-core/test/session/session-advisor.test.ts (2)

21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Close sessions in afterEach, not at the end of each test body.

Every test calls await fixture.session.close() as its last statement. If an assertion fails before that line, the session stays open. The afterEach hook then removes the temporary directory while the session still holds handles, which can produce follow-on failures in unrelated tests.

Track the created sessions in a module-level array inside createFixture, and close them in afterEach before the directory removal.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/agent-core/test/session/session-advisor.test.ts` around lines 21 -
25, Update the session-advisor test cleanup by tracking each session created in
createFixture in a module-level collection, then closing all tracked sessions at
the start of afterEach before removing temporary directories. Remove the
per-test session.close calls so cleanup remains reliable when assertions fail,
and clear the collection after closing.

---

126-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the parseNotes rejection branches.

No test exercises the validation failures in parseNotes. Four throw sites are uncovered: a missing notes array, a non-object entry, a non-string note, and an invalid severity. Each one feeds #recordFailure and moves the advisor toward the three-failure shutdown, so the behavior is worth pinning.

Add a case that queues a StructuredOutput call with { notes: [{ note: 42 }] } and assert that session.log.debug records 'advisor run failed' and that steer is not called.

As per path instructions, "New behavior should come with vitest coverage."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/agent-core/test/session/session-advisor.test.ts` around lines 126 -
136, Add a Vitest case in the session advisor tests that queues StructuredOutput
with { notes: [{ note: 42 }] }, runs the advisor flow, and asserts
session.log.debug records “advisor run failed” while the main turn’s steer is
not called. Reuse the existing fixture, queueReview, runMainTurn, and
waitForAdvisor helpers, targeting the parseNotes rejection path and its failure
recording behavior.

Source: Path instructions

packages/agent-core/src/session/session-advisor.ts (1)

176-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the as AdvisoryNote assertion.

TypeScript does not narrow an unknown value through !== literal comparisons, so severity stays unknown and the assertion hides that. Introduce a typed local instead.

♻️ Proposed fix

-    const { note, severity } = value as { note?: unknown; severity?: unknown };
+    const { note, severity } = value as { note?: unknown; severity?: unknown };
     if (typeof note !== 'string') throw new Error('Advisor returned an invalid note.');
-    if (
-      severity !== undefined &&
-      severity !== 'nit' &&
-      severity !== 'concern' &&
-      severity !== 'blocker'
-    ) {
+    const severities: readonly string[] = ['nit', 'concern', 'blocker'];
+    if (severity !== undefined && (typeof severity !== 'string' || !severities.includes(severity))) {
       throw new Error('Advisor returned an invalid severity.');
     }
-    return { note: Array.from(note.trim()).slice(0, 500).join(''), severity } as AdvisoryNote;
+    return {
+      note: Array.from(note.trim()).slice(0, 500).join(''),
+      severity: severity as AdvisoryNote['severity'],
+    };

A severity is AdvisoryNote['severity'] type predicate removes the remaining assertion entirely.

As per path instructions, "Flag any any, @ts-ignore, or type assertions added to silence errors."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/agent-core/src/session/session-advisor.ts` around lines 176 - 185,
In the advisory note mapping logic, remove the `as AdvisoryNote` assertion and
introduce a typed local for severity using a type predicate that validates the
allowed values (`nit`, `concern`, and `blocker`). Return the mapped object with
that narrowed severity so TypeScript infers it as an `AdvisoryNote` without any
type assertion.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/configuration/config-files.md`:
- Around line 180-190: Update the advisor documentation describing review
delivery timing to state that completed reviews are delivered during the next
turn, including when that turn is already active and receives the advisory via
steering. Remove claims that delivery always occurs at the turn start or that
the advisor never interrupts a running turn, while preserving the one-turn lag
guidance.

In `@packages/agent-core/src/agent/index.ts`:
- Around line 574-577: Update Agent’s emitEvent method to catch synchronous
failures from onEvent, log the callback error, and continue execution so
rpc.emitEvent is always attempted. Preserve the restoring guard and event
delivery behavior, and add a Vitest regression test covering a throwing onEvent
callback while verifying RPC delivery.

In `@packages/agent-core/src/session/index.ts`:
- Around line 1335-1344: The Session advisor currently drops completed turns
while a review is running. In packages/agent-core/src/session/index.ts lines
1335-1344, update the onEvent handling around SessionAdvisor.onMainTurnEnded()
to queue each completed-turn snapshot or sequence when busy and process queued
turns after the active review, preserving the “each completed user turn”
contract. In .changeset/advisor-runtime.md line 5, retain that contract only
after the runtime queues turns; otherwise revise the wording to state that
overlapping turns may be skipped.

In `@packages/agent-core/src/session/session-advisor.ts`:
- Around line 166-186: Update parseNotes to skip malformed note entries,
including invalid note text or severity, instead of throwing for individual
entries; only throw when the top-level notes array is missing or invalid. Apply
the ten-note cap to valid parsed notes rather than raw entries, and update the
corresponding session-advisor tests to reflect this behavior.
- Around line 44-49: Update onMainTurnStarted so the queued callback invokes
`#deliverPending` inside exception handling, preventing errors such as
main.turn.steer throws from escaping the microtask. Preserve the advisor’s
contract that it never throws and retain the existing review-state assignment
and asynchronous delivery behavior.

In `@packages/agent-core/test/session/session-advisor.test.ts`:
- Around line 21-25: Update
packages/agent-core/test/session/session-advisor.test.ts at lines 21-25 by
extending the afterEach teardown to restore Vitest mocks, close all tracked
sessions, then remove temporary directories. At lines 283-306, remove the manual
timeout.mockRestore() and narrow the AbortSignal.timeout stub to only the
120-second advisor wait, relying on the shared afterEach cleanup.

---

Nitpick comments:
In `@packages/agent-core/src/session/session-advisor.ts`:
- Around line 176-185: In the advisory note mapping logic, remove the `as
AdvisoryNote` assertion and introduce a typed local for severity using a type
predicate that validates the allowed values (`nit`, `concern`, and `blocker`).
Return the mapped object with that narrowed severity so TypeScript infers it as
an `AdvisoryNote` without any type assertion.

In `@packages/agent-core/test/session/session-advisor.test.ts`:
- Around line 21-25: Update the session-advisor test cleanup by tracking each
session created in createFixture in a module-level collection, then closing all
tracked sessions at the start of afterEach before removing temporary
directories. Remove the per-test session.close calls so cleanup remains reliable
when assertions fail, and clear the collection after closing.
- Around line 126-136: Add a Vitest case in the session advisor tests that
queues StructuredOutput with { notes: [{ note: 42 }] }, runs the advisor flow,
and asserts session.log.debug records “advisor run failed” while the main turn’s
steer is not called. Reuse the existing fixture, queueReview, runMainTurn, and
waitForAdvisor helpers, targeting the parseNotes rejection path and its failure
recording behavior.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9b58bb0b-a4d1-40d1-bdd1-a3e15582c5c0

📥 Commits

Reviewing files that changed from the base of the PR and between b71f094 and be2728e.

📒 Files selected for processing (9)

• .changeset/advisor-runtime.md
• docs/configuration/config-files.md
• packages/agent-core/src/agent/index.ts
• packages/agent-core/src/config/schema.ts
• packages/agent-core/src/config/toml.ts
• packages/agent-core/src/session/index.ts
• packages/agent-core/src/session/session-advisor.ts
• packages/agent-core/test/config/configs.test.ts
• packages/agent-core/test/session/session-advisor.test.ts

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/agent-core/src/session/session-advisor.ts (1)

160-165: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Retain the advisory when steer fails.

Line 164 clears #pendingAdvisory before main.turn.steer runs. steer can throw. The caller then only logs the error. The next main turn cannot receive the valid advisory.

Clear #pendingAdvisory only after steer returns. Add a test that makes the first delivery throw and verifies delivery on the next turn.

Proposed fix

     const block = this.#pendingAdvisory;
-    this.#pendingAdvisory = undefined;
     main.turn.steer([{ type: 'text', text: block }], {
       kind: 'hook_result',
       event: 'advisor',
     });
+    this.#pendingAdvisory = undefined;

Also applies to: 146-146

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/agent-core/src/session/session-advisor.ts` around lines 160 - 165,
Update `#deliverPending`() so `#pendingAdvisory` is cleared only after
main.turn.steer returns successfully; retain it when steer throws so the next
main turn retries delivery. Add a test covering a first delivery failure
followed by successful delivery on the next turn.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/agent-core/test/session/session-advisor.test.ts`:
- Around line 211-212: Update the surrogate-pair fixtures in the session advisor
test, including the `note` declaration and the corresponding fixture at the
second referenced location, to construct the same code point with
`String.fromCodePoint(0x1d400)` instead of embedding `𝐀` directly. Preserve the
existing string contents and code-point slicing coverage.

---

Outside diff comments:
In `@packages/agent-core/src/session/session-advisor.ts`:
- Around line 160-165: Update `#deliverPending`() so `#pendingAdvisory` is cleared
only after main.turn.steer returns successfully; retain it when steer throws so
the next main turn retries delivery. Add a test covering a first delivery
failure followed by successful delivery on the next turn.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7810214d-b966-42f8-b5b0-b79453597c47

📥 Commits

Reviewing files that changed from the base of the PR and between be2728e and 497aa37.

📒 Files selected for processing (5)

• .changeset/advisor-runtime.md
• docs/configuration/config-files.md
• packages/agent-core/src/agent/index.ts
• packages/agent-core/src/session/session-advisor.ts
• packages/agent-core/test/session/session-advisor.test.ts

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

• .changeset/advisor-runtime.md
• docs/configuration/config-files.md
• packages/agent-core/src/agent/index.ts

[greptile-apps]

This review was skipped because it would exceed your organization's monthly flex usage limit. Raise the limit in billing settings or wait until the next billing period resets limits.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/agent-core/test/session/session-advisor.test.ts`:
- Around line 81-87: Replace the conditional logic in the rawGenerate spy with
ordered mockImplementationOnce handlers: keep the default generation behavior,
then add explicit handlers for the advisor review call awaiting reviewGate and
the active main-turn call awaiting activeTurnGate. Preserve the existing call
ordering and return values while removing the currentCall checks.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 907dbf54-a9cb-4736-bec2-7a1c46702886

📥 Commits

Reviewing files that changed from the base of the PR and between bc9f0bc and 32c3701.

📒 Files selected for processing (3)

• docs/configuration/config-files.md
• packages/agent-core/src/session/session-advisor.ts
• packages/agent-core/test/session/session-advisor.test.ts

💤 Files with no reviewable changes (1)

• packages/agent-core/src/session/session-advisor.ts

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

• docs/configuration/config-files.md