diff --git a/src/api/routes/grading.py b/src/api/routes/grading.py index a4e8458..b934f15 100644 --- a/src/api/routes/grading.py +++ b/src/api/routes/grading.py @@ -20,6 +20,8 @@ router = APIRouter() +_MAX_GRADING_ATTEMPTS = 2 + SYSTEM_PROMPT = """ You grade short-text answers to onboarding knowledge-check questions. @@ -47,7 +49,7 @@ class _GradedItem(BaseModel): class _Payload(BaseModel): - results: list[_GradedItem] = [] + results: list[_GradedItem] def _build_prompt(answers: list[GradeAnswerItem]) -> list[Message]: @@ -64,6 +66,62 @@ def _build_prompt(answers: list[GradeAnswerItem]) -> list[Message]: ] +def _grade(llm: LLMClient, to_grade: list[GradeAnswerItem]) -> dict[str, _GradedItem]: + """Grade in one call, asking once more when the reply is unusable. + + An unreadable reply marks every answer in it "could not be graded" -- which the + caller records as wrong. A small local model breaks its JSON now and then, and + a hire whose right answer came back wrong for that reason has been told + something false about their own knowledge. One correction round is cheap + against that. + + Valid JSON is not enough: a reply that leaves an answer out (``{}``, or one id + missing) would record that answer as wrong just the same, so it gets the same + correction round. It must hold exactly one grade per requested id. + """ + expected_ids = {item.id for item in to_grade} + messages = _build_prompt(to_grade) + for attempt in range(_MAX_GRADING_ATTEMPTS): + try: + raw = llm.generate(messages) + except LLMUnavailableError as exc: + raise HTTPException(status_code=503, detail=str(exc)) from exc + try: + payload = _Payload.model_validate_json(extract_json_object(raw)) + graded_by_id = {item.id: item for item in payload.results} + if ( + len(payload.results) != len(to_grade) + or set(graded_by_id) != expected_ids + ): + raise ValueError( + "grading response does not match the requested answer ids: " + f"expected {sorted(expected_ids)}, got " + f"{[item.id for item in payload.results]}" + ) + return graded_by_id + except (ValidationError, json.JSONDecodeError, ValueError) as exc: + logger.warning( + "Could not parse grade-answers output (attempt %d): %s", + attempt + 1, + exc, + ) + if attempt + 1 == _MAX_GRADING_ATTEMPTS: + break + messages = [ + *messages, + Message(role="assistant", content=raw), + Message( + role="user", + content=( + f"That response could not be validated: {exc}. Return the same " + "grades again as one valid JSON object matching the schema " + "exactly. Return JSON only." + ), + ), + ] + return {} + + @router.post( "/grade-answers", summary="Semantically grade short-text knowledge-check answers", @@ -121,17 +179,7 @@ def grade_answers( ) if to_grade: - try: - raw = llm.generate(_build_prompt(to_grade)) - except LLMUnavailableError as exc: - raise HTTPException(status_code=503, detail=str(exc)) from exc - - try: - payload = _Payload.model_validate_json(extract_json_object(raw)) - graded_by_id = {item.id: item for item in payload.results} - except (ValidationError, json.JSONDecodeError, ValueError) as exc: - logger.warning("Could not parse grade-answers output: %s", exc) - graded_by_id = {} + graded_by_id = _grade(llm, to_grade) for item in to_grade: graded = graded_by_id.get(item.id) diff --git a/src/onboarding/buddy_agent.py b/src/onboarding/buddy_agent.py index 35e4b02..13d8e82 100644 --- a/src/onboarding/buddy_agent.py +++ b/src/onboarding/buddy_agent.py @@ -68,7 +68,28 @@ _SOURCE_CHARS = 800 # How many internal search hops before we force a final answer, so a confused model # can't loop forever gathering evidence it never uses. -_MAX_STEPS = 4 +# +# Raised from 4 after a testing session: only *search-only* hops consume this budget (a +# hop that asks for a backend tool returns immediately), and a model that searched four +# times before deciding to act never got to act at all -- it hit the forced answer +# below, which has no tools, and promised the hire a button that could not exist. Six +# leaves room for a thorough answer and still bounds the loop. +_MAX_STEPS = 6 + +# What the model is told when the search budget is spent. +# +# It answers the hire's question with the persona still in front of it -- "offer +# `add_path_step`", "offer to claim it" -- and without this it took those at face value +# and told the hire to confirm something no tool call had produced. A model that knows +# it has no tools this turn can only do the honest thing: answer with what it has and +# leave the offer for the next turn. +_NO_TOOLS_THIS_TURN = ( + "You have no tools available for this reply and cannot do anything or offer " + "anything: no confirm button can appear. Answer with what you already have. Do not " + "say you have done something, do not tell the hire to confirm, click or check " + "anything, and do not claim anything is on their screen. If something still needs " + "doing, say what it is and that you can set it up when they reply." +) @dataclass @@ -316,7 +337,11 @@ def run_agent_turn( # Only local searches this turn -- loop and let the model reason over them. # Step budget spent: force a final answer with no tools rather than loop forever. - forced = llm.generate(work) + # + # The notice goes to the model, not into the returned conversation: a final turn's + # messages are discarded by the caller, and a transcript carrying instructions about + # a budget nobody can see would be a strange thing to keep. + forced = llm.generate([*work, Message(role="system", content=_NO_TOOLS_THIS_TURN)]) return AgentTurnResult( final=True, text=forced, diff --git a/src/onboarding/buddy_persona.py b/src/onboarding/buddy_persona.py index 31a1f14..70b7a4d 100644 --- a/src/onboarding/buddy_persona.py +++ b/src/onboarding/buddy_persona.py @@ -29,10 +29,13 @@ from onboarding.vocabulary import DEFAULT_VOCABULARY, Vocabulary +# The buddy is a tutor, not a second onboarding. The onboarding is the path a PM's +# blueprint prescribes; the old framing -- "from their first day to doing real work" -- +# described a ramp towards a first accepted contribution that no longer exists. _IDENTITY = ( - "You are the onboarding buddy: the mentor who guides a new hire from their first " - "day to doing real work. You are warm, patient, and always available -- no " - "question is too basic.\n" + "You are the onboarding buddy: the tutor who guides a new hire through their " + "onboarding and helps with whatever comes up along the way. You are warm, " + "patient, and always available -- no question is too basic.\n" "How you work:\n" ) @@ -68,8 +71,35 @@ "not reflect the real process.\n" ) +# Picking work up is its own road, and it is open from day one. Real work alongside +# the curriculum is how people learn, and the pool belongs to everybody on the +# project -- so a mentor that withheld it until the path was far enough along would be +# inventing a gate nobody asked for, in the one place a hire cannot see it. +# +# Gated on the suggestion tool rather than on `claim_goal`: it is the read that shows +# the pool, and a role without it mounted must not be told to call it. +_DAY_ONE_CLAUSE = ( + "- Real work is open to them from day one. However far along their path they " + "are, a hire who wants something to pick up can have it. Never make it " + "conditional on onboarding progress, never imply they are not ready, and never " + "decide for them that a step should come first -- say what you would do and let " + "them choose. If the pool has nothing that fits, say that plainly: 'nothing in " + "there right now' is an answer, 'not yet' is not.\n" +) + _CLAIM_CLAUSE = "- When the hire picks a suggested task, offer `claim_goal`.\n" +# What happens *after* they claim one. Claiming is the first step of a road that ends +# in work somebody else looks at, and a mentor that goes quiet at exactly that point +# leaves the hire holding an issue id with no way in. +_CLAIMED_CLAUSE = ( + "- Once they have claimed something, stay with it. `open_orientation` is the " + "packet for the task they claimed -- what it is, where it lives, who to ask -- " + "and `search_docs` answers what the work itself raises: how this part works, " + "what the convention is, where to start reading. Think it through with them. " + "You do not write it for them, and you do not need to.\n" +) + # The one question this mentor exists for, and the one it answered from the wrong # place. "What should I work on?" is a question about *this hire*, but Starter Work # is also a product noun -- so a mentor told to use `search_docs` for how the product @@ -119,6 +149,218 @@ "later, so never call it a score, a result, or final.\n" ) +# The onboarding-path clauses. The first is mounted on the *read* tool rather than on +# the actions: a mentor that knows the path exists should talk about it even where it +# may not touch it, and the backend mounts that read only for a hire who has a path. +# +# Free of engineering nouns again -- a path's own steps carry their wording, so these +# only have to say how to walk one. +_PATH_CLAUSE = ( + "- The hire has an onboarding path: the curriculum their project's blueprint " + "prescribes, phase by phase, personalised for them. Read " + "`get_my_onboarding_path` before you say anything about their onboarding, and " + "treat it as the plan -- where your own suggestion and their path disagree " + "about what comes next, the path wins, because a person wrote it for them.\n" + "- You are their tutor along it, not a second plan. Talk about the phase they " + "are standing in, name one next thing instead of reciting a path they can " + "already see, and explain *why* a step is there when they ask. That is the " + "part a list of steps on a page cannot do.\n" + "- Phases are not a queue. A blueprint opens several at once, and when more than " + "one is open, which comes next is the hire's choice -- never tell them to take " + "the lowest number first. The phase they are working in is the one they say, even " + "when the path read has not seen them start it yet.\n" + "- The path is theirs; the blueprint behind it is their PM's. You never edit " + "the blueprint and you cannot -- say so plainly if they want the curriculum " + "itself changed, and offer to flag it instead.\n" +) + +# Reading the material a step points at. The path clause above says to explain why a +# step is there; this says the other half is allowed too -- a step naming an issue, a +# document or a part of the codebase is naming something the corpus holds, and reading +# it *with* the hire is the tutoring. Gated on the search tool, which owns retrieval. +_PATH_MATERIAL_CLAUSE = ( + "- When a step points at something -- an issue, a document, a part of the " + "codebase -- look it up with `search_docs` and go through it with them. Summarise " + "what it is about, what matters in it for this step, and answer what they ask " + "next. A step that says to read something is not a step you can only point at.\n" + "- Ground it the same way as anything else: what you say about the material comes " + "from what you found, and if the search turns up nothing, say that instead of " + "filling the gap.\n" +) + +# The one clause that holds a line rather than describing a capability: a tutor that +# gives the answer away has turned a knowledge check into a formality. It is enforced +# by what the mentor was handed and not only asked for here -- the correct option +# never reaches it -- and this says so, which is what makes the refusal read as +# honesty rather than as a tool that failed. +_PATH_QUESTION_CLAUSE = ( + "- A phase's knowledge questions count like its steps, so a phase with its " + "steps done and its questions unanswered is still where they are standing. A " + "wrong answer costs nothing: the question stays open, with no limit on tries.\n" + "- You are not told which answer is correct, for any question. Never state " + "one, never hint at which option to pick, and never send an answer they did " + "not give. Narrowing the options down is hinting too: never point at an option " + "that matches a title or wording elsewhere, never rule options out, and never " + "say how close a wrong answer was -- you do not know. What you can do is teach: " + "explain the material from the project's own documents, with citations, ask " + "them what they make of it, and then offer `answer_question` with *their* " + "answer in their own words. Where the path shows the answer they last got " + "wrong, start from what it shows they think, not from the top. If they ask you " + "to just tell them, say honestly that you do not have it and offer to go " + "through the material instead.\n" +) + +_PATH_COMPLETE_CLAUSE = ( + "- Only the hire knows whether they have actually done a step. Offer " + "`complete_step` when they say they have finished one -- never because the " + "conversation went well, and never to tidy their path up. Ask; do not " + "announce.\n" + "- The one time you raise it unprompted: a step the path lists as READY TO " + "CLOSE. They ticked every line of its checklist but never finished the step, " + "so whatever waits on it is still locked and they may not know why. That step " + "is where they are. Answer in that order: they are on it and the checklist is " + "done; what finishing it opens; then ask lightly whether they are done, with " + "`complete_step` in the same reply. Never lead with the button and never " + "lecture them about being sure -- once; if they say not yet, leave it.\n" +) + +_PATH_STEP_CLAUSE = ( + "- Some phases come back with nothing in them, because the project's own " + "material did not support them. That is not the hire's fault and not " + "something trying again fixes. Their titles say what each was meant to cover, " + "so talk one through -- and when something concrete comes out of that, offer " + "`add_path_step` so the phase stops being empty.\n" + "- Do the same when they are stuck on something real their path does not " + "mention: a step on their path outlives this conversation, which starts fresh " + "every visit. A step you add goes on *their copy* -- their PM's blueprint is " + "untouched, and they can change or delete it. Never add one just to have " + "added something -- but when they ask for a step themselves, offer it: it is " + "their copy.\n" + "- A knowledge question they got wrong means the material behind it did not " + "land. Teach it first. If what they missed is more than one explanation, offer " + "`add_path_step` for one short refresher step in that question's phase: what " + "to revisit and where to find it, never the answer. One per question, and " + "not after every wrong answer -- a single slip is what a retry is for.\n" + "- Put a step where it belongs, never just at the end: `waits_on` is what it " + "opens after, `unlocks` is what should wait on it. As the next thing, it waits " + "on what they are on and unlocks what that item opens; a refresher goes in " + "front of the question it is for. Pass BOTH -- the path read spells them out.\n" +) + +# Skipping is the PM's decision, and the mentor's part is the reason. A request that +# says why -- already known, not this role's, covered elsewhere -- is one a PM can +# decide on; "I'd rather not" sits. And it goes out in the hire's name, so it has to +# be what they said. +_PATH_SKIP_CLAUSE = ( + "- When they want to skip a step, that is a request their PM decides -- you " + "cannot skip anything yourself, and you never suggest skipping just to get " + "through faster. Ask why first, help them put it into one or two sentences a PM " + "can decide on, then offer `request_skip` with that reason. It goes out in their " + "name, so it says what they said. Never promise it will be accepted.\n" + "- A step marked SKIP REQUESTED is waiting on that decision: do not push them " + "to do it, and do not offer to finish it unless they did it anyway -- finishing " + "it withdraws the request. If their PM declined one, the comment says why; talk " + "that through before asking again.\n" +) + +_PATH_TASK_CLAUSE = ( + "- A step has a checklist, and the step they are on comes with its lines. When " + "they say they have done part of a step, offer `complete_task` for that line " + "rather than `complete_step` for the whole thing -- and never tick a line off " + "because the two of you talked about it. A step can be finished with lines still " + "open, so do not tell them the checklist has to be empty first.\n" +) + +# How to name a thing on the path so the hire can act on it. Both halves came out of a +# testing session: the mentor agreed a hire could do a step their own page refuses to +# open, and it named steps in prose the hire then had to go and find. +_PATH_REFERENCE_CLAUSE = ( + "- Every item on the path comes with a number and a link. Write it as the number, " + "linked, then its title in quotes -- exactly this shape:\n" + ' You are on [#3](/onboarding?step=8f2c1a40-...) "Read the runbook".\n' + " Copy the link the tool gave you for that item, character for character. Do it " + "every time you name a step, a question or a phase they could open, so they get " + "there in one click instead of going to look for it.\n" + '- A bare number from them -- "let us do 3", "what about 5" -- means that item. ' + "Say the number back, so they can see you have the right one.\n" + "- An item marked LOCKED cannot be started or answered yet. Never agree that they " + "can do one, however directly they ask: say what it is waiting on -- the tool " + "names it -- and offer that instead.\n" + "- The numbers are how their page counts items, not the order they come in. A " + "phase is a dependency graph: an item opens once everything it comes after is " + "done, several can be open at once, and finishing one can open several. Say " + "what finishing something opens, and never 'after #6 comes #7' unless the path " + "says #7 comes after #6.\n" +) + +# The clause that decides which half of this product answers a question. Without it the +# same "where am I?" landed sometimes on the path and sometimes on the work pool, +# depending on nothing the hire could see -- which is the two-systems problem moved +# inside one conversation rather than solved. +_PATH_ROUTING_CLAUSE = ( + '- Two different things can answer "where am I?" and they are not ' + "interchangeable. The **path** is their onboarding: the plan a person wrote for " + "them. Their **metrics, pull requests, suggested work and competency ledger** are " + "about their work, not their onboarding. Route deliberately:\n" + ' - "what should I do next", "where am I", "what is left", "how is my ' + 'onboarding going" -> the path, first and always.\n' + ' - "is there an issue I could pick up", "what is in the pool", "can I work on ' + 'something real" -> the suggested work, straight away. Picking work up is its own ' + "road and it is open from day one: never answer this one out of the path, and " + "never tell them they are not far enough along.\n" + ' - "what should I work on", "give me something to do" -> ambiguous, so say both ' + "in one breath: the step they are standing on, and that there is work in the pool " + "they could pick up. Then let them choose.\n" + ' - "how is my work going", "is something stuck", "what have I shown" -> the ' + "metrics and the ledger. Those say how their work is going; they never say what " + "comes next, and they never say how far along their onboarding is.\n" + "- Never answer a question about the path out of the work pool. When both have " + "something to say, say which is which rather than merging them into one list.\n" +) + +# Mounted whenever the hire has any action at all, because the failure it prevents is +# not specific to one: a refusal read as an offer had the mentor telling a hire to click +# a button that was never rendered. +_NO_BUTTON_CLAUSE = ( + "- **You offer something by calling its tool.** Saying that you will do it, or " + "that a button is there, does nothing at all: the button exists only because a " + "tool call produced it. So decide and call in the same turn, and pass every " + "argument the tool asks for -- a call missing one is refused, and then there is no " + "button.\n" + "- If a tool comes back saying NOT PROPOSED, there is no button. Never tell them " + "to confirm, click or check anything; say what you still need from them and offer " + "it again once you have it.\n" + '- Never describe a button instead of making one. "I have added it", "you will ' + 'see a confirm button", "click below" -- none of those are true unless the call ' + "happened.\n" +) + +# "Last resort" is about the mentor's own judgement, never about the hire's. Without +# this the mentor held a hire's explicit "flag this to my PM" against the last-resort +# rule and refused it as "not something for the PM" -- deciding on their behalf what +# they may raise, which is the one thing an escalation path must never do. +_FLAG_ON_REQUEST_CLAUSE = ( + "- When they ask you to flag, raise or pass something to their PM -- a question, " + "a problem, feedback on their path -- offer `flag_to_pm` in that reply. Their " + "asking is the reason: never decide for them that it is not a PM matter, and " + "never make them try the docs first. Say what you know alongside if it helps, " + "but still offer it.\n" +) + +_ACTION_TOOLS = ( + "flag_to_pm", + "open_orientation", + "claim_goal", + "request_attestation", + "set_github_login", + "record_assessment", + "complete_step", + "complete_task", + "answer_question", + "add_path_step", + "request_skip", +) + _SEARCH_ONLY_CLAUSE = ( "- This turn you have `search_docs` and nothing else: you are answering from " "the project's own material, not acting on anybody's behalf. Answer what the " @@ -222,12 +464,39 @@ def build_persona( parts.append(_FIXTURE_CLAUSE) return "".join(parts) - # First clause, because it is first in the conversation: what has to be true - # before somebody can work comes before what they should work on. The backend - # mounts the tool only when a step actually applies, so a project with no + # First: the path is the onboarding, so a mentor never told about it answers + # "what should I do next" out of the work pool while the hire is looking at a + # page that says something else. Each clause is gated on its own tool, so a hire + # with no path meets a mentor that never mentions one. + if "get_my_onboarding_path" in available: + parts.append(_PATH_CLAUSE) + parts.append(_PATH_REFERENCE_CLAUSE) + if "search_docs" in available: + parts.append(_PATH_MATERIAL_CLAUSE) + # Only where the ambiguity exists. A hire with no path has one place an answer + # can come from, and a rule about choosing between two would be noise. + if any(name in available for name in _STATE_TOOLS): + parts.append(_PATH_ROUTING_CLAUSE) + if "answer_question" in available: + parts.append(_PATH_QUESTION_CLAUSE) + if "complete_step" in available: + parts.append(_PATH_COMPLETE_CLAUSE) + if "complete_task" in available: + parts.append(_PATH_TASK_CLAUSE) + if "add_path_step" in available: + parts.append(_PATH_STEP_CLAUSE) + if "request_skip" in available: + parts.append(_PATH_SKIP_CLAUSE) + + # Setup is not part of the path, but it comes before any work: what has to be + # true before somebody can work comes before what they should work on. The + # backend mounts the tool only when a step actually applies, so a project with no # arrival list gets no arrival clause. if "get_arrival_steps" in available: parts.append(_ARRIVAL_CLAUSE) + if any(name in available for name in _ACTION_TOOLS): + parts.append(_NO_BUTTON_CLAUSE) + state_tools = [name for name in _STATE_TOOLS if name in available] if state_tools: rendered = ", ".join(f"`{name}`" for name in state_tools) @@ -241,12 +510,13 @@ def build_persona( # "what should I work on" belongs to no subject cleanly enough to be routed by it. if "get_suggested_tasks" in available: parts.append(_SUGGEST_CLAUSE) + parts.append(_DAY_ONE_CLAUSE) if "get_arrival_steps" in available: parts.append(_SUGGEST_AFTER_ARRIVAL_CLAUSE) # The escalation offer only makes sense when the hire can actually escalate. escalation = ( - "; offer `flag_to_pm` as the last resort.\n" + "; offer `flag_to_pm` as the last resort.\n" + _FLAG_ON_REQUEST_CLAUSE if "flag_to_pm" in available else ".\n" ) @@ -255,6 +525,10 @@ def build_persona( if "claim_goal" in available: parts.append(_CLAIM_CLAUSE) + # Only where the follow-through exists: the packet is the task they claimed, + # so a mentor without it would promise a door it cannot open. + if "open_orientation" in available: + parts.append(_CLAIMED_CLAUSE) # Gated on the *read*, not on `record_assessment`. The backend mounts # `get_competencies_to_assess` only while something is still unplaced, so a hire # who has been placed on everything meets a mentor with nothing to offer them -- @@ -264,8 +538,7 @@ def build_persona( parts.append( f"- Celebrate the {vocabulary.contribution_noun_plural} and milestones the " - "metrics report. Doing the real work is the point; everything else is the " - "path to it." + "metrics report." ) return "".join(parts) diff --git a/src/onboarding/diagram.py b/src/onboarding/diagram.py index bfd72f9..f3a14f6 100644 --- a/src/onboarding/diagram.py +++ b/src/onboarding/diagram.py @@ -71,6 +71,7 @@ # page load, so a diagram that churns between loads would read as the system # changing its mind about the codebase. _TEMPERATURE = 0.0 +_MAX_GENERATION_ATTEMPTS = 2 # Delimiter for the model-authored subject in the prompt. Same device the # artifact judge uses for hire-authored pull request text, for the same reason, @@ -233,6 +234,24 @@ def _build_prompt(subject: str, chunks: list[ScoredChunk]) -> list[Message]: ] +def _correction_prompt( + messages: list[Message], raw: str, error: AssemblyError +) -> list[Message]: + """Ask once for the same diagram with only its JSON corrected.""" + return [ + *messages, + Message(role="assistant", content=raw), + Message( + role="user", + content=( + f"That response could not be validated: {error}. Return the same " + "grounded diagram again as one valid JSON object matching the " + "schema exactly. Return JSON only." + ), + ), + ] + + def _parse_payload(raw: str) -> _GenPayload: try: return _GenPayload.model_validate_json(extract_json_object(raw)) @@ -470,16 +489,35 @@ def stream_diagram( return outcome yield progress.stage("generating", f"Drawing it from {len(chunks)} source(s)") - raw = llm.generate(_build_prompt(subject, chunks), temperature=_TEMPERATURE) - try: - payload = _parse_payload(raw) - except AssemblyError as exc: - logger.warning("Diagram assembly failed for subject %r: %s", subject, exc) + messages = _build_prompt(subject, chunks) + # One correction round, as phase assembly and orientation have: a small local + # model breaks the JSON now and then, and asked back it usually fixes it. + parse_error: AssemblyError | None = None + payload: _GenPayload | None = None + for attempt in range(_MAX_GENERATION_ATTEMPTS): + raw = llm.generate(messages, temperature=_TEMPERATURE) + try: + payload = _parse_payload(raw) + break + except AssemblyError as exc: + parse_error = exc + logger.warning( + "Diagram assembly attempt %d failed for subject %r: %s", + attempt + 1, + subject, + exc, + ) + if attempt + 1 < _MAX_GENERATION_ATTEMPTS: + yield progress.stage("generating", "Correcting invalid generated JSON") + messages = _correction_prompt(messages, raw, exc) + + if payload is None: + assert parse_error is not None outcome = DiagramOutcome( status="skipped", chunks_retrieved=len(chunks), chunks_collapsed=collapsed, - notes=[str(exc)], + notes=[str(parse_error)], ) yield progress.warning("The generated diagram could not be read") yield progress.done("No diagram could be assembled", _dump(outcome)) diff --git a/src/onboarding/orientation.py b/src/onboarding/orientation.py index 37e5f12..11ecf65 100644 --- a/src/onboarding/orientation.py +++ b/src/onboarding/orientation.py @@ -53,6 +53,7 @@ # the same packet. A packet is disposable, but a hire who reloads the page and # reads different instructions has no reason to trust either version. _TEMPERATURE = 0.0 +_MAX_GENERATION_ATTEMPTS = 2 # What each step's retrieval is looking for. These are *queries*, not prose the # hire ever sees — they exist so the evidence pool spans the whole path to a PR @@ -208,6 +209,25 @@ def _build_prompt( ] +def _correction_prompt( + messages: list[Message], raw: str, error: AssemblyError +) -> list[Message]: + """Ask once for the same packet with only its JSON corrected.""" + return [ + *messages, + Message(role="assistant", content=raw), + Message( + role="user", + content=( + f"That response could not be validated: {error}. Return the same " + "grounded packet again as one valid JSON object matching the schema " + "exactly. Return JSON only, with all quotes escaped and all commas " + "present." + ), + ), + ] + + def _parse_payload(raw: str) -> _GenPayload: try: return _GenPayload.model_validate_json(extract_json_object(raw)) @@ -374,19 +394,38 @@ def stream_orientation( yield progress.stage( "generating", f"Writing the packet from {len(chunks)} source(s)" ) - raw = llm.generate( - _build_prompt(task_title, task_body, labels or [], touched_paths or [], chunks), - temperature=_TEMPERATURE, + messages = _build_prompt( + task_title, task_body, labels or [], touched_paths or [], chunks ) - try: - payload = _parse_payload(raw) - except AssemblyError as exc: - logger.warning("Orientation assembly failed for task %r: %s", task_title, exc) + # One correction round, as phase assembly has: a small local model breaks the + # JSON now and then, and one broken quote used to cost the hire the whole + # packet. Asked back for the same content, it usually fixes its own syntax. + parse_error: AssemblyError | None = None + payload: _GenPayload | None = None + for attempt in range(_MAX_GENERATION_ATTEMPTS): + raw = llm.generate(messages, temperature=_TEMPERATURE) + try: + payload = _parse_payload(raw) + break + except AssemblyError as exc: + parse_error = exc + logger.warning( + "Orientation assembly attempt %d failed for task %r: %s", + attempt + 1, + task_title, + exc, + ) + if attempt + 1 < _MAX_GENERATION_ATTEMPTS: + yield progress.stage("generating", "Correcting invalid generated JSON") + messages = _correction_prompt(messages, raw, exc) + + if payload is None: + assert parse_error is not None outcome = OrientationOutcome( status="skipped", chunks_retrieved=len(chunks), chunks_collapsed=collapsed, - notes=[str(exc)], + notes=[str(parse_error)], ) yield progress.warning("The generated packet could not be read") yield progress.done("No orientation could be assembled", _dump(outcome)) diff --git a/tests/api/test_buddy.py b/tests/api/test_buddy.py index 024aea4..42fa1b9 100644 --- a/tests/api/test_buddy.py +++ b/tests/api/test_buddy.py @@ -178,7 +178,7 @@ def test_omitting_both_modes_is_the_hire_mentor(client: TestClient) -> None: assert response.status_code == 200 persona = response.json()["messages"][0]["content"] - assert "the mentor who guides a new hire" in persona + assert "the tutor who guides a new hire" in persona assert "manager of one project" not in persona diff --git a/tests/api/test_grading.py b/tests/api/test_grading.py index 2b8ce9e..5c01b9d 100644 --- a/tests/api/test_grading.py +++ b/tests/api/test_grading.py @@ -158,6 +158,83 @@ def generate(self, messages: list[Message]) -> str: assert result["correct"] is False +def test_broken_json_is_corrected_once_before_an_answer_counts_as_wrong( + client: tuple[TestClient, StubLLMClient], +): + """An unreadable reply used to record a right answer as wrong.""" + http_client, _ = client + calls: list[list[Message]] = [] + + class FlakyLLM(StubLLMClient): + def generate(self, messages: list[Message]) -> str: + calls.append(messages) + if len(calls) == 1: + return '{"results": [{"id": "q1", "correct": true' + return '{"results": [{"id": "q1", "correct": true, "feedback": "Yes."}]}' + + app.dependency_overrides[get_llm] = lambda: FlakyLLM() + + response = http_client.post( + "/api/v1/grade-answers", + json={ + "answers": [ + { + "id": "q1", + "question": "Q1", + "reference_answer": "a1", + "user_answer": "a1", + } + ] + }, + ) + + assert response.json()["results"][0]["correct"] is True + assert len(calls) == 2 + assert "could not be validated" in str(calls[1][-1]) + + +_TWO_ANSWERS = { + "answers": [ + {"id": "q1", "question": "Q1", "reference_answer": "a1", "user_answer": "a1"}, + {"id": "q2", "question": "Q2", "reference_answer": "a2", "user_answer": "a2"}, + ] +} +_BOTH_RIGHT = ( + '{"results": [{"id": "q1", "correct": true}, {"id": "q2", "correct": true}]}' +) + + +@pytest.mark.parametrize( + "first_reply", + [ + "{}", + '{"results": [{"id": "q1", "correct": true}]}', + '{"results": [{"id": "q1", "correct": true}, {"id": "q1", "correct": true}]}', + '{"results": [{"id": "q1", "correct": true}, {"id": "q9", "correct": true}]}', + ], + ids=["empty", "missing-id", "duplicate-id", "unexpected-id"], +) +def test_a_reply_that_does_not_grade_every_answer_is_corrected_first( + client: tuple[TestClient, StubLLMClient], first_reply: str +): + """Valid JSON that leaves an answer ungraded used to record it as wrong.""" + http_client, _ = client + calls: list[list[Message]] = [] + + class IncompleteLLM(StubLLMClient): + def generate(self, messages: list[Message]) -> str: + calls.append(messages) + return first_reply if len(calls) == 1 else _BOTH_RIGHT + + app.dependency_overrides[get_llm] = lambda: IncompleteLLM() + + response = http_client.post("/api/v1/grade-answers", json=_TWO_ANSWERS) + + assert [r["correct"] for r in response.json()["results"]] == [True, True] + assert len(calls) == 2 + assert "could not be validated" in str(calls[1][-1]) + + def test_llm_failure_returns_503(client: tuple[TestClient, StubLLMClient]): http_client, _ = client diff --git a/tests/onboarding/test_buddy_agent.py b/tests/onboarding/test_buddy_agent.py index 88c7df1..c66ab62 100644 --- a/tests/onboarding/test_buddy_agent.py +++ b/tests/onboarding/test_buddy_agent.py @@ -226,6 +226,45 @@ def _fake_retrieve(*args: object, **kwargs: object) -> list[ScoredChunk]: assert [getattr(f, "project_ids", None) for f in seen] == [None] +class _RecordingForcedAnswer(ScriptedLLMClient): + """Records what the forced, tool-less final answer was asked with.""" + + def __init__(self, *args: object, **kwargs: object) -> None: + super().__init__(*args, **kwargs) # type: ignore[arg-type] + self.generate_calls: list[list[Message]] = [] + + def generate( + self, messages: list[Message], *, temperature: float | None = None + ) -> str: + self.generate_calls.append(messages) + return self.answer + + +def test_a_spent_search_budget_tells_the_model_it_has_no_tools( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # The failure this pins: a model that searched until the budget ran out answered + # with the persona still saying "offer X", and told the hire to confirm a button + # no tool call had produced. + monkeypatch.setattr("onboarding.buddy_agent.retrieve", lambda *a, **k: []) + llm = _RecordingForcedAnswer( + turns=[[(SEARCH_DOCS, {"query": "again"})]] * 20, answer="Here is what I know." + ) + + result = run_agent_turn([_user("hi")], [_GET_MY_METRICS], llm, StubVectorStore()) + + assert result.final is True + assert len(llm.generate_calls) == 1 + notice = llm.generate_calls[0][-1] + assert notice["role"] == "system" + assert "no confirm button can appear" in (notice.get("content") or "") + # The notice is for the model only; it is not kept in the conversation. + assert all( + "no confirm button can appear" not in (msg.get("content") or "") + for msg in result.messages + ) + + def test_unknown_tool_is_answered_as_such_and_does_not_stall() -> None: llm = ScriptedLLMClient(turns=[[("does_not_exist", {})], []], answer="done") result = run_agent_turn([_user("hi")], [_GET_MY_METRICS], llm, StubVectorStore()) @@ -410,5 +449,5 @@ def test_the_modes_default_to_todays_behaviour() -> None: run_agent_turn([_user("hello")], [_GET_MY_METRICS], llm, StubVectorStore()) persona = _system_of(llm.chat_calls[0]) - assert "the mentor who guides a new hire" in persona + assert "the tutor who guides a new hire" in persona assert "search_docs` and nothing else" not in persona diff --git a/tests/onboarding/test_buddy_persona.py b/tests/onboarding/test_buddy_persona.py index 6c7b561..5903106 100644 --- a/tests/onboarding/test_buddy_persona.py +++ b/tests/onboarding/test_buddy_persona.py @@ -44,6 +44,18 @@ def test_without_an_escalation_tool_the_persona_offers_no_escalation() -> None: assert "rather than inventing an answer" in persona +def test_a_hire_who_asks_to_flag_something_is_offered_it() -> None: + """'Last resort' is the mentor's bar for flagging unasked, never the hire's. + + Read as the only rule, it had the mentor refuse an explicit "flag this to my PM" + as not being a PM matter. + """ + persona = build_persona(_ALL_TOOLS) + + assert "When they ask you to flag" in persona + assert "never decide for them that it is not a PM matter" in persona + + def test_hire_state_tools_are_listed_only_when_mounted() -> None: persona = build_persona(["search_docs", "get_my_metrics"]) @@ -215,6 +227,299 @@ def test_the_grounding_rule_survives_every_toolset() -> None: assert "test, fixture, or sample-data files" in persona +_PATH_TOOLS = ( + "get_my_onboarding_path", + "complete_step", + "complete_task", + "answer_question", + "add_path_step", + "request_skip", +) + + +def test_a_hire_without_a_path_meets_a_mentor_that_never_mentions_one() -> None: + """The backend mounts the path tools only for a hire who has a path, so a mentor + without them must not describe walking one.""" + persona = build_persona(_ALL_TOOLS) + + assert "onboarding path" not in persona + for tool in _PATH_TOOLS: + assert tool not in persona + + +def test_the_path_is_the_plan_and_the_blueprint_is_not_the_mentors() -> None: + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + # The whole point of reading it: one plan, and it is the one a person wrote. + assert "the path wins" in persona + # The authority line. The mentor edits the hire's copy, never the curriculum. + assert "You never edit the blueprint" in persona + assert "*their copy*" in persona + + +def test_the_mentor_is_a_tutor_on_a_question_and_never_a_shortcut() -> None: + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + # It is not told the answer, and the clause says so rather than only forbidding + # it -- an honest "I do not have it" is the behaviour we want when asked. + assert "not told which answer is correct" in persona + assert "you do not have it" in persona + + +def test_completing_a_step_is_asked_for_never_announced() -> None: + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "Only the hire knows whether they have actually done a step" in persona + assert "Ask; do not announce" in persona + + +def test_a_done_checklist_on_an_open_step_is_raised_unprompted() -> None: + """A hire who ticked every line but never finished the step is locked out of what + comes next without knowing why -- and will not ask about the step doing it.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "READY TO CLOSE" in persona + assert "if they say not yet, leave it" in persona + + +def test_a_wrong_answer_can_become_a_refresher_step_but_never_the_answer() -> None: + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "refresher step" in persona + assert "never the answer" in persona + assert "not after every wrong answer" in persona + + +def test_phases_are_a_choice_not_a_queue() -> None: + """A hire who finished phase 1 and picked phase 3 was sent back to phase 2.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "Phases are not a queue" in persona + assert "never tell them to take the lowest number first" in persona + + +def test_the_path_is_described_as_a_graph_not_a_sequence() -> None: + """It told a hire "after #6 comes #7" about items that did not depend on each + other, because it read the page's numbering as an order.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "dependency graph" in persona + assert "not the order they come in" in persona + + +def test_narrowing_the_options_is_hinting() -> None: + """No answer stated, and the question given away all the same: "one option + matches the title of #1 word for word".""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "Narrowing the options down is hinting too" in persona + assert "never say how close a wrong answer was" in persona + + +def test_a_ready_step_is_answered_where_they_are_first_never_button_first() -> None: + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "Never lead with the button" in persona + assert "never lecture them about being sure" in persona + + +def test_an_added_step_is_placed_in_the_graph() -> None: + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "never just at the end" in persona + assert "`waits_on`" in persona + assert "`unlocks`" in persona + + +def test_a_skip_is_requested_with_the_hires_reason_and_decided_by_the_pm() -> None: + """The mentor files the request; it does not grant it, and it does not invent + the reason. Without saying so it either promised a skip or sent a request that + would sit.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "a request their PM decides" in persona + assert "`request_skip`" in persona + assert "Ask why first" in persona + assert "Never promise it will be accepted" in persona + # Finishing a step drops its pending skip, which the hire would never notice. + assert "finishing it withdraws the request" in persona + + +def test_no_skip_clause_without_the_skip_action() -> None: + persona = build_persona([*_ALL_TOOLS, "get_my_onboarding_path", "complete_step"]) + + assert "request_skip" not in persona + + +def test_each_path_clause_is_gated_on_its_own_tool() -> None: + read_only = build_persona([*_ALL_TOOLS, "get_my_onboarding_path"]) + + # The read mounts the tutor framing; the actions stay unmentioned until they are + # mounted, so the mentor never offers a proposal it cannot make. + assert "onboarding path" in read_only + assert "`complete_step`" not in read_only + assert "`answer_question`" not in read_only + assert "`add_path_step`" not in read_only + + +def test_a_question_about_the_path_is_never_answered_from_the_work_pool() -> None: + """The two-systems problem, moved inside one conversation: "where am I?" has two + plausible answers and only one of them is the plan.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "the path, first and always" in persona + # The other direction too: progress questions belong to the metrics, not the path. + assert "never say what comes next" in persona + + +def test_how_the_onboarding_is_going_is_the_paths_question() -> None: + """The metrics are about work. Routing "how is my onboarding going" to them + would make the first accepted contribution the end of onboarding again.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert 'how is my onboarding going" -> the path' in persona + assert "never say how far along their onboarding is" in persona + + +def test_a_step_that_points_at_something_is_read_with_the_hire() -> None: + """Explaining why a step exists was in the persona; going through what it points + at was not, so a step reading "read issue 123" got pointed at rather than read.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "look it up with `search_docs` and go through it with them" in persona + assert "not a step you can only point at" in persona + + +def test_no_material_clause_without_the_search_tool() -> None: + persona = build_persona( + [t for t in _ALL_TOOLS if t != "search_docs"] + list(_PATH_TOOLS) + ) + + assert "go through it with them" not in persona + + +def test_picking_up_work_is_never_gated_on_how_far_the_path_got() -> None: + """A hire with issues in the pool asked whether there was something they could + do and was told there was not. The routing clause had the suggested work coming + "only when the path has nothing open", so a half-finished path read as a closed + door -- a gate nobody designed, in the one place the hire cannot see it.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "Real work is open to them from day one" in persona + assert "Never make it conditional on onboarding progress" in persona + # The routing table has to agree with the clause, or the mentor holds both. + assert "is there an issue I could pick up" in persona + assert "never tell them they are not far enough along" in persona + + +def test_the_ambiguous_question_gets_both_answers_rather_than_one() -> None: + """ "What should I work on" is genuinely two questions. Answering only the path + hides the pool; answering only the pool hides the plan somebody wrote.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "ambiguous, so say both" in persona + + +def test_the_mentor_stays_with_a_task_the_hire_claimed() -> None: + persona = build_persona([*_ALL_TOOLS, "open_orientation", *_PATH_TOOLS]) + + assert "Once they have claimed something, stay with it" in persona + assert "`open_orientation`" in persona + + +def test_no_follow_through_clause_without_the_packet() -> None: + # Promising a packet that is not mounted is the button-that-never-appears defect. + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "`open_orientation`" not in persona + assert "Once they have claimed something" not in persona + + +def test_the_path_comes_before_setup_and_setup_before_work() -> None: + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert ( + persona.index("The hire has an onboarding path") + < persona.index("`get_arrival_steps`") + < persona.index("`get_suggested_tasks`") + ) + + +def test_no_ramp_towards_a_first_contribution_is_left_in_the_persona() -> None: + """Task 0 and "doing real work is the point" were the old onboarding: a ramp + that ended at a first accepted contribution. The path replaced it.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS, "open_orientation"]).lower() + + assert "task 0" not in persona + assert "claim_task_zero" not in persona + assert "the path to it" not in persona + + +def test_no_routing_rule_where_there_is_nothing_to_route_between() -> None: + # A hire with a path but no state tools has one place an answer can come from, so a + # rule about choosing would be noise. + persona = build_persona(["search_docs", "get_my_onboarding_path"]) + + assert "onboarding path" in persona + assert "first and always" not in persona + + +def test_a_locked_item_is_never_agreed_to() -> None: + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + # It said yes to a step the hire's own page refuses to open. Told what locked means, + # the answer is what it is waiting on. + assert "LOCKED" in persona + assert "however directly they ask" in persona + + +def test_items_are_named_by_number_and_linked() -> None: + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "linked, then its title" in persona + assert "means that item" in persona + + +def test_a_refused_proposal_is_never_described_as_a_button() -> None: + """It told a hire to click something that was never rendered. Mounted with any + action at all, because the mistake is not specific to one.""" + for mounted in (["claim_goal"], ["add_path_step"], list(_PATH_TOOLS)): + persona = build_persona(mounted) + + assert "NOT PROPOSED" in persona + + assert "NOT PROPOSED" not in build_persona(["search_docs", "get_my_metrics"]) + + +def test_part_of_a_step_is_a_line_not_the_whole_step() -> None: + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS, "complete_task"]) + + assert "`complete_task`" in persona + # The product allows a finished step with open lines; the mentor must not invent a + # rule the product does not have. + assert "checklist has to be empty" in persona + + +def test_offering_is_described_as_a_tool_call_not_as_a_sentence() -> None: + """The failure this exists for: hires were told to click a button that no tool call + had produced, so there was nothing on screen.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "by calling its tool" in persona + assert "Never describe a button instead of making one" in persona + # And the arguments, because a call missing one is refused and also shows no button. + assert "pass every argument the tool asks for" in persona + + +def test_an_item_is_written_as_a_linked_number() -> None: + """A literal shape rather than a description of one: a small model copies a pattern + far more reliably than it follows an instruction about formatting.""" + persona = build_persona([*_ALL_TOOLS, *_PATH_TOOLS]) + + assert "[#3](/onboarding?step=" in persona + assert "character for character" in persona + + # --- Modes ----------------------------------------------------------------- # # Two flags pick which persona is assembled. The through-line above still holds @@ -262,7 +567,7 @@ def test_team_mode_addresses_the_manager_not_a_hire() -> None: assert "manager of one project" in persona assert "Never greet them as a new hire" in persona - assert "the mentor who guides a new hire" not in persona + assert "the tutor who guides a new hire" not in persona def test_team_mode_drops_every_hire_directed_clause() -> None: diff --git a/tests/onboarding/test_diagram.py b/tests/onboarding/test_diagram.py index 04c208c..ffb0c76 100644 --- a/tests/onboarding/test_diagram.py +++ b/tests/onboarding/test_diagram.py @@ -121,6 +121,35 @@ def test_assembles_a_diagram_with_the_sources_it_drew_on() -> None: assert outcome.provenance.corpus_fingerprint +def test_broken_json_is_corrected_once_rather_than_losing_the_diagram() -> None: + store = _store(*_TWO_CHUNKS) + llm = _llm( + _payload( + [ + _node("controller", "ReportController", ["c1"]), + _node("repo", "ReportRepository", ["c2"]), + ], + [_edge("controller", "repo")], + ) + ) + good = llm.generate + seen: list[list[object]] = [] + + def flaky(messages: list[object], **kwargs: object) -> str: + seen.append(messages) + if len(seen) == 1: + return '{"summary": "cut off' + return good(messages, **kwargs) # type: ignore[arg-type] + + llm.generate = flaky # type: ignore[method-assign] + + outcome = assemble_diagram(llm, store, subject=_SUBJECT, project_ids=_PIDS) + + assert outcome.status == "assembled" + assert len(seen) == 2 + assert "could not be validated" in str(seen[1][-1]) + + def test_a_node_that_cites_nothing_is_dropped_and_takes_its_edges_with_it() -> None: """The grounding rule, and the reason it has to reach the edges too. diff --git a/tests/onboarding/test_orientation.py b/tests/onboarding/test_orientation.py index a0cee21..5cd6a9d 100644 --- a/tests/onboarding/test_orientation.py +++ b/tests/onboarding/test_orientation.py @@ -225,6 +225,29 @@ def test_unparseable_output_is_skipped_not_a_half_packet() -> None: assert outcome.packet is None +def test_broken_json_is_corrected_once_rather_than_losing_the_packet() -> None: + """A local model's one missing quote used to cost the hire the whole packet.""" + store = _store("run make dev to start the service locally") + llm = _llm(_payload(_section("SET_UP", "Run it locally", ["c1"]))) + good = llm.generate + replies = iter(['{"summary": "cut off', None]) + seen: list[list[object]] = [] + + def flaky(messages: list[object], **kwargs: object) -> str: + seen.append(messages) + reply = next(replies) + return reply if reply is not None else good(messages, **kwargs) # type: ignore[arg-type] + + llm.generate = flaky # type: ignore[method-assign] + + outcome = assemble_orientation(llm, store, task_title=_TITLE, project_ids=_PIDS) + + assert outcome.status == "assembled" + # The second call carries the broken reply and asks for it corrected. + assert len(seen) == 2 + assert "could not be validated" in str(seen[1][-1]) + + def test_unchanged_corpus_serves_the_cached_packet() -> None: store = _store("run make dev to start the service locally") llm = _llm(_payload(_section("SET_UP", "Run it locally", ["c1"])))