Skip to content

feat(agent): a follow-up run is told about the run it follows (B36) - #486

Open
koraysrn wants to merge 3 commits into
libredb:mainfrom
koraysrn:feat/agent-follow-up-context
Open

feat(agent): a follow-up run is told about the run it follows (B36)#486
koraysrn wants to merge 3 commits into
libredb:mainfrom
koraysrn:feat/agent-follow-up-context

Conversation

@koraysrn

Copy link
Copy Markdown
Contributor

Summary

Closes B36 from docs/BACKLOG.md — a follow-up question was answered as if it were the first one.

A run can now be opened with the previous run's id. The route derives the previous run's objective and report from its own ledger, verifies that the run belongs to the session, and persists the result as priorContext on the new run's header. The investigation loop reads that context off the ledger — never the browser's memory — and hands it to the model fenced, so a pronoun or a demonstrative ("those groups", "it") either resolves against the earlier run or is refused for lack of a referent.

Background

Measured live on 2026-08-15: an analysis run answered "compare the average salary of employees hired before 1990 with those hired after" correctly, and the follow-up "and how many of those employees are there in each group?" was answered about departments. "Those groups" had no referent, because a run carried none — and neither the surface nor the model said so.

Changes

Core

Surface

Tests & docs

Verification

  • bun run typecheck
  • bun run lint (oxlint + eslint) — 0 errors ✅
  • bun run knip
  • bun run build
  • Unit + API + eval + component suites covering the changed modules ✅

@cevheri cevheri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this — the direction is right and the design reasoning is sound. Deriving the context server-side from the previous run's own ledger,
persisting it on the new run's header so a resumed drive is told the same thing, and fencing it before a model reads it are exactly the four properties B36 asked for, and all four are here. My comments are about the implementation and the packaging, not the approach.

Requesting changes on three blockers, one docs gap, and a request to unbundle four unrelated changes.

Blocking

  1. The route bypasses accessAgentRun, and reintroduces a bug that was already fixed there

src/lib/api/agent-run-access.ts is the one gate every per-run route passes through, and its docblock says why: "answered once so three routes cannot drift apart". The new call site is a fourth, and it drifted — it is missing this:

} catch (error) {
if (error instanceof AgentRunStoreError && error.reasonCode === "INVALID_RUN_ID") return notFound();
throw error;
}

