Skip to content

fix: relax ECA layout test to not assume parent-menu key ownership#362

Merged
tninja merged 2 commits into
mainfrom
copilot/fix-eca-transient-menu-layout-test
May 18, 2026
Merged

fix: relax ECA layout test to not assume parent-menu key ownership#362
tninja merged 2 commits into
mainfrom
copilot/fix-eca-transient-menu-layout-test

Conversation

Copilot AI commented May 18, 2026

Copy link
Copy Markdown
Contributor

ai-code-test-eca-menu-group-appears-in-layout asserted that "A" maps to ai-code-eca-add-workspace-folder in both parent transient layouts, but that key is already claimed by ai-code-derive-architecture-guardrails in ai-code--menu-other-tools, causing the assertion to fail.

Changes

  • Canonical ECA binding verification — all 9 ECA keys (E W D A X F M B Y) are now checked against ai-code-eca-menu, which is the authoritative source of truth for the complete ECA command set.
  • Remove "A" from parent-layout command assertions — replaced with a weaker (should (transient-get-suffix prefix "A")) that only documents the collision point without dictating which command wins.
  • "F" special-case preserved — the existing split behavior (ai-code--infix-toggle-auto-follow-up in ai-code-menu-default, ai-code-eca-share-file-context in ai-code-menu-2-columns and ai-code-eca-menu) is kept as-is.
