Skip to content

feat(ask): answer free-form questions concurrently under one budget - #108

Open
drewstone wants to merge 3 commits into
mainfrom
feat/ask-questions
Open

feat(ask): answer free-form questions concurrently under one budget#108
drewstone wants to merge 3 commits into
mainfrom
feat/ask-questions

Conversation

@drewstone

@drewstone drewstone commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

What this fixes

Brief classes F8 (plumbing) and F6 (the evidence gate hides its rejections), items T1, T2, and T7.

Today a reading subagent beats the CLI partly because the CLI throws work away. The registry keeps only findings, so the engine's prose answer is discarded; analysts run one at a time; there is no way to ask a question of your own, and an unknown command exits 0. A refused finding is reported only as a log line whose reason never reaches the operator, so "0 findings" and "the gate refused everything" look identical. A failed external analyzer writes its error into the report and exits 0.

T1 — traces ask and runTraceQuestions

One or many free-form questions over one or many sessions. Each question is its own runTraceAnalyst call, so the answer text survives.

traces ask --harness codex --session <id> \
  --question "Which shell commands exited non-zero?" \
  --questions questions.json --concurrency 6 --budget 2 --dir .traces/ask
  • Concurrency. A worker pool with a configurable limit (default 4). Unlike the import pool it does not abort the others on the first error: a failed question is its own answer.
  • One budget. A shared CostLedger bounds the whole run. --question-budget is the provider ceiling for one question. A budget below one model call's reservation refuses the run before any model call; a budget that admits fewer concurrent reservations than --concurrency runs with a warning naming how many it covers.
  • Checked citations. Every trace://<trace_id>/span/<span_id> URI in an answer is resolved against the store. An unresolvable citation fails that question.
  • Typed answers. A question may carry a JSON Schema from a small supported subset. Any keyword outside it is rejected when the run starts, because a silently ignored constraint would let a wrong answer pass as checked.
  • Output. answers.json (answers, parsed answers, citations with resolution, accepted findings, rejections by reason, model calls, tool calls, cost with observed/estimated/uncaptured provenance, latency) and report.md. Both are written before the exit code is decided; ask exits 1 when any question failed, returned no answer, broke its schema, or cited a missing span. Ctrl-C is one of those failures, not an exception: the answers already bought are kept, the questions the run never reached are recorded as aborted, and both artifacts are still written.

The --llm engine construction moves out of cli.ts into analysisEngineFromEnv (analyst-model-call.ts), and the shared trace-file plus store setup into analysis-store.ts. ask and --llm now build the same engine from one place instead of two.

T2 — gate rejections visible, external failures fail

  • finding rejected log lines print the reason and the offending URI.
  • Rejection counts by reason reach the analyst table's Detail cell, TraceInvestigationResult.findingRejections, and the ask JSON, so a zero finding count can be told apart from a gate that refused everything.
  • analyze, investigate, and improve exit 1 when a requested analyzer (halo, hodoscope, prime, or a command) fails, after writing the report that holds its error.

Behavior change: the external-analyzer exit code, on all three commands. assertRequestedAnalysesRan covers every analyzer the run requested, not only the ones named on the command line: investigate and improve load the default traces config, so an analyzer declared in its externalAnalyzers counts as requested and a flaky one turns those two commands red. analyze passes loadDefaultConfig: false, so only its own --analyzer flags reach the check. A script that treated exit 0 as "the analyzer ran" was reading a report that said otherwise, but it will now see a failure. It belongs in the release notes.

T7 — question layout for the DSPy preview

The engine shows the model the first 500 and last 500 characters of a long input. So the question field stays under the preview size and reaches the model whole (an over-long question is rejected before any model call, naming the limit), and the answer rules sit in the first 500 characters of the instructions. Detail goes in the entry's instructions, which the model reads after the rules.

Tests

All fixtures are synthetic, written inline in the style of the existing tests. A scripted fake engine implements the engine contract and calls the real tool handlers it receives.

tests/ask.test.ts

  • Answer text survives verbatim into the result and answers.json; citations resolve; the report contains the answer.
  • Concurrency is real: with five questions and --concurrency 3, deferred promises prove exactly three run at once and the fourth starts only when one finishes.
  • Overlap is measured: six 200 ms questions give a wall time below half the summed question time.
  • Budget under concurrency: five questions reserving $0.40 and settling at $0.30 against a $1 ledger give three answers and two budget-refused failures, with total spend at or below the ceiling.
  • A budget below one call's reservation throws before any engine call; a budget that serializes the pool warns.
  • One throwing engine call leaves the other four answers intact and marks the run failed.
  • A fabricated span citation fails as unresolved-citations; an empty answer fails as no-answer.
  • Gate rejections are counted by reason and named in the report.
  • A schema-bound answer parses; one that breaks the schema fails with the field errors.
  • Layout: the question field stays inside the preview and the rules sit in the instructions' first 500 characters.

tests/cli.test.ts

  • analyze --analyzer false writes the report, then exits 1 naming the analyzer.
  • ask with three questions against a fake TRACES_PYTHON bridge that dies at startup: exit 1, both artifacts written, every question recorded with its failure and the bridge's reason.

tests/improvement.test.ts, tests/report.test.ts

  • A fabricated excerpt is refused by the gate, counted per analyst, and named in the analyst table's Detail cell, while the caller's log still receives every event.

Green locally: pnpm check:source, pnpm typecheck, pnpm test (828 tests, 59 files), pnpm build, pnpm check:package.

Docs

traces ask is documented in README.md (Contents, CLI reference, flag table, and an "Ask questions" section) and in docs/trace-analysts.md (questions and the preview limit, answer schemas, the run's guarantees, budget under concurrency, an SDK example, gate rejections, and the external-analyzer exit code).

Follow-up that needs agent-eval

Nothing here is blocked on agent-eval, but three items would remove workarounds this PR carries:

  • A3. If the registry gained an answer field and a concurrency option, analyze --llm would get the same benefits without a separate command, and ask could drop its own pool.
  • A2. The gate compares an excerpt against decoded attribute text while searchTrace returns escaped raw JSON, so a correctly copied excerpt is refused. This PR only makes the refusal visible; T3 (PR fix(analyst): normalize model citations before the evidence gate #107) normalizes it in postProcess, and A2 would fix it at the owner. Passing the Node error text through the bridge would also stop raise_for_status from hiding causes, which is why one failure kind here is matched by message text rather than by class.
  • A1. searchTrace hits carry no time fields and cap at 500 matches with no cursor, so enumeration and "last X" questions still cannot be answered exhaustively however the questions are asked.

Review repairs (185f975)

The blocking finding is fixed, with a test that fails against the previous commit, and every non-blocking item except one is addressed.

Blocking — Ctrl-C discarded every answer. All three suggested steps:

  1. verifyCitations takes no signal. The store index is built by openAgenticTraceStore before the first question runs, so it is an in-memory lookup and need not be cancellable.
  2. The whole post-engine block (citations plus the schema parse) runs inside its own try/catch that records { kind: 'aborted' | 'error' } on that answer, so no later addition can escape either.
  3. The worker pool no longer lets an exception leave with the answers: on a rejection, every question the pool did not fill is recorded with the cause, the reason is added to warnings, and runTraceQuestions still returns. cmdAsk therefore reaches writeTraceQuestionsArtifacts on every path where an answer exists, and decides the exit code from the written result.

New test, in the existing style: abort in a microtask after the first answer returns, with a citation in that answer. It asserts [['q1','answered'],['q2','failed','aborted'],['q3','failed','aborted']], that the kept answer's citation resolved, and that both artifact files exist. Restoring only the signal on store.hasSpans turns it red.

Non-blocking, fixed:

  • 2 — budget-refused on the real path. The kind is now reconciled against the ledger: a failed question is budget-refused whenever budgetUsd - settled analyst spend < engineCallReservationFloorUsd, whatever the message says, and the original message is kept verbatim on the answer. Tested with an engine that spends $0.70 of a $1 budget and then fails with a bridge-shaped HTTPError that names no ceiling. Documented where the failure kinds are.
  • 3 — exit-code scope. The PR body above and docs/trace-analysts.md now name investigate, improve, and config-declared externalAnalyzers, and say why analyze differs.
  • 4 — wall time. The clock starts before writeAnalysisTraceFile, and totals.setupTimeMs names the writing-and-indexing part. Both appear in the report line and in answers.json.
  • 5 — effective concurrency. result.effectiveConcurrency is min(concurrency, questions); the summary line reads concurrency 8 (1 effective) when they differ.
  • 6 — ID collisions. Explicit IDs are reserved before any default is assigned. A file entry named q2 plus one positional question now yields q2, q3; q1 explicit after one default yields q2, q1.
  • 7 — doc surfaces. The README flag table's --otlp row lists ask, --source-bundle has its own row, and both shipped skills mention ask. inspect-agent-traces is capped at 5,000 bytes by tests/skills.test.ts, so the new section is paid for by tightening prose in place; no instruction was dropped.
  • 9 — report cosmetics. analystRunDetail now budgets the whole cell, so a rejection summary cannot push it past the length ANALYST_DETAIL_MAX_CHARS names (tested with a 400-character reason). Answer text embedded in report.md has its own Markdown headings demoted below the question's ## section, with fenced blocks left alone, so an answer cannot reflow the report's outline.

Non-blocking, not fixed — 8, no end-to-end CLI test of a successful ask. It cannot be closed offline. createDspyRlmTraceEngine asserts parsed.modelCalls === modelProxy.successfulCompletions() and then calls modelProxy.assertExecutionComplete(), so a synthetic TRACES_PYTHON bridge cannot report a successful run without actually driving the model proxy, which forwards to the configured provider. A CLI success case therefore needs a live provider call, which the test suite must not make. The in-process tests cover the success path through runTraceQuestions, which is where the answer, citation, budget, and artifact logic lives; cmdAsk's remaining exit-0 branch is the artifact write plus the two stderr lines. Happy to add it behind an opt-in environment guard if you would rather have it than not.

Local check set green on 185f975: pnpm check:source, pnpm typecheck, pnpm test (833 tests, 59 files), pnpm build, pnpm check:package.

Second review repairs (541e2eb)

Blocking — a correct citation in Markdown emphasis scored as unresolved. traceCitationsInText trimmed only .:!?, while the span-id class admits *, _ and ~, so **trace://<t>/span/<s>** carried its closing delimiters into the span ID, resolved against nothing, and failed the question with unresolved-citationsask exited 1 on a correct answer. The trailing strip is now /[.,:;!?*_~]+$/, covering emphasis and sentence punctuation in either order. Two tests, both red before the change: the unit case on traceCitationsInText for **…**, _…_ and ~~…~~, and an end-to-end case through runTraceQuestions where a bold citation resolves and result.ok is true. The answer rules were left alone on purpose: they must fit the 500-character DSPy preview head, which has no room for a formatting sentence, and the parser is the right place for the fix.

Non-blocking, fixed:

  • 2 — rejection causes. findingRejectionDetail reads the cause from fields.reason for any kind and from fields.issues for a schema failure, and omits a lead the message already carries, so finding rejected: schema failure — schema failure is gone and the dspy engine's bridge-row rejection keeps its cause. analystLog prints the message alone when nothing is left to add. Documented.
  • 3 — failure-kind precedence. One questionFailureKind decides in order: aborted, a refusal that names itself, a bridge mismatch (stays error), an empty bridge answer (no-answer), then the ledger fallback. The CLI's reinstall hint now fires on the message whatever the kind, so a mismatch on a nearly-exhausted budget keeps its actionable line.
  • no-answer reachability. The bridge's returned no answer maps to the kind, which was otherwise unreachable through the real engine.
  • Engine before collectSpans. A missing TANGLE_API_KEY is reported before a large session is parsed.
  • Multi-line questions. The report heading folds the question onto one line; answers.json keeps it verbatim.
  • Worker pool. Promise.allSettled over the workers, so a rejection cannot hand back an answers array the remaining workers still write into.
  • Answer schemas. assertAnswerSchema rejects required, properties or additionalProperties without "type": "object", and items without "type": "array"; { required: ['a'] } previously accepted the answer 5.

Not changed: the parseTraceSpanEvidenceUri duplication is an agent-eval export request (follow-up list next to A1/A2/A3); ask keeps printing the report to stdout with --dir, because its output is the answer rather than an artifact pack, and the pointer stays on stderr; the unknown-command exit code stays with F8.

Local check set green on 541e2eb: pnpm check:source, pnpm typecheck, pnpm test (839 tests, 59 files), pnpm build, pnpm check:package.

🤖 Generated with Claude Code

`traces ask` and the exported `runTraceQuestions` run one or many questions
over one or many sessions. Each question is its own `runTraceAnalyst` call,
so the engine's prose answer survives; the analyst registry keeps only
findings and runs analysts one at a time.

- Questions run concurrently through a worker pool with a configurable limit.
  Unlike the import pool, one failed question never stops the others.
- One shared `CostLedger` bounds the whole run, so `--budget` means the same
  thing whatever the number of questions; `--question-budget` bounds one
  question. A budget below one call's reservation refuses the run before any
  model call, and a budget that serializes the pool warns instead.
- Every `trace://` citation in an answer is resolved against the store; an
  unresolvable citation fails that question.
- A question may fix its answer's shape with a small JSON Schema subset. An
  unsupported keyword is rejected rather than ignored.
- The question layout fits the DSPy input preview: the question stays whole
  and the answer rules sit in the first 500 characters of the instructions.
- Output is `answers.json` and `report.md`, written before the exit code is
  decided; exit 1 when any question failed.

Also surface what the evidence gate refused, and stop reporting a failed
external analyzer as success:

- `finding rejected` log lines now name the reason and the offending URI,
  and rejection counts by reason reach the analyst table's Detail cell,
  `TraceInvestigationResult.findingRejections`, and the ask JSON.
- `analyze` exits 1 when a requested `--analyzer` fails, after writing the
  report that holds its error.

The `--llm` engine construction moves to `analysisEngineFromEnv` in
`analyst-model-call.ts`, and the shared trace-file and store setup to
`analysis-store.ts`, so `ask` reuses both instead of duplicating them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@drewstone drewstone left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adversarial review — T1 / T2 / T7

I checked out origin/feat/ask-questions (55044df) in a clean worktree, read the whole diff, and ran pnpm install --frozen-lockfile, check:source, typecheck, test, build, check:package locally. All green (828 tests, 59 files), and both CI jobs pass.

I verified each added test fails against origin/main's src/ by copying the changed test files onto a main worktree that shares this branch's node_modules:

Test On main
analyze --analyzer false exits 1 fails: analyze exited 0 although its requested analyzer failed
ask needs at least one question fails: ask exited 0 without a question
ask writes artifacts then exits 1 fails: expected '' to contain '# traces ask'
runTraceInvestigation counts gate rejections fails: findingRejections is undefined
analystRunDetail names rejection reasons fails: expected '—'

tests/ask.test.ts imports ../src/ask.js, so it cannot exist on main at all. No test passes vacuously.

Privacy: clean. Every fixture is synthetic and generic (git status --short && pnpm test, 12 passed, Running the checks now.). No absolute paths, home directories, rollout names, session UUIDs, or keys anywhere in the diff.

Reuse: clean. analysisEngineFromEnv genuinely replaces the duplicated --llm construction rather than adding a second one, and analysis-store.ts collapses the four copies of write-file-then-open-store. The repo carries no JSON Schema library (deps are the four @tangle-network/* packages plus sandbox), so the answer-schema.ts subset is not re-implementing something already present. traceCitationsInText is a parser; the existing spanEvidenceUri is only a builder.

Unknown shapes are counted, not guessed. I checked every finding rejected: emission in agent-eval 0.179.0 (kind-factory schema failure / subject / insufficient citations / rejectEvidence, plus dspy-rlm-engine's bridge-row rejection). All five are matched by REJECTION_MESSAGE, unresolved evidence correctly defers to fields.reason, and the registry's [<id>] prefix and field pass-through are preserved. preparedContext reports omitted_traces rather than silently truncating. Per-question cost attribution is sound: runTraceAnalyst builds costTags = { analystId, analystRunId: correlationId } and settles the receipt through that filter, so with id: ask.<qid> and correlationId: <runId>:<qid> each question's usage is its own. I also confirmed the DSPy engine really threads request.costLedger into startExternalOptimizerModelProxy, so the shared-budget claim holds against the real engine and not only the fake one, and that executionConfig really carries pricing.outputUsdPerMillion, max_output_tokens, and max_reasoning_tokensengineCallReservationFloorUsd is live in production, not dead code that only the hand-written test config satisfies. OtlpFileTraceStore shares its index through createSharedAbortableTask, so one store across concurrent questions is safe.

No regression found in other adapters, report formats, the evidence JSONL schema, or the improvement artifacts: findingRejections is an additive optional field on TraceInvestigationResult, TraceEvidenceRow is untouched, and analystRunDetail gained an optional second parameter.


Blocking

1. Ctrl-C on a run whose answers carry citations throws away every answer

src/ask.tsverifyCitations sits outside askOne's try/catch and forwards the run's AbortSignal to store.hasSpans, which begins with context?.signal?.throwIfAborted(). When the signal is aborted after an answer came back, askOne rejects, Promise.all rejects, and runTraceQuestions throws. cmdAsk then never reaches writeTraceQuestionsArtifacts, so no answers.json and no report.md are written, and the answers already paid for are gone.

Reproduced with a scripted engine on the branch (three questions, concurrency: 1, abort fired in a microtask after the first answer returns):

answer contains trace://<t>/span/tool-1  →  THREW Error: ctrl-c
answer contains no trace:// URI          →  OK [["q1","answered",null],["q2","failed","aborted"],["q3","failed","aborted"]]

The only difference is whether the answer cites a span, which pins the escape to verifyCitations.

This contradicts what the PR and the docs promise for exactly the path cmdAsk installs a SIGINT handler to serve:

  • PR body: "Both are written before the exit code is decided"
  • docs/trace-analysts.md: "The artifacts are written before the exit code is decided" and "A failed question never stops the others. Its failure is recorded on its own answer, and the remaining answers are written."

Interrupting a long concurrent run is the case where an operator most wants the partial answers already bought. Suggested fix, smallest first:

  1. Drop the signal from citation verification — it is an in-memory lookup against an index that is already built, so it does not need to be cancellable: await store.hasSpans({ trace_id, span_ids }).
  2. Wrap the whole post-engine block (verifyCitations, schema parse) in a try/catch that records { kind: 'aborted' | 'error' } on that answer, so no later addition to that block can escape either.
  3. In cmdAsk, write whatever artifacts exist even when runTraceQuestions throws, so the guarantee holds at the command level too.

A test in the style of the existing ones: abort after the first answer, assert three recorded answers (answered, aborted, aborted) and that both artifact files exist.


Non-blocking

2. budget-refused is only proven on the in-process path, and the real path will not reach it. isBudgetRefusal matches /would exceed ceiling|model cost limit reached/ on the message text. The test drives it by having the fake engine rethrow the ledger's own runPaidCall error inside Node. In a real run the refusal happens inside the model proxy, and this PR's own A2 note says the bridge's raise_for_status hides the Node error text — so shared-ledger exhaustion will most likely land as failed: error with a raise_for_status message. Exit code and retained answers are still right, but the report's failure taxonomy misleads in the one scenario --budget exists for. Either say so where the failure kinds are documented, or reconcile the kind against the ledger at failure time (if budgetUsd - settled spend < engineCallReservationFloorUsd, call it budget-refused whatever the message says).

3. The exit-code change is wider than the release note. assertRequestedAnalysesRan also runs in cmdInvestigate and cmdImprove, and externalFailureMessage sees result.external, which mergeTracesConfig fills from config.externalAnalyzers as well as --analyzer. investigate and improve load a default config file (analyze passes loadDefaultConfig: false), so a repo whose default traces config declares a flaky external analyzer now sees those two commands flip to exit 1. That is defensible behavior, but the PR body and docs/trace-analysts.md both say only "analyze exits 1 when a requested --analyzer fails". Name investigate, improve, and config-declared analyzers in the release note.

4. totals.wallTimeMs excludes setup. runStarted is taken after writeAnalysisTraceFile and openAgenticTraceStore, so the stderr line wall N s and the report's "Wall time" omit writing and indexing the OTLP file. On a 48 MB session that is not free, and wall time is the metric this command exists to improve. Either start the clock at the top of runTraceQuestions or report setup separately.

5. result.concurrency is the requested value, not the effective one. With one question and --concurrency 8 the report reads concurrency 8 while only one worker ever exists (effectiveConcurrency = min(concurrency, questions.length)). peakConcurrency carries the truth, but the summary line does not. Cheap to report both.

6. Default IDs collide with explicit ones. IDs default to q<index+1> over the combined file-then-flags list, so a file entry with "id": "q2" plus one positional --question both resolve to q2 and the run dies with duplicate question ID "q2" before anything runs. Assign defaults after explicit IDs are reserved, or namespace them.

7. Doc surfaces that still omit ask. --help was updated but the README flag table's --otlp row still reads "validate, analyze, investigate, improve, stream", and --source-bundle is absent from that table although collectSpans now accepts it for ask. The two shipped agent skills (skills/inspect-agent-traces/SKILL.md, skills/build-trace-analyst/SKILL.md) enumerate the command set and do not mention ask — they are what an agent reads to pick a command.

8. No end-to-end CLI test of a successful ask. Both tests/cli.test.ts cases are failure paths (no question; dead bridge). Artifact writing is covered by the dead-bridge case, but the exit-0 branch of cmdAsk is not exercised anywhere. The in-process tests cover success, so this is a gap rather than a hole — a scripted-engine CLI case would close it.

9. Two cosmetic report issues. analystRunDetail appends the rejection text after condenseAnalystError(raw, ANALYST_DETAIL_MAX_CHARS), so the Detail cell can now exceed the length that constant names. And answer text is embedded raw under ## <id>: <question> in report.md; I confirmed an answer of # traces ask plus a Markdown table renders as a second header and table inside the report. Neither is a security issue, but the per-question section has no delimiter.


Everything except item 1 is fine to land as follow-up. Item 1 is a few lines and has a direct test.

Ctrl-C on a run whose answers carry `trace://` citations discarded every
answer. `verifyCitations` sat outside `askOne`'s try/catch and forwarded the
run's AbortSignal to `store.hasSpans`, which starts with `throwIfAborted()`.
Once aborted, `askOne` rejected, the worker pool rejected, `runTraceQuestions`
threw, and `cmdAsk` never reached `writeTraceQuestionsArtifacts`: no
`answers.json`, no `report.md`, and the answers already paid for were lost.
That is the one path `cmdAsk` installs a SIGINT handler to serve, and it
contradicted both the docs and the command's own guarantee.

- Citation checking takes no signal. The index is built before the first
  question runs, so it is an in-memory lookup and need not be cancellable.
- The whole post-engine block (citations plus the schema parse) runs inside its
  own guard that records `aborted` or `error` on that answer, so no later
  check can throw an answer away either.
- The worker pool no longer lets an exception escape with the answers: any
  unfilled question is recorded with the cause and the run still returns, so
  `cmdAsk` writes both artifacts and decides the exit code from them.

Also, from the same review:

- `budget-refused` is decided from the ledger, not only from the error text.
  The refusal happens behind the DSPy bridge, whose HTTP error handling can
  replace the Node message, so a failure is reported as `budget-refused`
  whenever settled spend left less than one call's reservation.
- `totals.wallTimeMs` now starts before the trace file is written and indexed,
  with `totals.setupTimeMs` naming that part; wall time is the metric this
  command exists to improve.
- `result.effectiveConcurrency` reports the workers actually created, next to
  the requested limit, instead of only the request.
- Default question IDs are assigned after explicit ones are reserved, so a file
  entry named `q2` plus a positional question no longer kills the run.
- `analystRunDetail` bounds the whole cell, so appending a rejection summary
  cannot push it past the length `ANALYST_DETAIL_MAX_CHARS` names.
- An answer's own Markdown headings are demoted below its question's section,
  so an answer cannot reflow the report's outline.
- README, docs/trace-analysts.md, and both shipped skills name `ask`, the
  wider exit-code scope (`investigate`, `improve`, and config-declared
  analyzers), and the interrupt guarantee.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@drewstone

Copy link
Copy Markdown
Contributor Author

Repaired in 185f975. The blocking finding is fixed with all three of your steps, and every non-blocking item except 8 is addressed.

1 — Ctrl-C discarded every answer. Confirmed and fixed exactly as suggested.

  1. verifyCitations(text, store) no longer takes a signal. openAgenticTraceStore calls ensureIndexed() before the first question runs, so the lookup is in-memory and does not need to be cancellable.
  2. The whole post-engine block — citations plus the schema parse — runs inside its own try/catch that records { kind: 'aborted' | 'error' } on that answer, so a later addition to that block cannot escape either.
  3. The worker pool no longer lets an exception leave with the answers. On a rejection every unfilled question is recorded with the cause, the reason lands in warnings, and runTraceQuestions still returns. cmdAsk therefore always reaches writeTraceQuestionsArtifacts when an answer exists, and decides the exit code from the written result — which is the command-level guarantee your step 3 asked for, expressed where the answers live rather than as a second write path in cmdAsk.

The test is the one you described: three questions, concurrency: 1, abort fired in a microtask after the first answer returns, that answer carrying trace://<t>/span/tool-1. It asserts [['q1','answered',null],['q2','failed','aborted'],['q3','failed','aborted']], that the kept answer's citation resolved, and that answers.json and report.md both exist. Restoring only the signal on store.hasSpans turns it red (q1 becomes failed: aborted); on the original code, without the new guard, the run throws outright.

2 — budget-refused on the real path. You are right that message matching cannot reach it. The kind is now decided from the accounting: a failed question is budget-refused whenever budgetUsd - settled analyst spend < engineCallReservationFloorUsd at the time it failed, whatever the message says, and the message is kept verbatim on the answer. Tested with an engine that settles $0.70 of a $1 budget on the first question and then fails with a bridge-shaped HTTPError naming no ceiling. docs/trace-analysts.md says so where the failure kinds are documented.

3 — exit-code scope. Named in both the PR body and the docs: the check runs on analyze, investigate, and improve, and covers config-declared externalAnalyzers, with the reason analyze differs (loadDefaultConfig: false).

4 — wall time. The clock starts before writeAnalysisTraceFile. totals.setupTimeMs names the writing-and-indexing part, and the report line reads Wall time X s (Y s of it writing and indexing the trace file) against ....

5 — effective concurrency. result.effectiveConcurrency is min(concurrency, questions). One question with --concurrency 8 now reads concurrency 8 (1 effective).

6 — ID collisions. Explicit IDs are reserved before any default is assigned, and a default keeps its own position where it can. A file entry "id": "q2" plus one positional question gives q2, q3; a default followed by an explicit q1 gives q2, q1.

7 — doc surfaces. The --otlp row lists ask, --source-bundle has its own row, and both skills mention it. tests/skills.test.ts caps inspect-agent-traces at 5,000 bytes and it was at 4,984, so the new section is paid for by tightening prose in place — no instruction was dropped, and the diff shows the trades.

9 — report cosmetics. analystRunDetail budgets the whole cell now: the rejection summary is capped first and the condensed error takes what is left, so the cell keeps the same bound a lone condensed error keeps. Tested with a 400-character rejection reason. Answer text in report.md has its own Markdown headings demoted below the question's ## section, with fenced blocks left alone, so # traces ask inside an answer no longer closes the section.

8 — no CLI test of a successful ask: not fixed, and I do not think it can be offline. createDspyRlmTraceEngine checks parsed.modelCalls === modelProxy.successfulCompletions() and then calls modelProxy.assertExecutionComplete(), so a synthetic TRACES_PYTHON bridge cannot report a successful run without actually driving the model proxy, which forwards to the configured provider. A CLI success case therefore needs a live provider call. The in-process tests cover the success path through runTraceQuestions, where the answer, citation, budget, and artifact logic lives; what remains uncovered in cmdAsk's exit-0 branch is the artifact write and two stderr lines. Say the word if you want it behind an opt-in environment guard instead.

Local check set green: pnpm check:source, pnpm typecheck, pnpm test (833 tests, 59 files), pnpm build, pnpm check:package.

@drewstone drewstone left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adversarial review — PR #108 (feat/ask-questions, head 185f975)

Reviewed in a fresh worktree off origin/feat/ask-questions. I ran the full check set myself and re-ran the new tests against the pre-change source to confirm they are load-bearing.

Checks

Check Result
pnpm install --frozen-lockfile ok
pnpm check:source ok
pnpm typecheck ok
pnpm test 833 passed, 59 files
pnpm build ok
pnpm check:package ok
gh pr checks CI (Node 22.13.0) pass, CI (Node 24.18.0) pass
git merge-tree origin/main HEAD no conflicts

Do the tests fail without the change?

Yes, everywhere I could isolate it.

  • tests/ask.test.ts against 55044df's src/ask.ts (the pre-repair commit): 6 red — overlap/setupTimeMs, the Ctrl-C abort case, the ledger-reconciled budget-refused, heading nesting, effectiveConcurrency, and explicit-ID reservation. Each repair item in the PR body has a test that is red before it and green after.
  • tests/report.test.ts, tests/improvement.test.ts, tests/cli.test.ts against origin/main's src/: 6 red — both analystRunDetail cases, the findingRejections investigation case, analyze --analyzer false exit 1, and both traces ask CLI cases.

The concurrency test is a real test, not a timing coincidence: deferred gates prove exactly three of five questions are in flight and that the fourth starts only when one finishes, and a separate case measures wall time under half the summed question time.

Privacy

Clean. Every fixture is inline synthetic JSONL in the existing style (git status --short && pnpm test, session-ask, trace-ask). No path, session id, hex rollout id, or transcript text from this machine appears anywhere in the diff. The only URL added is the pre-existing router.tangle.tools constant, moved out of cli.ts.

Regressions I looked for and did not find

  • Adapters are untouched. Evidence JSONL schema is untouched; findingRejections is an additive optional field on TraceInvestigationResult and ReportMeta.
  • Report format: analystRunDetail keeps the same cell bound when a rejection summary shares it (tested with a 400-character reason), and the Detail cell is still when both parts are empty.
  • Store sharing under concurrency is safe: BufferedOtlpTraceStore memoizes the index behind indexTask/indexValue, and ensureIndexed() is awaited once before the pool starts, so every question's tool call is an in-memory lookup over one shared index. createDspyRlmTraceEngine's analyze() builds its own tool callback, model proxy, and Python process per call, so one engine object driving N concurrent questions has no shared mutable state.
  • Ledger accounting is correct: summary({ channel: 'analyst' }) matches the channel the dspy engine actually books under, totalCalls counts settled receipts only so totalCalls + pendingCalls does not double-count, and per-question usage comes from runTraceAnalyst's tag-filtered settlement, which isolates questions from each other.
  • Unknown counts are counted, not guessed: modelCalls/toolCalls collapse to null via sumOrNull if any question's count is unknown, cost provenance keeps uncaptured with a null amount, omitted_traces counts traces past MAX_CONTEXT_TRACES, and unparseable span times give null, not 0.
  • Duplication: analysis-store.ts genuinely removes the second copy of the store construction (no new OtlpFileTraceStore or GENERATED_TRACE_FILE_CEILING remains outside it), and analysisEngineFromEnv removes the second engine construction as the brief asked.

The investigate/improve exit-code widening to config-declared externalAnalyzers is a real behavior change, but it is deliberate, documented in docs/trace-analysts.md, and ok: false on an ExternalAnalysisResult only ever means a genuine failure (runExternalAnalyzers never sets it for a skip).


Blocking

1. A correct citation written in Markdown emphasis is scored as unresolved, and the question fails.

src/ask.tsTRACE_URI excludes backticks, brackets, and parentheses from the span-id character class, and the trailing strip is /[.:!?]+$/. It does not cover *, _, or ~, which are exactly the characters a model puts around a citation when it emphasises it.

"bold **trace://t/span/abc**"      -> "trace://t/span/abc**"
"emph _trace://t/span/abc_"        -> "trace://t/span/abc_"
"tilde ~~trace://t/span/abc~~"     -> "trace://t/span/abc~~"

Reproduced end-to-end through runTraceQuestions with the synthetic fixture: an answer reading The session ran one command (**trace://trace-bold/span/tool-1**). — where tool-1 is in the trace — comes back as

status  failed
citation {"uri":"trace://trace-bold/span/tool-1**","spanId":"tool-1**","resolved":false}
failure {"kind":"unresolved-citations","message":"1 cited span(s) do not exist: ..."}

so result.ok is false and ask exits 1 on a correct answer. This is the headline guarantee of T1 ("every trace:// citation is checked") returning a wrong verdict, and it drives the command's exit code. The answer text survives in answers.json, so it fails loudly rather than silently — but an operator scripting on the exit code gets a false negative, and the rules the model is given say nothing about formatting.

Suggested fix: extend the trailing strip to the Markdown emphasis run (/[.,:;!?*_~]+$/), or resolve once as-matched and retry with emphasis stripped before declaring a citation unresolved. Add a case to the existing traceCitationsInText test covering **…**, _…_, and ~~…~~ — it is red today.


Non-blocking

2. findingRejectionDetail drops the field that names the cause for two of the five rejection kinds, and echoes itself for a third.
fields.reason is read only when the message is unresolved evidence. The dspy engine's own finding rejected: bridge row failed schema validation carries the cause in fields.reason (<code> at <path>: <message>) and finding rejected: schema failure carries it in fields.issues; both are discarded, and the CLI line becomes finding rejected: schema failure — schema failure. docs/trace-analysts.md says "the CLI log prints the reason and the offending URI on each finding rejected line", which is only true for one kind. Cheap fix: fall back to fields.reason for any kind, add issues, and suppress the suffix when the detail equals the message.

3. The ledger reconciliation can relabel a genuine engine failure as budget-refused.
ledgerIsExhausted returns true whenever budgetUsd - settled < floor at the moment a question failed, whatever the cause. A bridge-version mismatch on the last question of a nearly-exhausted budget is reported as budget-refused — and cmdAsk's bridge hint only fires for failure.kind === 'error', so the actionable "install agent-eval-rpc[dspy]==<v>" line is suppressed exactly when it is needed. Consider checking isBridgeMismatchError before the ledger reconciliation, or emitting the hint on any kind whose message matches.

4. no-answer is unreachable through the real engine.
parseBridgeOutput throws DSPy RLM bridge returned no answer when answer is empty, so runTraceAnalyst rejects and the kind is error; only the fake engine can produce no-answer. Either map that message to no-answer, or drop the kind so the documented list matches what can actually occur.

5. The missing-key error arrives after the session is parsed.
cmdAsk calls collectSpans before analysisEngineFromEnv, so an operator with no TANGLE_API_KEY waits through a large session's adapter pass to be told the key is missing. Building the engine before collectSpans costs nothing and fails in a second.

6. A question containing a newline breaks the report heading.
normalizeTraceQuestions trims but does not reject internal newlines, and renderTraceQuestionsReport emits ## ${id}: ${question} raw. Collapse whitespace in the heading, or reject a multi-line question with the same message style as the length check.

7. The pool's failure path returns an array other workers are still writing into.
On poolError, wallTimeMs is taken and the holes are filled while the remaining workers are still awaiting askOne; their later answers[index] = … mutates the array already handed to the caller. Unreachable today because askOne catches everything, but that is the case this branch exists for. Promise.allSettled over the workers would make the branch safe on its own terms.

8. answerSchemaErrors skips required when the value is not an object.
{ "required": ["a"] } with no "type": "object" accepts the answer 5 with no error. Either require a type alongside required in assertAnswerSchema, or report a non-object against a schema carrying required.

9. The trace://…/span/… parse duplicates agent-eval's parseTraceSpanEvidenceUri.
Same regex, same decodeURIComponent and try/catch, but it is internal to kind-factory and not exported. Nothing to do here; it belongs on the PR's existing agent-eval follow-up list next to A1/A2/A3.

10. ask prints the whole report to stdout even with --dir.
improve prints a one-line pointer to the artifact directory. On a many-question run with long answers this is a lot of scrollback that is already in report.md.

11. An unknown command still prints usage and exits 0.
default: usage() in main. That is the other half of the brief's F8 bullet that motivated ask; out of scope for T1 as written, but it is one line and the same class of "exit 0 that means failure" this PR is otherwise closing.


Item 8 in the PR body (no end-to-end CLI test of a successful ask) — the argument is sound: createDspyRlmTraceEngine asserts parsed.modelCalls === modelProxy.successfulCompletions() and then modelProxy.assertExecutionComplete(), so a synthetic bridge cannot report success without driving the real proxy. I would not hold the PR for it, and I would not add an environment-guarded live-provider test to the suite either.

Fix item 1 and this is good to merge.

`traceCitationsInText` trimmed only `.:!?` from a matched URI, and the
span-id character class admits `*`, `_` and `~`. An answer that cited a
real span as `**trace://<t>/span/<s>**` therefore carried the closing
delimiters into the span ID, resolved against nothing, and failed the
question with `unresolved-citations` — so `traces ask` exited 1 on a
correct answer, and the command's headline guarantee returned a wrong
verdict. The trailing strip now covers the Markdown emphasis run.

Also from the same review, each with its own test:

- `findingRejectionDetail` reads the cause from `reason` for any
  rejection kind and from `issues` for a schema failure, and adds only
  what the message does not already say, so the CLI line no longer
  degenerates to "finding rejected: schema failure — schema failure".
- A failure whose own text names its cause is no longer relabelled
  `budget-refused` by the ledger reconciliation: a bridge mismatch stays
  `error` (keeping the CLI's reinstall hint, which now fires on the
  message whatever the kind) and an empty bridge answer maps to the
  `no-answer` kind, which was unreachable before.
- `cmdAsk` builds the engine before the adapter pass, so a missing
  TANGLE_API_KEY is reported before a large session is parsed.
- A multi-line question is folded onto one line in the report heading;
  `answers.json` keeps it verbatim.
- The worker pool awaits every worker with `allSettled`, so a rejection
  cannot hand back an answers array the remaining workers still write to.
- `assertAnswerSchema` rejects `required`, `properties` or
  `additionalProperties` without `"type": "object"`, and `items` without
  `"type": "array"`: those constraints are skipped for an answer of any
  other shape, so `{ required: ['a'] }` accepted the answer `5`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@drewstone

Copy link
Copy Markdown
Contributor Author

Repaired in 541e2eb. The blocking finding is fixed, and every non-blocking item except two is addressed; those two are answered below rather than changed.

1 — a correct citation in Markdown emphasis scored as unresolved. Confirmed and fixed at the parser. The trailing strip is now /[.,:;!?*_~]+$/, applied to the matched URI, so the closing run of **…**, _…_ or ~~…~~ is trimmed like sentence punctuation, in either order ((**trace://t/span/abc**). and **trace://t/span/def.** both resolve). Trace and span IDs are adapter-assigned and do not end in those characters, so trimming them cannot hide a citation that would otherwise resolve — the same trade the existing .:!? strip already makes.

Two tests, both red before the change:

  • the unit case you asked for on traceCitationsInText, covering **…**, _…_, ~~…~~, and emphasis interleaved with sentence punctuation;
  • an end-to-end case through runTraceQuestions with the synthetic fixture: the answer The session ran one command (**trace://trace-ask/span/tool-1**). now yields resolved: true, status: answered, and result.ok true, which is what decides the exit code.

I did not add a "cite bare, without emphasis" line to ASK_RULES. The rules must fit the 500-character DSPy preview head, and the existing budget has about 14 characters of slack — the sentence pushed rule 6 out of the preview and turned tests/ask.test.ts red. The fix belongs in the parser anyway: the model is not the one that has to be careful here. The reason is now a comment on rule 3.

2 — findingRejectionDetail dropped the cause for two kinds and echoed itself for a third. The detail is now "what the event carries beyond its own message": the cause comes from fields.reason for any kind (the gate's rejections and the dspy engine's bridge row failed schema validation) or from fields.issues for a schema failure, and the leading part is omitted when the message already ends with it. analystLog prints msg alone when nothing is left to add, so finding rejected: schema failure — schema failure is gone, and finding rejected: insufficient evidence citations now reads — 1 of 2 required distinct citation(s) instead of repeating its own kind. That last one changes an existing expectation in tests/finding-rejections.test.ts; the new case covers all three shapes. docs/trace-analysts.md now states the actual rule instead of claiming every line prints a reason.

3 — the ledger reconciliation relabelling a genuine failure. Fixed by ordering, in one questionFailureKind function: aborted, then a refusal that names itself, then a bridge mismatch (stays error), then an empty bridge answer, and only then the ledger fallback. The CLI's reinstall hint no longer requires kind === 'error' either — it fires on the message, whatever the kind. Tested with $0.70 settled against a $1 budget and a $0.41 reservation (so the ledger is exhausted) and two later failures: the bridge one stays error, the empty-answer one is no-answer.

4 — no-answer unreachable through the real engine. Same change: parseBridgeOutput's returned no answer now maps to the no-answer kind, so the kind describes a real path instead of only the in-process one. The empty-answer branch inside askOne keeps it reachable for an engine that returns rather than throws.

5 — engine built after collectSpans. Moved: analysisEngineFromEnv now runs before the adapter pass, so a missing TANGLE_API_KEY is reported immediately.

6 — a multi-line question breaking the report heading. The heading folds the question's whitespace onto one line; answers.json keeps the question verbatim, so nothing is lost. I did not reject newlines in normalizeTraceQuestions: a question read from a file can reasonably span lines, and the report is the only surface that needs one.

7 — the poolError branch mutating an array already returned. The pool now awaits every worker with Promise.allSettled and takes the first rejection from the settled results, so the branch is safe on its own terms rather than only because askOne catches everything.

8 — required without a type. assertAnswerSchema now rejects required, properties and additionalProperties without "type": "object", and items without "type": "array"; a union such as ["object", "null"] still passes. That is the same fail-closed rule the module already applies to unknown keywords: a constraint that is skipped for the answer's actual shape would let a wrong answer pass as checked. Documented next to the supported-keyword list.

Not changed, with reasons:

  • parseTraceSpanEvidenceUri duplication. Agreed — it belongs on the agent-eval follow-up list next to A1/A2/A3 (export it from @tangle-network/agent-eval/analyst). Nothing to change in traces, and this PR does not modify agent-eval.
  • The whole report on stdout with --dir. Deliberate, and different from improve. improve's output is an artifact pack; ask's output is the answer, so traces ask … | less and traces ask … --dir out should both put the answers where a person and a pipe can read them. --dir says where to keep them, not that they should be hidden. The one-line pointer is still on stderr, so redirecting stdout gives the report alone.
  • Unknown command exits 0. Out of scope for T1, as you noted, and it is the default: arm shared with traces and traces help; splitting an unknown command from those is a change to the top-level dispatch that belongs with F8 rather than here.

Local check set green on 541e2eb: pnpm check:source, pnpm typecheck, pnpm test (839 tests, 59 files), pnpm build, pnpm check:package. git merge-tree origin/main HEAD is clean.

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.

1 participant