Skip to content

Name the document an invoked Agent Skill came from - #213

Open
AshishKumar4 wants to merge 1 commit into
mainfrom
fix/unaddressed-agent-skill
Open

Name the document an invoked Agent Skill came from#213
AshishKumar4 wants to merge 1 commit into
mainfrom
fix/unaddressed-agent-skill

Conversation

@AshishKumar4

Copy link
Copy Markdown
Contributor

What does this change?

Invoking an Agent Skill hands the agent the skill's body and nothing else, so the agent cannot read
the documents the skill tells it to read.

buildAgentSkillMessage() receives only the body, and the slashCommand chat record that holds the
document ID is display-only — agent.ts notes that "only a generated message is model input". So a
skill saying "load references/research-sources.md" gives the agent a relative path with no root to
resolve it against. read() returns null for a wrong ID rather than an error, so the miss is
silent and the agent answers from memory instead.

In the skill library this deployment publishes, 32 of 97 skills cite references/… paths, 329
citations in total. None of them currently resolve.

This passes the document ID that #invokeAgentSkillCommand() already has into the message, and
names the skill's own directory as the root for the paths it cites.

Why is this obviously correct and trivially verifiable?

The patch adds one parameter and one appended line. Production change is 10 lines across 2 files:

  • buildAgentSkillMessage() takes docId and appends skill root: <dir> plus how to resolve
    against it. The existing <agent_skill> envelope and $ARGUMENT handling are untouched.
  • The one caller passes id, the argument it already receives and already uses for session.read().

The root is stated as prose rather than an element attribute because validateDocumentPath()
permits quotes and angle brackets in a path, so any delimiter could be closed by the path itself.
The test uses a path containing a quote to pin that.

The four existing buildAgentSkillMessage tests assert the complete output string, so they
establish the whole effect by equality.

Verified with pnpm lint (lint plus type-check) and pnpm test, both clean; gatekeeper-context
is 28/28.

Checklist

Checking every item does not guarantee acceptance. Maintainers determine whether
a pull request meets the contribution policy.

  • This is a small, concrete change; it is not a feature, refactor, or low-value cleanup.
  • I understand that maintainers decide whether the change is obviously correct and trivially verifiable.
  • I have read and followed the contribution guidelines.

@github-actions github-actions Bot added the gatekeeper Changes to a gatekeeper integration label Aug 14, 2026
@ask-bonk

ask-bonk Bot commented Aug 14, 2026

Copy link
Copy Markdown

Only one production caller, correctly updated. All test call sites updated. The test file uses let docId / let root at module scope inside the describe — a minor style nit (could be const), but the codebase uses let pervasively and no-shadow/prefer-const isn't enforced as error, so it's consistent.

One small observation worth noting: the test declares docId and root with let but never reassigns them; const would be marginally more correct. Not a blocker and consistent with repo style.

The change is correct, minimal, and well-tested. The reasoning about prose-vs-attribute (paths can contain quotes/angle brackets, verified against validateDocumentPath's permissiveness per the PR body) is sound, and the test pins it with a quote-containing path. The docId derivation, envelope preservation, and argument handling are all intact.

No actionable issues found.

LGTM!

github run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gatekeeper Changes to a gatekeeper integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant