Conversation
There was a problem hiding this comment.
Pull Request Overview
Provides the initial implementation of the Kairix cognition engine, including core modules, configuration, and tests
- Introduces core data types, perception/proposal/execution components, and an interactive CLI
- Adds schedulers (inline and hesitating) and a persona orchestrating the cognition cycle
- Configures project metadata (pyproject.toml, Justfile) and comprehensive tests for all modules
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| kairix-engine/Justfile | Updated lint-fix commands to use unsafe fixes on root |
| cognition_engine/justfile | Added recipes for install, lint, test, and dev |
| cognition_engine/pyproject.toml | Defined project metadata, dependencies, and tooling |
| cognition_engine/src/types.py | Defined Stimulus, Perception, Action, and bus types |
| cognition_engine/src/scheduler.py | Implemented Inline and Hesitating execution schedulers |
| cognition_engine/src/proposer.py | Added Proposer for generating actions from perceptions |
| cognition_engine/src/perceptor.py | Added Perceptor for translating stimuli into perceptions |
| cognition_engine/src/executor.py | Implemented Executor for handling 'say' and 'do' actions |
| cognition_engine/src/persona.py | Composed Persona coordinating perception, proposal, and scheduling |
| cognition_engine/main.py | Built interactive CLI loop integrating all components |
| cognition_engine/tests/ | Added unit tests covering core modules |
| architecture/taxonomy.md | Added high-level architecture skeleton |
Comments suppressed due to low confidence (2)
cognition_engine/src/scheduler.py:34
- The exception path in
InlineExecutionScheduler.do_nowis not covered by existing tests. Add a test that simulates an executor throwing an exception to verifysuccess=Falseand error handling.
except Exception as e:
architecture/taxonomy.md:105
- There's an extraneous closing brace
}after the code block. Removing it will clean up the markdown syntax.
</details>
cognition_engine/src/scheduler.py
Outdated
| attempt = self.do_now(action) | ||
|
|
||
| # Emit result as stimulus | ||
| if isinstance(attempt, ActionResult): |
There was a problem hiding this comment.
The inline scheduler emits a stimulus for every ActionResult, even failures. Consider checking attempt.success before emitting to avoid treating failed executions as successful attempts.
| if isinstance(attempt, ActionResult): | |
| if isinstance(attempt, ActionResult) and attempt.success: |
There was a problem hiding this comment.
No this is correct keep it.
| fix: | ||
| uv run ruff check --fix src/ tests/ | ||
| uv run ruff format src/ tests/ | ||
| uv run ruff check --fix --unsafe-fixes . |
There was a problem hiding this comment.
[nitpick] Using --unsafe-fixes applies potentially breaking changes across the entire repository. Consider restricting the scope to source and test directories to avoid unintended modifications.
| uv run ruff check --fix --unsafe-fixes . | |
| uv run ruff check --fix --unsafe-fixes src tests |
marklubin
left a comment
There was a problem hiding this comment.
Address feedback and resubmit.
cognition_engine/justfile
Outdated
| # Run the main application | ||
| run: | ||
| uv run python main.py |
There was a problem hiding this comment.
no main application in this package, it is a library
cognition_engine/justfile
Outdated
|
|
||
| # Run linting with ruff | ||
| lint: | ||
| uv run ruff check --fix src/ tests/ main.py |
There was a problem hiding this comment.
add target for unsafe fixes
| [build-system] | ||
| requires = ["hatchling"] | ||
| build-backend = "hatchling.build" | ||
|
|
There was a problem hiding this comment.
Should use uv build backend
| [tool.hatch.build.targets.wheel] | ||
| packages = ["src"] | ||
|
|
||
| [tool.ruff] |
There was a problem hiding this comment.
You didn't add the dev dependencies on ruff and pyright. But also use Astral's new TY type checker instead of pyright.
cognition_engine/src/__init__.py
Outdated
| "HesitatingExecutionScheduler", | ||
| "Hesitator", |
There was a problem hiding this comment.
These we don't need to export they are just an example.
cognition_engine/src/scheduler.py
Outdated
| attempt = self.do_now(action) | ||
|
|
||
| # Emit result as stimulus | ||
| if isinstance(attempt, ActionResult): |
There was a problem hiding this comment.
No this is correct keep it.
| class StimulusBus: | ||
| def __init__(self): | ||
| self.listeners = [] | ||
|
|
||
| def emit(self, stimulus: Stimulus): | ||
| console.print(Panel(f"[bold cyan]Emitting: {stimulus.type.value}[/bold cyan]")) | ||
| for listener in self.listeners: | ||
| listener(stimulus) | ||
|
|
||
| def subscribe(self, listener): | ||
| self.listeners.append(listener) No newline at end of file |
There was a problem hiding this comment.
This should be in it's own dedicated file
| class Source: | ||
| def get(self, key: str) -> Any: | ||
| return None | ||
|
|
||
|
|
||
| class Sink: | ||
| def put(self, key: str, value: Any) -> None: | ||
| pass | ||
|
|
||
|
|
There was a problem hiding this comment.
We don't need these as part of external interface.
| @@ -0,0 +1,109 @@ | |||
| from dataclasses import dataclass | |||
There was a problem hiding this comment.
Update these types to use pydantic.
| @@ -0,0 +1,24 @@ | |||
| [project] | |||
There was a problem hiding this comment.
Create CLAUDE.md describing the project incorporate the key points from the original prompt directive and how use run and update the project. Have it reference a CONTRIBUTION.md which spells out procedure to making changes. It should highlight:
- All changes include 90%+ branch and line level code coverage. (Add code converge check to the a new comprehensive
just buildtarget. - Must execute
just buildand run entire build, check and test suite before submitting changes for review. - All new work should be done on feature branch.
- All new work should include updates to documentation. Including doc strings for all classes, public methods and functions and names which will be exported out of the project. Add an enforcement mechanism for this.
- uv and venvs should be used for all work. Never run any commands related to this project on system level python intreprator.
- All implementation code should use the rich library for logging.
- All implementation should explicitly handle ambiguous input and throw an error rather than trying to infer intention of caller.
- All code requires complete python type annotations.
- No dependencies should be added to the project without prior sign off from project owners. Create a tracking document listing the original rationale for adding each dependency and how it was signed off on. You can include openai, rich, pydantic, ruff and ty as the original default dependencies.
PR Updated - All Review Feedback AddressedI've updated this PR with comprehensive changes addressing all review feedback: Major Refactoring:
Specific Feedback Addressed:
Testing:
The implementation now provides a clean separation between interfaces and example implementations, making it suitable as a library. |
No description provided.