Hardening/additional r bug fixes#51
Merged
Merged
Conversation
The shared dialogue reader in [workflow_dialogue.R](/ai-agent/HadesProject/OHDSI-Study-Agent/R/slashOhdsiStrategusAssistant/R/workflow_dialogue.R) now recognizes `/back` and returns a navigation signal only when a caller explicitly enables it. Normal `/ohdsi` handling is unchanged. In [strategus_incidence_shell.R](/ai-agent/HadesProject/OHDSI-Study-Agent/R/slashOhdsiStrategusAssistant/R/strategus_incidence_shell.R), I wired that signal into major boundaries only: - study intent / intent-split editing - target selection entry - outcome selection entry - time-at-risk confirmation - Keeper-review entry This is intentionally not a universal rewind. It is a bounded step-back mechanism at stage boundaries, which keeps the implementation small and avoids trying to undo arbitrary downstream side effects mid-stage. The shell banner now also tells the user that `/back` is available at supported boundaries. I added focused static coverage in [test_r_workflow_context_dialogue_wrapper.py](/ai-agent/HadesProject/OHDSI-Study-Agent/tests/test_r_workflow_context_dialogue_wrapper.py). Verified with: - `Rscript -e "parse(file='R/strategus_incidence_shell.R'); parse(file='R/workflow_dialogue.R')"` - `pytest -q ../../tests/test_r_workflow_context_dialogue_wrapper.py ../../tests/test_keeper_dialogue_integration_static.py`
It now uses the shared dialogue navigation pattern and supports stepping back at major boundaries only: - study intent -> target selection - target -> comparator selection - comparator -> outcome selection - outcome -> study configuration - study configuration -> Keeper review options The shell banner now advertises `/back`, and the same `readline_with_navigation()` / `is_back_signal()` pattern from incidence is wired in. I left the lower Keeper defaults unchanged. I also extended the static coverage in [test_r_workflow_context_dialogue_wrapper.py](/ai-agent/HadesProject/OHDSI-Study-Agent/tests/test_r_workflow_context_dialogue_wrapper.py) for the cohort-methods shell boundaries. Verified with: - `Rscript -e "parse(file='R/strategus_cohort_methods_shell.R')"` - `pytest -q ../../tests/test_r_workflow_context_dialogue_wrapper.py ../../tests/test_keeper_dialogue_integration_static.py` One limitation remains intentional: this is still stage-boundary navigation, not universal rewind inside every prompt.
Findings** 1. Fixed a real Keeper workflow regression in [keeper_review_workflow.R](/ai-agent/HadesProject/OHDSI-Study-Agent/R/slashOhdsiStrategusAssistant/R/keeper_review_workflow.R:223). When a per-domain ACP call failed and the user chose `rerun`, the old error stayed in `keeper_review_state.json` even if the rerun succeeded, so the workflow could finish with a false `status="error"`. I added `clear_workflow_errors()` and keyed domain-generation errors by `domain_key` before re-appending them. 2. Fixed stale shell documentation in [R_STRATEGUS_COHORT_METHODS_SHELL.md](/ai-agent/HadesProject/OHDSI-Study-Agent/docs/R_STRATEGUS_COHORT_METHODS_SHELL.md:18). It still documented a 50-character cohort/comparison analysis-label cap, but the code now allows 100. That doc now matches the implementation. 3. Fixed stale workflow/docs references in [R_STRATEGUS_COHORT_METHODS_SHELL.md](/ai-agent/HadesProject/OHDSI-Study-Agent/docs/R_STRATEGUS_COHORT_METHODS_SHELL.md:15) and [WORKFLOW_COHORT_METHODS.md](/ai-agent/HadesProject/OHDSI-Study-Agent/docs/WORKFLOW_COHORT_METHODS.md:1). The shell doc pointed to the wrong workflow filename and still used incidence-style work/results folder examples in the execution-settings template. No additional behavioral issues stood out in the recent R-shell changes after this pass. **Updates** I aligned the package/docs with the implemented behavior in: - [README.md](/ai-agent/HadesProject/OHDSI-Study-Agent/R/slashOhdsiStrategusAssistant/README.md:1) - [R_STRATEGUS_INCIDENCE_SHELL.md](/ai-agent/HadesProject/OHDSI-Study-Agent/docs/R_STRATEGUS_INCIDENCE_SHELL.md:1) - [R_STRATEGUS_COHORT_METHODS_SHELL.md](/ai-agent/HadesProject/OHDSI-Study-Agent/docs/R_STRASHOhdsiStrategusAssistant/../../docs/R_STRATEGUS_COHORT_METHODS_SHELL.md:1) - [WORKFLOW_COHORT_METHODS.md](/ai-agent/HadesProject/OHDSI-Study-Agent/docs/WORKFLOW_COHORT_METHODS.md:1) Those updates now document: - `/back` support at major shell boundaries - bounded inline Keeper stage gates - suppressed Keeper reuse/resume prompts when no artifacts exist - the 100-character cohort-method analysis-label limit I also extended the focused static coverage in [test_keeper_dialogue_integration_static.py](/ai-agent/HadesProject/OHDSI-Study-Agent/tests/test_keeper_dialogue_integration_static.py:1). **Verification** Ran: - `Rscript -e "parse(file='R/keeper_review_workflow.R'); parse(file='R/strategus_incidence_shell.R'); parse(file='R/strategus_cohort_methods_shell.R')"` - `pytest -q ../../tests/test_keeper_dialogue_integration_static.py ../../tests/test_r_workflow_context_dialogue_wrapper.py` Result: `10 passed`.
Two issues were involved: - `cached_inputs` / `cached_manual_intent` were initialized to `NULL` but never reloaded from `manual_inputs.json` / `manual_intent.json` when `resume=TRUE`. That was a real bug. - Empty target/comparator recommendation selections could fall through to a generic `Missing Target.` / `Missing Comparator.` stop. I replaced that with a safer resolver that: - uses recommendation-selected IDs when present - falls back to explicit or cached IDs when available - prompts interactively if needed - gives a more informative non-interactive error with the statement and recommendation artifact path I also added a small static guard so this resume-cache load path does not disappear again. Verified with: - `Rscript -e "parse(file='R/strategus_cohort_methods_shell.R')"` - `pytest -q ../../tests/test_r_workflow_context_dialogue_wrapper.py ../../tests/test_keeper_dialogue_integration_static.py` This looks like a genuine bug in the cohort-method shell, and the recent control-flow changes likely made it easier to hit.
Collaborator
Author
|
bug fixes to R scripts and |
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.
No description provided.