bug fixes for #388

[yangm2]

refactor into ScenarioId class

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update
• Infrastructure
• Maintenance

Description

• code review fixes
• refactor into ScenarioId class
• optimizations

Related Tickets & Documents

• Related Issue download new scenarios #388
• Closes #

QA Instructions, Screenshots, Recordings

Please replace this line with instructions on how to test your changes, a note on the devices and browsers this has been tested on, as well as any relevant images for UI changes.

Added/updated tests?

• Yes
• No, and this is why: please replace this line with details on why tests have not been included
• I need help with writing tests

Documentation

• If this PR changes the system architecture, Architecture.md has been updated

[optional] Are there any post deployment tasks we need to perform?

[yangm2]

@claude code-review

[yangm2]

@claude code-review

[yangm2]

@claude code-review

[yangm2]

@claude code-review

[yangm2]

@claude code-review

[yangm2]

@claude code-review

[yangm2]

@claude code-review

[leekahung]

@claude pr-review

[claude]

Claude finished @leekahung's task in 5m 39s —— View job

---

Code Review

• Gather context (PR diff, commit history)
• Review langchain_tools.py (ORS 90.160 deadline tool)
• Review langsmith_dataset.py (ScenarioId refactor)
• Review tests + conftest.py + graph.py
• Check status of findings from the 2026-09-09 review + @leekahung's inline comments
• Post review

---

The tree is unchanged since the 2026-09-09 review — bdaff30 is still HEAD and was committed one minute before that review ran, so all ten findings from that round are still open. Rather than restate them at length, they're confirmed-and-compressed at the bottom; the top of this review is @leekahung's two inline comments and two findings that round missed.

---

@leekahung's conftest.py comments — both correct

On the lru_cache / TRACING_V2-outranks-TRACING comment (conftest.py:35): right, and the fix is a line. langsmith.utils.get_env_var is cached and tracing_is_enabled() resolves TRACING_V2 with TRACING only as its default, so a test that sets LANGSMITH_TRACING=true gets neither the fresh read nor the winning variable. The docstring's last sentence — "A test that needs the key sets it itself, and its own monkeypatch wins inside its body" — promises something the fixture cannot deliver.

Worth adding: the cache cuts both ways. If anything resolves tracing before the first test runs — the tenantfirstaid.constants import chain at collection time is the obvious candidate — the cached value defeats the fixture's own setenv(..., "false") just as thoroughly as it defeats a test's opt-in. get_env_var.cache_clear() on entry and teardown is what makes the fixture's guarantee real, not just what restores opt-in. That's the direction that actually matters for the fixture's stated purpose, so I'd treat the cache_clear() as the fix and the docstring trim as the follow-on.

On LANGSMITH_API_KEY (conftest.py:37): right that delenv can't reach make_client() — constants.py:235 binds os.getenv("LANGSMITH_API_KEY") at import and langsmith_dataset.py:212-216 reads that constant, so the delenv is invisible to it and test_langsmith_dataset.py:530's patch-the-constant is the only lever. One nuance for the rewrite: the delenv isn't inert — the tracer's own implicit Client() reads the key from the environment when it's constructed, which is the span-upload path the fixture exists to block. So the accurate claim is narrower than the docstring's and broader than "can't reach it": it blocks a tracer client constructed after the fixture runs, and does nothing for make_client() or for a client already built at import time. Trimming to that also removes the "a test that needs the key sets it itself" clause, which is wrong for the reason in your first comment.

---

1. The two early-return paths have no AGENT NOTES fence and no relay marker — but the docstring promises one

langchain_tools.py:500-508, :575-580, docstring :493-498, system_prompt.md:22

The restructuring this round moved the relay marker onto the scaffolding side so a model relaying "everything below" can't leak agent-directed text. That holds for the computed path. The two early returns bypass the structure entirely — each returns a bare string with no === AGENT NOTES fence and no marker:

• SERVICE INVALID, ... — ends with "Tell the tenant this service was defective; do not compute or state a deadline for it. If the notice was also sent by first-class mail, call this tool again with service_method=email_and_mail."
• MISSING INPUT, NO DEADLINE COMPUTED: ... — ends with "If the tenant can supply the exact time of service, call this tool again with service_time set."

Meanwhile the docstring's Returns: says the result always carries "a single section header, the AGENT NOTES fence" with "everything after the marker" being the tenant-facing answer, and system_prompt.md:22 says "Its output is divided by the marker". Neither says what to do when there is no marker. A model that has been told the boundary is always there, handed a string that has none, plausibly relays the whole thing — and the tenant reads "call this tool again with service_time set."

The MISSING INPUT path is not exotic: a 72-hour termination notice served by first-class mail with an unknown time of service reaches it, which is an ordinary intake state. The exhaustive tables assert "SERVICE INVALID" in result and "DEADLINE:" not in result but nothing about structure, so the leak is untested on both paths.

Fix this →

2. The prompt and the tool now disagree about the mail+e-mail non-termination extension

system_prompt.md:36, :38 vs langchain_tools.py:513-539, :660

The tool applies the ORS 90.155(2) three-day extension to email_and_mail + non-termination, on the reading that the mail leg is (1)(b) service and the e-mail copy is a 90.155(3) alternative. Both prompt clarifications say the opposite, in the model's own voice:

