Skip to content

feat(cosh): concatenate hook systemMessages with [name] prefix#387

Merged
samchu-zsl merged 1 commit intoalibaba:mainfrom
kongche-jbw:feat/cosh/merge_hook_sysmsg
Apr 27, 2026
Merged

feat(cosh): concatenate hook systemMessages with [name] prefix#387
samchu-zsl merged 1 commit intoalibaba:mainfrom
kongche-jbw:feat/cosh/merge_hook_sysmsg

Conversation

@kongche-jbw
Copy link
Copy Markdown
Collaborator

Description

Previously, when multiple hooks returned systemMessage for the same event
(e.g. PreToolUse), HookAggregator.mergeWithOrLogic used last-write-wins
semantics — the final message was overwritten by whichever hook ran last,
even if that hook didn't trigger the ask / block decision.

This caused a confusing UX where hook A raised ask with message "A", but
a later allow-hook C silently replaced the message with "C", so the user
saw "C" in the confirmation dialog without any hint that A was the one
that actually triggered the prompt.

This PR changes the aggregation rule to concatenate every non-empty
systemMessage as [hookName] message on separate lines, so users can
see exactly which hook contributed which message. The change applies
uniformly to both parallel and sequential hook execution since both
paths funnel into the same aggregateResults entry point.

Related Issue

no-issue: internal UX polish for hook message attribution

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional change)
  • Performance improvement
  • CI/CD or build changes

Scope

  • cosh (copilot-shell)
  • sec-core (agent-sec-core)
  • skill (os-skills)
  • sight (agentsight)
  • tokenless (tokenless)
  • Multiple / Project-wide

Checklist

  • I have read the Contributing Guide
  • My code follows the project's code style
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the documentation accordingly
  • For cosh: Lint passes, type check passes, and tests pass
  • For sec-core (Rust): cargo clippy -- -D warnings and cargo fmt --check pass
  • For sec-core (Python): Ruff format and pytest pass
  • For skill: Skill directory structure is valid and shell scripts pass syntax check
  • For sight: cargo clippy -- -D warnings and cargo fmt --check pass
  • For tokenless: cargo clippy -- -D warnings and cargo fmt --check pass
  • Lock files are up to date (package-lock.json / Cargo.lock)

Testing

Added 4 new unit tests under
mergeWithOrLogic - systemMessage concatenation in hookAggregator.test.ts:

  • Single hook fast path: passes through message untouched (no [name]
    prefix), preserving backward compatibility.
  • Multiple hooks: messages concatenated as [rm-guard] rm detected\n[audit] logged;
    decision=ask wins over allow.
  • Missing name field: falls back to the command basename, e.g.
    /usr/local/bin/guard.py --strict[guard.py] ....
  • Skip rules: hooks with systemMessage === undefined or '' do not
    contribute to the concatenation.

Run:

cd src/copilot-shell/packages/core
npx vitest run src/hooks/hookAggregator.test.ts

Result: 28/28 passed (4 new + 24 existing).
Type check: npx tsc --noEmit clean.

Additional Notes

  • Single-hook behavior is unchanged (no [name] prefix) to keep existing
    UX identical for the most common case.
  • Behavior is identical for parallel and sequential hook execution — the
    aggregator is the single merge point; hookRunner differences don't
    affect message attribution.
  • Name resolution priority: hookConfig.name → basename of the first
    token in command → fallback 'hook'.

    - Replace last-write-wins with per-hook attribution in merge
    - Prefix each message with [hookName] so users see all voices
    - Fall back to command basename when hook name is missing
    - Single-hook fast path keeps backward-compatible output
    - Unified behavior for parallel and sequential hook execution
    - Add 4 unit tests covering fast path, concat, basename, skip
@kongche-jbw kongche-jbw requested a review from yangdao479 April 27, 2026 11:50
@kongche-jbw kongche-jbw self-assigned this Apr 27, 2026
@kongche-jbw kongche-jbw requested a review from samchu-zsl as a code owner April 27, 2026 11:50
@gemini-code-assist
Copy link
Copy Markdown

Important

Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services.

@github-actions github-actions Bot added the component:cosh src/copilot-shell/ label Apr 27, 2026
Copy link
Copy Markdown
Collaborator

@samchu-zsl samchu-zsl left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

@samchu-zsl samchu-zsl merged commit 3588a4e into alibaba:main Apr 27, 2026
13 checks passed
@casparant casparant added this to the cosh/v2.3.0 milestone Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component:cosh src/copilot-shell/

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants