-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(messaging): retain stopped channel provider bindings #10047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6f8a969
6897e5d
bb7dfcf
1c87c6b
1c73738
c3f59fb
ad684ba
0e45894
34af94e
a42d03d
c162cb1
d365811
40a0510
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import fs from "node:fs"; | |
| import os from "node:os"; | ||
| import path from "node:path"; | ||
| import { setTimeout as sleep } from "node:timers/promises"; | ||
| import { loadAgent } from "../../../src/lib/agent/defs"; | ||
| import { shellQuote } from "../../../src/lib/core/shell-quote"; | ||
| import { readSandboxBaseImageResolutionMetadata } from "../../../src/lib/sandbox-base-image"; | ||
| import { buildAvailabilityProbeEnv } from "../fixtures/availability-env.ts"; | ||
|
|
@@ -14,6 +15,7 @@ import { assertExitZero as expectExitZero } from "../fixtures/clients/command.ts | |
| import { type HostCliClient, resultText } from "../fixtures/clients/index.ts"; | ||
| import { validateSandboxName } from "../fixtures/clients/sandbox.ts"; | ||
| import { expect, test } from "../fixtures/e2e-test.ts"; | ||
| import { expectSandboxProviderAttachment } from "../fixtures/gateway-providers.ts"; | ||
| import { | ||
| readJsonFileOr, | ||
| restoreFile, | ||
|
|
@@ -79,7 +81,6 @@ process.env.NEMOCLAW_CLI_BIN ??= CLI_ENTRYPOINT; | |
| // local NemoClaw registry/session state, and `nemoclaw <name> rebuild --yes`. | ||
| // Literal interactive issue #3025 reproduction paths (`hermes rebuild`, modal | ||
| // prompt, and `Y` confirmation) remain outside this Vitest migration. | ||
| const HERMES_MANIFEST = path.join(REPO_ROOT, "agents", "hermes", "manifest.yaml"); | ||
| const OLD_HERMES_VERSION = `v${REBUILD_HERMES_OLD_BASE_FIXTURE.hermesCalver}`; | ||
| const OLD_HERMES_REGISTRY_VERSION = OLD_HERMES_VERSION.slice(1); | ||
| const STALE_BASE_REBUILD = process.env.NEMOCLAW_HERMES_STALE_BASE_REBUILD_E2E === "1"; | ||
|
|
@@ -278,21 +279,10 @@ function fail(message: string): never { | |
| } | ||
|
|
||
| function expectedHermesVersion(): string { | ||
| const manifest = fs.readFileSync(HERMES_MANIFEST, "utf8"); | ||
| const match = manifest.match(/^expected_version:\s*"?([^"\n]+)"?/m); | ||
| expect(match?.[1], `Could not parse expected Hermes version from ${HERMES_MANIFEST}`).toEqual( | ||
| expect.any(String), | ||
| return ( | ||
| loadAgent("hermes").expectedVersion ?? | ||
| fail("Hermes manifest must declare expected_version for live rebuild coverage") | ||
| ); | ||
| return match![1].trim(); | ||
| } | ||
|
|
||
| function expectEqual(actual: string | undefined, expected: string, message: string): void { | ||
| switch (actual === expected) { | ||
| case true: | ||
| return; | ||
| default: | ||
| throw new Error(message); | ||
| } | ||
| } | ||
|
|
||
| async function bestEffortPrecleanHermesResources( | ||
|
|
@@ -1225,6 +1215,16 @@ test(STALE_BASE_REBUILD | |
| /Hermes gateway (?:restarted and verified|recovered) after state restore/u, | ||
| ); | ||
| await waitForSandboxReady(host, apiKey, activeOpenshellBin, "phase-6-post-rebuild"); | ||
| await expectSandboxProviderAttachment( | ||
| sandbox, | ||
| SANDBOX_NAME, | ||
| `${SANDBOX_NAME}-discord-bridge`, | ||
| "present", | ||
| { | ||
| artifactName: "phase-6-post-rebuild-provider-attachments", | ||
| env: testEnv(apiKey), | ||
| }, | ||
| ); | ||
|
Comment on lines
+1218
to
+1227
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline test/e2e/live/rebuild-hermes.test.ts --items all --type function
ast-grep outline test/e2e/fixtures/gateway-providers.ts --items all --type function
sed -n '1180,1240p' test/e2e/live/rebuild-hermes.test.ts
sed -n '65,88p' test/e2e/fixtures/gateway-providers.ts
rg -n -C 5 \
'provider.*(create|attach)|sandbox provider|discord-bridge|provider(Id|Name)|token.*(upsert|create)' \
src testRepository: NVIDIA/NemoClaw Length of output: 50372 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- target test imports and provider assertions ---'
sed -n '1,35p' test/e2e/live/rebuild-hermes.test.ts
sed -n '1188,1235p' test/e2e/live/rebuild-hermes.test.ts
sed -n '1260,1320p' test/e2e/live/rebuild-hermes.test.ts
printf '%s\n' '--- helper implementation ---'
sed -n '1,115p' test/e2e/fixtures/gateway-providers.ts
printf '%s\n' '--- helper call sites in the target test ---'
rg -n -C 12 'expectSandboxProviderAttachment|phase-6-post-rebuild-provider-attachments|discord-bridge' test/e2e/live/rebuild-hermes.test.ts
printf '%s\n' '--- provider identity and rebuild contract references ---'
rg -n -C 6 \
'provider (get|list|create)|providerId|providerHandle|reused-destination-owned|reuse.*provider|provider.*reuse|attachment' \
test/e2e src/lib/actions/sandbox/snapshot \
-g '*.ts' -g '*.md' -g '*.yml' -g '*.yaml'Repository: NVIDIA/NemoClaw Length of output: 50372 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact change in the target test and helper ---'
git diff --unified=30 d92acac1c40364702eaae92a169a2b06d1bfda4b c162cb188cee8a0d76d7156e7daf86fc64afd21e -- \
test/e2e/live/rebuild-hermes.test.ts test/e2e/fixtures/gateway-providers.ts
printf '%s\n' '--- provider setup and pre-rebuild assertions ---'
sed -n '900,1020p' test/e2e/live/rebuild-hermes.test.ts
sed -n '1020,1110p' test/e2e/live/rebuild-hermes.test.ts
printf '%s\n' '--- rebuild provider handling ---'
rg -n -C 10 \
'managed.*provider|provider.*managed|providerName|provider.*disposition|reused-destination-owned|sandbox.*provider|provider.*sandbox' \
src/lib/actions/sandbox src/lib/actions -g '*.ts' | head -n 500
printf '%s\n' '--- repository documentation for provider identity/output ---'
rg -n -C 8 \
'provider (get|list|create|update)|provider name|provider identity|immutable.*provider|exact.*provider|provider.*reuse' \
README.md docs test src -g '*.md' -g '*.ts' -g '*.yml' -g '*.yaml' | head -n 500
printf '%s\n' '--- static behavioral witness for the helper ---'
python3 - <<'PY'
from pathlib import Path
import re
helper = Path("test/e2e/fixtures/gateway-providers.ts").read_text()
target = Path("test/e2e/live/rebuild-hermes.test.ts").read_text()
body = re.search(
r"export async function expectSandboxProviderAttachment\(.*?\n\}",
helper,
re.S,
)
assert body, "helper definition not found"
body = body.group(0)
print("helper_runs:", re.findall(r'sandbox\.openshell\(\s*(\[[^\]]+\])', body, re.S)[0].replace("\n", " "))
print("helper_extracts:", "split(/\\s+/u)" in body)
print("helper_asserts_name_membership:", "toContain(providerName)" in body)
print("helper_reads_stable_id:", bool(re.search(r'provider(?:Id|Handle)|\bid\b', body)))
print("target_has_pre_rebuild_provider_create_or_update:",
'provider create --name "$DISCORD_PROVIDER"' in target and
'provider update "$DISCORD_PROVIDER"' in target)
print("target_has_post_rebuild_provider_list:",
'expectSandboxProviderAttachment' in target and
'phase-6-post-rebuild-provider-attachments' in target)
PYRepository: NVIDIA/NemoClaw Length of output: 50372 Assert reuse of the exact Discord provider after rebuild.
🤖 Prompt for AI AgentsSource: Path instructions |
||
|
|
||
| const backupPathText = rebuildOutput.match(/^\s*Backup:\s+(.+)$/mu)?.[1]?.trim(); | ||
| const rebuildBackupPath = backupPathText | ||
|
|
@@ -1301,11 +1301,10 @@ test(STALE_BASE_REBUILD | |
| expectExitZero(hermesVersion, "Hermes version after rebuild"); | ||
| const hermesVersionText = resultText(hermesVersion); | ||
| const actualHermesVersion = hermesVersionText.match(/v(\d+\.\d+\.\d+)/)?.[1]; | ||
| expectEqual( | ||
| expect( | ||
| actualHermesVersion, | ||
| expectedVersion, | ||
| `Hermes version output did not include expected release ${expectedVersion}: ${hermesVersionText}`, | ||
| ); | ||
| ).toBe(expectedVersion); | ||
| await cronRestore.verify(rebuildOutput, rebuildBackupPath); | ||
| await cronRestore.verifyStrandedGateRecovery(); | ||
| const restoredKanbanDatabase = await host.command( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict NemoClaw detection to command position.
At Line [369], the regex matches
nemoclawinside shell comments and quoted arguments. For example,# nemoclaw --exampleandecho "nemoclaw --example"enter this branch.emit_flagsthen records--examplefrom unrelated shell text, which can produce a false flag-drift failure.Skip shell comments and require the
nemoclawtoken in command position before callingemit_flags. Use a shell-aware tokenizer if wrapped command forms must remain supported.This follows the PR objective that flags from unrelated shell commands are ignored.
Suggested fix
if ($in_fence) { # Ignore flags belonging to shell tools shown alongside the # CLI, while preserving flags on multiline NemoClaw examples. - if ($in_nemoclaw_command || /(?:^|\s)(?:\$\$)?nemoclaw(?:\s|$)/) { + next if /^\s*`#/`; + if ($in_nemoclaw_command || /^\s*(?:\$\s+|\$\$)?nemoclaw(?:\s|$)/) { emit_flags($_); $in_nemoclaw_command = /\\\s*$/ ? 1 : 0; }📝 Committable suggestion
🤖 Prompt for AI Agents