• :36 — "the three-day extension, which applies only to first-class mail service under (1)(b) alone" … "still extended by 90.155(2) when served by first-class mail alone".
• :38 — "Do not apply the mail-only three-day extension here", inside the (1)(d)/(5) e-mail clarification.

So on that path the model relays a tool block asserting "The ORS 90.155(2) mail extension adds 3 days because the notice was served by first-class mail" while its instructions twice say that case gets nothing. This is the failure finding #3 (below) describes, arriving by a different route: the relayed deadline and the model's own prose disagree about a three-day swing, and it's the tenant who reconciles them.

The tool is careful about this — it emits an explicit "not settled … confirm it with a lawyer" note — and the prompt carries none of that nuance. Whichever reading wins, the prompt should carry the same one and the same hedge.

Fix this →

---

Still open from the 2026-09-09 round — all re-confirmed against bdaff30

#	Where	Finding
3	test_langchain_tools.py:118, graph.py:84	Nothing exercises the tool's schema conversion. test_tool_schema_matches_function_signature still covers only retrieve_city_state_laws; every deadline test goes through .invoke(), which validates the Pydantic model and never builds a JSON schema. date/time/Optional[time]/StrEnum are the corners of Gemini's OpenAPI subset most likely to be rejected, and the tool is bound to the live model at graph.py:219/:233, so a converter failure 400s every conversation that reaches it while CI stays green. Still the highest-value open item.
4	langchain_tools.py:329-334	service_date's ge/le are enforced at validation but Pydantic v2 doesn't render date constraints into JSON Schema, so the model sees format: "date" with no range to correct against. Cheapest fix: state the range in the field description. The model_json_schema() assertion in #3 settles it either way.
5	langchain_tools.py:656-671	The tenant block asserts a bare DEADLINE: with none of its premises — period_value, period_unit, service_date, service_method are all above the marker, and the prompt forbids restating them. One premise line below the marker makes it self-checking.
6	langchain_tools.py:653, system_prompt.md:22, test_langchain_tools.py:763	The relay marker is still a triplicated literal with no test tying tool to prompt; a prompt-side edit drifts silently and the likely failure is relaying the AGENT NOTES header.
7	test_langchain_tools.py:875, :960	assert basis in result is a substring check, so "ORS 90.160(1)" also passes against "ORS 90.160(1) and ORS 90.155(2)" — the ten no-extension rows can't independently detect a spurious extension. assert f"Legal basis: {basis}." in result anchors both ends.
8	langchain_tools.py:402	_parse_service_time still misses "2:30 p.m." and "2:30:00 PM". Recoverable ValidationError, so minor.
9	langsmith_dataset.py:1290, :1293 vs :1665	Two sentinels for one concept: the comment says -1 so that scenario_id=0 isn't "unknown", while both readers default to 0. Dead today only because every key is populated.
10	langsmith_dataset.py:253-256, :301-304	dict(inputs_schema) is shallow, but the comment claims the SDK gets "a dict it owns"; _load_schema_properties' own docstring is honest that the shallowness leaves nested dicts aliased to the lru_cache. Overstated comment, or use copy.deepcopy.
11	langsmith_dataset.py:338-342	cmd_dataset_pull's decorate-sort buys nothing — it caches the key to avoid recomputation, then re-reads the same metadata via is_unlabeled on the next line. sorted(..., key=ScenarioId.sort_key), as cmd_example_list:816 now does, is the same work in one line.
12	langchain_tools.py:649; ScenarioId.partition:496	Minor, both unchanged: the UNIT CHECK reads "1 HOURS, NOT 1 DAYS" at period_value == 1; and partition's "silently keeps the last" docstring is now unreachable from push/validate but still live via cmd_dataset_diff:684, which partitions never-validated remote examples — worth naming diff as the remaining exposure.

Two smaller notes on this round's new files: test_period_value_bounds.py now covers service_date bounds too, but its name and module docstring still say period_value only — and the five tests would sit naturally in test_langchain_tools.py beside the rest of the tool's suite. And cmd_example_adopt's new collision check (langsmith_dataset.py:906-915) compares --start-id only against remote ids; a local JSONL record not yet pushed could still collide, which the newly-fatal duplicate check would then surface at the next push. Probably the right scope, since pull overwrites local anyway — just worth knowing the check isn't total.

The ScenarioId refactor itself reads well: parse's bool/whole-float/IntEnum handling is right, __new__ correctly rejects the subclass so parse stays the only untrusted entry point, and the bool-vs-1 remote-lookup test (test_langsmith_dataset.py:1235) pins the bug the old == comparison had. The duplicate-id line-number reporting and the _adopt_metadata unknown-key preservation are both clean fixes.

---

Not verified: uv and pytest aren't on this runner and executing python isn't permitted here, so nothing above was run — it's static reading plus the statute text in the repo's own corpus. Finding #4's claim about Pydantic's date-constraint schema output remains reasoning, not observation. To have the suite run as part of the review, the job needs Bash(uv run pytest:*) in --allowedTools.
· branch more-evals

[leekahung]

Non-blocking follow-up: the CodeQL unused-import alert at #391 (comment) is still open — # noqa: F401 silences ruff but not CodeQL. Note-severity in test code, so the check passes regardless. Worth a cleanup pass later.