Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 47 additions & 13 deletions docs/use/review-learnings.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,24 +33,58 @@ agent-initiated saves.

## When a learning gets created

`v1` is **agent-initiated only**: the `review` and `resolve` workflow prompts
expose three MCP tools on the existing `repo_memory` server.
Three capture paths exist today; the `review` and `resolve` workflow
prompts expose three MCP tools on the existing `repo_memory` server that
all three paths share:

- `mcp__repo_memory__save_review_learning({ directive, rationale, file_glob, scope, source_pr, source_thread, source_author })`
- `mcp__repo_memory__delete_review_learning({ id })`
- `mcp__repo_memory__get_review_learnings()`: returns the same set already
rendered into the prompt; useful when the agent needs the IDs to call
- `mcp__repo_memory__get_review_learnings()`: returns the full applicable
set for this run, including any rows the prompt's 24KB byte budget
omitted from the `<review_learnings_…>` block (the truncation marker
points here as its escape hatch). Useful for dedup before
`save_review_learning`, and for fetching IDs before
`delete_review_learning`.

The recommended trigger inside the `resolve` workflow is: when a maintainer
pushes back on an inline finding with a clear "this is intentional because…"
rationale and the agent's reply classifies the finding as `Invalid`, the
agent saves a learning capturing the rationale, the file scope, and the
source PR. The next `review` run on the same repo (or any repo under the
same owner, if `scope: global`) sees the directive as policy.

There is **no extraction LLM**: capture is the agent's responsibility. There
is also no `@chrisleekr-bot remember <…>` command; that is a follow-up.
### Path 1: autonomous capture in `resolve`

When a maintainer pushes back on an inline finding with a clear "this is
intentional because…" rationale and the resolve agent's reply classifies
the finding as `Invalid`, the agent decides autonomously whether the
rationale represents durable repo policy. If yes, it calls
`save_review_learning` with the directive, the maintainer's rationale,
the file glob, and full provenance. No propose step, no confirmation.

### Path 2: autonomous capture in `review`

When the discussion digest's maintainer-authoritative directives section
contains a rule the bot would otherwise have flagged AND that rule is
not already in the `<review_learnings_…>` block, the review agent
captures the digest's directive verbatim. Same autonomous decision rule
as the resolve path; same MCP save.

### Path 3: explicit `@chrisleekr-bot remember` (issue #160 Option A)

The dedicated [`bot:remember`](workflows/remember.md) workflow handles
explicit directive capture from any comment surface: issue comments, PR
comments, and PR review comments. Two trigger forms:

- **Inline:** `@chrisleekr-bot remember: do not flag fixture duplication
in test/**/*.test.ts`
- **Referential:** `@chrisleekr-bot remember this`, the agent walks the
discussion digest to locate the upstream maintainer policy statement
the trigger refers to.

The handler runs a focused agent session with a narrowed tool surface
(`save_review_learning`, `get_review_learnings`,
`update_claude_comment` only, no Bash, no Edit, no code mutation).
Dedup against existing entries happens before the save. The tracking
comment is the audit log: directive, scope, rationale, provenance, and
the outcome (`saved as <id>` / `deduped against <existing_id>` /
`refused (reason)`).

The next `review` or `resolve` run on the same repo (or any repo under
the same owner, when `scope: 'global'`) sees the directive as policy.

## How a learning is surfaced in the prompt

Expand Down
21 changes: 11 additions & 10 deletions docs/use/workflows/index.md
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
# Workflows

Six workflows are registered today (`src/workflows/registry.ts`). Each has a single label, a single comment-mention verb, and produces one Markdown artifact that becomes the body of the tracking comment.

| Workflow | Label | Surfaces | What it does | Detail |
| --------------------------- | --------------- | ---------------------------------------------- | ---------------------------------------------------------------------------- | ---------------- |
| [`triage`](triage.md) | `bot:triage` | Issue label or comment | Decides whether an issue is actionable, with structural or runtime evidence | `TRIAGE.md` |
| [`plan`](plan.md) | `bot:plan` | Issue label or comment, after `triage` | Writes an implementation plan | `PLAN.md` |
| [`implement`](implement.md) | `bot:implement` | Issue label or comment, after `plan` | Opens a PR with code, tests, and a filled-out PR template | `IMPLEMENT.md` |
| [`review`](review.md) | `bot:review` | PR label or comment | Reads the diff in full, posts findings as inline comments | `REVIEW.md` |
| [`resolve`](resolve.md) | `bot:resolve` | PR label or comment | Fixes failing CI, replies to review threads, pushes new commits | `RESOLVE.md` |
| [`ship`](ship.md) | `bot:ship` | PR comment, label, or natural-language mention | Shepherds an open PR to merge-ready: probe → fix → reply → wait, until clean | tracking comment |
Seven workflows are registered today (`src/workflows/registry.ts`). Each has a single label, a single comment-mention verb, and produces one Markdown artifact that becomes the body of the tracking comment.

