Skip to content

upsert-case-&-dynamic-choices - #21

Merged
alain-sv merged 4 commits into
developfrom
upsert-case-&-dynamic-choices
Apr 13, 2026
Merged

upsert-case-&-dynamic-choices#21
alain-sv merged 4 commits into
developfrom
upsert-case-&-dynamic-choices

Conversation

@alain-sv

Copy link
Copy Markdown
Contributor
  • Add upsert/patch behavior for Case steps so incoming answers with a casestep_index update an existing step instead of always appending a one - Add up field to CaseUpdate dataclass and it in serialization to allow update-or-create on steps - Case.patch_step(index updateNode to mark updates as upsert, send to account, update in-memory list, and persist - Route requests with answer.casestep_indextocase.patch(...)and emit a RECEIVED lifecycle event viaPersistentEntity; back to regular receive when is missing - unit test ensuring withcasestep_index update the specified step (and create when appropriate)
  • Bump version from0.13.2.dev0 to0.13.2- Implement human_answer hook in HITL callback and handle missingCases()` registry

…psert" capability to Case nodes route incominganswers that a casestep_index to patch an existing stepinstead of always appending a new one.

- Add up field to CaseUpdate dataclass include it in serialization so
  can perform update_create on steps.
- Implement Case.patch_step(index, updateCaseNode) that marks the update
  as upsert, sends it to the account, updates the in-memory updates
  list, and persists the.
- Route update requests with answer.casestep_index in routes.py to call
  case.patch_step(...) and emit an_RECEIVED lifecycle event via
  PersistentEntityLifecycle. Fall back to receiveuman_input(...)
  casestep_index is.
- Add a unit test ensuring requests with casestep_index the specified
  stepupsert) of creating a new one.
-ump version from0.13.2.dev0 to0.13.2 and commit the version.

This enables enriching or completing a previously sent step(e.g. adding
interview end time to start step) without creatingduplicate steps in.
@qodo-code-review

Copy link
Copy Markdown

Review Summary by Qodo

Add case step upsert/patch behavior and human_answer hook support

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Add upsert/patch capability to Case steps via CaseNodeUpdate.upsert field and
  Case.patch_step() method
• Route incoming answers with casestep_index to patch existing steps instead of always appending
  new ones
• Implement human_answer hook invocation in case update endpoint with graceful fallback for
  stateless replicas
• Update case endpoint to handle missing Cases registry without returning 404 errors
Diagram
flowchart LR
  A["POST /update with answer"] --> B{"casestep_index present?"}
  B -->|Yes| C["Case.patch_step()"]
  B -->|No| D["Case.receive_human_input()"]
  C --> E["Set upsert=True"]
  D --> E
  E --> F["send_update_case()"]
  F --> G["Call human_answer hook"]
  H["Case not in registry"] --> G
  G --> I["Return 200 success"]
Loading

Grey Divider

File Changes

1. src/supervaizer/case.py ✨ Enhancement +22/-0

Add upsert field and patch_step method to Case

src/supervaizer/case.py


2. src/supervaizer/routes.py ✨ Enhancement +48/-40

Route casestep_index to patch; implement human_answer hook

src/supervaizer/routes.py


3. tests/test_case.py 🧪 Tests +49/-0

Add unit tests for upsert and patch_step behavior

tests/test_case.py


View more (10)
4. tests/test_routes_case_update.py 🧪 Tests +67/-10

Test casestep_index routing and missing registry handling

tests/test_routes_case_update.py


5. docs/CHANGELOG.md 📝 Documentation +7/-1

Document upsert, patch_step, and human_answer features

docs/CHANGELOG.md


6. justfile ⚙️ Configuration changes +3/-0

Add env_upgrade target for dependency updates

justfile


7. AGENTS.md 📝 Documentation +102/-0

Add GitNexus code intelligence documentation

AGENTS.md


8. .claude/skills/gitnexus/gitnexus-cli/SKILL.md 📝 Documentation +82/-0

Document GitNexus CLI commands and workflows

.claude/skills/gitnexus/gitnexus-cli/SKILL.md


9. .claude/skills/gitnexus/gitnexus-debugging/SKILL.md 📝 Documentation +89/-0

Document GitNexus debugging patterns and tools

.claude/skills/gitnexus/gitnexus-debugging/SKILL.md


10. .claude/skills/gitnexus/gitnexus-exploring/SKILL.md 📝 Documentation +78/-0

Document GitNexus code exploration workflows

.claude/skills/gitnexus/gitnexus-exploring/SKILL.md


11. .claude/skills/gitnexus/gitnexus-guide/SKILL.md 📝 Documentation +64/-0

Document GitNexus tools and resources reference

.claude/skills/gitnexus/gitnexus-guide/SKILL.md


12. .claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md 📝 Documentation +97/-0

Document GitNexus impact analysis patterns

.claude/skills/gitnexus/gitnexus-impact-analysis/SKILL.md


13. .claude/skills/gitnexus/gitnexus-refactoring/SKILL.md 📝 Documentation +121/-0

Document GitNexus refactoring workflows

.claude/skills/gitnexus/gitnexus-refactoring/SKILL.md


Grey Divider

Qodo Logo

@qodo-code-review

qodo-code-review Bot commented Apr 13, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. human_answer dispatch is broken🐞
Description
update_case_with_answer calls every registered agent’s human_answer hook and passes the answer
under the answer kwarg, but existing hook implementations and the workbench HITL route expect the
answer under fields (and often context), so hooks will receive empty/wrong data and/or run for
the wrong agent.
Code

src/supervaizer/routes.py[R260-276]

+        # Call the agent's human_answer method if registered
+        import importlib
+        for sv_agent in server.agents:
+            if sv_agent.methods and sv_agent.methods.human_answer:
+                try:
+                    method_path = sv_agent.methods.human_answer.method
+                    module_name, func_name = method_path.rsplit(".", 1)
+                    module = importlib.import_module(module_name)
+                    func = getattr(module, func_name)
+                    func(
+                        job_id=job_id,
+                        case_id=case_id,
+                        answer=request.answer,
+                        message=request.message,
+                    )
+                except Exception as _hook_exc:
+                    log.error(f"[human_answer hook] {sv_agent.name}: {_hook_exc}")
Evidence
The route invokes human_answer for all server.agents and calls the function with answer=...
(not fields=...). In the codebase, the reference implementation reads fields from kwargs, and
the workbench HITL endpoint passes fields (and context) to the hook via agent._execute. The
Job model also has agent_name and the Jobs registry is keyed by agent, supporting a single owning
agent per job; calling all agents is therefore incorrect when multiple agents are registered.

src/supervaizer/routes.py[260-276]
src/supervaizer/examples/hello_world_agent.py[197-208]
src/supervaizer/admin/workbench_routes.py[472-531]
src/supervaizer/job.py[28-59]
src/supervaizer/job.py[244-268]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`update_case_with_answer` directly imports and calls `human_answer` with a kwarg contract (`answer`, `message`) that doesn’t match existing hook implementations (`fields`, `context`) and executes the hook for every agent.
## Issue Context
- `Job` has `agent_name`, and `Jobs()` is keyed by agent, so this endpoint should resolve the owning agent for `job_id` and invoke only that agent’s hook.
- Workbench HITL already shows the intended calling convention: run `agent._execute(human_answer_method, params)` with `fields` and `context`.
## Fix Focus Areas
- src/supervaizer/routes.py[227-276]
- src/supervaizer/admin/workbench_routes.py[472-540]
- src/supervaizer/agent.py[824-838]
- src/supervaizer/job.py[60-91]
## Proposed fix
1. Resolve the job (preferably `include_persisted=True`) via `Jobs().get_job(job_id, include_persisted=True)`.
2. If the job cannot be found, return a 404 (or at minimum do not dispatch hooks).
3. Select the owning agent from `server.agents` using `job.agent_name`.
4. Invoke the hook via `agent._execute(agent.methods.human_answer.method, params)` and pass a param dict compatible with existing hooks, e.g.:
- `fields`: request.answer (optionally strip `casestep_index` from fields)
- `context`: {"job_id": job_id, "case_id": case_id}
- `job_id`, `case_id`
- optionally `payload`: request.answer and `message`: request.message
5. Execute this in a thread executor (`asyncio.to_thread` / `loop.run_in_executor`) to avoid blocking the event loop (see workbench pattern).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Missing case returns success🐞
Description
When a case isn’t found in the in-memory registry, update_case_with_answer still returns HTTP 200
"success" and proceeds to run human_answer hooks, which masks client errors and enables arbitrary
hook execution for unknown job_id/case_id pairs.
Code

src/supervaizer/routes.py[R227-258]

+        # Try in-memory registry (populated on job_start; may be empty on Cloud Run replicas)
      case = Cases().get_case(case_id, job_id)
-        if not case:
-            log.warning(f"Case with ID {case_id} not found for job {job_id} §SRCU02")
-            raise HTTPException(
-                status_code=http_status.HTTP_404_NOT_FOUND,
-                detail=f"Case with ID {case_id} not found for job {job_id} §SRCU02",
+        if case is not None:
+            if case.status != EntityStatus.AWAITING:
+                raise HTTPException(
+                    status_code=http_status.HTTP_400_BAD_REQUEST,
+                    detail=f"Case {case_id} is not awaiting input. Current status: {case.status.value} §SRC01",
+                )
+            update = CaseNodeUpdate(
+                name="Human Input Response",
+                payload={
+                    "answer": request.answer,
+                    "message": request.message,
+                    "response_type": "human_input",
+                },
+                is_final=False,
          )
-        # Check if the case is in AWAITING status (waiting for human input)
-        if case.status != EntityStatus.AWAITING:
-            raise HTTPException(
-                status_code=http_status.HTTP_400_BAD_REQUEST,
-                detail=f"Case {case_id} is not awaiting input. Current status: {case.status.value} §SRC01",
+            casestep_index = request.answer.get("casestep_index")
+            if casestep_index is not None:
+                case.patch_step(int(casestep_index), update)
+                from supervaizer.lifecycle import EntityEvents
+                from supervaizer.storage import PersistentEntityLifecycle
+                PersistentEntityLifecycle.handle_event(case, EntityEvents.INPUT_RECEIVED)
+            else:
+                case.receive_human_input(update)
+            case_status = case.status.value
+        else:
+            log.warning(
+                f"[Case update] Case {case_id} not in registry for job {job_id} — "
+                "calling human_answer hook only (stateless replica)"
          )
-
-        # Create a case node update with the answer
-        update = CaseNodeUpdate(
-            name="Human Input Response",
-            payload={
-                "answer": request.answer,
-                "message": request.message,
-                "response_type": "human_input",
-            },
-            is_final=False,
-        )
-
-        # Update the case with the answer
-        # case.update(update) - Redundant, receive_human_input calls update()
-
-        # Transition the case from AWAITING to IN_PROGRESS
-        case.receive_human_input(update)
-
-        # TODO CALL CUSTOM HOOKS HERE - AS DEFINED IN THE AGENT CONFIGURATION
-        # TODO REDEFINE AGENT TO ADD CUSTOM HOOKS HERE
+            case_status = "unknown"
Evidence
The route explicitly logs a registry miss and sets case_status = "unknown" but still continues to
hook dispatch and returns a success response. In contrast, the workbench HITL endpoint treats a
missing case as a 404, indicating the expected API behavior is to reject unknown cases rather than
acknowledging them as success.

src/supervaizer/routes.py[227-258]
src/supervaizer/routes.py[282-288]
src/supervaizer/admin/workbench_routes.py[482-487]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The case-update endpoint returns HTTP 200 even when it cannot resolve the case (and potentially the job), and still runs hooks. This makes callers believe the answer was applied and creates an unintended “webhook” to trigger `human_answer` for arbitrary IDs.
## Issue Context
This route is API-key protected, but returning success on unknown IDs still hides operational problems and allows unintended execution paths.
## Fix Focus Areas
- src/supervaizer/routes.py[227-289]
- src/supervaizer/admin/workbench_routes.py[482-493]
## Proposed fix
1. Attempt to resolve the job (`Jobs().get_job(..., include_persisted=True)`) and case.
2. If the job is not found: return 404.
3. If the case is not found: return 404 (or 409/410 depending on lifecycle semantics), matching workbench behavior.
4. Only dispatch the `human_answer` hook once the owning agent and job/case context are validated.
5. Keep the "stateless replica" logging, but do not acknowledge success unless the system can actually apply/route the update deterministically.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment thread src/supervaizer/routes.py Outdated
…ensive imports fixtures to tests/test_routes_case_update.py:

-ce pytest fixtures: _jobs_registryolation to reset Jobs()
 registry tests, and_on_server to register a Job under the server's agent so POST /jobs/{id}/cases/{id}/update resolves.
 - Import additional domain types (Agent, AgentMethod, AgentMethods,
 JobResponse,, ParametersSetup and Jobs helper to support and assertions.
 - Add cryptography and typing imports by fixtures.

 Update to use job_server fixture:
 - Replace direct use of job_fixture with job_server in tests that need an in-memory job registration so the route can locate the job.
 - Adjust expected job_id assertions to match job_on_server.id.

- Clarify and tighten test expectations for error cases:
 - Change test_update_case_job_not_found docstring and assertions to expect a404 job is and assert the error detail includes the id.
 - Update test_update_case_not_found docstring to reflect404 behavior for unknown case_id under a known.

- Minor formatting and comment fixes:
 - Fix capitalization in header.
 - Organize into a grouped style for readability.

Why:
- Ensure tests run deterministically by isolating the global Jobs()
 registry and by registering authoritative job under the server's agent name. This allows the update route to both success and failure paths correctly and makes assertions reflect the actual behavior (404 for missing resources).
@alain-sv
alain-sv merged commit 2de1def into develop Apr 13, 2026
5 checks passed
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