some cleanup and arch improvements #118
Closed
Conversation
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
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to fee9925 in 3 minutes and 9 seconds. Click for details.
- Reviewed
10967lines of code in49files - Skipped
0files when reviewing. - Skipped posting
15draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. acp/internal/server/server.go:955
- Draft comment:
The updateAgent function is lengthy and handles multiple responsibilities (parsing, validation, namespace checks, resource existence, and error handling). Consider refactoring it into smaller helper functions to improve readability and testability. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. claude.projectstructure.md:1
- Draft comment:
The project structure document is clear and comprehensive. It might help to add a brief note on how the refactoring plan documents integrate with the overall development workflow for new contributors. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to add a note, which is similar to asking them to update the PR description. It doesn't provide a specific code suggestion or ask for a test to be written. It seems to be purely informative and doesn't align with the rules provided.
3. go-bestpractices.md:1
- Draft comment:
Excellent write‐up of Go best practices. The guidelines are clear and comprehensive—no changes required. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. mcp-controller-refactor.md:1
- Draft comment:
The MCP Controller Refactoring Plan is thorough and well-structured, covering dependency injection, interface consistency, error handling, logging, and testing improvements. Ensure that milestone timelines are clearly communicated to the team. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide a specific code suggestion or highlight a potential issue in the code. It is asking the PR author to ensure communication, which is not relevant to the code review process.
5. refactoring-plan.md:1
- Draft comment:
The Task Controller Refactoring Plan is comprehensive and sets out clear phases, steps, and success criteria. It would be beneficial to ensure all global mutable dependencies (e.g. newHumanLayerClientFactory) are removed to enhance testability. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. CLAUDE.md:9
- Draft comment:
Typographical error: "DONT" should likely be corrected to "DON'T". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The document has an intentionally informal, direct tone throughout. The missing apostrophe appears to be part of that style choice. The comment is purely about grammar/spelling and doesn't impact functionality. The document even has other similar informal spellings that weren't flagged. Maybe maintaining consistent professional documentation standards is important, even in informal guides? Some could argue proper spelling helps readability. The informal style is clearly an intentional choice to match Dan Abramov's persona. Enforcing formal grammar here would go against the document's voice and purpose. Delete this comment. It's a purely stylistic suggestion that goes against the intentionally informal tone of the document.
7. CLAUDE.md:40
- Draft comment:
Typo: In the header "## 🔄 THE WORKFLOW THAT ACTUALLY WORKS - DONT DEVIATE", consider using an apostrophe in "DON'T" for correct punctuation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, pointing out a typo in a header. It doesn't provide a code suggestion or ask for a specific change related to functionality or testing. It doesn't align with the rules for useful comments in a pull request review.
8. CLAUDE.md:195
- Draft comment:
There are some typographical errors in this line: "and dont just read 20 lines and add 500 or im gonna cry. Loveyou". Consider changing "dont" to "don't", "im" to "I'm", and adding a space between "Love" and "you" for clarity. - 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% This is a documentation file that was completely rewritten. The typos are real but very minor. The informal style appears intentional as it's written in a casual, direct voice meant to mimic Dan Abramov. The typos don't impact understanding and may even contribute to the casual tone. Fixing them would be a trivial change that doesn't meaningfully improve the document. The typos could theoretically reduce professionalism of the documentation. Some readers might find them distracting. The casual style is clearly intentional throughout the document. These minor typos align with the informal tone and don't impede understanding. Delete this comment as it suggests trivial changes that don't meaningfully improve the document and may work against its intentionally casual style.
9. acp/docs/getting-started.md:10
- Draft comment:
Typo: Consider replacing "macos" with "macOS" for proper capitalization. - 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 rules state not to make comments that are obvious or unimportant. While technically correct, changing "macos" to "macOS" is a very minor stylistic issue that doesn't affect functionality. The rules emphasize focusing on code changes that require action, not purely cosmetic or informative changes. The capitalization of "macOS" is the official Apple style guide recommendation, so there is some merit to maintaining brand consistency. While technically correct, this kind of minor stylistic feedback creates noise and doesn't meaningfully improve the code or documentation. The current spelling is still completely clear and understandable. This comment should be deleted as it's too minor and doesn't require action. It violates the rule about not making obvious or unimportant comments.
10. acp/docs/getting-started.md:464
- Draft comment:
Typographical error: "Agent's aren't that interesting" should be "Agents aren't that interesting" (remove the possessive apostrophe). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment correctly identifies a grammatical error, the rules state we should not make purely informative comments or comments that are obvious/unimportant. A simple apostrophe error is a minor issue that doesn't affect code functionality or understanding. The rules emphasize focusing on code changes that require action, not documentation typos. The grammar error could potentially affect readability or professionalism of the documentation. Documentation quality is important. While documentation quality matters, this is an extremely minor issue that doesn't significantly impact understanding. The rules clearly state to avoid obvious/unimportant comments. The comment should be deleted as it addresses a trivial grammatical issue rather than a substantive code or documentation problem that requires action.
11. acp/docs/getting-started.md:1202
- Draft comment:
Typographical error: "Remove the any secrets:" should be corrected to "Remove any secrets:". - 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 technically correct, this is an extremely minor grammatical issue that doesn't affect understanding or functionality. The current wording, while slightly awkward, is still perfectly clear. PR comments should focus on more substantial issues that actually impact code quality or documentation clarity. The comment is technically accurate - the grammar would be better without "the". But is a single word article in cleanup instructions really worth a PR comment? No, this level of minor grammatical nitpicking would create noise in the PR review process and distract from more important issues. The current wording is clear enough. Delete this comment as it addresses an extremely minor grammatical issue that doesn't meaningfully impact the documentation's clarity or usefulness.
12. acp/internal/controller/agent/agent_controller.go:31
- Draft comment:
Typo: In the comment "AgentReconciler reconciles a Agent object with simple, direct validation", consider changing 'a Agent' to 'an Agent'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the grammar correction is technically correct, the PR rules state not to make purely informative comments that don't require code changes. Grammar in comments, while nice to fix, is not a critical issue that impacts functionality. The comment is still perfectly understandable despite the minor grammar error. The grammar error could theoretically make the code less professional or harder to maintain. Some might argue that clean documentation is important for code quality. While clean documentation is good, this is an extremely minor grammar issue that doesn't impact understanding. The PR rules explicitly say not to make purely informative comments. This comment should be deleted as it's a purely informative comment about grammar that doesn't impact code functionality or maintainability in any meaningful way.
13. acp/internal/controller/toolcall/toolcall_controller.go.backup:1079
- Draft comment:
Typo detected: In the comment "the current namends up like -", it seems 'namends' is a typo. It should be corrected to "the current name ends up like -". - 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 comment correctly identifies a typo, it's just a typo in a comment that doesn't affect code functionality. The comment is also part of a TODO note that is informal in nature. The rules state not to make purely informative comments or comments that are obvious/unimportant. A typo in a TODO comment seems very low priority. The typo could make the comment slightly harder to read at first glance. Since this is new code being added, now would be the ideal time to fix such issues. While it's true this is new code, the typo is minor and in an informal TODO comment. The meaning is still clear from context. This kind of nitpick creates noise in the PR review. The comment should be deleted as it points out a trivial typo in an informal comment that doesn't impact code functionality or readability in any meaningful way.
14. acp/internal/humanlayer/test_client.go:1
- Draft comment:
Typographical error: In the comment on line 1, "its used for one off testing" should be changed to "it's used for one off testing" for correct grammar. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and focuses on a typographical error in a comment, which is not critical to the functionality or logic of the code. It doesn't suggest a code change or improvement, nor does it address any potential issues with the code itself.
15. mcp-controller-refactor.md:330
- Draft comment:
Typo/lexical issue: In the validation rules snippet (Phase 5.2, AFTER section), the variable 'spec' is referenced in validation.Required.When(spec.Transport == "stdio") without being defined. Consider using a defined variable (e.g., mcpServer.Spec) to ensure clarity. - 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% This is a documentation/example code issue, not a real code issue. The code is part of a refactoring plan document showing before/after examples. While technically correct that 'spec' is undefined, this is just example code in a planning document. The document's purpose is to outline high-level refactoring plans, not to provide exact implementation details. The comment is technically accurate - there is a variable reference error. And clear code examples, even in planning docs, can be valuable. However, this is a minor issue in example code within a planning document. The focus should be on the high-level refactoring concepts being demonstrated, not exact variable references. Delete the comment. While technically correct, it's nitpicking example code in a planning document rather than addressing actual implementation issues.
Workflow ID: wflow_4qoUFycqMyVfa0UL
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| otlptracehttp.WithEndpoint(endpoint), | ||
| otlptracehttp.WithInsecure(), | ||
| ) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
Consider logging the original error from otlptracehttp.New before falling back to a no‑op tracer provider. This will aid in diagnosing connectivity issues with the OTEL collector.
README.md
Outdated
| <img width="1918" alt="Screenshot 2025-04-10 at 9 00 58 AM" src="https://github.com/user-attachments/assets/854a2496-0e6c-438f-aee5-6805263fface" /> | ||
|
|
||
| ### Core Objects | ||
| Out of the box support for OpenTelemetry, async humana approval, and more. |
Contributor
There was a problem hiding this comment.
Typographical suggestion: The phrase "async humana approval" appears to contain a typo. Revise it to "async human approval" to improve clarity.
Suggested change
| Out of the box support for OpenTelemetry, async humana approval, and more. | |
| Out of the box support for OpenTelemetry, async human approval, and more. |
|
|
||
|
|
||
|
|
||
| ### Open Telemetry support |
Contributor
There was a problem hiding this comment.
Typo: "Open Telemetry support" should be "OpenTelemetry support" (one word).
Suggested change
| ### Open Telemetry support | |
| ### OpenTelemetry support |
8e89191 to
4115335
Compare
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.
Important
Refactors MCP and Task controllers to improve testability, maintainability, and performance by introducing factory patterns, phase-based reconciliation, and structured error handling.
mcp-controller-refactor.md.mcpserver_controller.go.task_helpers.gofor better testability.HumanLayerClientFactoryandToolAdapter.task_controller.go.This description was created by
for fee9925. You can customize this summary. It will automatically update as commits are pushed.