Skip to content

Reconcile ACP release caveat tracker#2422

Merged
rmusser01 merged 3 commits into
devfrom
codex/acp-final-reconciliation-2398
Jul 4, 2026
Merged

Reconcile ACP release caveat tracker#2422
rmusser01 merged 3 commits into
devfrom
codex/acp-final-reconciliation-2398

Conversation

@rmusser01

@rmusser01 rmusser01 commented Jun 21, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add a final Epic: ACP release-caveat closeout and live verification tracker #2398 ACP release-caveat closeout record that maps each closed child issue to the remaining release-support boundary.
  • Link the closeout from ACP Production Readiness and correct Goose, Hermes, and OpenCode registry notes to include June 20 workspace-live-E2E MCP evidence while preserving sandbox, artifact, reviewer-loop, and failure-diagnostic caveats.
  • Add focused registry metadata checks so the stale non-empty-MCP caveat does not return for those three agents.
  • Rebased onto current dev and addressed review feedback by removing custom assertion helpers and exact workspace-live commit hash checks.

Verification

  • source /Users/macbook-dev/Documents/GitHub/tldw_server2/.venv/bin/activate && python -m pytest tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py -q -> 32 passed, 3 warnings.
  • git diff --check -> passed.
  • Targeted helper/hash rg check -> no _require_note or commit ac93d96d9c matches in test_acp_agent_registry.py.
  • Targeted stale-wording rg check -> only the expected Codex non-empty-MCP caveat remains.
  • source /Users/macbook-dev/Documents/GitHub/tldw_server2/.venv/bin/activate && python -m bandit -r tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py -f json -o /tmp/bandit_pr2422_registry.json -> existing test-file B101 assert baseline only.

Change summary

Human-authored Change summary required before merge per project policy. Please replace this section with the requester-owned explanation of what changed and why.

