Post-ship review: two billing/integrity defects, live test suite, export and feedback commands - #9
Merged
Merged
Conversation
…lt complete Two defects that every test passed over, both found by an adversarial panel and both confirmed by reverting the fix and watching the new tests go red. **Non-idempotent requests were being retried.** The standard resilience handler retries POST by default. start-conversation and create-message are not idempotent, so a transient 5xx asked Genie the same question a second time — running the SQL warehouse twice, billing twice, and orphaning a conversation whose id the caller never receives. The code comment beside it asserted the opposite, claiming the default classification already excluded POSTs. It did not. Guard added via DisableForUnsafeHttpMethods; the false comment replaced with what the code does. The regression test takes 7s without the guard and 0.7s with it, because the retries are the difference. **A chunked result was reported as complete.** The client reads the first chunk only, and derived truncation from manifest.truncated — which reports statement- level truncation by Databricks and is false for a result that is merely split. A large export therefore arrived labelled complete, which is the one failure this client must never have, and it directly contradicted the processing-integrity claim in the SOC 2 mapping. Completeness is now conservative: truncated if Databricks truncated it, or a next_chunk_index is present, or fewer rows arrived than the manifest advertises. next_chunk_index had to be added to the wire model; it was never deserialised. Both documents that made the false claims are corrected rather than quietly adjusted, and limitations.md now states what a large result actually does.
…straint they found The live test project existed but was empty — spec item 20 was a directory, not a test. It now holds eight opt-in tests that reproduce the manual verification: listing, asking, follow-up conversations staying in one conversation, unknown-agent handling, feedback, cancellation, and an assertion that live result cells contain no scientific notation and no thousands separators. Excluded from CI by trait rather than omitted from the solution, so they compile and cannot rot. Running them found an undocumented API constraint. Databricks rejects a feedback request carrying both a comment and a NONE rating, with an HTTP 400 that reads like a transport fault. It appears in no SDK and no documentation page. The client now refuses the combination before the request, as an argument error so it exits 2 rather than 1 — a script should see "you passed the wrong flags", not "it crashed". Spec sections 6.7 and 6.8 wanted `export last` and `feedback last` as commands, not only as chat slash commands. Both now exist, backed by a pointer file holding identifiers and nothing else: no question, no answer, no row. The result is re-fetched from Databricks on export rather than cached locally, because a local copy of governed data would have none of the governance, and Databricks is already the system of record. Export warns loudly when the result is incomplete. Verified live: ask, export last, feedback last, the client-side guard exiting 2, and the full eight-test live suite green against a real workspace.
This was referenced Aug 1, 2026
ivanvyd
added a commit
that referenced
this pull request
Aug 1, 2026
…defects (#11) A five-lens panel reviewed the shipped code. Its stale findings (already fixed in #9) are excluded; these are the ones that reproduced. CRITICAL - the scrubber leaked the credential it existed to hide. The named-secret pattern captured only the first whitespace-delimited token after a key, so "Authorization: Bearer <token>" redacted the word "Bearer" and passed the token through in full. Reachable from two live paths: Databricks CLI stderr scrubbing and every GenieException message. Proven by twelve evasion tests, seven of which failed before the fix. The earlier tests all used single-token values with no embedded space, which is exactly the shape the pattern handled - they confirmed it worked on convenient input rather than trying to defeat it. HIGH - a symlink or Windows junction beside a pack file defeated the output-path guard. The check was purely lexical, so "link/report.md" passed every string comparison while the write followed the reparse point outside the pack directory: traversal with no ".." at all. Each component between root and target is now resolved. Reproduced with a real junction before and after. HIGH - YamlDotNet assigns its own dictionary over the field initializer, discarding the OrdinalIgnoreCase comparer, so an alias written "Finance:" did not match "--agent finance". Resolution then fell through to title matching, which can select a different Agent entirely - the answer-against-the-wrong-data failure the resolver exists to prevent. The comparer is rebuilt after load. HIGH - timestamps were parsed as milliseconds unconditionally, though the project's own API notes say the unit is undocumented and the one published example is seconds. On a seconds value that yields 1970, a silent 55-year error on a public property. Now detected by magnitude. MEDIUM - EXTERNAL_LINKS results omit data_array entirely, which would have returned zero rows as a complete success. Refused loudly instead; an empty-looking success is worse than a failure. Also: the CSV formula guard missed a leading tab or carriage return, both of which OWASP documents as accepted prefixes. Two false claims corrected. --verbose was registered but never read while its help text promised "credentials are always redacted"; it now emits diagnostics through a stderr logger that scrubs every record, verified live against a real workspace with a grep for the token. compatibility.md asserted a live verification in one section and "no live workspace has been contacted" seventeen lines later; SECURITY.md referenced a --debug flag that never existed. Also dropped a dead ProjectReference from QuestionPacks to Application, which used nothing from it. 127 tests, up from 109.
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.
An adversarial review panel plus a real live test suite found four defects that 89 passing tests had missed. Each fix is proven by reverting it and watching the new test go red.
Defects
Non-idempotent requests were retried — double billing. The standard resilience handler retries POST by default.
start-conversationandcreate-messageare not idempotent, so a transient 5xx asked Genie the same question twice: two warehouse runs, two bills, and an orphaned conversation the caller never sees. The comment beside the config asserted the opposite. Guarded withDisableForUnsafeHttpMethods(); the regression test takes 7s without the guard and 0.7s with it, because the retries are the difference.A chunked result was reported as complete. The client reads the first chunk only and derived truncation from
manifest.truncated, which reports statement-level truncation and isfalsefor a merely-split result. Large exports arrived labelled complete — the one failure this client must never have, and a direct contradiction of the processing-integrity claim in the SOC 2 mapping. Completeness is now conservative, andnext_chunk_indexwas added to the wire model; it had never been deserialised.The live test project was empty. Spec item 20 was a directory. It now holds eight opt-in tests reproducing the manual verification.
An undocumented API constraint. Databricks rejects a feedback request carrying both a comment and a
NONErating, with an HTTP 400 that reads like a transport fault. In no SDK, on no docs page — found by running the live suite. Now refused client-side as an argument error, exiting 2 rather than 1.Also
export lastandfeedback lastfrom spec §6.7/§6.8, backed by a pointer file holding identifiers and nothing else — no question, no answer, no row. The result is re-fetched from Databricks rather than cached, because a local copy of governed data would have none of the governance.Corrected claims
docs/compliance/soc2-mapping.mdanddocs/limitations.mdboth made claims that were false until this branch. Both are corrected explicitly rather than quietly adjusted.Verification
93 tests green. Eight live tests green against a real Azure Databricks workspace, covering feedback and cancellation for the first time.
export last,feedback lastand the client-side guard all verified live.