| Workflow | Label | Surfaces | What it does | Detail |
| --------------------------- | --------------- | ---------------------------------------------- | ---------------------------------------------------------------------------------------- | ---------------- |
| [`triage`](triage.md) | `bot:triage` | Issue label or comment | Decides whether an issue is actionable, with structural or runtime evidence | `TRIAGE.md` |
| [`plan`](plan.md) | `bot:plan` | Issue label or comment, after `triage` | Writes an implementation plan | `PLAN.md` |
| [`implement`](implement.md) | `bot:implement` | Issue label or comment, after `plan` | Opens a PR with code, tests, and a filled-out PR template | `IMPLEMENT.md` |
| [`review`](review.md) | `bot:review` | PR label or comment | Reads the diff in full, posts findings as inline comments | `REVIEW.md` |
| [`resolve`](resolve.md) | `bot:resolve` | PR label or comment | Fixes failing CI, replies to review threads, pushes new commits | `RESOLVE.md` |
| [`ship`](ship.md) | `bot:ship` | PR comment, label, or natural-language mention | Shepherds an open PR to merge-ready: probe → fix → reply → wait, until clean | tracking comment |
| [`remember`](remember.md) | `bot:remember` | Issue or PR (any comment) | Captures a maintainer review-policy directive into `review_learnings` for future reviews | tracking comment |

## How they relate

Expand Down
99 changes: 99 additions & 0 deletions docs/use/workflows/remember.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
# `bot:remember`

Captures a maintainer-authored review-policy directive and persists it to
`review_learnings` so future PR reviews respect it.

| Field | Value |
| --------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| Label | `bot:remember` |
| Mention | `@chrisleekr-bot remember: do not flag fixture duplication in test/**/*.test.ts` · `@chrisleekr-bot remember this` · `@chrisleekr-bot remember the rule above` |
| Accepted target | Issue OR pull request |
| Requires prior | _none_ |
| Artifact | _none_, the tracking comment IS the audit log (no sibling file; the agent has no `Write` tool) |
| Side effects | One row inserted into the `review_learnings` table via the `save_review_learning` MCP tool. No code edits, no commits, no review-thread changes. Tracking comment carries audit. |
| Source | `src/workflows/handlers/remember.ts` |

## When to use it

`bot:remember` is the **explicit** complement to the agent's autonomous
capture path (see [`resolve.md`](resolve.md) for the post-rebuttal save).
Reach for it when you want to ensure a review rule lands in the bot's
memory without depending on a `resolve` flow to fire, e.g. you noticed a
recurring false-positive in a different repo, or you want to seed the
policy table proactively before the bot has had a chance to flag the
pattern.

It is intentionally invocable on **any** comment surface: issue
comments, PR comments, and PR review (inline) comments all reach the
same handler. The trigger phrase + the word `remember` is what routes
the intent classifier here.

## Two trigger forms

**Inline**, the directive sits in the trigger comment:

```text
@chrisleekr-bot remember: do not flag fixture duplication in test/**/*.test.ts
```

**Referential**, the directive lives upstream in the thread:

```text
> [earlier maintainer comment]
> We keep these per-file rather than centralising; the closure needs
> the literal at module-evaluation time.

@chrisleekr-bot remember this
```

The handler always feeds the agent the full discussion digest, so the
referential form has the context it needs to locate and extract the
policy statement.

## Refusal cases

The agent does NOT save anything (and says so in the tracking comment)
when:

- The trigger comment is referential but the upstream thread carries no
policy-shaped maintainer statement to pin to.
- The proposed directive paraphrases an existing entry in
`review_learnings` (the agent dedupes via `get_review_learnings`
before saving).
- The directive collapses to empty after sanitization (HTML / BIDI /
zero-width strip).

In each refusal case the tracking comment explains which case fired
so the maintainer can rephrase if needed.

## Trust boundary

Trigger classification + the directive extraction both depend on the
`ALLOWED_OWNERS` author-trust set. A non-owner saying
`@chrisleekr-bot remember [...]` reaches the intent classifier, but the
discussion digest splits owner-authoritative directives from
untrusted-context comments before the agent reads them. The handler
does not gate the trigger by author (an owner can ask the bot to capture
a non-owner's stated rule), but the agent's prompt requires a
maintainer-authoritative source for the directive itself.

## Scope and provenance

- `scope` defaults to `'local'` (this repo only). `'global'` requires
explicit maintainer language ("across all our repos") AND a
single-tenant deploy; the orchestrator silently downgrades `'global'`
to `'local'` when `ALLOWED_OWNERS` has more than one owner.
- `file_glob` is captured when the directive cites a path pattern.
Pathological globs (catastrophic-backtracking shapes) are rejected at
the durability boundary; the directive saves with `file_glob = null`
rather than failing.
- `source_pr`, `source_author`, `source_thread` carry the provenance the
`🧠 Learnings used` footer renders on every future review.

## What it does NOT do

- It does not modify code, commit anything, or push.
- It does not resolve review threads.
- It does not auto-merge.
- It does not run on every comment, only when the intent classifier
picks `remember` for the trigger comment.
1 change: 1 addition & 0 deletions mkdocs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ nav:
- bot:implement: use/workflows/implement.md
- bot:review: use/workflows/review.md
- bot:resolve: use/workflows/resolve.md
- bot:remember: use/workflows/remember.md
- Scheduled actions: use/scheduled-actions.md
- Review learnings: use/review-learnings.md
- Safety: use/safety.md
Expand Down
22 changes: 21 additions & 1 deletion src/core/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,19 @@ export interface RunPipelineOverrides {
* inadvertently suppress findings or persist review-policy directives.
*/
enableReviewLearnings?: boolean;
/**
* When `enableReviewLearnings` is true, the pipeline filters
* `ctx.reviewLearnings` down to the PR's changed-file matches before
* forwarding into both the prompt block and the MCP env. That's the right
* default for review / resolve where the agent only acts on directives
* applicable to the diff. The `remember` workflow needs the unfiltered
* universe so its dedup pre-check (`get_review_learnings`) doesn't miss
* directives sitting outside the current PR's changed files (or every
* glob-scoped directive on issue-context invocations). Set to `true` to
* skip the applicability filter; the MCP tool then enumerates the
* orchestrator's full pre-loaded set.
*/
unfilteredReviewLearnings?: boolean;
}

/**
Expand Down Expand Up @@ -309,7 +322,10 @@ export async function runPipeline(
// erase the property entirely so downstream `!== undefined` checks
// remain accurate.
delete enrichedCtx.reviewLearnings;
} else if (enrichedCtx.reviewLearnings !== undefined) {
} else if (
enrichedCtx.reviewLearnings !== undefined &&
overrides.unfilteredReviewLearnings !== true
) {
// Narrow the orchestrator-loaded universe to directives applicable to
// this PR's changed files BEFORE downstream consumers see it. Without
// this, the prompt block (glob-filtered + byte-capped) and the MCP
Expand All @@ -318,6 +334,10 @@ export async function runPipeline(
// would return "every active directive (including the omitted ones)"
// but the tool also returned glob-non-matching rows. Filtering here
// makes both surfaces enumerate the same applicable universe.
//
// The `remember` workflow opts out via `unfilteredReviewLearnings`:
// its dedup pre-check needs the full set, including directives
// outside the current PR's changed files.
enrichedCtx.reviewLearnings = pickApplicableLearnings(
enrichedCtx.reviewLearnings,
enrichedCtx.isPR ? data.changedFiles.map((f) => f.filename) : [],
Expand Down
7 changes: 6 additions & 1 deletion src/orchestrator/job-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@ import { requireValkeyClient } from "./valkey";

const workflowRunRefSchema = z.object({
runId: z.string().min(1),
workflowName: z.enum(["triage", "plan", "implement", "review", "resolve", "ship"]),
// Mirror of registry.ts WorkflowNameSchema. Hardcoded rather than imported
// to avoid the registry → handlers → job-queue cycle (handlers/ship.ts
// and ship/iteration.ts call enqueueJob, so importing the schema back
// here would init-deadlock). When adding a new workflow, extend both
// lists; TypeScript will surface the gap at every enqueueJob call site.
workflowName: z.enum(["triage", "plan", "implement", "review", "resolve", "ship", "remember"]),
parentRunId: z.string().min(1).optional(),
parentStepIndex: z.number().int().nonnegative().optional(),
});
Expand Down
Loading
Loading