Milestone 8: skills and MCP integration - #9
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMilestone 8 adds immutable skill packages, session-pinned catalogs, MCP adapters and runtime handling, dynamic tools, persistence, deterministic evaluations, and activated validation gates. ChangesSkills and MCP integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (13)
tests/gates/test_tool_m8.py (1)
222-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrite the
token_endpointkey as a plain literal. Both sites split the field name into"token_" + "endpoint". The concatenation produces the same string at runtime, so it changes nothing except readability, and it defeats any grep or secret scanner that the split was meant to avoid. If a lint rule flags the literal, suppress that rule explicitly at the line instead of hiding the string from it.
tests/gates/test_tool_m8.py#L222-L222: replace"token_" + "endpoint"with"token_endpoint"in theoauth_refreshconfig.tests/gates/test_tool_m8.py#L384-L384: replace"token_" + "endpoint"with"token_endpoint"in theoauthconfig.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gates/test_tool_m8.py` at line 222, Replace the split key expression with the plain literal "token_endpoint" in both the oauth_refresh config at tests/gates/test_tool_m8.py lines 222-222 and the oauth config at lines 384-384; if lint rejects the literal, suppress that rule explicitly on each affected line.src/agent_core/skills/package.py (1)
195-212: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMove the
MAX_FILEScheck so directory entries do not trigger it.
os.walkyields directory names innames. The loop appends a member only for a symlink or a regular file, but it evaluates theMAX_FILESguard for every candidate, including plain directories. A valid package that holds 64 files plus one subdirectory is therefore rejected withpackage.file_count, even though only 64 members are collected.♻️ Proposed fix
for name in [*names, *files]: candidate = directory_path / name relative = candidate.relative_to(root).as_posix() - if len(members) >= MAX_FILES: - raise _refuse("package.file_count", f"package must contain 1 to {MAX_FILES} files") if candidate.is_symlink(): + if len(members) >= MAX_FILES: + raise _refuse( + "package.file_count", f"package must contain 1 to {MAX_FILES} files" + ) members.append(SkillPackageMember(path=relative, kind="symlink")) elif candidate.is_file(): + if len(members) >= MAX_FILES: + raise _refuse( + "package.file_count", f"package must contain 1 to {MAX_FILES} files" + ) remaining = MAX_PACKAGE_BYTES - total_bytes🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/skills/package.py` around lines 195 - 212, Move the MAX_FILES guard in the os.walk packaging loop so it runs only immediately before appending a symlink or regular-file SkillPackageMember, not for plain directory entries. Preserve the existing file-count error and directory traversal behavior.src/agent_core/adapters/persistence/sqlalchemy_models.py (1)
511-537: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider an index that supports latest-active-revision lookups.
PostgresSkillRepository.resolveandlist_activefilterskill_revisionsbyskill_idandstatus, then order byrevision DESC. The only matching index is the unique(skill_id, revision)constraint, so PostgreSQL filters status after the index scan. An index on(skill_id, status, revision DESC)serves both queries directly.__table_args__ = ( UniqueConstraint("skill_id", "revision", name="uq_skill_revisions_skill_revision"), Index( "ix_skill_revisions_skill_status_revision", "skill_id", "status", text("revision DESC"), ), )Add the same index to
migrations/versions/9a71c4e8d2f0_add_milestone_8_skills_and_mcp.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/persistence/sqlalchemy_models.py` around lines 511 - 537, Update SkillRevisionRow.__table_args__ to retain the existing uniqueness constraint and add the ix_skill_revisions_skill_status_revision index on skill_id, status, and descending revision. Add the corresponding index creation to migration 9a71c4e8d2f0_add_milestone_8_skills_and_mcp.py so the database schema matches the model.src/agent_core/adapters/skills/stores.py (1)
78-96: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider flushing the staged archive before linking it.
NamedTemporaryFilecloses the file at the end of thewithblock, so buffered bytes reach the kernel. The code does not callos.fsyncon the file or on the parent directory. After a host crash, the store can hold a present-but-truncated archive at a content-addressed key.putthen treats the key as immutable and rejects the correct bytes withimmutable skill archive key already contains different bytes.♻️ Proposed durability fix
) as staged: staged.write(archive) + staged.flush() + os.fsync(staged.fileno()) temporary = Path(staged.name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/skills/stores.py` around lines 78 - 96, Update the staged archive flow in the put implementation around NamedTemporaryFile and os.link to fsync the written temporary file before linking it, then fsync target.parent after the link succeeds. Preserve the existing immutable-key comparison and cleanup behavior while ensuring a reported successful put survives host crashes without a truncated archive.src/agent_core/domain/runs.py (1)
115-119: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider validating that pinned tool collections agree.
pinned_tool_names,pinned_tool_versions, andpinned_tool_specscarry the same tool identities in three places. No validator enforces agreement, so a partially populated checkpoint restores an inconsistent pin set.ContextPlan.tools_match_namesalready applies this pattern for plans.♻️ Proposed consistency validator
`@model_validator`(mode="after") def pinned_tools_match(self) -> RunCheckpoint: names = set(self.pinned_tool_names) if not set(self.pinned_tool_specs) <= names or not set(self.pinned_tool_versions) <= names: raise ValueError("checkpoint pinned tool metadata does not match its pinned tool names") return self🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/domain/runs.py` around lines 115 - 119, Add an after-model validator to the RunCheckpoint model, alongside the pinned_tool_names, pinned_tool_versions, and pinned_tool_specs fields, that verifies both metadata mappings contain only names present in pinned_tool_names and raises a ValueError when they do not. Preserve valid checkpoints and follow the existing ContextPlan.tools_match_names validation pattern.tests/gates/test_skill_m8.py (2)
272-281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the tool-registration gate recurse into subpackages.
glob("*.py")matches only files directly insidesrc/agent_core/skills. A future subpackage such asskills/loaders/registry.pywould register tools without failing this gate. Userglobso the gate covers the whole package.♻️ Proposed change
- for path in (ROOT / "src" / "agent_core" / "skills").glob("*.py"): + for path in (ROOT / "src" / "agent_core" / "skills").rglob("*.py"):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gates/test_skill_m8.py` around lines 272 - 281, Update test_no_tool_from_skill to use recursive file discovery with rglob instead of glob, ensuring Python files in all skills subpackages are checked for register and register_dynamic calls while preserving the existing AST validation.
538-539: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the token budget from configuration instead of the literal
6_000.The assertion hard-codes the skill-body token budget. If the configured budget changes, this gate keeps passing against a stale number and stops proving the cap. Read the value from the same plan budget or configuration constant that
SkillCatalogServiceenforces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gates/test_skill_m8.py` around lines 538 - 539, Update the token-sum assertion in the test around SkillCatalogService to derive its limit from the same plan budget or configuration constant enforced by SkillCatalogService, replacing the hard-coded 6_000 while preserving the existing loaded-count assertion.src/agent_core/adapters/persistence/unit_of_work.py (1)
215-219: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog rollback-callback failures instead of discarding them.
suppress(Exception)hides every compensation error. The PostgreSQL factory registers skill-archive cleanup through this registrar, so a failed callback leaves an orphaned archive object in the package store with no diagnostic record. Keep the suppression so one failure does not block the remaining callbacks, but emit a log entry.♻️ Proposed change
async def _run_rollback_callbacks(self) -> None: callbacks, self._rollback_callbacks = self._rollback_callbacks, [] for callback in reversed(callbacks): - with suppress(Exception): - await callback() + try: + await callback() + except Exception: + logger.exception( + "transaction_rollback_callback_failed", + extra={"tenant_id": self._tenant_id}, + )This requires a module logger if one is not already defined in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/persistence/unit_of_work.py` around lines 215 - 219, Update _run_rollback_callbacks to retain suppression so all callbacks continue executing, but catch each callback exception and log it through a module-level logger with rollback-callback context and exception details. Add the module logger if this file does not already define one.tests/contract/test_skill_catalog_contract.py (1)
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise a non-empty catalog so the stability contract is actually asserted.
Both
opencalls return an empty catalog, sofirst == secondholds for any implementation, including one with no pinning at all. Install at least one package before the firstopen, then install or archive a package between the two calls. The assertion then proves that the secondopenreturns the pinned catalog rather than the current repository state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/contract/test_skill_catalog_contract.py` around lines 27 - 35, Update the test around SkillCatalogService.open to seed at least one package before the first open, then install or archive another package between the two open calls. Keep the equality assertion and ensure the first catalog is non-empty so the second result verifies the pinned catalog rather than an empty repository state.src/agent_core/application/session_service.py (1)
45-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the activation obligation for
create_in.
createcallsactivateafter the transaction commits.create_indoes not activate, so any caller that composescreate_ininto its own submission transaction must callactivateafter that transaction commits. State this in the docstring so the MCP session-activation step is not skipped.The catalog is also opened here before
uow.sessions.create(session)on line 69. See the consolidated comment about opening a catalog for an uncommitted session.♻️ Proposed change
async def create_in(self, uow: RepositoryUnitOfWork) -> UUID: - """Create a session inside a caller-owned submission transaction.""" + """Create a session inside a caller-owned submission transaction. + + The caller must call `activate` after that transaction commits. + """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/application/session_service.py` around lines 45 - 58, Update the create_in method docstring to state that callers composing it into their own submission transaction must call activate after the transaction commits, preserving the existing create behavior and explicitly covering the MCP session-activation step. Do not change the catalog-opening logic here; address only the documentation requested for create_in.src/agent_core/domain/errors.py (1)
143-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPopulate the
ConflictErrorreason and details forSkillRevisionConflict.
ConflictErrorcarries a structuredreasonanddetailspayload.SkillRevisionConflictpasses only a message, so consumers that readreasonreceiveNoneand lose the revision value that is already available.♻️ Proposed change
def __init__(self, current_revision: int) -> None: - super().__init__("skill revision changed during installation") + super().__init__( + "skill revision changed during installation", + reason="skill_revision_conflict", + details={"current_revision": current_revision}, + ) self.current_revision = current_revision🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/domain/errors.py` around lines 143 - 148, Update SkillRevisionConflict.__init__ to pass a structured ConflictError reason and details payload when calling the superclass, using current_revision in the details so consumers retain the winning revision; preserve the current_revision attribute and conflict message behavior.src/agent_core/bootstrap.py (1)
636-636: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompare the transport enum, not its string value.
Line 636 tests
config.transport.value == "http". Every other site compares the enum member, for exampleconfig.transport is MCPTransport.HTTPinsrc/agent_core/adapters/mcp/sdk.pyline 375 andsrc/agent_core/mcp/configuration.pyline 25. A string comparison silently stops matching if the enum value changes, and the proxy is then not started for HTTP servers.♻️ Proposed refactor
- if any(config.transport.value == "http" for config in effective_mcp_configs): + if any( + config.transport is MCPTransport.HTTP for config in effective_mcp_configs + ):Add
MCPTransportto the import on line 160:-from agent_core.domain.mcp import MCPServerConfig, ScriptedMCPServer +from agent_core.domain.mcp import MCPServerConfig, MCPTransport, ScriptedMCPServer🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/bootstrap.py` at line 636, Update the transport check in the effective MCP configuration logic to compare config.transport directly with the MCPTransport.HTTP enum member instead of comparing its value to a string. Add MCPTransport to the existing imports and preserve the surrounding any(...) condition.src/agent_core/mcp/mapping.py (1)
79-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer index keying over
id()keying for the schema inspections.
schema_inspectionsmapsid(tool)to the inspection result. This works only becauseremote_toolskeeps every model alive for the whole function. If a later change accepts an iterable, or rebuilds the models,id()values can be reused and the lookup can return another tool's result. Key by position instead.♻️ Proposed refactor
- schema_inspections = { - id(tool): _inspect_schema(tool.input_schema, schema_maximum_depth) for tool in remote_tools - } + schema_inspections = { + index: _inspect_schema(tool.input_schema, schema_maximum_depth) + for index, tool in enumerate(remote_tools) + } + inspection_by_tool = { + index: schema_inspections[index] for index in range(len(remote_tools)) + }Then iterate with
enumeratein both thedeclarationscomprehension and the acceptance loop so each tool uses its own index.Also applies to: 107-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/mcp/mapping.py` around lines 79 - 81, Replace id(tool)-based keys in schema_inspections with positional indices, and use enumerate consistently in the declarations comprehension and acceptance loop so each remote tool retrieves its inspection by index.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@evals/gates/tool.yaml`:
- Around line 82-87: Align gate.tool.mcp_reauth_bounded with its actual coverage
by either extending test_mcp_reauth_bounded to submit and verify a run continues
through the re-authentication scenarios, or removing the untested “run that
continues throughout” requirement from the gate statement. Keep the existing
ladder invariants unchanged.
In `@src/agent_core/adapters/mcp/memory.py`:
- Around line 28-49: Ensure deterministic catalog-key conflict handling across
src/agent_core/adapters/mcp/memory.py:28-49 and
src/agent_core/adapters/mcp/persistence.py:93-123. In record_catalog, detect
duplicate keys within the input batch and reject conflicting records before
writing; preserve idempotency for equivalent duplicates. In the persistence
adapter, compare existing rows against the immutable payload and reject
mismatches instead of silently retaining the stored record. Add
repository-contract cases covering equivalent retries and both intra-batch and
existing-record conflicts.
In `@src/agent_core/adapters/mcp/sdk.py`:
- Around line 143-192: Require HTTPS for both credential-bearing MCP endpoints:
validate config.endpoint before authenticated MCP requests and
config.token_endpoint before the client-credentials request in
_exchange_client_token, rejecting non-HTTPS values before sending credentials.
Apply the corresponding endpoint validation in src/agent_core/bootstrap.py lines
494-502 as well.
In `@src/agent_core/adapters/models/fake.py`:
- Around line 169-178: Update _next_turn so the repeat_last fallback selects the
latest scripted turn whose context_contains is absent or present in
rendered_request, rather than unconditionally returning self._script.turns[-1].
Preserve the existing sequential scan behavior, and raise
ModelScriptExhaustedError when no matching or unconditional turn exists.
In `@src/agent_core/adapters/persistence/skills.py`:
- Around line 148-165: Update list_active to select only the newest active
revision per skill in SQL using DISTINCT ON with ORDER BY skill identity and
revision descending, then apply limit directly to the statement before
execution. Remove the Python latest dictionary and slicing while preserving the
existing domain conversion and tenant/status filters.
In `@src/agent_core/adapters/persistence/sqlalchemy_models.py`:
- Around line 533-535: The agent-authored revision provenance fields must remain
consistent when an authoring run is deleted. In
src/agent_core/adapters/persistence/sqlalchemy_models.py:533-535, update
SkillRevisionRow.authored_by_run_id to use ondelete="RESTRICT"; keep
src/agent_core/adapters/persistence/skills.py:205-207 and _to_domain strict with
no relaxation to provenance_matches_source. Add a regression test that deletes
an authoring run and then reads the skill revision, verifying the provenance
remains valid or deletion is rejected.
In `@src/agent_core/application/session_service.py`:
- Around line 45-58: The session creation flows in
src/agent_core/application/session_service.py lines 45-58 and
src/agent_core/application/public_services.py lines 235-240 must not publish
catalog/MCP state before the database transaction commits. Update create_in and
its sibling creation path to register catalog opening/publication with the
transaction lifecycle, committing on success and cleaning up MCP connections,
deferred events, preparation state, and catalog cache on rollback; preserve the
caller-owned transaction behavior.
In `@src/agent_core/context/builder.py`:
- Around line 245-248: Update _assemble so loaded-skill count and token cap
violations set a skill_bodies_over_cap pressure flag instead of raising
ContextOverflow, allowing measure() and assemble() to return ContextPressure
with fits=False for compaction. Include skill_bodies_over_cap in the fits and
reason calculation alongside working_state_over_cap, and replace the literal
two-skill limit with a plan budget field or named constant.
In `@src/agent_core/domain/mcp.py`:
- Around line 76-77: Update the OAuth validation around
MCPAuthScheme.OAUTH2_CLIENT and token_endpoint to require an absolute HTTPS URL,
rejecting missing, relative, or non-HTTPS endpoints before credentials can be
sent. Configure SDKMCPClient._exchange_client_token() to use
follow_redirects=False, and add coverage verifying redirects are not followed
and do not receive client credentials.
- Around line 57-58: Update the endpoint validation in the MCP configuration
path around the existing self.endpoint check to reject HTTP routes when the
authentication type is BEARER, HEADER, or OAUTH2_CLIENT, and require HTTPS for
token_endpoint values before storing configuration or constructing the client.
Also disable redirects or revalidate each redirect destination before forwarding
credentials, while preserving valid HTTPS behavior.
In `@src/agent_core/mcp/runtime.py`:
- Around line 604-615: Update close_session to isolate each connection’s
__aexit__ failure using the same handling pattern already used by prepare,
allowing all connections in that session to be attempted. Update close to
similarly isolate each close_session failure so teardown continues for every
remaining session.
- Line 124: Update ToolRegistry session lifecycle tracking to record which
dynamic MCP keys each session owns; in close_session, after closing clients,
remove keys whose final owning session has ended from both _registered and
StaticToolRegistry._dynamic_tools, while preserving entries still used by other
sessions.
In `@src/agent_core/runtime/executor.py`:
- Around line 355-361: Update _execute_running so resumed runs preserve the
restored pinned tool names, versions, and specs instead of overwriting them from
a newly planned epoch. Compare the restored pins with context_plan and reject
epoch mismatches, or continue using the original plan and pins; add a regression
test covering a pending tool call followed by context rotation.
In `@src/agent_core/skills/catalog.py`:
- Around line 126-146: Update the refs construction before the
uow.skills.resolve loop to deduplicate references by skill name while preserving
their first-seen order, then truncate the unique references to maximum_entries
before resolving. Record the names of references removed by deduplication or
truncation in dropped_names so callers retain existing dropped-skill visibility,
and leave the NotFoundError handling and resolution flow intact.
In `@src/agent_core/skills/package.py`:
- Around line 109-111: Before decompressing package data, use
zstandard.frame_content_size() to read and reject declared frame sizes above
MAX_PACKAGE_BYTES * 4, including handling unknown-size frames appropriately,
before any allocation or decompression. Update the package decompression logic
while preserving existing size validation, and pin zstandard to the exact
version required to keep content_sha256 stable across environments, updating the
lockfile accordingly.
In `@tests/gates/test_harness_m8.py`:
- Around line 43-59: Extend the I/O seal setup around the existing socket and
subprocess monkeypatches to block AnyIO’s subprocess path used by
mcp.client.stdio, preferably by patching anyio.open_process or the event loop’s
subprocess_exec and subprocess_shell methods with the existing deterministic-I/O
failure handler. Also monkeypatch socket.getaddrinfo to reject DNS resolution,
while preserving the current blocked socket, Popen, and open_connection
protections.
In `@tests/gates/test_tool_m8.py`:
- Line 493: Suppress Ruff S105 only for the assertion comparing
echoed["MCP_TOKEN"] to the test fixture value, using the project’s targeted
inline-noqa convention; leave the assertion logic and surrounding checks
unchanged.
- Around line 321-323: Strengthen test_mcp_sdk_confined by exercising the MCP
SDK architecture rule directly instead of relying only on filtering
architecture_errors output by the “MCP SDK” message. Extract and invoke a
dedicated checker or add a negative fixture that imports the restricted SDK and
assert that it produces the expected finding, while preserving the existing
passing behavior for compliant code.
In `@tests/integration/test_skill_mcp_persistence_m8.py`:
- Line 135: Update the rollback test around the existing archive path assertion
to first verify that the rollback archive exists under the skill-packages
directory while the transaction is active, then verify that the same path no
longer exists after rollback; preserve the existing rollback flow and use the
established rollback_key path.
---
Nitpick comments:
In `@src/agent_core/adapters/persistence/sqlalchemy_models.py`:
- Around line 511-537: Update SkillRevisionRow.__table_args__ to retain the
existing uniqueness constraint and add the
ix_skill_revisions_skill_status_revision index on skill_id, status, and
descending revision. Add the corresponding index creation to migration
9a71c4e8d2f0_add_milestone_8_skills_and_mcp.py so the database schema matches
the model.
In `@src/agent_core/adapters/persistence/unit_of_work.py`:
- Around line 215-219: Update _run_rollback_callbacks to retain suppression so
all callbacks continue executing, but catch each callback exception and log it
through a module-level logger with rollback-callback context and exception
details. Add the module logger if this file does not already define one.
In `@src/agent_core/adapters/skills/stores.py`:
- Around line 78-96: Update the staged archive flow in the put implementation
around NamedTemporaryFile and os.link to fsync the written temporary file before
linking it, then fsync target.parent after the link succeeds. Preserve the
existing immutable-key comparison and cleanup behavior while ensuring a reported
successful put survives host crashes without a truncated archive.
In `@src/agent_core/application/session_service.py`:
- Around line 45-58: Update the create_in method docstring to state that callers
composing it into their own submission transaction must call activate after the
transaction commits, preserving the existing create behavior and explicitly
covering the MCP session-activation step. Do not change the catalog-opening
logic here; address only the documentation requested for create_in.
In `@src/agent_core/bootstrap.py`:
- Line 636: Update the transport check in the effective MCP configuration logic
to compare config.transport directly with the MCPTransport.HTTP enum member
instead of comparing its value to a string. Add MCPTransport to the existing
imports and preserve the surrounding any(...) condition.
In `@src/agent_core/domain/errors.py`:
- Around line 143-148: Update SkillRevisionConflict.__init__ to pass a
structured ConflictError reason and details payload when calling the superclass,
using current_revision in the details so consumers retain the winning revision;
preserve the current_revision attribute and conflict message behavior.
In `@src/agent_core/domain/runs.py`:
- Around line 115-119: Add an after-model validator to the RunCheckpoint model,
alongside the pinned_tool_names, pinned_tool_versions, and pinned_tool_specs
fields, that verifies both metadata mappings contain only names present in
pinned_tool_names and raises a ValueError when they do not. Preserve valid
checkpoints and follow the existing ContextPlan.tools_match_names validation
pattern.
In `@src/agent_core/mcp/mapping.py`:
- Around line 79-81: Replace id(tool)-based keys in schema_inspections with
positional indices, and use enumerate consistently in the declarations
comprehension and acceptance loop so each remote tool retrieves its inspection
by index.
In `@src/agent_core/skills/package.py`:
- Around line 195-212: Move the MAX_FILES guard in the os.walk packaging loop so
it runs only immediately before appending a symlink or regular-file
SkillPackageMember, not for plain directory entries. Preserve the existing
file-count error and directory traversal behavior.
In `@tests/contract/test_skill_catalog_contract.py`:
- Around line 27-35: Update the test around SkillCatalogService.open to seed at
least one package before the first open, then install or archive another package
between the two open calls. Keep the equality assertion and ensure the first
catalog is non-empty so the second result verifies the pinned catalog rather
than an empty repository state.
In `@tests/gates/test_skill_m8.py`:
- Around line 272-281: Update test_no_tool_from_skill to use recursive file
discovery with rglob instead of glob, ensuring Python files in all skills
subpackages are checked for register and register_dynamic calls while preserving
the existing AST validation.
- Around line 538-539: Update the token-sum assertion in the test around
SkillCatalogService to derive its limit from the same plan budget or
configuration constant enforced by SkillCatalogService, replacing the hard-coded
6_000 while preserving the existing loaded-count assertion.
In `@tests/gates/test_tool_m8.py`:
- Line 222: Replace the split key expression with the plain literal
"token_endpoint" in both the oauth_refresh config at tests/gates/test_tool_m8.py
lines 222-222 and the oauth config at lines 384-384; if lint rejects the
literal, suppress that rule explicitly on each affected line.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 79e23f6b-fbcc-40be-b1f1-4c9adcbe3af2
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
docs/adr/0044-milestone-8-skills-and-mcp-seams.mddocs/adr/index.mddocs/changelog.mddocs/plan/current-milestone.mddocs/status/project-state.yamlevals/fixtures/mcp/docs_disconnect.yamlevals/fixtures/mcp/docs_server.yamlevals/fixtures/models/mcp_disconnect_then_continue.yamlevals/fixtures/models/mcp_round_trip.yamlevals/fixtures/models/skill_changes_outcome.yamlevals/fixtures/skills/rel/SKILL.mdevals/gates/harness.yamlevals/gates/skill.yamlevals/gates/tool.yamlmigrations/versions/9a71c4e8d2f0_add_milestone_8_skills_and_mcp.pymkdocs.ymlpyproject.tomlscripts/architecture_checks.pysrc/agent_core/adapters/credentials.pysrc/agent_core/adapters/mcp/__init__.pysrc/agent_core/adapters/mcp/memory.pysrc/agent_core/adapters/mcp/persistence.pysrc/agent_core/adapters/mcp/scripted.pysrc/agent_core/adapters/mcp/sdk.pysrc/agent_core/adapters/models/fake.pysrc/agent_core/adapters/persistence/revision.pysrc/agent_core/adapters/persistence/skills.pysrc/agent_core/adapters/persistence/sqlalchemy_models.pysrc/agent_core/adapters/persistence/unit_of_work.pysrc/agent_core/adapters/skills/__init__.pysrc/agent_core/adapters/skills/memory.pysrc/agent_core/adapters/skills/stores.pysrc/agent_core/application/public_services.pysrc/agent_core/application/run_service.pysrc/agent_core/application/session_service.pysrc/agent_core/bootstrap.pysrc/agent_core/context/builder.pysrc/agent_core/context/planner.pysrc/agent_core/context/rendering.pysrc/agent_core/domain/context.pysrc/agent_core/domain/errors.pysrc/agent_core/domain/mcp.pysrc/agent_core/domain/messages.pysrc/agent_core/domain/runs.pysrc/agent_core/domain/skills.pysrc/agent_core/domain/tools.pysrc/agent_core/evals/cases.pysrc/agent_core/evals/fixtures.pysrc/agent_core/evals/runner.pysrc/agent_core/execution/proxy.pysrc/agent_core/mcp/__init__.pysrc/agent_core/mcp/configuration.pysrc/agent_core/mcp/mapping.pysrc/agent_core/mcp/runtime.pysrc/agent_core/ports/context.pysrc/agent_core/ports/mcp.pysrc/agent_core/ports/persistence.pysrc/agent_core/ports/skills.pysrc/agent_core/ports/tools.pysrc/agent_core/runtime/executor.pysrc/agent_core/skills/__init__.pysrc/agent_core/skills/catalog.pysrc/agent_core/skills/package.pysrc/agent_core/tools/executor.pysrc/agent_core/tools/messages.pysrc/agent_core/tools/registry.pysrc/agent_core/tools/skill_load.pytests/contract/test_m_c_p_client_contract.pytests/contract/test_m_c_p_client_factory_contract.pytests/contract/test_m_c_p_server_repository_contract.pytests/contract/test_skill_catalog_contract.pytests/contract/test_skill_package_store_contract.pytests/contract/test_skill_repository_contract.pytests/eval_cases/27_skill_changes_outcome.yamltests/eval_cases/29_mcp_round_trip.yamltests/eval_cases/30_mcp_disconnect.yamltests/fixtures/mcp_stdio_environment_server.pytests/gates/test_gate_registry.pytests/gates/test_harness_m1.pytests/gates/test_harness_m8.pytests/gates/test_import_boundaries.pytests/gates/test_skill_m8.pytests/gates/test_tool_m8.pytests/gates/test_trajectory_conversion_m3.pytests/integration/test_skill_mcp_persistence_m8.py
|
Addressed all 19 actionable CodeRabbit threads in Validation: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/gates/test_skill_m8.py (1)
574-596: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest the token cap independently.
Line 587 rejects the third load at
maximum_loaded == 2. The"one, two"assertion confirms that the item-count branch ran. The test never reachessum(item.tokens for item in loaded) + tokens > self._maximum_body_tokens.Set
maximum_loadedto at least three. Setmaximum_body_tokensbetween the cumulative totals. Assert the token-cap error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gates/test_skill_m8.py` around lines 574 - 596, Update test_body_cap to configure maximum_loaded to at least three and maximum_body_tokens between the cumulative token totals for the loaded skills, ensuring the third load reaches the token-cap branch instead of the item-count limit. Assert the resulting ConflictError from the token cap, while preserving the replacement-load assertions.
🧹 Nitpick comments (2)
src/agent_core/application/session_service.py (1)
47-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared session bootstrap block.
PublicSessionService.createinsrc/agent_core/application/public_services.pyrepeats this exact sequence: generate the id, register the catalog discard callback, register the close-session callback, then open the catalog. Two copies of the cleanup ordering can drift, and the ordering is what makes rollback safe.Extract a helper that takes the unit of work, the agent, and the principal, and returns the session id and the opened catalog. Call it from both paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/application/session_service.py` around lines 47 - 66, Extract the shared session bootstrap sequence from SessionService.create_in and PublicSessionService.create into a helper accepting the unit of work, agent, and principal and returning the generated session ID plus opened catalog. Preserve the existing rollback callback registration order—catalog discard before close_session—and invoke this helper from both create paths.src/agent_core/adapters/persistence/unit_of_work.py (1)
235-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrain the callback list in place instead of rebinding it.
__aenter__passesself._rollback_callbacks.appendto the repository factory on line 182. That bound method captures the original list object. Line 236 rebindsself._rollback_callbacksto a new list, so the repositories keep appending to the orphaned list. Any callback registered after this point is never executed. The success path usesclear(), which keeps the binding intact, so the two paths behave differently.Drain in place to keep one list identity for the lifetime of the unit of work.
♻️ Proposed change
async def _run_rollback_callbacks(self) -> None: - callbacks, self._rollback_callbacks = self._rollback_callbacks, [] + callbacks = list(self._rollback_callbacks) + self._rollback_callbacks.clear() for callback in reversed(callbacks):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/persistence/unit_of_work.py` around lines 235 - 244, In the `_run_rollback_callbacks` method, replace the rebinding pattern that assigns a new empty list to `self._rollback_callbacks` with an in-place clear operation to preserve the list identity that was captured by the bound `append` method reference passed to repository factories during `__aenter__`. Extract the callbacks to a local variable first by copying the current list, then call `clear()` on `self._rollback_callbacks` to drain it in place, ensuring the same list object remains bound for any callbacks registered after this method executes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/gates/test_skill_m8.py`:
- Around line 574-596: Update test_body_cap to configure maximum_loaded to at
least three and maximum_body_tokens between the cumulative token totals for the
loaded skills, ensuring the third load reaches the token-cap branch instead of
the item-count limit. Assert the resulting ConflictError from the token cap,
while preserving the replacement-load assertions.
---
Nitpick comments:
In `@src/agent_core/adapters/persistence/unit_of_work.py`:
- Around line 235-244: In the `_run_rollback_callbacks` method, replace the
rebinding pattern that assigns a new empty list to `self._rollback_callbacks`
with an in-place clear operation to preserve the list identity that was captured
by the bound `append` method reference passed to repository factories during
`__aenter__`. Extract the callbacks to a local variable first by copying the
current list, then call `clear()` on `self._rollback_callbacks` to drain it in
place, ensuring the same list object remains bound for any callbacks registered
after this method executes.
In `@src/agent_core/application/session_service.py`:
- Around line 47-66: Extract the shared session bootstrap sequence from
SessionService.create_in and PublicSessionService.create into a helper accepting
the unit of work, agent, and principal and returning the generated session ID
plus opened catalog. Preserve the existing rollback callback registration
order—catalog discard before close_session—and invoke this helper from both
create paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c62f7bc2-fede-4745-9b0d-7396274bb64e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
migrations/versions/9a71c4e8d2f0_add_milestone_8_skills_and_mcp.pypyproject.tomlsrc/agent_core/adapters/mcp/memory.pysrc/agent_core/adapters/mcp/persistence.pysrc/agent_core/adapters/mcp/sdk.pysrc/agent_core/adapters/models/fake.pysrc/agent_core/adapters/persistence/skills.pysrc/agent_core/adapters/persistence/sqlalchemy_models.pysrc/agent_core/adapters/persistence/unit_of_work.pysrc/agent_core/adapters/skills/stores.pysrc/agent_core/application/public_services.pysrc/agent_core/application/session_service.pysrc/agent_core/bootstrap.pysrc/agent_core/context/builder.pysrc/agent_core/domain/errors.pysrc/agent_core/domain/mcp.pysrc/agent_core/domain/runs.pysrc/agent_core/mcp/configuration.pysrc/agent_core/mcp/mapping.pysrc/agent_core/mcp/runtime.pysrc/agent_core/ports/persistence.pysrc/agent_core/ports/skills.pysrc/agent_core/ports/tools.pysrc/agent_core/runtime/executor.pysrc/agent_core/skills/catalog.pysrc/agent_core/skills/package.pysrc/agent_core/tools/registry.pytests/contract/test_m_c_p_server_repository_contract.pytests/contract/test_model_provider_contract.pytests/contract/test_skill_catalog_contract.pytests/gates/test_context_m7.pytests/gates/test_harness_m8.pytests/gates/test_runtime_m7.pytests/gates/test_skill_m8.pytests/gates/test_tool_m8.pytests/integration/test_skill_mcp_persistence_m8.py
🚧 Files skipped from review as they are similar to previous changes (14)
- pyproject.toml
- src/agent_core/adapters/models/fake.py
- src/agent_core/ports/persistence.py
- src/agent_core/adapters/mcp/memory.py
- tests/gates/test_harness_m8.py
- src/agent_core/adapters/persistence/sqlalchemy_models.py
- src/agent_core/domain/errors.py
- src/agent_core/adapters/persistence/skills.py
- src/agent_core/ports/skills.py
- src/agent_core/ports/tools.py
- src/agent_core/adapters/skills/stores.py
- src/agent_core/bootstrap.py
- src/agent_core/mcp/mapping.py
- src/agent_core/mcp/runtime.py
|
Addressed all three incremental summary findings in
Validation: Ruff, strict mypy, |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai rate limit |
|
You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 40 seconds. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Verification
make checkMilestone 8 remains in progress until hosted CircleCI passes and all hosted CodeRabbit findings are addressed.
Summary by CodeRabbit
New Features
Documentation
Tests