;; Before: asserted ECA owns "A" in parent layouts (breaks when another command claims it)
(dolist (expected '(...("A" . ai-code-eca-add-workspace-folder)...))
  (dolist (prefix '(ai-code-menu-default ai-code-menu-2-columns))
    (should (eq (plist-get (cdr (transient-get-suffix prefix (car expected)))
                           :command)
                (cdr expected)))))

;; After: verify "A" in ai-code-eca-menu only; weak collision check in parent layouts
(dolist (expected '(...("A" . ai-code-eca-add-workspace-folder)...))
  (let ((suffix (transient-get-suffix 'ai-code-eca-menu (car expected))))
    (should (eq (plist-get (cdr suffix) :command) (cdr expected)))))

(dolist (prefix '(ai-code-menu-default ai-code-menu-2-columns))
  ;; "E W D X M B Y" still checked against their ECA commands in parent layouts
  ;; "A": known collision — just assert something is bound
  (should (transient-get-suffix prefix "A")))
Original prompt

The unit test job is failing in repository tninja/ai-code-interface.el for PR #361.

Please create a pull request that fixes the failing test in the ECA transient menu layout tests.

Context:

  • Failing workflow job: unit-tests
  • Workflow file: .github/workflows/unit-tests.yml
  • Failing test: ai-code-test-eca-menu-group-appears-in-layout
  • Failing assertion from logs shows a key collision in the parent transient layout:
    • expected key "A" to map to ai-code-eca-add-workspace-folder
    • actual command found was ai-code-derive-architecture-guardrails
  • Commit/ref where failure was observed: 2dc0081fd6de843cc123be16d4bfb9e238878d02

Relevant files:

  • test/test_ai-code-eca.el
  • ai-code-eca.el
  • .github/workflows/unit-tests.yml

What to change:

  • Fix the test so it no longer incorrectly assumes that all ECA keybindings are directly reachable at the top level of both parent transient layouts.
  • Treat ai-code-eca-menu as the canonical source of truth for the complete ECA command set.
  • Keep verifying parent layouts only for bindings that are intentionally exposed there and stable.
  • Preserve or clarify the existing special handling for the "F" key if needed.
  • Prefer the smallest targeted change that makes the test accurately reflect actual transient behavior.

Success criteria:

  • The failing test passes.
  • The updated test still verifies meaningful ECA menu integration behavior.
  • No unrelated code changes.

Please open a PR with the fix.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Please find a solution for this failing job. Use the logs, job definition, and any referenced files where the failure occurred. Keep your response focused on the solution and include code suggestions when appropriate.

Assistant: The failure is a test/code mismatch in the ECA transient menu layout, not a workflow problem.

Root cause

The failing test is ai-code-test-eca-menu-group-appears-in-layout.

The assertion failure shows:

  • expected "A"ai-code-eca-add-workspace-folder
  • actual "A"ai-code-derive-architecture-guardrails

From the job log:

  • failing test: ai-code-test-eca-menu-group-appears-in-layout
  • failure form: (eq ai-code-derive-architecture-guardrails ai-code-eca-add-workspace-folder)

So the test assumes the ECA group is directly visible at top-level in both layouts, but in the actual loaded transient layout, "A" is already occupied by another command.

Relevant code

The ECA menu is injected here:

It appends this group to both menu layouts:

(dolist (prefix '(ai-code-menu-default ai-code-menu-2-columns))
  (when (commandp prefix)
    (transient-append-suffix prefix '(0 -1)
      ["ECA"
       ("E" "Start ECA for project..." ai-code-eca-create-session-for-workspace)
       ("W" "Switch session" ai-code-eca-switch-session)
       ("D" "ECA Workspace Dashboard" eca-workspaces)
       ("A" "Add project to session" ai-code-eca-add-workspace-folder)
       ("X" "Remove project from session" ai-code-eca-remove-workspace-folder)
       ("F" "Add file to shared context" ai-code-eca-share-file-context)
       ("M" "Add repo map to shared context" ai-code-eca-share-repo-map-context)
       ("B" "Add clipboard as file context" ai-code-eca-chat-add-clipboard-context-now)
       ("Y" "Clear shared" ai-code-eca-clear-shared-context)])))

The test is too strict because it looks up single-key suffixes on the full parent transient:

That breaks as soon as the main menu already uses one of those keys.

Solution

Update the test so it verifies the dedicated ECA submenu layout, and only checks parent layouts for keys that are intentionally exposed there and not shadowed.

Recommended test change

Replace the broad top-level assertions with:

  • assert all ECA bindings exist in ai-code-eca-menu
  • assert only the known non-conflicting parent-layout bindings you actually want to expose
  • keep the special-case handling for "F" if that is intentional

Example patch for test/test_ai-code-eca.el:

(ert-deftest ai-code-test-eca-menu-group-appears-in-layout ()
  "ECA commands should exist in the ECA transient, and selected ones
should be reachable from ai-code layouts where not shadowed."
  (skip-unless (and (featurep 'transient)
                    (fboundp 'transient-define-group)))
  (condition-case nil
      (require 'ai-code)
    (err...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created from Copilot chat.*
>

The test ai-code-test-eca-menu-group-appears-in-layout incorrectly
asserted that key "A" maps to ai-code-eca-add-workspace-folder in
both parent transient layouts.  PR #361 added ("A" "Derive
Architecture Guardrails" ai-code-derive-architecture-guardrails) to
ai-code--menu-other-tools, creating a collision.

Changes:
- Add canonical check of all ECA bindings against ai-code-eca-menu
- Remove "A" from parent-layout command assertions (known collision)
- Add weak (should (transient-get-suffix prefix "A")) to document
  the collision without dictating what "A" maps to
- Keep "F" special-case handling unchanged

Agent-Logs-Url: https://github.com/tninja/ai-code-interface.el/sessions/1882eb4f-2064-491d-9972-b79610269ee4

Co-authored-by: tninja <714625+tninja@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix failing test in ECA transient menu layout fix: relax ECA layout test to not assume parent-menu key ownership May 18, 2026
Copilot AI requested a review from tninja May 18, 2026 00:30
Copilot finished work on behalf of tninja May 18, 2026 00:30
@tninja tninja marked this pull request as ready for review May 18, 2026 00:39
@tninja tninja merged commit 7ccd471 into main May 18, 2026
2 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.

2 participants