fix(skill): deduplicate repeat SKILL.md loads for already-active skills

[chaucerj]

AgentScope-Java Version

2.0.3-SNAPSHOT, based on main @ 39cd304a.

Description

Fixes #1569.

Problem

An LLM often calls load_skill_through_path for the same skill several times in one batch, and every call re-sent the full SKILL.md markdown — thousands of tokens for content already in context, plus a wasted iteration each time.

Solution

loadSkillResourceImpl now short-circuits the SKILL.md branch when the skill is already active:

• SkillRegistry gains isSkillActive(skillId) (mirrors setSkillActive; RegisteredSkill.isActive() already existed).
• A repeat SKILL.md load for an active skill returns one line: Skill 'X' is already loaded and active.
• First-load behavior is unchanged (RegisteredSkill.active defaults to false; the only production activation path is the load itself).
• Specific resource paths keep returning full content after activation, so a model that lost the entry file to context compaction can still re-fetch individual resources. This escape hatch is called out in the code comment.
• Unknown skills still fail validation before the short-circuit.

Note: a host that manually pre-activates a skill via the public SkillBox.setSkillActive before the model ever loads it would see the one-line notice on the first SKILL.md load. That API has no in-framework callers and its documented purpose is host-managed skill lifecycle, so I kept the behavior simple rather than tracking a separate "content delivered" flag.

Validation

• TDD: new SkillToolFactoryReloadDedupTest fails on unmodified main (repeat load re-sends full markdown) and passes with the fix; a second test pins that resource paths still return full content once active.
• agentscope-core full suite: 2341 tests, 0 failures; spotless clean.

Note: @Qinyu0924 expressed interest in #1569 on 08-30; happy to coordinate if that work is still planned.

---

This change was developed with AI assistance (Claude Code); I verified the red → green cycle, the full core suite, and the boundary cases above, and can explain every line of the diff.

🤖 Generated with Claude Code

[oss-maintainer]

Summary

Deduplicates repeat load_skill_through_path(skillId, "SKILL.md") calls by short-circuiting when the skill is already active, so a model that fires the same load several times in one batch no longer re-pays the full markdown in tokens (#1569). The change is well-scoped to the SKILL.md branch, first-load behavior and unknown-skill validation are preserved, and the TDD test is a clean red/green pin.

Two things keep this from an approve: the guard returns before activateSkill, which is the only in-framework path that re-enables a skill's tool groups, and the documented escape hatch does not cover the case where context compaction drops SKILL.md itself. Both look like targeted follow-ups rather than redesign, so I'm leaving this as a comment for the maintainers to weigh.

Findings

• [Warning] SkillToolFactory.java:183 — short-circuit skips the tool-group activation side effect of activateSkill. SkillBox.syncToolGroupStates() and deactivateAllSkills() have no in-framework callers, so re-loading SKILL.md was the only self-heal for a registry-active / group-inactive desync.
• [Warning] SkillToolFactory.java:184 — individual resource paths stay re-fetchable, but a model that lost SKILL.md to compaction has no way to discover which resources exist; the one-line notice is terminal.
• [Info] SkillRegistry.java:67 — duplicates the existing SkillBox.isSkillActive lookup against the same map; consider delegating to keep one source of truth.
• [Info] SkillToolFactoryReloadDedupTest.java:66 — re-activation after external deactivation, and the disk-fallback branch that also calls activateSkill, are not covered.

Reviewed as correct

• SkillRegistry is a package-private class rather than an interface, so the new method with a body is valid; the ConcurrentHashMap read is thread-safe and the registered != null guard plus the earlier validateSkillExists cover the unknown-skill case.
• RegisteredSkill.active defaults to false, so the first load is unaffected, and SkillBox.setSkillActive(id, false) clears the flag — a genuine re-load after deactivation still returns full content.
• CLA is signed (license/cla = success).

CI note (not attributable to this PR)

build (ubuntu-latest) and build (windows-latest) are red, but the failing test is io.agentscope.builder.web.toolbus.ToolConfirmationCoordinatorTest.replacementTurnLeaseCannotReleaseOldTicketAndMayReuseToolUseId:452 (CannotStubVoidMethodWithReturnValue) in the service-dataplane module, which this PR does not modify; agentscope-core built and its tests passed. Main-branch push CI is also currently red on a different module, so this is likely pre-existing/flaky — worth a re-run to confirm rather than a change from the author.

One doc suggestion

The PR description notes that a host which pre-activates via the public SkillBox.setSkillActive will see the one-line notice on its first SKILL.md load. Since that is a public API other integrators may rely on, it would be worth stating explicitly in the SkillBox.setSkillActive javadoc alongside the existing "Warning on Deactivation" note.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] This guard returns before activateSkill(skillId), but activateSkill does more than flip the registry flag — it also calls toolkit.updateToolGroups(..., true) for the <skillId>_skill_tools group and for every SkillToolGroup bound via activateOnSkill (see SkillToolFactory.activateSkill, ~line 370-398).

The registry's active flag and the toolkit's group state are two separate pieces of state, and re-loading SKILL.md was previously an idempotent way to re-sync them (ReActAgent works on a deep copy of the Toolkit, and Toolkit.updateToolGroups(..., false) is public API a host can call without touching SkillRegistry). In those cases the model now gets "already loaded and active" while the tools are actually still disabled, and nothing repairs it — the exact failure mode the doc comment on SkillBox.setSkillActive warns about.

Suggest keeping the dedup but still reconciling group state before returning, e.g. extract the group-activation part of activateSkill into a small ensureSkillToolGroupsActive(skillId) and call it on the short-circuit path. That preserves the token saving (no re-send of the markdown) while keeping the self-healing behavior.

[oss-maintainer]

[Warning] The escape hatch described in the comment above only covers individual resource paths, but the file that context compaction is most likely to drop is SKILL.md itself — it is the largest block in the tool result and the entry document that lists which resources even exist.

Once the skill is active, a model that has lost the body of SKILL.md has no way to get it back: asking for SKILL.md yields this one line, and it cannot guess the resource paths to ask for. The dedup therefore converts a token-waste problem into a potential unrecoverable-context-loss problem for long-running sessions, which is a first-class scenario for this framework.

Two cheap mitigations, either would be enough:

1. Make the notice actionable instead of terminal, e.g. append the resource listing that buildResourceNotFoundMessage already knows how to enumerate, so the model can re-fetch what it needs.
2. Track a per-transaction "content delivered" signal and reset it on compaction (or honor an explicit force/reload argument), so a genuine re-read is still possible. This is the flag the PR description deliberately avoided — worth reconsidering given (1) alone keeps the entry file itself unrecoverable.

[oss-maintainer]

[Info] Minor duplication: SkillBox.isSkillActive(String) (~line 206-213) already implements exactly this lookup against the same registeredSkills map with the same null handling. Adding a second copy in the registry means the two can drift.

Since the registry-level primitive is the cleaner place for it, consider having SkillBox.isSkillActive delegate here (return skillRegistry.isSkillActive(skillId);) so there is a single source of truth for "is this skill active". Public behavior of SkillBox is unchanged either way.

[oss-maintainer]

[Info] Nice red/green coverage for the token path. Two gaps that map directly to the two warnings above:

1. Re-activation after external deactivation — disable the skill's tool group (or call setSkillActive(id, false) then force the registry flag back), re-load SKILL.md, and assert the tool group is active again. That is the behavior the new guard changes, and it is currently untested.
2. Recovery after the active flag is cleared — assert that a load following SkillBox.setSkillActive(id, false) still returns the full markdown, which pins the intended boundary of the dedup.

Also worth noting: both tests drive path="notes.md" through the in-memory getResources() branch (step 1 of loadSkillResourceImpl), so the disk-fallback branch that also calls activateSkill is not exercised here.

[oss-maintainer]

Summary

Re-reviewed at 9fb29060. The follow-up commit addresses everything from the previous round: the dedup is now keyed on entryLoaded (entry actually delivered) instead of active, ensureSkillToolGroupsActive runs on both the normal and the short-circuit path, the notice lists the skill's resources so a model that lost content to compaction can re-fetch, and SkillBox.isSkillActive is delegated instead of duplicated. Test coverage is now substantial (first-load, resource-first, deactivation recovery, externally-disabled tool group, notice contents, disk fallback, non-dedup of resources).

One gap left, left as an inline comment below: the entryLoaded reset lives only in setSkillActive, so setAllSkillsActive(false) (public SkillBox.deactivateAllSkills(), documented as called at the start of each agent call) leaves a stale flag and the next load serves only the notice. Small, targeted change — no redesign needed, so this does not block a maintainer approve if you judge bulk deactivation out of scope.

Reviewed as correct

• isSkillEntryLoaded / setSkillEntryLoaded null-guard the registry lookup; unknown skill validation still happens before the dedup branch, so error behaviour is unchanged.
• entryLoaded defaults to false on a freshly registered skill, so the first load always returns the full entry.
• The notice keeps resources fetchable and calls out the recovery lever explicitly, which is the right shape for context-compaction resilience.

CI note (not attributable to this PR)

At this head build (ubuntu-latest) fails only on the Upload coverage reports to Codecov step (compile and tests succeeded) and build (windows-latest) was cancelled — neither is a code failure, and the ToolConfirmationCoordinatorTest breakage flagged in the previous review is gone from this run.

---

Automated review by github-manager-bot

[oss-maintainer]

entryLoaded is only reset on the single-skill deactivation path. setAllSkillsActive(false) — reached from the public SkillBox.deactivateAllSkills(), whose javadoc says it "is typically called at the start of each agent call to ensure a clean state" — still calls r.setActive(false) directly, so the flag survives a bulk deactivation. The next SKILL.md load then hits the dedup short-circuit and returns only the notice even though the entry document is no longer in the model's context; the notice's own recovery lever ("deactivating and reactivating the skill") doesn't help such a skill unless the host deactivates it individually. Tool-group state is still re-synced here, so this is about lost content, not lost tools. Consider mirroring the reset in setAllSkillsActive (registeredSkills.values().forEach(r -> { r.setActive(active); if (!active) r.setEntryLoaded(false); });), or routing bulk deactivation through setSkillActive so there is one place that owns the invariant, plus one bulk-deactivation test next to deactivationRecoversFullEntryLoad.

[chaucerj]

Thanks for both review rounds — everything is addressed.

Round 1 (9fb29060): extracted ensureSkillToolGroupsActive() for the short-circuit path; the notice enumerates resources and the dedup keys on an entryLoaded delivery flag cleared by deactivation; SkillBox.isSkillActive delegates to the registry; the two test gaps plus a disk-fallback resource-first case are covered.

Round 2 (cd72985f): good catch on setAllSkillsActive. Bulk deactivation now routes through setSkillActive so one method owns the deactivation invariants, with a deactivateAllSkills() test next to the single-skill case. Net semantics: the dedup now only applies within a single agent call, and every new turn re-sends the entry on first load.

Local: core suite 2347 tests, 0 failures.

CI note: the previous run's tests all passed on both platforms; ubuntu was marked red only by the tokenless Codecov upload step from the fork (fail_ci_if_error: true). Happy to push an empty retrigger if the latest run hits the same upload flake.

[codecov]

Codecov Report

❌ Patch coverage is 90.47619% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ava/io/agentscope/core/skill/SkillToolFactory.java	89.18%	0 Missing and 4 partials ⚠️
...io/agentscope/core/skill/EntryDeliveryTracker.java	91.30%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Summary

Re-reviewed the increment since my last pass (9fb29060 -> bbba7fad). The one gap I left open is closed and the added tests pin both the bulk-deactivation behaviour and the unknown-id guards, so this is an approve. On this head build (ubuntu-latest), Check License, Check Module Sync and codecov/patch are green; build (windows-latest) was still running when this review was posted, so the merge call is left to a maintainer once it finishes. CLA is signed.

What the increment does

• SkillRegistry.setAllSkillsActive now routes every id through setSkillActive, so SkillBox.deactivateAllSkills() — which runs at the start of each agent call — also clears entryLoaded. Previously only the single-skill path reset it, so a bulk deactivation could leave the next SKILL.md load returning just the dedup notice for content a fresh context never received.
• bulkDeactivationRecoversFullEntryLoad reproduces exactly that sequence (full load -> dedup notice -> deactivate all -> full document again), which is the right regression guard for the report in #1569.
• Activation behaviour is unchanged: setSkillActive only clears entryLoaded on the !active branch, so routing the active == true case through it is equivalent to the old direct r.setActive(true).
• Iterating registeredSkills.keySet() while only mutating the RegisteredSkill value means the shared map is not structurally touched, so there is no concurrent-modification risk here.
• SkillRegistryGuardTest covers the unknown-id defaults of isSkillActive / isSkillEntryLoaded and the tolerate-and-ignore behaviour of both setters — guards the dedup path relies on that had no test before.

---

Automated review by github-manager-bot

[gmjneko]

I have a few concerns about the scope and lifecycle of the new state, so I’d like to discuss them with you before we proceed.

1. In the current v2 design, one ReActAgent can serve multiple (userId, sessionId) pairs, but entryLoaded is stored in the RegisteredSkill held by a potentially shared SkillBox.

   The discussion mentions that deactivateAllSkills() runs at the start of every agent call, but I couldn’t find a production call site that guarantees this. Without that reset, even sequential calls from different sessions could incorrectly share the flag.

   Would it be better to keep this flag in call-scoped or session-scoped state instead, depending on the intended lifecycle of the loaded entry?

2. The current change only covers the core SkillBox / SkillToolFactory path, but HarnessAgent uses HarnessSkillMiddleware, SkillRuntime, and SkillLoadTool, rather than the core SkillBox / SkillToolFactory loading path. It therefore does not have this specific entryLoaded sharing issue, but it also does not receive the deduplication behavior introduced here.

   If possible, for consistency, we should also apply the same call or session-scoped state model to the HarnessAgent path.

[chaucerj]

Both points are well taken. On point 1 you're right that entryLoaded on RegisteredSkill was agent-global state, and I verified your second observation too: deactivateAllSkills() has no production call site today, so nothing resets the flag between conversations — a real cross-session leak (the new regression test sessionsDoNotShareEntryDelivery fails on the previous head: session B received the notice for content its context never got).

Implemented in 7adbe7f0 — delivery tracking is now scoped to the conversation, not the agent:

• entryLoaded is gone from the registry. SkillToolFactory tracks delivery keyed by the tool call's RuntimeContext (userId::sessionId), with one shared fallback bucket for context-less direct tool use — the same conversation-identity model HarnessSkillMiddleware's IsolationScope uses for .skills-cache.
• Token semantics: the entry is paid exactly once per context that needs it — repeat loads within a session dedup; every new session re-sends once; no session can suppress another's first load.
• Sub-agents are covered by construction: DefaultAgentManager.invokeAgent always assigns a fresh child sessionId (propagating only userId), so a spawned agent's scope key differs from its parent's and its first entry load receives the full document — matching its fresh context.
• Shared-conversation gateways are covered the same way: HarnessGateway derives one stable sessionId per room/thread/team gate and passes the sender's userId. Because the delivery scope uses the same (userId, sessionId) pair the AgentState slot uses, delivery automatically follows whatever identity policy the host chooses — per-sender state slots get per-sender delivery; a channel that maps no userId shares one slot and one delivery for the whole room. No new knob is needed: the host already expresses "who shares a context" through the identity it puts in RuntimeContext.
• Everything from the approved rounds is unchanged: group re-sync on the short-circuit path, the actionable notice, the isSkillActive delegation. Core suite: 2347 tests, 0 failures.

On point 2 (harness path): agreed SkillRuntime/SkillLoadTool is a separate loading model — it neither has this bug nor gets the dedup today. Mirroring conversation-scoped delivery there (reusing its own scopeKeyFor) is straightforward; this PR is already 6 commits through three review rounds, so I'd keep it scoped to the core path and file the harness dedup as a follow-up unless you'd rather have both here — your call.

[oss-maintainer]

Summary

Delta re-review of the increment since my last pass (bbba7fad -> 7adbe7f0). The session-scoping fix itself is right and I agree with the diagnosis: entryLoaded on RegisteredSkill was agent-global state while one agent serves many (userId, sessionId) pairs, and dropping the deactivateAllSkills()-based reset is correct now that the tracking is per conversation. On this head build (ubuntu-latest), build (windows-latest), Check License, Check Module Sync and codecov/patch are all green and the CLA is signed, so nothing here blocks on CI.

What keeps this at a comment rather than a second approve: the replacement state is now per-session and never evicted, and the increment removes the last way to get an entry document back inside a live session — the two failure modes are detailed inline on SkillToolFactory.java:61 and :235. The null-session scope collapse and the non-atomic claim in the dedup path are smaller but sit in the same code, so they are cheap to fold into the next push.

Findings

• [Warning] SkillToolFactory.java:61 — entryDeliveredByScope accumulates one scope entry per conversation for the lifetime of the factory with no eviction; the previous registry-side flag was bounded by registered skills.
• [Warning] SkillToolFactory.java:235 — per-session-forever tracking plus removal of the deactivation reset leaves a session that lost SKILL.md to depth compaction or tool-result eviction no way to recover the entry body without starting a new session.
• [Info] SkillToolFactory.java:198 — "*" placeholders fold every session with a null sessionId under one user into a single dedup bucket.
• [Info] SkillToolFactory.java:245 — check-then-act between isEntryDelivered and markEntryDelivered can let two parallel same-batch loads both return the full document, which is exactly the case #1569 targets.

Suggestions

The two warnings have small, local fixes: bound or evict the scope map (LRU on scopes, or clear a scope when it is deactivated / when the session is cleared), and expose an in-session re-delivery lever (a reload flag on the tool, or clearing the scope bucket from the compaction path that drops the tool result). Marking the claim atomic — markEntryDelivered returning Set.add(skillId) and short-circuiting on false — covers the fourth point without restructuring the load path.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] entryDeliveredByScope has no eviction path, so it grows with the number of conversations an agent has ever served. The factory is long-lived — ReActAgent.Builder.configureSkillBox() binds one SkillBox per agent, and DynamicSkillMiddleware deliberately reuses currentSkillBox while the skill-view signature is unchanged (signature.equals(lastSignature)) — so a Set is retained per userId::sessionId scope forever, while the commit's whole premise is that one agent serves many sessions. Nothing removes a scope when a session ends, is cleared, or when a skill is unregistered, so this is a slow leak proportional to distinct sessions x delivered skills. Since the registry-side flag it replaces was bounded by registered skills, could this map be bounded too (an LRU cap on scopes, or eviction driven by the existing session lifecycle / setSkillActive(id, false))? A regression test that loads in 10k scopes and asserts the map size would pin whichever contract is chosen.

[oss-maintainer]

[Warning] Per-session-forever tracking removes the last recovery lever for an entry document that left the context window. The previous revision made deactivation end the delivery window and the notice documented that; this commit deletes both (the setSkillActive reset and the deactivationRecoversFullEntryLoad / bulkDeactivationRecoversFullEntryLoad tests), so within a live session a SKILL.md that is no longer in context can never be re-delivered — the notice now only says "a new session receives the full SKILL.md". That is reachable here: harness compaction drops old bodies by depth (CompactionConfig/keepTokens), and load_skill_through_path is not in ToolResultEvictionConfig.DEFAULT_EXCLUDED_TOOLS, so a large entry document is rewritten to a large_tool_results/ placeholder that a later compaction pass can still drop. A long-running session that loses the body is then stuck with resource files only. Worth keeping an in-session escape hatch: a reload/force argument on the tool, or clearing the scope bucket from the compaction path that evicts the tool result.

[oss-maintainer]

[Info] A null sessionId silently collapses a whole user's conversations into one dedup bucket. "*" as the placeholder means (u1, null) and any other (u1, null) call share a scope, which is the same cross-session suppression this commit removes — just narrowed to the user dimension. Real tool paths should carry a session id (buildMergedRuntimeContext(rc)), so this may be unreachable in production, but the safe default for an unidentifiable conversation is to not dedup. Suggest returning NO_CONTEXT_SCOPE (or better: skipping the dedup entirely and always delivering the full entry) when sessionId == null, and reserving the wildcard form only for the genuinely context-less direct-tool-use case the fallback bucket is meant for.

[oss-maintainer]

