-
Notifications
You must be signed in to change notification settings - Fork 2
simplify workflow history and add page tracking #354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
services/job_chat/job_chat.py
Outdated
| return content | ||
|
|
||
| prefix_parts = [page.get('type', ''), page.get('name', '')] | ||
| if page.get('adaptor'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put page.get(name) into an if as well
We should know the page type because in job chat, the page type is always job chat
|
Just added a final change to make sure the prefix is always added for consistency. |
josephjclark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, a few observations. The review is a bit scattered so here are the important bits I think:
- Having looked closely and seen the code, I'm not sold on the prefix. I think we should just save
{ url, meta }keys on the chat history - I don;t think Lightning should send us
last_page, it should just send us thecurrent_page. And I think it's easiest for everyone if it just sends the URL - When re-running rag, we're starting to conflate "run rag because we've changed agents" with "re-run rag because the adaptor changed within a session".
| @dataclass | ||
| class ChatConfig: | ||
| model: str = "claude-sonnet-4-20250514" | ||
| model: str = "claude-sonnet-4-5-20250929" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like any model changes to be surfaced in PRs and release notes. I know it''s not likely to change much but I want really clear disclosure on it
services/job_chat/job_chat.py
Outdated
|
|
||
| # Construct current_page from context | ||
| # Always create page with type, extract name/adaptor from context if available | ||
| page_name = data.context.get("page_name") if data.context else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make sure that data.context gets set to an empty dict early on? Would remove a lot of complexity from this code
services/job_chat/job_chat.py
Outdated
| } | ||
| # Only add adaptor if we successfully extracted it | ||
| if adaptor_short_name: | ||
| current_page["adaptor"] = adaptor_short_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The adaptor_short_name var isn't really needed. If we declare current_page a bit earlier, we can just assign the adaptor key at the end of the try block - instead of assigning the var, just assign the key. Leaves you with shorter, cleaner code
| } | ||
|
|
||
| # Meta shows navigation happened | ||
| meta = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think meta should show the last page - just the current one. The last page is implied in the history.
I also want to be careful about what assumptions we're making in the last page data. I think the app should just send us a URL really?
| assert not any(word in response_text for word in ["workflow", "yaml", "trigger", "edge"]), \ | ||
| "Response should be about job code, not workflow structure" | ||
|
|
||
| print("\n✓ Navigation test passed: Model correctly inferred navigation from workflow to job editor") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this test tells us that the history was correctly interpreted? Would we have had a different/worse result without rag?
| assert response["suggested_code"] is not None, "Model should have generated code for the job" | ||
|
|
||
| # Verify logging was added to the code | ||
| assert "console.log" in response["suggested_code"], "Log statement not found in suggested code" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interesting thing about this example is that I would prefer to see the common log function be used, rather than console.log.
If anything I'd take that as evidence that rag has NOT run - it's not picking up the dedicated log function.
Or maybe it's just AI being AI. Not damning feedback, but also not a resounding success for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SO we'll just try and trick the model:
- ask a question like "how do I get data"
- expect the model to refer to the OG adaptor, eg salesforce, in the adaptor
- change page and ask the same question again
- the response should now name drop the new adaptor, eg dhis2
|
|
||
| updated_history = history + [ | ||
| {"role": "user", "content": content}, | ||
| {"role": "user", "content": prefixed_content}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so we'll add more metadata to the history objects here. Don't worry about tidying up or bloating history for now (we should only be adding a very small amount of data)
For now add the page prefix (as page?). Maybe adaptor stuff but that can be handled later
|
The Anthropic messages object in |
Short Description
Supports merging workflow and job code chats OpenFn/lightning#4109
Adds page navigation tracking to both
job_chatandworkflow_chatservices to improve context awareness and auto-refresh documentation when users navigate between different jobs or workflows. When users switch pages (different job, workflow, or adaptor), the conversation history shows page context via prefixes like[pg:job_code/Transform data/language-common], andjob_chatautomatically refreshes RAG documentation to ensure relevant adaptor docs are retrieved.Also updates from claude-sonnet-4 to claude-sonnet-4-5-20250929
Fixes #350
Implementation Details
Page Context Construction
contextpayload:job_chat: Extracts currentpage_nameandadaptorfromcontext.page_nameandcontext.adaptor, sets type to"job_code"workflow_chat: Extracts currentpage_namefromcontext.page_name, sets type to"workflow"History Prefixing
add_page_prefix()helper function prefixes user messages in history with format:[pg:type/name/adaptor]or[pg:type/name][pg:job_code/Get FHIR data/language-fhir@4.6.10](job_chat with adaptor)[pg:workflow/fridge-statistics-processing](workflow_chat)Simplified History Format
"I'll add error handling..."instead of{"text": "I'll add error handling...", "yaml": "..."}Navigation Detection & RAG Auto-refresh
has_navigated()helper tojob_chatto detect page changescurrent_page(constructed from context) to the last user conversation turn prefixtype,name, ORadaptorchangesjob_chatautomatically setsrefresh_rag=trueto fetch fresh documentation for the new page/adaptorBackward Compatibility
job_chatalways includesmetakey withragdata (maintains existing behaviour)workflow_chatconditionally includesmetaonly when page data is present (no breaking change asmetawas never returned before)contextfield toworkflow_chatPayload (previously only injob_chat)Prompt Changes
Tests Added
Anthropic's structured outputs NOT used
Requests for Lightning:
contextkey in workflow_chat calls, with acurrent_pagekey. This should just be the name of the workflow/step, like 'fetch-fhir-data'.current_pagekey to thecontextkeyAI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy