Close config knobs that did nothing, two side effects, and the untested resolver - #20
Merged
Conversation
Four reviewers found real work here. One finding was wrong and is discarded: the pack output path IS validated at load time (QuestionPack.cs:156), and a traversal pack is still rejected with exit 2 — the reviewer read the write site and did not trace back to the loader. Config that lied to the user: - defaults.output was parsed and never consulted, so "defaults.output: json" silently stayed text. It is now the fallback when no --format flag is given. - displaySettings.showTimings had no rendering path at all. Removed rather than faked, since nothing shows or hides a timing line. - spec.output.format accepted "json" and then always wrote Markdown. There is no JSON writer, so the validator now rejects it. A file that validated cleanly and is silently the wrong format is worse than a rejected pack. Two side effects: - The databricks child process was killed on the internal timeout but not when the caller cancelled. Disposing a Process releases the handle without stopping the program, so Ctrl+C during a token refresh left the CLI — and any browser it opened for a login — running detached. - RecentConversation.Profile was written on every ask and never read back, so export last and feedback last could resolve a different profile and address the wrong workspace. It now sits between the flag and the config default. Also: ConsoleOutput.Width had no call sites; and a comment asserted a "must not" relationship between AttemptTimeout and RequestTimeout that nothing enforces, which now says so plainly rather than implying safety. LakeSpeak.Application.Tests had zero test files while AgentResolver — whose whole job is refusing to guess between two Agents called Finance — had no coverage anywhere. Twelve tests now pin the resolution order, the alias short-circuit (asserting no network call), exact-over-case-insensitive precedence, and the ambiguous-versus-not-found distinction. 147 tests, up from 135.
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.
Four independent reviewers. One finding was wrong and is discarded: the pack output path is validated at load time (
QuestionPack.cs:156) and a traversal pack is still rejected with exit 2 — the reviewer read the write site and never traced back to the loader. I verified empirically before discarding.Config that lied to the user
defaults.outputwas parsed and never consulted, sodefaults.output: jsonsilently stayed text. Now the fallback when no--formatis given.displaySettings.showTimingshad no rendering path at all — removed rather than faked.spec.output.formatacceptedjsonthen always wrote Markdown. There is no JSON writer, so the validator now rejects it. A file that validated cleanly and is silently the wrong format is worse than a rejected pack.Two side effects
databrickschild process was killed on the internal timeout but not when the caller cancelled. Disposing aProcessreleases the handle without stopping the program, so Ctrl+C during a token refresh left the CLI — and any browser it opened for a login — running detached.RecentConversation.Profilewas written on everyaskand never read back, soexport last/feedback lastcould resolve a different profile and address the wrong workspace. It now sits between the flag and the config default.Dead code and one more false comment
ConsoleOutput.Widthhad zero call sites. A comment asserted a "must not" relationship betweenAttemptTimeoutandRequestTimeoutthat nothing enforces — the third false guarantee found in this codebase, now stating what is actually true.The untested resolver
LakeSpeak.Application.Testshad zero test files, whileAgentResolver— whose entire job is refusing to guess between two Agents called Finance — had no coverage anywhere. Twelve tests pin resolution order, the alias short-circuit (asserting no network call is made), exact-over-case-insensitive precedence, and ambiguous-vs-not-found.147 tests, up from 135. All five suites now report.