store.read calls assertRunId, which throws AgentRunStoreError("INVALID_RUN_ID") for anything outside /^[A-Za-z0-9_]{1,64}$/. createErrorResponse
does not recognise that error, so it falls through to a generic 500. Measured on the branch: previousRunId: "arun-1" (a dash), "../../etc/passwd",
or any 65-character string answers 500, not the 400 the code intends. That is the same defect agent-run-access.ts documents having fixed ("asking
for ../../etc/passwd produced a bodyless 500 instead of an answer").

It also splits the answer the route is trying to keep uniform: a malformed id 500s while an unknown or foreign id 400s, which is a distinction a caller guessing ids can read.

The API tests do not catch this because mockStatus is a Map.get that never throws. Please route the lookup through accessAgentRun (or add the catch), and cover it with a test that exercises the real store rather than the mock.

  1. The connection scope is enforced in the browser only

The route checks actor.sessionId and nothing else. There is no comparison against connection.id, and connection is already resolved a few lines above.

Three things depend on that check existing:

  • The prompt tells the model "This run follows an earlier run on the same connection." The server does not establish that, so the sentence can be false.
  • AgentRail.tsx's new comment says "the route refuses anything else". It does not.
  • AgentRunRecord.connectionId is documented as "The single connection this run may reach". Carrying one connection's report into another
    connection's prompt — and into its ledger — crosses that boundary. It stays inside one user's own authorization, so it is not an escalation, but it is not the invariant the record claims either.

Same class: the rail requires the predecessor to be terminal, the route accepts a running one, so a crafted request can follow a run that has established nothing yet.

  1. The eval does not assert what B36 asked for, and B36 was deleted on its strength

B36's "Done when" is "an eval that drives two runs and asserts the second does not answer a different question", and the PR body repeats that claim.

In tests/evals/follow-up-context.test.ts the model is scripted: callsTool("run_read_query", { sql: GROUP_COUNT_SQL, ... }) hands the correct SQL to the fixture, and expect(secondDrive.modelStatements).toEqual([GROUP_COUNT_SQL]) then asserts the fixture returned what it was told to return.
Nothing in that path can distinguish a run that resolved "those groups" from one that did not. What the test genuinely proves — that the context reaches the transcript, fenced, before anything is done — is worth having, but it is a different claim.

The second run also does not chain: prior.report is a hand-written "Claim 1: …" string rather than derivePriorRunContext(first.record), so the derivation never runs in the eval at all.

Two acceptable resolutions, either is fine:

  • Chain the runs for real (derive from first.record) and state plainly in the docblock what a scripted model can and cannot establish, leaving the referent-resolution half to a live measurement; or
  • Keep B36 open and land this as the mechanism, with the entry narrowed to the part still unproven.

Also on B36: the entry framed the defect as "the surface does not say so, and the model does not either". This closes the model half. The rail still says nothing about whether a run is following another, or about the cases where it deliberately is not (connection switched, run replaced), so a user cannot tell how their follow-up was read. That half, and the "run history" larger shape, should survive as a new backlog entry rather than being removed with B36.

Docs

previousRunId is not documented on the request.

  • docs/AGENT.md's HTTP surface table still describes POST /api/agent/runs as "(mode, optional workflowType, objective, connectionId)".
  • docs/API_DOCS.md is untouched. That page carries the field-by-field parameter table for this endpoint and a Refusals section, and AGENT.md points at it as the request-by-request reference. Both the new field and the new refusal message need to land there.

Please split out

Four changes in this diff are unrelated to B36 and are not mentioned in the PR description. Each may well be fine on its own; none of them should be decided inside an agent feature review.

  1. .oxlintrc.json — the policy comments were deleted. The two blocks recording the #100 rationale (why no-autofocus is off, why src/components/ui/** stays upstream-pure) are gone while the rules they explain remain. That looks like a tool artifact rather than a decision, and it is the part of the file with the most value. Please restore them.
  2. no-await-in-loop: "off". Measured: this rule produces 155 warnings across the repo today, all pre-existing, and bun run lint does not fail on warnings — so it is not unblocking anything here. Worth doing as its own PR with a one-line rationale in the config, the way the other entries carry theirs.
  3. actions/cache v4.3.0 → v6.1.0. The pinned SHA is genuine (55cc834), so no concern there, but a two-major CI action bump with no rationale and no cache-hit verification belongs in its own PR — dependabot is configured for this.
  4. The docs/AGENT.md table of contents regeneration. The hand-curated flat list was replaced by a generated nested one (including the document's own H1). Unrelated churn; please revert it so the diff stays about B36.

Minor

  • derivePriorRunContext has no bound on the report. A run with many claims puts all of them into every follow-up prompt and copies them into the new ledger header. describePriorProgress is unbounded too, so this is consistent with the codebase — flagging it because B52 is the same shape.
  • The chain is depth 1: derivePriorRunContext does not read priorContext, so a third run is told about the second and not the first. Reasonable for a first cut; worth stating in the docs paragraph so it is a documented limit rather than a surprise.

Process

The verification list names four gates. CLAUDE.md requires six — bun run format and the full bun run test are missing — and 100% line coverage is a hard CI gate, so bun run test:coverage && bun run coverage:check is the check worth quoting rather than "suites covering the changed modules". If that list was not easy to find, tell me and I will fix the contributor docs.

Closes B36 (docs/BACKLOG.md).

A follow-up question — "and how many of those employees are there in each
group?" — used to be answered about a different question entirely, because a
run carried no memory of the one before it and the model silently picked a
plausible referent.

What changes

- types: AgentPriorRunContext, and AgentRunRecord.priorContext
- run-store: the header carries priorContext; pre-existing ledgers fold
  unchanged, so no migration is needed
- run-service + route: POST /api/agent/runs accepts previousRunId, derives
  the context from that run's own ledger, and refuses a run the session did
  not open (one answer for both "does not exist" and "not yours")
- investigation: describePriorRunContext fences the previous objective and
  report into the prompt, so a pronoun or demonstrative either resolves
  against the earlier run or is refused for lack of a referent
- prior-run-context: derivePriorRunContext assembles the inert context
- AgentRail: a terminal run on the same connection is followed automatically;
  switching connection drops the referent
- tests: unit (derive, store, service), route, component, and a two-run eval
  asserting the second run does not answer a different question
- docs: B36 removed from BACKLOG; follow-up context documented in AGENT.md
  and AGENT_DEMO.md
…nd is scoped to the connection

- route: an id the ledger cannot name joins the uniform 400 instead of a 500; the predecessor must be terminal, this session's, and on the same connection

- eval: the second run is derived from the first run's own record, and the docblock states what a scripted model can and cannot establish

- BACKLOG: B36's surface half and run history survive as B65; docs: previousRunId on the HTTP surface and in API_DOCS, chain depth 1 stated
@koraysrn
koraysrn force-pushed the feat/agent-follow-up-context branch from 444c844 to 2e1e526 Compare August 25, 2026 10:19
…the store-error branch is covered

The B67 entry in docs/AGENT.md pointed at docs/BACKLOG.md B36, which was removed when the follow-up work landed; agent-documentation.test.ts pins that every B-number cited in docs/AGENT.md still names an open backlog entry, so the dangling reference failed the CI documentation gate.

The route's non-INVALID_RUN_ID store failure (the ledger unreachable, rather than an id it refuses) is now exercised too: it is answered as the unhandled error the store reported, not folded into the follow-up refusal, which keeps the route at 100 percent line coverage.
@koraysrn

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review — the four properties you named are exactly the ones B36 asked for, and all three blockers were real. Here's what changed on each point.

Blocking

INVALID_RUN_ID / the missing catch. The lookup now catches AgentRunStoreError with reasonCode === "INVALID_RUN_ID" and folds it into the same 400 as an unknown, foreign, or still-running run — one refusal for every id this session may not follow, so a caller guessing ids cannot tell a malformed id from someone else's. I kept it an inline catch rather than routing through agent-run-access.ts because the contract differs from the per-run family: there is no single run being accessed, and the whole point is to collapse four different failures into one answer. The store's own refusal is pinned in run-store.test.ts (read/openRun/stream/close all raise INVALID_RUN_ID), and the route test now makes mockStatus throw that exact error class — previously the mock never threw, which is why the defect slipped past — so a missing catch now fails the test. If you'd rather the route test drive the literal real store, I'll rework it.

Connection and terminal scope. The route now refuses a predecessor whose connectionId differs from the resolved connection, and one whose status is not terminal. Both are covered by tests ("on another connection" and "has not ended"), so the prompt's "same connection" claim and the rail's comment are now true on the server, not only in the browser.

The eval. The second run now derives its context from derivePriorRunContext(await first.record()), so the derivation itself runs rather than being hand-written past it. The docblock states exactly what a scripted model proves — the fenced objective and report reach the transcript before anything runs — and what it cannot: referent resolution, which is left to a live measurement. B36's surface half (the rail naming the run it follows, or the cases where it deliberately does not, plus run history) survives as B67 rather than being deleted with B36.

Docs

previousRunId is documented on the request in both places: AGENT.md's HTTP surface table and API_DOCS.md's parameter table, together with the new refusal message.

Split out

.oxlintrc.json — the #100 rationale comments are restored.
no-await-in-loop: "off" — dropped; the 155 warnings are pre-existing and lint doesn't fail on them. Worth its own PR.
actions/cache v6.1.0 — reverted to the pinned v4 SHA; left to dependabot / its own PR.
The TOC regeneration — reverted to the hand-curated flat list.
Minor

derivePriorRunContext report bound — acknowledged; consistent with describePriorProgress, same shape as B52.
Chain depth 1 — stated in the docs paragraph as a documented limit, not a surprise.
Process — my verification list was under-specified. The checks that actually gate here are bun run format, bun run lint, bun run typecheck, the full bun run test, and the hard gate bun run test:coverage && bun run coverage:check. If that list wasn't discoverable from CLAUDE.md, I'm glad to fix the contributor docs.

@koraysrn
koraysrn requested a review from cevheri August 25, 2026 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants