fix: deterministic session-intent routing, honest tool governance, and completion hard gate (#100) - #106
Closed
PM-pinou wants to merge 20 commits into
Closed
Conversation
ACTION_NOT_ALLOWED_IN_PHASE previously told the model only what it could not do, so a mis-routed agent probed tools one by one (observed 4 rejected calls in issue #100). The rejection message now carries the current phase's allowedActions as a fourth segment, and the model-facing observation surfaces them in error.details.allowedActions plus a concrete recovery instruction. The message keeps its three-segment prefix, so existing split(":") parsers and startsWith checks are unaffected.
…uctions The system prompt advertised every selected data tool regardless of the governing protocol, while general-task rejects all four data actions at the phase gate. A mis-routed run therefore told the model it had tools it could never use. Instructions now state explicitly, under general-task, that the data tools are disabled by the protocol and that protocol_handoff to data-analysis is the supported path. Also introduces protocol/data-actions.ts as the single source for the governed data action names; DATA_AGENT_TOOL_NAMES re-exports it and the instructions builder consumes it instead of a hardcoded array. Protocol definitions migrate to the shared constant in a follow-up refactor.
packages/**/*.test.ts were type-checked by tsc but never executed: CI only ran the web and TUI suites plus smoke scripts, so protocol-layer behavior had no unit gate. Adds a root test:packages script (vitest declared as a root devDependency instead of relying on hoisting) and runs it in the build-and-web-tests job.
Phase rejections now carry the allowed actions of the current phase, and agent instructions declare protocol-disabled data tools instead of advertising them. Package unit tests join CI.
…d data actions completeProtocolRun auto-committed a general.answer.commit for any general-task run with assistant text, then forced a terminal decision and finalized the run as completed. A mis-routed run whose data tool calls were all rejected by the phase gate therefore ended as a metadata 'completed' even though the datasource was never touched (issue #100). The completion path now inspects the protocol action ledger first: when the governing protocol is general-task and it rejected data actions with ACTION_NOT_ALLOWED_IN_PHASE, the run records its terminal protocol decision for replay and finalizes as failed with an explicit DATA_ACTIONS_REJECTED_BY_PROTOCOL reason. Time-budget and other partial completions keep their existing completed semantics.
Covers the completion hard gate: general-task runs with phase-rejected data actions finalize as failed with DATA_ACTIONS_REJECTED_BY_PROTOCOL; non-data rejections and data-analysis runs keep completing. Extends the test:packages script to also execute the apps/api vitest suite, which was previously type-checked but never run.
Runs whose data tool calls were all rejected by the governing protocol finalize as failed instead of completed; assistant text no longer launders a refused analysis into success.
…olution
Persists the authoritative record of what a session is working on:
{protocol_id, protocol_version, intent_text, source_run_id} keyed by
session. resolveForSession walks up session_branches lineage (bounded
depth) so a branched session inherits its ancestor's task instead of
starting blind — guessing 'the previous terminal run' cannot express
this and breaks on gap runs, branches, and suspended runs. Session
deletion cascades to intents.
…ndidate Weak continuation follow-ups (再次尝试 / retry / continue …) now inherit the session's recorded protocol through the existing deterministic candidate channel at priority 300 — above the keyword accelerator, below same-run restore — so no model call, low classifier confidence, or classifier failure can drop the run to general-task while the session is mid-analysis. For ambiguous non-continuation follow-ups the classifier still runs, but its input now carries the session intent as context and its prompt explains how to use it. Weak follow-ups without any recorded intent keep today's default-route behavior byte for byte.
…xtraction and semantic query with intent text Requirement extraction previously ran before routing and was gated by the analyticIntent keyword regex, so a classifier- or intent-routed data-analysis run started with zero user requirements — the entire assertion pipeline (contract grounding, SQL semantic validation, result checks, evidence-bound claims) silently short-circuited, and the semantic service was queried with the literal follow-up wording. Protocol definitions now register requirement-free for routing, and extraction runs after the route resolves to data-analysis (including at protocol handoff, so a general-task run moving to data-analysis starts its new segment with a full contract). Both the extractor and the semantic.context.resolve query consume the effective intent text: the recorded session intent with the follow-up appended, or the user's own words whenever they carry a task. The regex no longer gates any quality-critical path — it remains only a routing accelerator.
Before assembling a run, the server resolves the session's recorded intent (following branch lineage) and passes it into the protocol boundary. After routing, strong-signal resolutions — explicit protocol, keyword accelerator, confident classifier — persist the user's wording as the session intent. Inherited weak follow-ups, restored segments, default-route fallbacks, and blank input never overwrite the record, so '再次尝试' can inherit the task but can never become the task.
…t its role Adds common English analytic phrasing (revenue, trend, breakdown, group by, cohort, …) and further Chinese metrics vocabulary to the keyword accelerator, and documents that it is a routing accelerator only — after the extraction reorder it gates no quality-critical path, so a false hit is corrected by session-intent inheritance and a miss costs one classifier call.
Sessions now record an authoritative intent (protocol + task text) that weak follow-ups inherit deterministically, requirement extraction and semantic grounding consume the intent text after routing, and the keyword regex is demoted to a routing accelerator.
…col definitions Completes the deduplication started with protocol/data-actions.ts: both formal protocol definitions now consume the shared set instead of re-declaring the four governed data action names locally.
analysis_requirements_commit and protocol_handoff hand-rolled the same ActionRouter execution body (tool-call id, fallback action id, idempotency key, observation unwrap, error shaping). Both now share createProtocolBoundExecute, which also documents why segmentId must be read at call time (handoffs move the active segment).
The run's tool set previously passed through several disconnected steps (resource-gated spreads, always-allow set, skill allow/deny filter, MCP merge) with no record of why a tool was in or out. buildToolPlan now performs the same pipeline in one place with identical semantics — deny before allow, always-allow and skill-meta exemptions, MCP joining after the skill policy under its own per-server allowlist — and every entry carries a reason trail. createDataFoundry returns the plan entries so diagnostics and audit surfaces can answer 'why does the model (not) see this tool' without a debugger. Protocol-phase permissions intentionally stay with the protocol runtime's action gate.
Data action names come from one constant, protocol-bound tools share one execute helper, and tool assembly runs through a single reasoned plan.
Helper models (protocol classifier, session title) each assembled their own prompt from a single sentence. buildHelperContext produces one compact background block — session intent, recent queries, conversation summary, relevant memories — inside reference-only delimiters so recorded history cannot act as instructions. The block is hard-capped by a registered limit (DATAFOUNDRY_HELPER_CONTEXT_MAX_CHARS) and sections drop in fixed priority order; the session intent is never dropped.
…ssion title The server assembles a helper-context block from the session intent, latest conversation summary, and recalled long-term memories, and forwards it to the protocol classifier as reference-only background for ambiguous follow-ups. The classifier prompt states explicitly that the block is history, not instructions. Session titles now prefer the session's recorded intent text, so a branched session whose first message is '再次尝试' is titled by its inherited task instead of the follow-up wording.
Helper models share one budgeted, injection-guarded background block; the classifier and session title consume it first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #100 at the root instead of patching the classifier prompt: sessions now carry an authoritative intent record that weak follow-ups inherit deterministically, tools stop being advertised as usable when the protocol forbids them, and a run whose data work was refused can no longer end as
completed.Why not the routingContext approach
Per the review discussion on #105: feeding more context to the LLM classifier lowers the failure probability but establishes no invariant — a classifier error, a low-confidence score, or a gap
general-taskrun still reproduces the bug, and only the protocol label was inherited (requirements extraction and the semantic query still ran on the literal "再次尝试" text). This branch makes continuation a deterministic fact lookup and threads the real task text through everything downstream.What's inside (five sub-branches, merged in order)
1.
fix/phase-rejection-guidanceACTION_NOT_ALLOWED_IN_PHASEnow carries the current phase'sallowedActions(message keeps its 3-segment prefix; the list rides a 4th segment + structurederror.details), so a blocked model learns the way out on the first rejection instead of probing tools one by one.general-task: they are declared protocol-disabled with an explicitprotocol_handoffpointer.chore(ci):packages/**/*.test.tsand theapps/apivitest suite were type-checked but never executed in CI; a roottest:packagesscript now runs them in the build job.2.
fix/completion-hard-gatecompleteProtocolRuninspects the protocol action ledger: ageneral-taskrun whose data actions were all phase-rejected finalizes as failed withDATA_ACTIONS_REJECTED_BY_PROTOCOLinstead of being laundered intocompletedby a closing apology message. Time-budget partials keep their existingcompletedsemantics (agreed in review of the plan).3.
fix/session-intent-routing(core)session_intentstable + repository (migration0018); resolution walkssession_brancheslineage, so branched sessions inherit their ancestor's task — the "most recent terminal run" guess (breaks on gap runs, branches, suspended runs) is not used.deterministicCandidateschannel (priority 300,SESSION_INTENT_INHERITED) — no model call involved. The classifier still handles ambiguous non-continuation follow-ups, now with the session intent as explicitly-untrusted context.semantic.context.resolvequery consume the effective intent text instead of the follow-up wording. This closes the silent hole where a classifier-routed analysis run started with zero user requirements and the whole assertion pipeline short-circuited.analyticIntentregex is demoted to a routing accelerator (documented as such), with English analytic vocabulary added.4.
refactor/tool-planDATA_ACTION_NAMESconstant (was declared in 3 places), one shared protocol-bound tool execute helper (was hand-rolled 3×), and tool assembly through a singlebuildToolPlanpipeline whose entries carry reason trails (source:*,skill-policy:*,always-allow,mcp-policy:server-allowlist). Semantics are unchanged and covered by tests;createDataFoundryreturns the plan for diagnostics.5.
feat/helper-contextbuildHelperContext: a budgeted (DATAFOUNDRY_HELPER_CONTEXT_MAX_CHARS, registered limit), injection-guarded background block (session intent, summary, recalled memories) shared with helper models. Wired to the protocol classifier and to session titles (a branched session first-messaged with "再次尝试" is titled by its inherited task).Verification
npm run build(tsc -b, all workspaces incl. tests)npm run test:packages(packages + apps/api vitest)npm run test:webnode --test)smoke-sql-readonlysmoke-agui-streamsmoke-agent-runtimemain("artifact event must be a slim reference") — pre-existing, unrelatedsmoke:agent-protocol-deepseek,smoke:task-state)Key regression scenarios covered by new tests: weak follow-up inherits without a classifier call; classifier failure/low confidence cannot affect an intent-bearing session; gap-chat and branched-session inheritance; extraction fires for classifier- and handoff-routed analysis; rejected-data-action runs cannot finalize
completed; legacy 3-segment rejection messages still parse.