Repository navigation
A reachability follow-up can reuse what the last answer already resolved (#318) - #361
patelankeet2 wants to merge 9 commits into
Conversation
#318) answer_question() gains one new, optional, backward-compatible parameter: previous: Optional[Dict[str, Any]] = None, shaped {"source_device", "destination_ip", "destination_display"} -- exactly the three values the reachability path already produces internally for every successful resolution. Every existing caller (including every existing test) is unaffected by omitting it. Deliberately does NOT relax intent classification. A follow-up still needs a reach-style keyword to be recognised as reachability at all -- "does it reach 10.20.20.5" works, a bare "what about rtr-branch" with no reach keyword still refuses exactly as before. Only entity resolution, inside the already-existing reachability path, gains a fallback: where _find_device()/_resolve_destination() would refuse today, previous's corresponding value is tried first. CLAUDE.md 7c is explicit that this module's narrow scope is what makes it safe rather than merely limited, and #318's own acceptance criteria say "entities", not "intents" -- so widening the three-way classifier was rejected as a materially bigger, riskier change than extending one fallback inside one path. A carried-forward device is re-validated against the CURRENT snapshot's device_names(), never trusted blindly -- a device from an earlier, different upload must not silently survive into this one. Explicit text in the current question always wins over previous, for free, by construction: the fallback only ever runs when direct resolution returns None, not as a separate override rule. Return value gains a fourth key, always present: resolved_entities (Optional[Dict]), populated whenever a reachability question's source and destination both resolved -- fresh, carried forward, or a mix -- independent of whether the downstream Batfish call itself then succeeded. None for a refusal and for both non-reachability intents, which have no per-question entity to carry. This is the piece a caller persists for the next turn; web/main.py wiring is the next commit. 9 new tests in tests/test_query_translation.py, same FakeSession/ FakeQuestions pattern as the existing 31 (now 40 total, all green), proving grounding via traceroute_calls (the real parameters sent to Batfish), not just returned prose: - a fresh resolution populates resolved_entities - both non-reachability intents populate nothing - source-side and destination-side fallback, independently - explicit text always overrides previous (the property that matters most -- mutation-tested before committing: temporarily let previous override explicit text, confirmed this exact test fails for the right reason, reverted) - no previous behaves exactly as before (regression guard) - a previous device absent from the current snapshot is refused, not trusted - a bare follow-up with no reach keyword still refuses (the scope boundary, pinned as a test rather than left as prose) python -m pytest tests/test_query_translation.py -v -> 40 passed.
New module-level _last_resolved_entities: Dict[str, Dict[str, Any]] = {},
keyed by current_session_id(), placed beside _uploaded_sessions and
reusing its exact "unbounded, deliberately, and small" justification --
ephemeral conversation state for a handful of local users, no reason to
touch disk the way the file-based policy/business-context state does.
ask_question() reads this session's stored state, passes it into
answer_question() as `previous`, then pops the returned resolved_entities
key and stores it for next time. Popped before the response goes out --
/api/ask's wire contract stays exactly the three keys it has always
returned; the fourth key is a signal between this endpoint and
answer_question(), never new public API surface. Verified directly:
test_resolved_entities_never_reaches_the_http_response asserts the exact
three-key response shape.
_discard_last_resolved_entities(), called from upload_config() beside the
existing _discard_staged_policy()/_discard_staged_business_context()
calls: a follow-up must not carry a device or address forward across an
upload boundary into a different network, even before answer_question()'s
own re-validation would catch a device that genuinely does not exist in
the new snapshot.
Fixed the one existing test this signature change touched:
test_a_successful_connection_delegates_to_answer_question's fake now
accepts previous=None, matching the real function it stands in for.
Five new tests, deliberately faking answer_question() rather than
re-proving its own carry-forward logic a second time (that is
tests/test_query_translation.py's job, with a real fake Batfish session)
-- these are about what THIS layer does with the fourth key:
- a follow-up in the same session receives the previous resolution
- resolved_entities never reaches the HTTP response
- two sessions never share a carried-forward entity (same class of
guarantee tests/test_session_isolation.py already establishes for
uploads and the analysis cache)
- uploading a new config clears the previous network's carried-forward
entity
python -m pytest tests/test_web_ask_endpoint.py -v -> 9 passed.
patelankeet2
added
phase: 3
SamikaPerera
left a comment
SamikaPerera
left a comment
There was a problem hiding this comment.
Approving. I checked the design decision you flagged for a second look rather than taking the description's word for it, and it holds — including the part that is easiest to claim and hardest to prove.
The scope boundary is real, not just asserted
You say option 2 leaves intent classification untouched. Verified by comparing the actual source of every symbol that decides which question gets asked, origin/main against this branch:
_REACH_KEYWORDS IDENTICAL
_DEAD_RULE_KEYWORDS IDENTICAL
_UNDEFINED_REF_KEYWORDS IDENTICAL
_split_on_reach_keyword IDENTICAL
Byte-identical, all four. The only change to the dispatch is threading previous into the reachability branch.
And test_a_bare_follow_up_with_no_reach_keyword_still_refuses actually binds
A test named after a boundary is not the same as a test that holds it, so I mutated the boundary itself — implemented option 1, the one you rejected, by classifying any bare question as reachability when previous exists:
KILLED bare follow-up classified as reachability when `previous` exists
KILLED `previous` OVERRIDES explicit text instead of only filling a gap
The second is the one I most wanted to see fail, because "explicit text always wins, for free, by construction" is the kind of claim that is true of the code as written and quietly stops being true after one refactor. It is pinned.
Full suite on your branch: 1466 passed, 10 skipped.
Why I think option 2 was right beyond the acceptance criteria
You justify it from #318's wording ("entities", not "intents"), which is correct but is the weaker argument. The stronger one is §7c's: every guard in this project checks that the answer is grounded in the query, and none check that the query was the right one. Widening classification is the one change that makes a mistranslation more likely, and a mistranslation produces a real, evidenced, confidently wrong answer that passes every other guard. Entity resolution failing just refuses.
_discard_last_resolved_entities() on upload is the right instinct too, and I like that the docstring says the belt-and-braces part out loud — re-validating against the current snapshot is not a reason to skip clearing, because "there is no previous turn" is a better answer than "check and see".
Two things to pick up, neither blocking
1. _last_resolved_entities is a third module-level cache and tests/conftest.py does not clear it.
That file's docstring is explicit: "a new cache added to web/main.py must be added here too, and the loop below fails loudly on a name that no longer exists rather than skipping it silently." Measured:
conftest resets: reset_explanation_cache, reset_analysis_cache
after both resets: _last_resolved_entities still holds 1 entry
It almost certainly does not bite today — it is keyed by session id and each TestClient gets its own — which is exactly why it is worth adding now rather than after someone writes a test that shares one. The same dict also only ever loses entries for the current session on upload, so a long-running server accumulates one entry per session that never uploads again. Small, bounded by traffic rather than by anything in the code.
2. CLAUDE.md §7c line 418 still says the return shape is three keys.
answer_question() now returns four. The endpoint pops the fourth before responding, so the API surface is unchanged and I do not think anything is misled today — but §7c names the function, not the endpoint, and it sits in the section people read to understand what this module guarantees. One fact in two places, and the copy that moved is the code.
Happy either way on both: fold them in here, or a follow-up PR — they are not reasons to hold this.
One note on process, not on your code
Your last commit is from 21 September and mine is the first review on it after three days. That is the queue problem #356 is about rather than anything you did, and I would rather say so than let the gap read as hesitation about the change.
|
Following up after approving — while checking a related pattern, found a live instance here: reload destroys the client-side chat log but not the server's memory of the prior turn, so a follow-up can silently inherit a resolved entity (like a device name) from a turn the user can no longer see. It's currently caught, but by accident — Measured on your branch against real Batfish, so it is not a thought experiment:
Why I am not asking you to change anything. The echo-back means turn 2 renders as "Can rtr-us5 reach 10.10.10.9?" — the user sees the device that was inherited, even though they cannot see where it came from. That is a visible, checkable claim rather than a hidden one, which is the difference between this and a real defect. The part worth recording is why it holds. §7c requires showing the translated question back so somebody can notice a mistranslation. Nothing in that argument is about reload; it catches this because a resolved question happens to name its own entities. Two of the three instances in #365 had no such accident and needed a deliberate fix. One concrete thing you might consider, your call and not a review ask: if |
ARSH871-bot
left a comment
ARSH871-bot
left a comment
There was a problem hiding this comment.
@patelankeet2, requesting changes for one finding, and it is the one §7c exists to prevent. The design choice itself is right: relaxing entity resolution only, never intent, is the narrow version, and your bare-"what about" regression test pins the half it covers. The gap is in when the entity fallback fires.
The fallback fires on failure, not on a back-reference
if source_device is None and previous is not None:
...
if resolved is None and previous is not None:
...None here means "did not resolve", and that covers two different questions:
- the question left the side out: "Does it reach 10.20.20.5?" Filling this is the feature.
- the question named something that does not resolve: a typo, a plain-English name, a malformed address. Before this PR every one of these refused, and §7c says they must: "Can the guest network reach the finance server" is refused ... widening it is future work that must keep the refusal path intact.
The code cannot tell them apart, so the second kind now gets the previous turn's entity substituted in.
Measured, /api/ask end to end, real Batfish, your branch merged onto today's main
Same questions, first each in a fresh session, then in one session in order:
fresh session after "Can rtr-us5 reach 10.10.10.5?"
Can the guest network reach the finance server? refused understood 'Can rtr-us5 reach 10.10.10.5?'
"Yes. Traffic from rtr-us5 reaches
10.10.10.5." grounded=True
Can rtr-brnch reach 10.10.10.5? (typo) refused same: 'Can rtr-us5 reach 10.10.10.5?' Yes
Can it reach 10.10.10.500? (not an IP) refused same: 'Can rtr-us5 reach 10.10.10.5?' Yes
Does it also reach 8.8.8.8? (the feature) refused 'Can rtr-us5 reach 8.8.8.8?' No, NO_ROUTE <- correct
The first row is §7c's own must-refuse example, answered "Yes", grounded=True. That is the shape §7c calls the worst possible: evidenced, confident, and reproducible, a correct answer to a question nobody asked. The third is subtler and worse for a human reader: they typed .500, got "Yes ... 10.10.10.5", and the two strings differ by two characters.
question_understood does show the substitution, and §7c leans on it. But it is the safety net for a mistranslation we could not have prevented. These three are refusals we already had and would be giving away.
No test covers it. Every follow-up test asks with a pronoun ("Does it reach ...", "Can rtr-branch reach it?") or with text that fully resolves. None passes previous alongside a side that names something unresolvable. That is exactly the input that breaks the guarantee, so the suite could not see it.
Suggested fix: carry forward on a back-reference, not on a miss
Fall back only when the unresolved side is an explicit reference to the last turn, from a closed set, and otherwise refuse exactly as main does:
- source side: nothing left after the reach keyword except
it/that/this/the same device - destination side:
it/that/there/the same address, and never when the segment contains a digit (anything IP-shaped that failed to parse is a malformed address, not a reference)
Same spirit as the rest of §7c: recognise positively from a closed set rather than infer. It keeps every question above in the refused column, and keeps your working examples working.
Tests I would want, all passing previous: the three questions above must each come back grounded=False with resolved_entities=None, parametrised. Then mutate the gate (delete the back-reference check) and watch all three fail. That is the test that could have caught this.
What I checked that holds
- Session isolation. Every fresh-session "Does it also reach 8.8.8.8?" above refused, so one session's entities do not leak into another's.
- Explicit text wins when it resolves: "Can rtr-us5 reach 8.8.8.8?"-style questions ignore
previous, as your mutation showed. /api/askstill returns exactly three keys;resolved_entitiesis popped server-side.- Merged onto
7111133:pytest tests/1525 passed, 13 skipped; ruff clean.
@SamikaPerera, flagging because your approval is on this same commit (e9e1648). This was not visible from the diff; it only shows up when you ask a follow-up that names something.
Re-request me when it is pushed and I will re-run the table above against it.
… failure (#318 review) Found by @ARSH871-bot: the first version fell back to the previous turn's entity whenever a side failed to resolve, for ANY reason -- which did not distinguish "the question left this side out" (the feature) from "the question named something that does not resolve" (a typo, a plain-English name, a malformed address). Every one of the second kind used to refuse, and the module's own SCOPE section says it must: "Can the guest network reach the finance server" is CLAUDE.md's own must-refuse example. Measured with the old version, previous turn resolved to rtr-us5 / 10.10.10.5: "Can the guest network reach the finance server?" Yes, grounded=True "Can rtr-brnch reach 10.10.10.5?" (typo) Yes, grounded=True "Can it reach 10.10.10.500?" (malformed IP) Yes, grounded=True All three silently answered using the PREVIOUS turn's entity -- the worst shape a query-layer mistake can take: evidenced, confident, and reproducible, answering a question nobody asked. Fixed exactly as suggested: the fallback now fires on a BACK-REFERENCE, recognised positively from a closed set ("it", "that device", "the same address", ...; never anything containing a digit on the destination side), the same discipline _REACH_KEYWORDS already applies to intent classification. A segment that names something and fails to resolve it is refused, never guessed at as a reference to the last turn. One refinement beyond the literal suggestion: a single filler word ("also"/"still"/"too"/"even") is tolerated from either end, so "does it ALSO reach" still matches "it" -- the exact phrasing used in this PR's own live verification. Caught my own bug while building this: the first attempt applied the strip twice, which silently tolerated TWO filler words end to end ("does it also still reach" wrongly matched). Fixed to strip at most once, leading tried first; pinned by test_a_second_filler_word_is_not_tolerated. 6 new tests, all in tests/test_query_translation.py: - the three cases from Arsh's review, parametrised, each asserting grounded=False, resolved_entities=None, and zero traceroute calls - a monkeypatch-based mutation test that reverts the gate and confirms the previously-guarded case wrongly succeeds - filler-word tolerance, and the second-filler-word boundary Also mutation-tested at the source level, not just via monkeypatch: reverted both gates to the old unconditional fallback, ran the suite, watched 4 tests fail for the right reason (including one that crashed rather than merely asserted wrong, which is still a real failure of the guarantee), reverted. python -m pytest tests/test_query_translation.py -v -> 46 passed (was 40).
1. _last_resolved_entities was a third module-level cache and tests/conftest.py's own docstring says a new one must be added to its reset list or the loop's own assert should have caught it. Added reset_last_resolved_entities(), mirroring reset_explanation_cache()/ reset_analysis_cache() exactly, wired into conftest.py's list. Verified the safeguard itself works, not just the fix: renamed the function temporarily, confirmed conftest's assert fires loudly (9 errors, not a silent skip) exactly as its own docstring promises. Reverted after. 2. CLAUDE.md 7c said the return shape is "three keys" -- answer_question() has returned four since this PR's first commit. Corrected, and the correction states its own history (three keys for six days) rather than silently overwriting, matching this file's own established practice elsewhere for a fact that moved. python -m pytest tests/ -q -> 1666 passed, 13 skipped.
|
Fixed exactly as suggested, plus one refinement I want to flag rather than let you find. Merged current Reproduced your table before touching anythingRan your three questions against this branch, unpatched, with a previous turn resolved to The fix
One addition beyond your suggestion: a single filler word ( Verified, not assumedRe-ran your exact table against the fix, live, through 6 new tests in Full suite: 1,666 passed, 13 skipped. Also picked up both of Samika's non-blocking items
No em-dash bytes in the diff. Re-requesting both of you -- Arsh for the fix itself, Samika since her two items are folded in too. |
5bf659c to
2a77d8c
Compare
SamikaPerera
left a comment
SamikaPerera
left a comment
There was a problem hiding this comment.
Small correction for the record — the three questions and the closed-set gate are Arsh's, from his CHANGES_REQUESTED review, not mine. My one comment there suggested a sentence, not a gate. Don't want credit sitting on the wrong person for catching a real §7c-class defect.
To be precise about who found what, since all three are in this PR now:
| found by | |
|---|---|
| the follow-up survives a reload and inherits an invisible entity | me, #365 |
| the fallback fires on any resolution failure, so §7c's must-refuse example answers "Yes" | Arsh |
| the closed-set back-reference gate as the fix | Arsh |
| the double-filler-word bug in the gate itself | you, while building it |
Mine was the weaker of the two: I looked at the reload and concluded the echo-back contained it. Arsh looked at the same fallback and found the input where the echo-back is the problem — the user types .500, reads "Yes … 10.10.10.5", and the two strings differ by two characters. I had the mechanism and stopped at the wrong conclusion.
Re-approving. I re-ran Arsh's table myself rather than taking yours
One continuous session, a real uploaded config, real Batfish — the condition under which the defect appeared:
seed turn: 'Can rtr-us5 reach 10.10.10.5?' grounded=True
(so every row below runs with the server remembering a turn)
7c's must-refuse example "Can the guest network reach the finance server?" -> REFUSED
typo'd device "Can rtr-brnch reach 10.10.10.5?" -> REFUSED
malformed address "Can it reach 10.10.10.500?" -> REFUSED
the actual feature "Does it also reach 8.8.8.8?" -> 'Can rtr-us5 reach 8.8.8.8?'
Three refusals restored, feature intact. 1666 passed, 13 skipped.
Mutation-tested at source level: 2 of 3 killed, and the third is not a gap
KILLED fallback fires on any failure again (the original defect)
KILLED the closed set becomes anything (gate always true)
SURVIVED the destination gate stops excluding digits
I chased the survivor before reporting it, because "the .500 case is untested" would have been the obvious and wrong conclusion.
It is an equivalent mutant. No input distinguishes the two versions:
destination back-references: ['it', 'that', 'that address', 'the same address', 'there']
any containing a digit? False
input with digit guard without
10.10.10.500 False False
10.10.10.5 False False
it True True
also it True True
host 10.0.0.1 False False
it 1 False False
Since no member of the closed set contains a digit, anything digit-bearing already fails the set test. The guard is defensive redundancy, not load-bearing — which is fine, and I would keep it: it states the intent at the point of decision, and it would start mattering the moment somebody adds a member like that /24. Worth a one-line comment saying it is redundant today so the next person does not spend the twenty minutes I just did deciding whether it was a hole.
The .500 case itself is covered, at tests/test_query_translation.py:587, via the closed set rather than the digit guard.
Both of my non-blocking items, verified rather than taken
tests/conftest.py:120 "reset_last_resolved_entities" present in the reset list
web/main.py:373 reset_last_resolved_entities() exists
CLAUDE.md §7c names the fourth key, dated, with the six-day admission
The CLAUDE.md wording is better than what I asked for. I flagged a stale sentence; you wrote why it was stale and for how long, in the same register the file's other corrections use — and drew the distinction I had not: the fourth key never reaches /api/ask's response, so the public shape genuinely is three keys and only the internal one changed. That is a more accurate correction than the one I requested.
And checking conftest's own safeguard by renaming the function to watch the assert fire — 9 errors rather than a silent pass — is the right instinct. A reset list that silently skips a renamed entry is worse than no list.
One thing I'd still like, not blocking
_strip_back_reference_filler tolerating exactly one filler word is the kind of boundary that invites a third case later ("does it maybe also reach"). test_a_second_filler_word_is_not_tolerated pins the current line, so this is a note rather than an ask: if that set ever grows, the "at most once" rule is the thing to re-test, not the word list.
Approving 2a77d8c.
ARSH871-bot
left a comment
ARSH871-bot
left a comment
There was a problem hiding this comment.
@patelankeet2, the fix for my 27 Sep finding holds, and I re-ran the whole table against it. I am still requesting changes for two new instances of the same class, both small and both measured below. Neither is something the closed set itself got wrong.
Everything below is /api/ask end to end on real Batfish, with your head (2a77d8c) merged locally onto today's main (1874289, which has #372 to #375 that your branch does not). The merge is clean.
What holds
session A, rtr-us5-insecure uploaded
Can rtr-us5 reach 10.10.10.5? grounded=True 'Yes. Traffic from rtr-us5 ...'
-- the three from my review, after that resolved turn --
Can the guest network reach the finance server? grounded=False refused
Can rtr-brnch reach 10.10.10.5? grounded=False refused
Can it reach 10.10.10.500? grounded=False refused
-- inputs your set was not written for --
Can it reach that server? grounded=False refused
Can this router reach 10.10.10.5? grounded=False refused
Can it reach the 2nd floor? grounded=False refused
Does it also still reach 8.8.8.8? grounded=False refused (two fillers)
-- the feature --
Does it also reach 8.8.8.8? grounded=True understood 'Can rtr-us5 reach 8.8.8.8?'
-- after /api/upload --
Does it reach 10.10.10.5? grounded=False refused (context cleared)
session B, fresh
Does it also reach 8.8.8.8? grounded=False refused (no leak between sessions)
Mutated, with Batfish up and down: deleting the source gate kills 2 tests, deleting the destination gate kills 1, and a faithful "filler stripped twice" mutant kills test_a_second_filler_word_is_not_tolerated. Full suite on the merge: 1773 passed, 13 skipped (Batfish up) and 1762 passed, 24 skipped (down). ruff clean.
Blocking 1: /api/sample does not clear the carried-forward context
Your docstring on _discard_last_resolved_entities() says a new network must not inherit the old one's resolved device, "even if a same-named device happens to exist in the new one by coincidence". upload_config() calls it. load_sample(), the other way a new network arrives, clears the staged policy but not this. And the sample's only device is rtr-us5, the same hostname as every rtr-us5-* fixture, so re-validation passes:
Can rtr-us5 reach 10.10.10.5? (their upload) grounded=True
POST /api/sample
Does it reach 10.10.10.5? grounded=True understood 'Can rtr-us5 reach 10.10.10.5?'
'Yes. Traffic from rtr-us5 reaches 10.10.10.5.'
Fix: one line, _discard_last_resolved_entities() in load_sample() beside _discard_staged_policy(). Test: ask, POST /api/sample, then a pronoun question must refuse. Mutate by deleting the new line and watch it fail. It is the property your docstring states, not just the one call site.
Blocking 2: "IT" is read as the pronoun "it"
_is_source_back_reference() lowercases before the set lookup, so the department name becomes a back-reference:
Can IT reach 10.10.10.5? grounded=True understood 'Can rtr-us5 reach 10.10.10.5?'
'Yes. Traffic from rtr-us5 reaches 10.10.10.5.'
That is the 27 Sep shape again: a word that names something, answered with the previous turn's entity. "Can IT reach the finance server?" still refuses on its destination, but any IT question with an address in it does not.
Fix: refuse the all-caps form before lowercasing, for example if re.search(r"\bIT\b", source_text): return False. Test: add "Can IT reach 10.10.10.5?" to test_a_named_but_unresolvable_segment_still_refuses_even_with_previous.
Not blocking
-
After a refused question, "it" means the turn before.
/api/askonly overwrites the stored entities when a question resolves, so:Can rtr-hq reach 10.1.1.1? refused (no such device) Does it reach 8.8.8.8? grounded=True understood 'Can rtr-us5 reach 8.8.8.8?'The person meant rtr-hq.
question_understoodshows the substitution, so it is visible. But "a back-reference means the last question" is a simpler rule to defend: clear the stored entities when a question does not resolve. It is close to what @SamikaPerera raised on #365 about server memory outliving the chat log, so it may be better decided there. -
The destination digit check cannot fire. No member of
_DESTINATION_BACK_REFERENCEScontains a digit, so the set lookup already rejects anything IP-shaped. Deleting the check changes no behaviour, and no test fails. That is not a test gap: either drop it or say in its comment that it is belt and braces.
Re-request me when it is pushed and I will re-run this table against it the same day.
|
Owning a miss: I re-approved on 2 Oct and Arsh's review four hours later found two blocking instances of the same class that I had not caught. I reproduced both on the branch just now: Both reproduced on (a) (b) 'IT' the department is read as the pronoun The control matters: with no previous turn it refuses, so the substitution is what made it answer — Why I missed them, since that is the more useful part. I verified the fix against the three cases in Arsh's review and the four-row table, and treated reproducing someone else's cases as having checked the gate. I did not ask the next question — what else is in the closed set's blind spot — which is where both of these live. 'IT' is a real department name in exactly the kind of organisation this tool is for, and That is the same defect I have flagged in other people's tests twice this sprint: checking the instances rather than the property. No change requested from me beyond what Arsh has already specified — his two fixes are right and his suggested tests are the ones I would have asked for. Re-request me when it is pushed and I will run the full table rather than the cases. |
Dismissing my own approval: Arsh's 2 Oct review found two blocking items of the same class I had re-approved over, and I reproduced both on 2a77d8c just now. The tick should not count toward merge while they stand. Support for the approach is unchanged.
Clean merge, no conflicts. Re-ran the query translation and web endpoint tests against the result: 55 passed, matching pre-merge.
…iew, round two) Both found by Arsh re-running his full verification table against last week's fix, which still holds -- these are two new instances of the same class, not a regression in the fix itself. 1. load_sample() never cleared _last_resolved_entities the way upload_config() does. The sample's only device is rtr-us5, the same hostname as every rtr-us5-* fixture, so a real upload's resolved entity re-validated cleanly against the sample by coincidence and survived the switch. Reproduced live before fixing: uploaded a real config, resolved a question, loaded the sample, asked a follow-up -- it answered using the previous network's device. One-line fix, _discard_last_resolved_entities() beside the other three discards load_sample() already calls. 2. _is_source_back_reference() lowercases before the set lookup, so "Can IT reach 10.10.10.5?" -- the department, not the pronoun -- matched the back-reference set the same way "it" does. The exact failure shape from the first round: a word that names something real, answered with the previous turn's entity instead of refusing. Reproduced directly before fixing. Fixed by refusing the literal all-caps form, case-sensitively, before any lowercasing happens -- the pronoun "it" is untouched. Also addressed the one non-blocking note: clarified _is_destination_back_reference()'s digit check is deliberate belt and braces (no current back-reference phrase contains a digit, so the set lookup alone already rejects anything IP-shaped) rather than leaving it looking like an untested gap. Two new tests: "Can IT reach ..." added to the existing parametrised refusal test, and a new web-layer test using the real /api/sample endpoint against the real committed sample fixture (not a stub, since the gap was in upload_config()/load_sample() themselves). python -m pytest tests/test_query_translation.py tests/test_web_ask_endpoint.py -v -> 57 passed (was 55).
|
Both findings fixed, in f2c4da6.
Also addressed the non-blocking note: clarified in Mutation-tested both before pushing:
Re-requested as reviewer. |
ARSH871-bot
left a comment
ARSH871-bot
left a comment
There was a problem hiding this comment.
@patelankeet2, both of my round-two findings are fixed, and I re-ran the whole table to confirm it. I am requesting changes once more for the mirror image of the IT finding, and part of that miss is mine: my suggested fix was re.search(r"\bIT\b", source_text), source side only, and you implemented exactly what I asked for.
Everything is /api/ask end to end on real Batfish, with f2c4da6 merged locally onto today's main (393183f, which has #383). The merge is clean.
Both fixes hold
after "Can rtr-us5 reach 10.10.10.5?" resolved:
Can IT reach 10.10.10.5? grounded=False refused <- fixed
Can IT also reach 10.10.10.5? grounded=False refused
POST /api/sample, then
Does it reach 10.10.10.5? grounded=False refused <- fixed
Every earlier row still refuses (the guest network, the typo, .500, "that server", "this router", "the 2nd floor", two filler words). The feature still answers, the upload path still clears, and a fresh session still refuses.
Mutated, with Batfish up and stopped: disabling the IT guard fails [Can IT reach 10.10.10.5?], and dropping the new discard from load_sample() fails test_loading_the_sample_network_clears_the_carried_forward_entity. Full suite on the merge: 1832 passed, 13 skipped (Batfish up); 1805 passed, 40 skipped (down). ruff clean.
Blocking: "IT" on the destination side is still the pronoun
_is_destination_back_reference() lowercases before the set lookup, so:
Can rtr-us5 reach IT? grounded=True understood 'Can rtr-us5 reach 10.10.10.5?'
'Yes. Traffic from rtr-us5 reaches 10.10.10.5.'
Does it reach IT? grounded=True understood 'Can rtr-us5 reach 10.10.10.5?'
"Can the router reach IT?" is a natural question in exactly the organisations this tool is for, and it is answered with the previous turn's address. Same shape as the source-side finding, mirrored.
Fix: the same raw-text check at the top of _is_destination_back_reference(), before the lowercasing. Better still, one small helper both functions call, so the next side cannot miss it. Test: add "Can rtr-us5 reach IT?" to the must-refuse parametrisation.
(I also tried "Can it reach It?". Capitalised "It" mid-sentence is a pronoun as often as not, so I am not counting it.)
Still not blocking, unchanged
After a refused question, "it" still binds to the turn before it: "Can rtr-hq reach 10.1.1.1?" refuses, then "Does it reach 8.8.8.8?" answers about rtr-us5. question_understood shows the substitution, and I suggested on 2 Oct that it may belong on #365 with the other server-memory questions. Fine to leave it there.
Re-request me when it is pushed and I will run the table the same day.
…view, round three) _is_destination_back_reference() had the identical gap the source side was fixed for in round two: lowercasing before the set lookup meant 'Can rtr-us5 reach IT?' matched _DESTINATION_BACK_REFERENCES the same way '...reach it?' does, answering with the previous turn's address instead of refusing. Extracted the raw-text, case-sensitive 'IT' check into one shared helper (_is_literally_the_word_IT()) that both _is_source_back_reference() and _is_destination_back_reference() call, rather than each carrying its own copy -- the actual lesson of this bug: the duplicated check meant fixing one side left the other exactly as broken as before. Added 'Can rtr-us5 reach IT?' to the existing must-refuse parametrization. python -m pytest tests/test_query_translation.py tests/test_web_ask_endpoint.py -v 58 passed (was 57).
|
Fixed, and your own framing of the fix is exactly what I built -- a shared helper, not a second private copy. Reproduced the destination-side bug directly before fixing, same as the earlier rounds: Extracted Added Also merged current Left the #365 "it after a refusal" question exactly where you put it -- not touched here. Re-requesting you. |
3 participants
Closes #318.
What #318 actually scopes out of #240
#240 (direction only, explicitly "do not start work off the back of this") describes problem B as a full planner decomposing a compound question -- "why can't X reach Y, and what would fix it" -- into an ordered sequence of sub-queries. #318 is the scoped issue that actually has acceptance criteria, and they are much narrower: a reachability question may reuse the previous answer's resolved entities, nothing about sequencing or new intents.
Design decision worth a second look
Two ways to let a follow-up work:
Went with 2.
CLAUDE.md7c is explicit that this module's narrow scope is what makes it safe rather than merely limited, quoted directly in #318's own rationale ("widening it must not widen what the model decides"). Touching the three-way classifier is a materially bigger, riskier change than extending one fallback inside one path, and the acceptance criteria say "entities", not "intents". Concretely: "does it reach 10.20.20.5" or "can rtr-branch reach that instead" work; a bare "what about rtr-branch" with no reach keyword still refuses, exactly as today. Pinned as a real regression test (test_a_bare_follow_up_with_no_reach_keyword_still_refuses), not just prose.What changed
ai/query.py--answer_question()gains one new, optional, backward-compatible parameter:previous: Optional[Dict[str, Any]] = None, shaped{source_device, destination_ip, destination_display}-- exactly the three values the reachability path already produces internally. Where_find_device()/_resolve_destination()would refuse today,previous's corresponding value is tried first, but only when the current text resolves nothing -- explicit text always wins, for free, by construction (no separate override rule to get wrong). A carried-forward device is re-validated against the current snapshot'sdevice_names(), never trusted blindly. Return value gains a fourth key, always present:resolved_entities, populated whenever source and destination both resolved (fresh, carried forward, or a mix), independent of whether the Batfish call itself then succeeded.web/main.py-- new_last_resolved_entitiesdict keyed bycurrent_session_id(), placed beside_uploaded_sessionsand reusing its exact "unbounded, deliberately, and small" justification./api/askreads this session's stored state, passes it asprevious, stores whatever comes back, and pops the fourth key before the response goes out -- the wire contract of/api/askis unchanged, still exactly three keys. Cleared on a new upload (_discard_last_resolved_entities(), called beside the existing policy/business-context discards) so a follow-up can't carry a device across an upload boundary into a different network.Verified, not assumed
Mutation-tested the two safety properties that matter most before committing either:
ai/query.py): temporarily letpreviousoverride text the question actually resolved.test_explicit_text_always_wins_over_the_previous_answerfailed immediately, for the right reason.web/main.py): temporarily readpreviousfrom any session's stored state instead of this one's.test_two_sessions_never_share_a_carried_forward_entityandtest_uploading_a_new_config_clears_the_carried_forward_entityboth failed immediately.Both reverted after.
Live, against real Batfish, not just the fake session in unit tests -- uploaded
rtr-us5-insecure, asked a full question, then a genuine follow-up with no device name at all:A genuinely different, freshly-computed answer -- proof this is a real second query, not a cached first answer. Then opened a second session (its own cookie jar, its own upload) and confirmed the same follow-up-shaped question correctly refuses there rather than reusing the first session's
rtr-us5/10.10.10.5.No em-dash bytes anywhere in the diff.
Acceptance criteria (#318)
bf.q.traceroute(...)call as a fresh one).question_understoodshows the fully resolved question -- no new formatting logic needed, the existing resolved-value formatting already does this.