[solver] add unseccessful perfomance option in DirectInferenceAgent#82
Open
piligrinik wants to merge 1 commit intoostis-ai:mainfrom
Open
[solver] add unseccessful perfomance option in DirectInferenceAgent#82piligrinik wants to merge 1 commit intoostis-ai:mainfrom
piligrinik wants to merge 1 commit intoostis-ai:mainfrom
Conversation
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 9557d62 in 1 minute and 32 seconds
More details
- Looked at
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. problem-solver/cxx/inferenceModule/agent/DirectInferenceAgent.cpp:67
- Draft comment:
Fix typo in comment: change 'successSoclutionEdge' to 'successSolutionEdge'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the typo exists and the comment is technically correct, fixing typos in comments is a very minor issue that doesn't affect functionality. Comments are for documentation purposes only. The rules state not to make comments that are obvious or unimportant, and this seems to fall into that category.
The typo could potentially confuse future developers reading the code. Documentation quality is important for maintainability.
While documentation quality matters, this particular typo is minor enough that most readers would easily understand the intended meaning. The cost of the PR review interaction outweighs the benefit.
Delete this comment as it points out an unimportant typo that doesn't impact code functionality or significantly impair understanding.
2. problem-solver/cxx/inferenceModule/agent/DirectInferenceAgent.cpp:75
- Draft comment:
Review iterator logic: if multiple success solution edges exist with mixed types, returning on the first negative edge may be premature. Ensure this is the desired behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that the behavior is intended, which violates the rule against asking for confirmation of intention. It does not provide a specific suggestion or point out a clear issue with the code.
3. problem-solver/cxx/inferenceModule/agent/DirectInferenceAgent.cpp:67
- Draft comment:
Typo in comment: change 'successSoclutionEdge' to 'successSolutionEdge'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the typo exists and the comment is technically correct, comments about typos in comments are very minor issues. They don't affect functionality and can be fixed during code review without needing a formal comment. This feels like unnecessary noise in the review process.
The typo could potentially cause confusion for future developers reading the code. Documentation quality is important.
While documentation is important, this is an extremely minor issue that can be fixed inline during review. The typo is obvious enough that it won't cause real confusion.
Delete this comment as it's too minor of an issue to warrant a formal PR comment. This falls under the rule about not making obvious or unimportant comments.
4. problem-solver/cxx/inferenceModule/agent/DirectInferenceAgent.cpp:75
- Draft comment:
Logic clarification: the code only checks for a negative edge to determine failure. If both negative and positive edges exist, the agent immediately fails. Confirm that this is the intended behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment violates the rule about not asking authors to confirm their intention ("Confirm that this is the intended behavior"). Additionally, it's speculative - it points out what could happen in a specific edge case without evidence that this is actually a problem. The code seems to be implementing a straightforward failure-on-negative-edge policy.
The logic could potentially be buggy if positive edges should take precedence over negative ones. Without understanding the broader system requirements, I can't be 100% sure this isn't a real issue.
While there might be an edge case here, the comment is phrased as a request for confirmation rather than pointing out a clear bug or suggesting a specific fix. The implementation seems intentional.
Delete this comment as it violates rules against asking for confirmation and making speculative comments without clear evidence of an actual problem.
5. problem-solver/cxx/inferenceModule/agent/DirectInferenceAgent.cpp:67
- Draft comment:
Typo in the comment: "searching for successSoclutionEdge Type" should be corrected to "searching for successSolutionEdge Type". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the typo exists and the correction is accurate, comments about typos in comments are generally not important enough to warrant a PR comment. The typo doesn't affect code functionality, and it's a minor issue that could be fixed during code review without needing a formal comment. The rules specifically mention not making comments that are obvious or unimportant.
The typo could potentially cause confusion for future developers reading the code. Documentation quality is important for maintainability.
While documentation quality matters, this particular typo is minor enough that readers would still understand the intent, and fixing comment typos doesn't warrant the overhead of a PR comment.
Delete this comment as it points out a trivial issue that doesn't affect code functionality and falls under the category of "obvious or unimportant" comments that should be avoided.
Workflow ID: wflow_YCqzMRIFLvLtqPY8
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| action.FormResult(solutionNode); | ||
|
|
||
| ScIterator3Ptr successSolutionEdgeIterator = | ||
| m_context.CreateIterator3(InferenceKeynodes::concept_success_solution, ScType::Unknown, solutionNode); //searching for successSoclutionEdge Type |
There was a problem hiding this comment.
Consider specifying the expected edge type in CreateIterator3 instead of using ScType::Unknown for clarity and to avoid ambiguity.
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
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.
Important
Adds unsuccessful performance handling in
DirectInferenceAgentby checking for negative edge types inDoProgram.DoPrograminDirectInferenceAgent.cppfor unsuccessful target achievement usingScIterator3Ptr.FinishUnsuccessfully()ifEdgeAccessConstNegPermis found.DoProgram.This description was created by
for 9557d62. It will automatically update as commits are pushed.