Separate the two advisory checks, and strengthen what protects them - #69
Merged
Conversation
Findings from the sixth cold review's medium list. --no-coverage-check silenced the array-layering warning too, though the README presents that one as unconditional. They report different hazards — a key you forgot to set somewhere versus .NET keeping base array elements you thought you had replaced — and sharing one switch meant turning off the first quietly turned off the second. Each has its own flag now, --no-coverage-check and --no-array-layering-check, with matching MSBuild properties. The test that asserted the old coupling now asserts the separation in both directions. ForLoop_IteratorVariable_IsLocalToForScope did not test what it is named after: it checked only that two elements came out, which holds whether or not the iterator is scoped, and mutation testing confirmed the invariant was unprotected. It now declares an outer variable of the same name and checks both directions — the body sees the iterator, the outer binding survives the loop. Seven bare Assert.ThrowsAsync calls accepted any exception, including a crash; that is what let the division-by-zero guard be removed with the suite still green. They are typed now. One of them turned out to expect a ParserException rather than an EvaluatorException, which the blanket first pass got wrong and the suite caught immediately — the assertion had been hiding which layer the error came from. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Constats de la liste moyenne de la sixième revue.
Un drapeau, deux dangers
--no-coverage-checkdésactivait aussi l'avertissement de superposition de tableaux, que le README présente pourtant comme inconditionnel.Les deux signalent des dangers différents — une clé oubliée quelque part, contre .NET qui conserve des éléments de tableau de base qu'on croyait remplacés. Chacun a désormais son drapeau, avec les propriétés MSBuild correspondantes :
Le test qui épinglait l'ancien couplage épingle maintenant la séparation dans les deux sens.
Un test qui ne testait pas son propre nom
ForLoop_IteratorVariable_IsLocalToForScopevérifiait seulement que deux éléments sortaient — vrai que l'itérateur soit correctement porté ou non, ce que le test par mutation avait confirmé. Il déclare maintenant une variable externe de même nom et vérifie les deux directions : le corps voit son itérateur, la liaison externe survit à la boucle.Sept assertions non typées
Assert.ThrowsAsync(...)nu accepte n'importe quelle exception, y compris un plantage — c'est ce qui permettait de supprimer la garde de division par zéro avec la suite au vert. Elles sont typées.L'une d'elles attendait en réalité une
ParserException, pas uneEvaluatorException: ma première passe globale s'est trompée et la suite l'a attrapée immédiatement. L'assertion nue masquait de quelle couche venait l'erreur.391 tests, tous verts.
🤖 Generated with Claude Code