[Info] isEntryDelivered -> markEntryDelivered is a check-then-act pair, so a same-batch duplicate can slip through. The scenario from #1569 is the model emitting several load_skill_through_path(SKILL.md) calls in one reply, and ToolkitConfig defaults to parallel = true; the skill tool is a plain AgentTool (not a ToolBase), so ToolExecutor.isConcurrencySafe treats it as safe and the calls can overlap. Two overlapping loads both observe "not delivered" and both return the full document — the duplicate-token case the short-circuit exists to prevent (harmless, self-limiting to one extra copy, but it undercuts the point of the dedup). An atomic claim would close it: make markEntryDelivered return Set.add(skillId)'s boolean and short-circuit on false.

[oss-maintainer]

Summary

Round 4 (df68fcf3) — the LRU-bounded tracker, atomic claims, the explicit reload lever and the null scope for a user without a session all address the previous round, and the new tests pin the behaviors that were missing (externally disabled tool group, bounded scopes, cross-session independence, no-session dedup). The remaining concerns are that the reload escape hatch is not actually reachable/robust as written: it is missing from the tool parameter schema and is parsed strictly as a JSON boolean. Nothing here changes the design, so flagging as comments rather than changes requested.

Findings

• [Warning] SkillToolFactory.java:117 — reload is described to the model but never declared in getParameters(), so schema-driven emitters and strict-mode providers will not pass it.
• [Warning] SkillToolFactory.java:173 — Boolean.TRUE.equals(...) silently ignores "true"/1, so a model that does send the flag can still get the dedup notice.
• [Info] SkillToolFactory.java:190 — duplicated/stale Javadoc block left above scopeOf.
• [Info] EntryDeliveryTracker.java:76 — mark() does not promote LRU recency, so the "access-ordered" wording is slightly optimistic.

CI (Check License, Check Module Sync, build (ubuntu-latest), build (windows-latest), codecov/patch) is green and license/cla is signed.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] reload is advertised to the model in the tool description, but it is never declared in getParameters() (~line 124), which still exposes only skillId and path. In practice a model emits arguments from the JSON schema, not from prose, so the recovery lever added for compaction is likely unreachable for many providers — and on providers that enable strict tool-schema mode (strict: true, e.g. via ToolSchema.getStrict() in the OpenAI formatter), an undeclared argument can be dropped or rejected outright.

Suggest declaring it explicitly:

"reload",
        Map.of(
                "type", "boolean",
                "description",
                        "Set true on a SKILL.md load to force re-sending the full"
                                + " document, e.g. after context compaction removed it.")

(required stays [skillId, path].)

[oss-maintainer]

[Warning] Boolean.TRUE.equals(...) only matches a real JSON boolean. Because the parameter is not type-constrained by the schema (see the comment on the description line), it is very common for models to send "reload": "true" (string) or 1 — those fall through to the dedup path and the model gets the one-line notice instead of the document it explicitly asked to reload, which is the exact failure mode this lever exists to avoid, and is silent.

skillId/path above are handled the same way (they are schema-typed strings, so casting is safe); for an optional boolean a lenient parse would be more robust, e.g.:

Object reloadArg = input.get("reload");
boolean reload = reloadArg instanceof Boolean b
        ? b
        : reloadArg != null && Boolean.parseBoolean(String.valueOf(reloadArg));

[oss-maintainer]

[Info] Two consecutive Javadoc blocks now sit on scopeOf: the pre-existing one ("calls without a runtime context share one fallback bucket") plus the new one describing the null semantics. The first is now stale — it no longer mentions that a user-without-session returns null — and only the block directly above the declaration is attached by the Javadoc tool, so the older one is a dangling comment that reads as a contradiction.

Suggest deleting the first block (lines 188-193) and keeping the new <ul> version as the single source of truth.

[oss-maintainer]

[Info] Minor: the class javadoc advertises an access-ordered LRU, but computeIfAbsent on an existing key is not treated as an access by LinkedHashMap, so mark(scope, skillId) (the reload=true path) neither creates nor promotes the entry's recency. A scope that is repeatedly reloaded without a fresh tryClaim can therefore be evicted while quieter scopes survive.

Impact is negligible — eviction is safe-directional here (a forgotten scope only re-delivers the entry) — so this is a nit, not a blocker. Either promote explicitly (deliveredByScope.get(scope) after computeIfAbsent) or drop "access-ordered" from the comment in favour of "insertion-ordered with eldest eviction".

[chaucerj]

All four points from round 5 are addressed in 2371cda8:

• W1 (missing schema declaration): reload is now declared in getParameters() as an optional boolean property, so schema-driven models can emit it and strict tool-schema mode will not drop it. Good catch — the schema edit had silently failed to apply during the earlier rework; it is now verified in the file directly.
• W2 (strict parsing): parsed via lenientBoolean — a real boolean, a non-zero number, and the parseBoolean string form all count — with a regression test feeding "true" and 1 that re-delivers the full entry while the dedup is armed.
• Info (dangling javadoc): removed; the <ul> block is now the single javadoc attached to scopeOf.
• Info (LRU recency): mark() now promotes the scope's recency explicitly after computeIfAbsent, so reload-heavy scopes are not evicted before quieter ones.

Local: core suite 2354 tests, 0 failures; a behavior trace covering all identity shapes (same/cross session, reload in all three forms, user-without-session, shared room, context-less) passes against this head.

[oss-maintainer]

Summary

Re-review at 2371cda (my sixth pass on this PR). Every finding from the previous round is closed: reload is now declared in getParameters() so schema-driven and strict-tool-schema providers can actually emit it, the value is parsed leniently instead of Boolean.TRUE.equals(...), the duplicated Javadoc block above scopeOf is gone, and the LRU-recency wording in EntryDeliveryTracker.mark() is addressed. The added reloadAcceptsLenientForms test pins both shapes a model realistically sends.

build (ubuntu-latest), build (windows-latest), Check License, Check Module Sync and codecov/patch are all green on this head and license/cla is signed. Two Info-level nits below, neither of which changes behaviour for the common case, so this is an approve — the merge call stays with the maintainers.

Reviewed as correct

• reload is added to properties but stays out of required, so existing callers and previously recorded tool calls are unaffected.
• The Boolean branch in lenientBoolean short-circuits first, so a well-formed true never reaches the string path.
• mark() after a reload=true keeps the scope marked, so a reload does not hand the next duplicate load a free second full send.
• Scope derivation and the null (user-without-session) case are unchanged from the previously approved behaviour, so this increment is confined to the argument plumbing.

Verification note

The mark() recency fix rests on a claim that does not hold for the JDK: HashMap.computeIfAbsent calls afterNodeAccess(e) on the existing-mapping path (if (old != null && (oldValue = old.value) != null) { afterNodeAccess(old); return oldValue; }), and LinkedHashMap.afterNodeAccess is exactly the hook that moves the entry to the tail under accessOrder. Detail in the inline comment.

---

Automated review by github-manager-bot

[oss-maintainer]

[Info] This line is a no-op, and the comment just above it describes the opposite of what the JDK does.

HashMap.computeIfAbsent calls afterNodeAccess(e) on the existing-mapping path (OpenJDK, src/java.base/share/classes/java/util/HashMap.java: if (e != null && (old = e.value) != null) { ... afterNodeAccess(e); return old; }), and LinkedHashMap.afterNodeAccess is precisely the hook that relinks the entry to the tail when accessOrder == true. So the computeIfAbsent on line 74 already promoted the scope's recency, and my previous round's suggestion here was wrong — sorry for the churn.

Suggest dropping the statement and replacing the two comment lines with nothing (or with a pointer for the next reader if you'd rather keep the explicit promotion as documentation). Left as-is it reads like a call with a discarded return value, which is the kind of line a future cleanup silently deletes.

[oss-maintainer]

[Info] One asymmetry left in the lenient parser: the Number branch handles 1, but a string "1" falls through to Boolean.parseBoolean("1"), which is false. Providers that stringify every scalar would send "1" for a flag, and that model still silently gets the dedup notice for the document it explicitly asked to reload — the exact failure mode this helper was added to prevent.

Cheap to close, e.g.:

String s = value.toString().trim();
return "1".equals(s) || Boolean.parseBoolean(s);

Not a blocker and the "true" / 1 matrix in reloadAcceptsLenientForms already covers the common cases; mentioning it because java.util.List.of("true", 1) is one value away from covering both shapes.