Skip to content

fix: add isInvalidAgentIdFormat guard to runMemoryReflection (Issue #686)#722

Open
jlin53882 wants to merge 3 commits intoCortexReach:masterfrom
jlin53882:fix/issue-686-v2
Open

fix: add isInvalidAgentIdFormat guard to runMemoryReflection (Issue #686)#722
jlin53882 wants to merge 3 commits intoCortexReach:masterfrom
jlin53882:fix/issue-686-v2

Conversation

@jlin53882
Copy link
Copy Markdown
Contributor

@jlin53882 jlin53882 commented Apr 29, 2026

Summary

Adds a minimal isInvalidAgentIdFormat() guard to the runMemoryReflection command hook (Issue #686), consistent with other hook sites in the plugin. Only the #686 fix — no scope creep.

Changes

index.ts

runMemoryReflection hook guard:

const sourceAgentId = parseAgentIdFromSessionKey(sessionKey) || "main";
// Guard: skip if agentId is invalid format — consistent with other hook sites (Issue #686)
// NOTE: Layer 3 (declaredAgents) intentionally omitted — scope discipline for #686;
// Layer 3 is a separate concern for a follow-up PR (not a technical limitation).
if (isInvalidAgentIdFormat(sourceAgentId)) {
  api.logger.debug?.(`memory-reflection: command hook skipped \u2014 invalid agentId '${sourceAgentId}'`);
  return;
}

Exported: isInvalidAgentIdFormat (public, for testability)

New test: test/agentid-validation-686.test.mjs

  • 14 test cases, all pass
  • Directly imports the production isInvalidAgentIdFormat export (not a skipped helper)
  • Covers Layer 1 (empty/undefined), Layer 2 (numeric = chat_id), signature compat with Layer 3 param
  • Includes integration test for runMemoryReflection with numeric sessionKey

CI manifest

  • Added test/agentid-validation-686.test.mjs to scripts/ci-test-manifest.mjs (required for CI to execute the new test)

Scope Discipline

What was intentionally NOT included Reason
serialCooldownMs configurable cooldown Out of scope for #686
excludeAgents pattern matching Out of scope for #686
declaredAgents Layer 3 in guard call Scope discipline: #686 fix is Layer 1+2 only (numeric chat_id); Layer 3 is a separate concern for a follow-up PR
src/reflection-store.ts mass reformatter Not in #686 scope
openclaw.plugin.json schema changes Not in #686 scope
Other hook site guards Out of scope for #686

Layer 3 Note

Layer 3 (declaredAgents Set validation against openclaw.json agents.list) is intentionally omitted from the guard call in this PR. This is a scope discipline decision — not a technical limitation. The isInvalidAgentIdFormat function body does contain Layer 3 logic, but this PR's specific guard call passes only sourceAgentId (Layer 1+2) to maintain a minimal fix surface area. A follow-up PR should address Layer 3 as a separate concern once the API integration path is confirmed with maintainers.

Testing

node --test test/agentid-validation-686.test.mjs
# → 14 tests, 0 failures, 0 skipped

Existing tests still pass:

node --test test/reflection-bypass-hook.test.mjs  # 4 pass
node --test test/scope-access-undefined.test.mjs  # 29 pass

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c0a00cadda

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread index.ts Outdated
// We skip them rather than block all numeric IDs to avoid false positives.
function isChatIdBasedAgentId(agentId: string | undefined): boolean {
if (!agentId) return false;
return /^\d+$/.test(agentId);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Allow numeric agent IDs in reflection path

The new guard marks every digits-only agentId as invalid (/^\d+$/), so runMemoryReflection now exits early for sessions like agent:123:.... That is a behavioral regression because this codebase otherwise accepts numeric agent IDs (the canonical parser in src/scopes.ts only rejects empty and reserved bypass IDs), meaning valid agents can silently lose reflection generation and memory updates after this commit. Consider restricting this check to known chat-id extraction failures (or gating it with declared-agent validation) instead of blocking all numeric IDs.

Useful? React with 👍 / 👎.

jlin53882 added 2 commits May 3, 2026 20:02
…ortexReach#686)

Minimal fix addressing Issue CortexReach#686 without the scope creep of PR CortexReach#687.

Changes:
- Add isInvalidAgentIdFormat() guard (Layer 1: empty/undefined, Layer 2: pure numeric = chat_id)
  to runMemoryReflection command hook, consistent with other hook sites.
- Export isInvalidAgentIdFormat for testability.
- Add integration test (agentid-validation-686.test.mjs) that directly imports
  the production export — NOT skipped/helper-only.

Layer 3 (declaredAgents Set validation) is intentionally omitted from this fix.
The plugin config does not carry agents.list from openclaw.json root config;
implementing Layer 3 requires proper api.config.agents access from the plugin API,
which is not reliably available in all install configurations. A follow-up PR
should address Layer 3 once the correct API integration path is confirmed.

No schema changes, no CI manifest changes, no serialCooldownMs,
no excludeAgents — focused scope on the CortexReach#686 guard only.
Add test/agentid-validation-686.test.mjs to core-regression CI group
and update verify-ci-test-manifest baseline to include it.
@jlin53882 jlin53882 force-pushed the fix/issue-686-v2 branch from 4587aac to 140493d Compare May 3, 2026 12:11
- Add scope note to runMemoryReflection guard: Layer 3 omission is intentional
  scope discipline for CortexReach#686, not a technical limitation
- Update test Layer 3 describe block to clarify 'guard call omits param'
- Fix misleading comment: 'Layer 3 is no-op in this guard call' (not impl)
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