@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4a9ba9c8-1124-4add-90b7-70c657a9fb81

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/acp-final-reconciliation-2398

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request finalizes the Agent Client Protocol (ACP) release-caveat reconciliation for epic #2398. It adds comprehensive documentation regarding the closeout outcomes, remaining caveats, and implementation plans, and updates the compatibility notes in agents.yaml for Goose, Hermes, and OpenCode to reflect the June 20, 2026 workspace-live-e2e test results. Unit tests were also updated to verify these compatibility notes. The review feedback advises against using custom helper functions that manually raise AssertionError to bypass Bandit's B101 check, recommending standard assert statements instead to preserve pytest's assertion introspection.

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 thread tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py Outdated
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Reconcile ACP release-caveat tracker (#2398) with closeout record + registry checks
📝 Documentation 🧪 Tests ✨ Enhancement 🕐 20-40 Minutes

Grey Divider

Description

• Add a #2398 closeout doc mapping closed child issues to remaining support boundaries.
• Link closeout from ACP Production Readiness and align wording with June 20 workspace-live
 evidence.
• Update Goose/Hermes/OpenCode registry notes and add tests preventing stale MCP caveat regressions.
Diagram

graph TD
A["ACP_Production_Readiness.md"] --> B["ACP_Release_Caveat_Closeout_2026_06_21.md"]
C["agents.yaml"] --> D["ACP registry loader"] --> E["test_acp_agent_registry.py"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move evidence/caveats from free-text to structured fields
  • ➕ Eliminates brittle string-based regressions and substring tests
  • ➕ Enables consistent UI rendering/filtering (e.g., workspace/MCP vs sandbox vs artifacts)
  • ➕ Makes compatibility claims auditable and machine-checkable
  • ➖ Requires schema evolution and migration of existing notes
  • ➖ Touches more code/UX surfaces than this closeout-focused PR
2. Add schema validation for compatibility_notes format (lint/test)
  • ➕ Keeps current YAML shape while enforcing a consistent phrasing template
  • ➕ Reduces future drift without introducing new structured fields
  • ➖ Still relies on a template over semantic fields
  • ➖ May be too constraining for nuanced notes over time

Recommendation: For a closeout PR, the current approach (minimal doc + registry-note corrections plus targeted regression tests) is appropriate and low-risk. Longer-term, consider moving “evidence dimensions” (workspace binding, MCP injection, sandbox, artifacts, reviewer-loop, failure diagnostics) into structured registry fields to avoid substring coupling and make support boundaries queryable.

Files changed (6) +205 / -3

Enhancement (1) +3 / -3
agents.yamlUpdate Goose/Hermes/OpenCode compatibility notes with workspace-live MCP evidence +3/-3

Update Goose/Hermes/OpenCode compatibility notes with workspace-live MCP evidence

• Amends compatibility_notes for Goose, Hermes, and OpenCode to include June 20 workspace-live-e2e evidence of workspace binding and non-empty MCP server injection (mcp_server_count=1). Keeps support_state as supported_with_caveats and preserves the unverified caveats for sandbox, artifacts, reviewer-loop, and failure diagnostics.

tldw_Server_API/Config_Files/agents.yaml

Tests (1) +31 / -0
test_acp_agent_registry.pyAdd regression checks for updated registry caveat wording +31/-0

Add regression checks for updated registry caveat wording

• Adds helper functions to require presence/absence of specific phrases in compatibility_notes. Extends existing agents.yaml tests to assert workspace-live-e2e/commit evidence is present and that the stale “non-empty MCP injection, artifact-producing workflows” wording does not reappear for Hermes/Goose/OpenCode.

tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py

Documentation (4) +171 / -0
ACP_Production_Readiness.mdLink #2398 closeout and add tracker entry +15/-0

Link #2398 closeout and add tracker entry

• Adds a short release-caveat tracker reconciliation note pointing to the new closeout record. Extends the issue map with #2398 and adds a dedicated “Release-Caveat Closeout” section to centralize the final status and remaining support boundaries.

Docs/Development/ACP_Production_Readiness.md

ACP_Release_Caveat_Closeout_2026_06_21.mdAdd #2398 release-caveat closeout record +61/-0

Add #2398 release-caveat closeout record

• Introduces a new closeout document mapping each #2398 child issue to outcomes and explicit release-interpretation boundaries. Captures remaining caveats as expected support limits (sandbox/runtime scope, artifacts, reviewer-loop, and failure diagnostics) and records the verification steps used to reconcile wording.

Docs/Development/ACP_Release_Caveat_Closeout_2026_06_21.md

IMPLEMENTATION_PLAN_acp_final_reconciliation_2398.mdDocument reconciliation plan and completion status +22/-0

Document reconciliation plan and completion status

• Adds a concise three-stage implementation plan (inventory, surface reconciliation, closeout) with goals, success criteria, and verification steps. Marks each stage as complete to preserve the closeout procedure as project documentation.

IMPLEMENTATION_PLAN_acp_final_reconciliation_2398.md

task-2396 - Finalize-ACP-release-caveat-reconciliation-for-epic-2398.mdAdd backlog task capturing scope and verification for #2398 closeout +73/-0

Add backlog task capturing scope and verification for #2398 closeout

• Creates a task record with references, modified file list, acceptance criteria, and an implementation plan. Includes verification notes (pytest, diff check, rg audit, bandit baseline) to make the reconciliation work auditable.

backlog/tasks/task-2396 - Finalize-ACP-release-caveat-reconciliation-for-epic-2398.md

@qodo-code-review

qodo-code-review Bot commented Jun 21, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 74 rules

Grey Divider


Action required

1. _require_note_contains missing docstring ✓ Resolved 📘 Rule violation ✧ Quality
Description
New helper functions _require_note_contains and _require_note_not_contains were added without
docstrings. This violates the project requirement that all functions in modified Python files
include docstrings, reducing maintainability and reviewability.
Code

tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py[R13-20]

+def _require_note_contains(notes: str, phrase: str) -> None:
+    if phrase not in notes:
+        raise AssertionError(f"Expected compatibility note to contain {phrase!r}")
+
+
+def _require_note_not_contains(notes: str, phrase: str) -> None:
+    if phrase in notes:
+        raise AssertionError(f"Expected compatibility note to omit {phrase!r}")
Evidence
PR Compliance ID 224214 requires docstrings for all functions in new/modified Python files. The
added helper functions at the cited lines have no docstring as their first statement.

Rule 224214: Require docstrings for all modules, classes, and functions
tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py[13-20]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New helper functions `_require_note_contains` and `_require_note_not_contains` were added without docstrings, violating the docstring requirement for all functions in modified Python files.

## Issue Context
These helpers are used as assertion-style checks in tests; they should still be documented to clarify intent and expected inputs.

## Fix Focus Areas
- tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py[13-20]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Brittle note hash checks ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The new registry tests hard-code a specific workspace-live-e2e commit hash ("ac93d96d9c") inside the
free-form compatibility_notes field, so any future evidence refresh that updates the note text
will fail CI even when registry behavior is unchanged.
Code

tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py[R351-356]

+    _require_note_contains(entry.compatibility_notes, "workspace-live-e2e")
+    _require_note_contains(entry.compatibility_notes, "commit ac93d96d9c")
+    _require_note_contains(entry.compatibility_notes, "non-empty MCP server injection")
+    _require_note_not_contains(
+        entry.compatibility_notes,
+        "non-empty MCP injection, artifact-producing workflows",
Evidence
The tests pin an exact commit hash inside compatibility_notes, but the registry loader coerces
compatibility_notes from YAML as a free-form string, so future note edits (including normal
evidence refreshes) will deterministically break these tests without indicating a behavioral
regression.

tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py[330-357]
tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py[360-389]
tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py[391-420]
tldw_Server_API/app/core/Agent_Client_Protocol/agent_registry.py[441-467]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Tests now require an exact commit hash substring (e.g., `"commit ac93d96d9c"`) to appear in `AgentRegistryEntry.compatibility_notes`, which is a documentation-style free-form string. This makes CI fail for legitimate documentation/evidence updates.

### Issue Context
`compatibility_notes` is loaded as a plain string from YAML and not treated as a stable API contract field.

### Fix Focus Areas
- tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py[13-21]
- tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py[330-420]

### Suggested fix
- Replace the exact hash requirement (e.g., `_require_note_contains(..., "commit ac93d96d9c")`) with a semantic check, such as:
 - Assert only `"workspace-live-e2e"` and `"non-empty MCP server injection"` are present, **or**
 - Use a regex like `re.search(r"commit [0-9a-f]{7,40}", notes)` to confirm a commit is recorded without pinning a specific value.
- Optionally include the full `notes` content in failure messages to reduce debugging time when the wording changes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py Outdated
Comment thread tldw_Server_API/tests/Agent_Client_Protocol/test_acp_agent_registry.py Outdated
@rmusser01 rmusser01 force-pushed the codex/acp-final-reconciliation-2398 branch from cf72736 to 5723e42 Compare July 4, 2026 01:40
@rmusser01

Copy link
Copy Markdown
Owner Author

Rebased onto current origin/dev and addressed the open review feedback in 5723e42cda:

  • Removed _require_note_contains / _require_note_not_contains, so the docstring finding no longer applies and pytest assertion introspection is preserved.
  • Removed the exact commit ac93d96d9c checks from registry tests; the tests now assert the semantic evidence that matters: workspace-live-e2e, non-empty MCP server injection, and absence of the stale combined caveat wording.
  • Updated the closeout doc and Backlog task verification notes to match the revised test approach.

Validation: focused registry pytest passed (32 passed), git diff --check passed, helper/hash rg check has no matches, and Bandit reports the existing pytest B101 assert baseline only.

@rmusser01 rmusser01 merged commit c34dcd5 into dev Jul 4, 2026
27 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant