refactor(e2e): add a typed target evidence API - #6359
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a typed target evidence API in the E2E fixture layer, adds normalization/validation coverage, and migrates live E2E specs from direct ChangesTyped Target Evidence API
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/live/channels-stop-start-helpers.ts (1)
412-542: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmit target completion evidence here
declare()only writestarget.json; this helper never callsartifacts.target.complete(...), sotarget-result.jsonis never produced for this target.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/live/channels-stop-start-helpers.ts` around lines 412 - 542, The target is declared in runChannelsStopStartTarget but never marked complete, so target-result.json is missing. Add a call to artifacts.target.complete(...) at the end of runChannelsStopStartTarget after the final channel/start rebuild assertions and before the function returns, using the same target metadata already established by artifacts.target.declare.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/fixtures/artifacts.ts`:
- Around line 24-60: Contract normalization in normalizeTargetEvidence is being
applied to both metadata and result payloads, which can accidentally rewrite or
reject unrelated extension fields on TargetResult. Update
normalizeTargetEvidence so the contract/contacts singular-vs-plural validation,
normalization, and delete logic only runs for kind === "metadata", while result
payloads keep their extension fields untouched except for the existing
status/default runner handling.
---
Outside diff comments:
In `@test/e2e/live/channels-stop-start-helpers.ts`:
- Around line 412-542: The target is declared in runChannelsStopStartTarget but
never marked complete, so target-result.json is missing. Add a call to
artifacts.target.complete(...) at the end of runChannelsStopStartTarget after
the final channel/start rebuild assertions and before the function returns,
using the same target metadata already established by artifacts.target.declare.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5631f805-e73d-4b7e-8a85-6833e691c0ad
📒 Files selected for processing (66)
test/e2e/fixtures/artifacts.tstest/e2e/live/agent-turn-latency.test.tstest/e2e/live/bedrock-runtime-compatible-anthropic.test.tstest/e2e/live/brave-search.test.tstest/e2e/live/channels-add-remove.test.tstest/e2e/live/channels-stop-start-helpers.tstest/e2e/live/cloud-inference.test.tstest/e2e/live/cloud-onboard.test.tstest/e2e/live/common-egress-agent.test.tstest/e2e/live/concurrent-gateway-ports.test.tstest/e2e/live/credential-migration.test.tstest/e2e/live/credential-sanitization.test.tstest/e2e/live/cron-preflight-inference-local.test.tstest/e2e/live/dashboard-remote-bind.test.tstest/e2e/live/device-auth-health.test.tstest/e2e/live/diagnostics.test.tstest/e2e/live/docs-validation.test.tstest/e2e/live/double-onboard.test.tstest/e2e/live/full-e2e.test.tstest/e2e/live/gateway-guard-recovery.test.tstest/e2e/live/gateway-health-honest.test.tstest/e2e/live/gpu-double-onboard.test.tstest/e2e/live/gpu-e2e.test.tstest/e2e/live/hermes-discord.test.tstest/e2e/live/hermes-e2e.test.tstest/e2e/live/hermes-gpu-startup.test.tstest/e2e/live/hermes-inference-switch.test.tstest/e2e/live/hermes-root-entrypoint-smoke.test.tstest/e2e/live/hermes-sandbox-secret-boundary.test.tstest/e2e/live/hermes-slack-e2e-helpers.tstest/e2e/live/inference-routing.test.tstest/e2e/live/issue-2478-crash-loop-recovery.test.tstest/e2e/live/issue-4434-tui-unreachable-inference.test.tstest/e2e/live/issue-4462-scope-upgrade-approval.test.tstest/e2e/live/jetson-nvmap-gpu.test.tstest/e2e/live/kimi-inference-compat.test.tstest/e2e/live/launchable-smoke.test.tstest/e2e/live/messaging-compatible-endpoint.test.tstest/e2e/live/model-router-provider-routed-inference.test.tstest/e2e/live/network-policy.test.tstest/e2e/live/ollama-auth-proxy.test.tstest/e2e/live/onboard-negative-paths.test.tstest/e2e/live/onboard-repair.test.tstest/e2e/live/openclaw-discord-pairing.test.tstest/e2e/live/openclaw-inference-switch.test.tstest/e2e/live/openclaw-plugin-runtime-exdev.test.tstest/e2e/live/openclaw-skill-cli.test.tstest/e2e/live/openclaw-slack-pairing.test.tstest/e2e/live/openclaw-tui-chat-correlation.test.tstest/e2e/live/openshell-version-pin.test.tstest/e2e/live/registry-targets.test.tstest/e2e/live/runtime-overrides.test.tstest/e2e/live/sandbox-operations.test.tstest/e2e/live/sandbox-rlimits-connect.test.tstest/e2e/live/sandbox-survival.test.tstest/e2e/live/sessions-agents-cli.test.tstest/e2e/live/shields-config.test.tstest/e2e/live/skill-agent.test.tstest/e2e/live/snapshot-commands.test.tstest/e2e/live/spark-install.test.tstest/e2e/live/state-backup-restore.test.tstest/e2e/live/telegram-injection.test.tstest/e2e/live/token-rotation.test.tstest/e2e/live/ubuntu-repo-cli-smoke.test.tstest/e2e/live/upgrade-stale-sandbox.test.tstest/e2e/support/e2e-target-evidence.test.ts
2fa8843 to
b655d1f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/support/e2e-target-evidence.test.ts (1)
75-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMigration-completeness check is inherently source-text based.
This regex scan for
.writeJson("target...json")proves the superseded path is unreachable intest/e2e/live, which satisfies the migration-test requirement, though it relies on source-text matching rather than observable behavior. Since it directly implements the PR's stated acceptance criterion (a repo scan rejecting direct target evidence writes), this is acceptable as-is; just flagging the trade-off for awareness — a brittle regex could miss writes via aliased/renamed methods or computed filenames.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/support/e2e-target-evidence.test.ts` around lines 75 - 83, The migration-completeness check in e2e-target-evidence.test.ts is brittle because it relies on a regex over source text to detect direct target evidence writes. Update the test around liveTypescriptFiles() to use a more robust source-level check for writeJson calls on target/target-result filenames, or otherwise explicitly document and constrain the accepted aliasing/computed-name cases so the invariant remains enforceable even if the method is renamed or wrapped.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e/support/e2e-target-evidence.test.ts`:
- Around line 75-83: The migration-completeness check in
e2e-target-evidence.test.ts is brittle because it relies on a regex over source
text to detect direct target evidence writes. Update the test around
liveTypescriptFiles() to use a more robust source-level check for writeJson
calls on target/target-result filenames, or otherwise explicitly document and
constrain the accepted aliasing/computed-name cases so the invariant remains
enforceable even if the method is renamed or wrapped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 689e691f-f86b-4352-8b87-95a2dc7dd2dc
📒 Files selected for processing (66)
test/e2e/fixtures/artifacts.tstest/e2e/live/agent-turn-latency.test.tstest/e2e/live/bedrock-runtime-compatible-anthropic.test.tstest/e2e/live/brave-search.test.tstest/e2e/live/channels-add-remove.test.tstest/e2e/live/channels-stop-start-helpers.tstest/e2e/live/cloud-inference.test.tstest/e2e/live/cloud-onboard.test.tstest/e2e/live/common-egress-agent.test.tstest/e2e/live/concurrent-gateway-ports.test.tstest/e2e/live/credential-migration.test.tstest/e2e/live/credential-sanitization.test.tstest/e2e/live/cron-preflight-inference-local.test.tstest/e2e/live/dashboard-remote-bind.test.tstest/e2e/live/device-auth-health.test.tstest/e2e/live/diagnostics.test.tstest/e2e/live/docs-validation.test.tstest/e2e/live/double-onboard.test.tstest/e2e/live/full-e2e.test.tstest/e2e/live/gateway-guard-recovery.test.tstest/e2e/live/gateway-health-honest.test.tstest/e2e/live/gpu-double-onboard.test.tstest/e2e/live/gpu-e2e.test.tstest/e2e/live/hermes-discord.test.tstest/e2e/live/hermes-e2e.test.tstest/e2e/live/hermes-gpu-startup.test.tstest/e2e/live/hermes-inference-switch.test.tstest/e2e/live/hermes-root-entrypoint-smoke.test.tstest/e2e/live/hermes-sandbox-secret-boundary.test.tstest/e2e/live/hermes-slack-e2e-helpers.tstest/e2e/live/inference-routing.test.tstest/e2e/live/issue-2478-crash-loop-recovery.test.tstest/e2e/live/issue-4434-tui-unreachable-inference.test.tstest/e2e/live/issue-4462-scope-upgrade-approval.test.tstest/e2e/live/jetson-nvmap-gpu.test.tstest/e2e/live/kimi-inference-compat.test.tstest/e2e/live/launchable-smoke.test.tstest/e2e/live/messaging-compatible-endpoint.test.tstest/e2e/live/model-router-provider-routed-inference.test.tstest/e2e/live/network-policy.test.tstest/e2e/live/ollama-auth-proxy.test.tstest/e2e/live/onboard-negative-paths.test.tstest/e2e/live/onboard-repair.test.tstest/e2e/live/openclaw-discord-pairing.test.tstest/e2e/live/openclaw-inference-switch.test.tstest/e2e/live/openclaw-plugin-runtime-exdev.test.tstest/e2e/live/openclaw-skill-cli.test.tstest/e2e/live/openclaw-slack-pairing.test.tstest/e2e/live/openclaw-tui-chat-correlation.test.tstest/e2e/live/openshell-version-pin.test.tstest/e2e/live/registry-targets.test.tstest/e2e/live/runtime-overrides.test.tstest/e2e/live/sandbox-operations.test.tstest/e2e/live/sandbox-rlimits-connect.test.tstest/e2e/live/sandbox-survival.test.tstest/e2e/live/sessions-agents-cli.test.tstest/e2e/live/shields-config.test.tstest/e2e/live/skill-agent.test.tstest/e2e/live/snapshot-commands.test.tstest/e2e/live/spark-install.test.tstest/e2e/live/state-backup-restore.test.tstest/e2e/live/telegram-injection.test.tstest/e2e/live/token-rotation.test.tstest/e2e/live/ubuntu-repo-cli-smoke.test.tstest/e2e/live/upgrade-stale-sandbox.test.tstest/e2e/support/e2e-target-evidence.test.ts
✅ Files skipped from review due to trivial changes (3)
- test/e2e/live/token-rotation.test.ts
- test/e2e/live/hermes-inference-switch.test.ts
- test/e2e/live/credential-migration.test.ts
🚧 Files skipped from review as they are similar to previous changes (62)
- test/e2e/live/docs-validation.test.ts
- test/e2e/live/ubuntu-repo-cli-smoke.test.ts
- test/e2e/live/telegram-injection.test.ts
- test/e2e/live/kimi-inference-compat.test.ts
- test/e2e/live/spark-install.test.ts
- test/e2e/live/gpu-e2e.test.ts
- test/e2e/live/openshell-version-pin.test.ts
- test/e2e/live/issue-2478-crash-loop-recovery.test.ts
- test/e2e/live/hermes-root-entrypoint-smoke.test.ts
- test/e2e/live/upgrade-stale-sandbox.test.ts
- test/e2e/live/snapshot-commands.test.ts
- test/e2e/live/gateway-guard-recovery.test.ts
- test/e2e/live/cron-preflight-inference-local.test.ts
- test/e2e/live/openclaw-slack-pairing.test.ts
- test/e2e/live/concurrent-gateway-ports.test.ts
- test/e2e/live/channels-add-remove.test.ts
- test/e2e/live/sessions-agents-cli.test.ts
- test/e2e/live/dashboard-remote-bind.test.ts
- test/e2e/live/ollama-auth-proxy.test.ts
- test/e2e/live/openclaw-skill-cli.test.ts
- test/e2e/live/state-backup-restore.test.ts
- test/e2e/live/hermes-gpu-startup.test.ts
- test/e2e/live/jetson-nvmap-gpu.test.ts
- test/e2e/live/onboard-negative-paths.test.ts
- test/e2e/live/runtime-overrides.test.ts
- test/e2e/live/hermes-e2e.test.ts
- test/e2e/live/channels-stop-start-helpers.ts
- test/e2e/live/device-auth-health.test.ts
- test/e2e/live/brave-search.test.ts
- test/e2e/live/issue-4462-scope-upgrade-approval.test.ts
- test/e2e/live/double-onboard.test.ts
- test/e2e/live/cloud-inference.test.ts
- test/e2e/live/openclaw-inference-switch.test.ts
- test/e2e/live/hermes-sandbox-secret-boundary.test.ts
- test/e2e/live/launchable-smoke.test.ts
- test/e2e/live/common-egress-agent.test.ts
- test/e2e/live/diagnostics.test.ts
- test/e2e/live/bedrock-runtime-compatible-anthropic.test.ts
- test/e2e/fixtures/artifacts.ts
- test/e2e/live/onboard-repair.test.ts
- test/e2e/live/credential-sanitization.test.ts
- test/e2e/live/full-e2e.test.ts
- test/e2e/live/hermes-slack-e2e-helpers.ts
- test/e2e/live/openclaw-discord-pairing.test.ts
- test/e2e/live/openclaw-tui-chat-correlation.test.ts
- test/e2e/live/sandbox-rlimits-connect.test.ts
- test/e2e/live/agent-turn-latency.test.ts
- test/e2e/live/registry-targets.test.ts
- test/e2e/live/skill-agent.test.ts
- test/e2e/live/gpu-double-onboard.test.ts
- test/e2e/live/gateway-health-honest.test.ts
- test/e2e/live/hermes-discord.test.ts
- test/e2e/live/cloud-onboard.test.ts
- test/e2e/live/sandbox-survival.test.ts
- test/e2e/live/openclaw-plugin-runtime-exdev.test.ts
- test/e2e/live/model-router-provider-routed-inference.test.ts
- test/e2e/live/sandbox-operations.test.ts
- test/e2e/live/network-policy.test.ts
- test/e2e/live/issue-4434-tui-unreachable-inference.test.ts
- test/e2e/live/inference-routing.test.ts
- test/e2e/live/messaging-compatible-endpoint.test.ts
- test/e2e/live/shields-config.test.ts
06e1e6a to
eed6422
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/support/e2e-target-evidence.test.ts`:
- Around line 77-85: The migration guard in liveTypescriptFiles() is too brittle
because the .writeJson("target.json") regex only matches same-line
single/double-quoted calls, so multi-line formatting or template literals can
bypass it. Update the check in e2e-target-evidence.test.ts to use a more robust
detection strategy in the it("keeps live target evidence behind the typed API")
test, so it still flags writeJson calls to target.json or target-result.json
regardless of line breaks or quote style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 29baa5a4-35e3-4679-8af6-1be5128491d1
📒 Files selected for processing (66)
test/e2e/fixtures/artifacts.tstest/e2e/live/agent-turn-latency.test.tstest/e2e/live/bedrock-runtime-compatible-anthropic.test.tstest/e2e/live/brave-search.test.tstest/e2e/live/channels-add-remove.test.tstest/e2e/live/channels-stop-start-helpers.tstest/e2e/live/cloud-inference.test.tstest/e2e/live/cloud-onboard.test.tstest/e2e/live/common-egress-agent.test.tstest/e2e/live/concurrent-gateway-ports.test.tstest/e2e/live/credential-migration.test.tstest/e2e/live/credential-sanitization.test.tstest/e2e/live/cron-preflight-inference-local.test.tstest/e2e/live/dashboard-remote-bind.test.tstest/e2e/live/device-auth-health.test.tstest/e2e/live/diagnostics.test.tstest/e2e/live/docs-validation.test.tstest/e2e/live/double-onboard.test.tstest/e2e/live/full-e2e.test.tstest/e2e/live/gateway-guard-recovery.test.tstest/e2e/live/gateway-health-honest.test.tstest/e2e/live/gpu-double-onboard.test.tstest/e2e/live/gpu-e2e.test.tstest/e2e/live/hermes-discord.test.tstest/e2e/live/hermes-e2e.test.tstest/e2e/live/hermes-gpu-startup.test.tstest/e2e/live/hermes-inference-switch.test.tstest/e2e/live/hermes-root-entrypoint-smoke.test.tstest/e2e/live/hermes-sandbox-secret-boundary.test.tstest/e2e/live/hermes-slack-e2e-helpers.tstest/e2e/live/inference-routing.test.tstest/e2e/live/issue-2478-crash-loop-recovery.test.tstest/e2e/live/issue-4434-tui-unreachable-inference.test.tstest/e2e/live/issue-4462-scope-upgrade-approval.test.tstest/e2e/live/jetson-nvmap-gpu.test.tstest/e2e/live/kimi-inference-compat.test.tstest/e2e/live/launchable-smoke.test.tstest/e2e/live/messaging-compatible-endpoint.test.tstest/e2e/live/model-router-provider-routed-inference.test.tstest/e2e/live/network-policy.test.tstest/e2e/live/ollama-auth-proxy.test.tstest/e2e/live/onboard-negative-paths.test.tstest/e2e/live/onboard-repair.test.tstest/e2e/live/openclaw-discord-pairing.test.tstest/e2e/live/openclaw-inference-switch.test.tstest/e2e/live/openclaw-plugin-runtime-exdev.test.tstest/e2e/live/openclaw-skill-cli.test.tstest/e2e/live/openclaw-slack-pairing.test.tstest/e2e/live/openclaw-tui-chat-correlation.test.tstest/e2e/live/openshell-version-pin.test.tstest/e2e/live/registry-targets.test.tstest/e2e/live/runtime-overrides.test.tstest/e2e/live/sandbox-operations.test.tstest/e2e/live/sandbox-rlimits-connect.test.tstest/e2e/live/sandbox-survival.test.tstest/e2e/live/sessions-agents-cli.test.tstest/e2e/live/shields-config.test.tstest/e2e/live/skill-agent.test.tstest/e2e/live/snapshot-commands.test.tstest/e2e/live/spark-install.test.tstest/e2e/live/state-backup-restore.test.tstest/e2e/live/telegram-injection.test.tstest/e2e/live/token-rotation.test.tstest/e2e/live/ubuntu-repo-cli-smoke.test.tstest/e2e/live/upgrade-stale-sandbox.test.tstest/e2e/support/e2e-target-evidence.test.ts
✅ Files skipped from review due to trivial changes (2)
- test/e2e/live/openclaw-slack-pairing.test.ts
- test/e2e/live/hermes-root-entrypoint-smoke.test.ts
🚧 Files skipped from review as they are similar to previous changes (63)
- test/e2e/live/kimi-inference-compat.test.ts
- test/e2e/live/telegram-injection.test.ts
- test/e2e/live/openclaw-discord-pairing.test.ts
- test/e2e/live/channels-add-remove.test.ts
- test/e2e/live/spark-install.test.ts
- test/e2e/live/sandbox-rlimits-connect.test.ts
- test/e2e/live/upgrade-stale-sandbox.test.ts
- test/e2e/live/jetson-nvmap-gpu.test.ts
- test/e2e/live/channels-stop-start-helpers.ts
- test/e2e/live/registry-targets.test.ts
- test/e2e/live/device-auth-health.test.ts
- test/e2e/live/gpu-e2e.test.ts
- test/e2e/live/hermes-inference-switch.test.ts
- test/e2e/live/agent-turn-latency.test.ts
- test/e2e/live/issue-2478-crash-loop-recovery.test.ts
- test/e2e/live/credential-sanitization.test.ts
- test/e2e/live/brave-search.test.ts
- test/e2e/live/shields-config.test.ts
- test/e2e/live/openshell-version-pin.test.ts
- test/e2e/live/hermes-gpu-startup.test.ts
- test/e2e/live/launchable-smoke.test.ts
- test/e2e/live/docs-validation.test.ts
- test/e2e/live/token-rotation.test.ts
- test/e2e/live/gateway-health-honest.test.ts
- test/e2e/live/full-e2e.test.ts
- test/e2e/live/openclaw-plugin-runtime-exdev.test.ts
- test/e2e/live/onboard-negative-paths.test.ts
- test/e2e/live/credential-migration.test.ts
- test/e2e/live/gateway-guard-recovery.test.ts
- test/e2e/live/hermes-discord.test.ts
- test/e2e/live/sandbox-survival.test.ts
- test/e2e/live/dashboard-remote-bind.test.ts
- test/e2e/live/diagnostics.test.ts
- test/e2e/live/network-policy.test.ts
- test/e2e/live/sessions-agents-cli.test.ts
- test/e2e/live/double-onboard.test.ts
- test/e2e/live/ollama-auth-proxy.test.ts
- test/e2e/live/onboard-repair.test.ts
- test/e2e/live/common-egress-agent.test.ts
- test/e2e/live/openclaw-tui-chat-correlation.test.ts
- test/e2e/live/runtime-overrides.test.ts
- test/e2e/live/ubuntu-repo-cli-smoke.test.ts
- test/e2e/live/issue-4434-tui-unreachable-inference.test.ts
- test/e2e/live/hermes-slack-e2e-helpers.ts
- test/e2e/live/inference-routing.test.ts
- test/e2e/live/issue-4462-scope-upgrade-approval.test.ts
- test/e2e/live/hermes-sandbox-secret-boundary.test.ts
- test/e2e/live/skill-agent.test.ts
- test/e2e/fixtures/artifacts.ts
- test/e2e/live/cloud-onboard.test.ts
- test/e2e/live/hermes-e2e.test.ts
- test/e2e/live/openclaw-skill-cli.test.ts
- test/e2e/live/snapshot-commands.test.ts
- test/e2e/live/state-backup-restore.test.ts
- test/e2e/live/cron-preflight-inference-local.test.ts
- test/e2e/live/messaging-compatible-endpoint.test.ts
- test/e2e/live/cloud-inference.test.ts
- test/e2e/live/concurrent-gateway-ports.test.ts
- test/e2e/live/bedrock-runtime-compatible-anthropic.test.ts
- test/e2e/live/model-router-provider-routed-inference.test.ts
- test/e2e/live/gpu-double-onboard.test.ts
- test/e2e/live/sandbox-operations.test.ts
- test/e2e/live/openclaw-inference-switch.test.ts
1ac7254 to
570942b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/support/e2e-target-evidence.test.ts (1)
78-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMigration guard still has bypass paths via composed filenames or non-
writeJsonwriters.The regex requires the
target.json/target-result.jsonliteral to appear directly inside.writeJson(...)(allowing only leading whitespace). A composed path such as.writeJson(path.join(dir, "target.json"), data), or any rawfs.writeFileSync/fs.promises.writeFilecall targeting the same filenames, would silently evade this check — undermining the guarantee that the manual write path is unreachable.As per path instructions, "Migration tests must prove the superseded path is unreachable or removed, not merely prove that the new path also works," so this scan should catch writes to these filenames regardless of how the path argument is constructed or which write API is used.
🔧 Broaden detection to catch composed paths and alternate write APIs
- const violations = liveTypescriptFiles() - .filter((file) => - /\.writeJson\(\s*[`'"]target(?:-result)?\.json[`'"]/.test(fs.readFileSync(file, "utf8")), - ) - .map((file) => path.basename(file)); + const violations = liveTypescriptFiles() + .filter((file) => { + const source = fs.readFileSync(file, "utf8"); + return ( + /\.writeJson\(/.test(source) && + /["'`]target(?:-result)?\.json["'`]/.test(source) + ) || /\bwriteFileSync?\(.*target(?:-result)?\.json/.test(source); + }) + .map((file) => path.basename(file));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/support/e2e-target-evidence.test.ts` around lines 78 - 85, The migration guard in e2e-target-evidence.test.ts only matches literal target filenames passed directly to .writeJson, so composed paths and other write APIs can bypass it. Broaden the scan in the liveTypescriptFiles() check to detect any reference to target.json or target-result.json regardless of how the path is built, and include writes through writeJson, writeFileSync, and writeFile so the superseded path cannot slip through. Use the existing violations assertion to flag any file that targets those names through any writer.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e/support/e2e-target-evidence.test.ts`:
- Around line 78-85: The migration guard in e2e-target-evidence.test.ts only
matches literal target filenames passed directly to .writeJson, so composed
paths and other write APIs can bypass it. Broaden the scan in the
liveTypescriptFiles() check to detect any reference to target.json or
target-result.json regardless of how the path is built, and include writes
through writeJson, writeFileSync, and writeFile so the superseded path cannot
slip through. Use the existing violations assertion to flag any file that
targets those names through any writer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8f571191-90e8-4d6b-9824-d886e9c6e77c
📒 Files selected for processing (66)
test/e2e/fixtures/artifacts.tstest/e2e/live/agent-turn-latency.test.tstest/e2e/live/bedrock-runtime-compatible-anthropic.test.tstest/e2e/live/brave-search.test.tstest/e2e/live/channels-add-remove.test.tstest/e2e/live/channels-stop-start-helpers.tstest/e2e/live/cloud-inference.test.tstest/e2e/live/cloud-onboard.test.tstest/e2e/live/common-egress-agent.test.tstest/e2e/live/concurrent-gateway-ports.test.tstest/e2e/live/credential-migration.test.tstest/e2e/live/credential-sanitization.test.tstest/e2e/live/cron-preflight-inference-local.test.tstest/e2e/live/dashboard-remote-bind.test.tstest/e2e/live/device-auth-health.test.tstest/e2e/live/diagnostics.test.tstest/e2e/live/docs-validation.test.tstest/e2e/live/double-onboard.test.tstest/e2e/live/full-e2e.test.tstest/e2e/live/gateway-guard-recovery.test.tstest/e2e/live/gateway-health-honest.test.tstest/e2e/live/gpu-double-onboard.test.tstest/e2e/live/gpu-e2e.test.tstest/e2e/live/hermes-discord.test.tstest/e2e/live/hermes-e2e.test.tstest/e2e/live/hermes-gpu-startup.test.tstest/e2e/live/hermes-inference-switch.test.tstest/e2e/live/hermes-root-entrypoint-smoke.test.tstest/e2e/live/hermes-sandbox-secret-boundary.test.tstest/e2e/live/hermes-slack-e2e-helpers.tstest/e2e/live/inference-routing.test.tstest/e2e/live/issue-2478-crash-loop-recovery.test.tstest/e2e/live/issue-4434-tui-unreachable-inference.test.tstest/e2e/live/issue-4462-scope-upgrade-approval.test.tstest/e2e/live/jetson-nvmap-gpu.test.tstest/e2e/live/kimi-inference-compat.test.tstest/e2e/live/launchable-smoke.test.tstest/e2e/live/messaging-compatible-endpoint.test.tstest/e2e/live/model-router-provider-routed-inference.test.tstest/e2e/live/network-policy.test.tstest/e2e/live/ollama-auth-proxy.test.tstest/e2e/live/onboard-negative-paths.test.tstest/e2e/live/onboard-repair.test.tstest/e2e/live/openclaw-discord-pairing.test.tstest/e2e/live/openclaw-inference-switch.test.tstest/e2e/live/openclaw-plugin-runtime-exdev.test.tstest/e2e/live/openclaw-skill-cli.test.tstest/e2e/live/openclaw-slack-pairing.test.tstest/e2e/live/openclaw-tui-chat-correlation.test.tstest/e2e/live/openshell-version-pin.test.tstest/e2e/live/registry-targets.test.tstest/e2e/live/runtime-overrides.test.tstest/e2e/live/sandbox-operations.test.tstest/e2e/live/sandbox-rlimits-connect.test.tstest/e2e/live/sandbox-survival.test.tstest/e2e/live/sessions-agents-cli.test.tstest/e2e/live/shields-config.test.tstest/e2e/live/skill-agent.test.tstest/e2e/live/snapshot-commands.test.tstest/e2e/live/spark-install.test.tstest/e2e/live/state-backup-restore.test.tstest/e2e/live/telegram-injection.test.tstest/e2e/live/token-rotation.test.tstest/e2e/live/ubuntu-repo-cli-smoke.test.tstest/e2e/live/upgrade-stale-sandbox.test.tstest/e2e/support/e2e-target-evidence.test.ts
✅ Files skipped from review due to trivial changes (3)
- test/e2e/live/upgrade-stale-sandbox.test.ts
- test/e2e/live/openclaw-slack-pairing.test.ts
- test/e2e/live/openshell-version-pin.test.ts
🚧 Files skipped from review as they are similar to previous changes (62)
- test/e2e/live/brave-search.test.ts
- test/e2e/live/ollama-auth-proxy.test.ts
- test/e2e/live/openclaw-discord-pairing.test.ts
- test/e2e/live/jetson-nvmap-gpu.test.ts
- test/e2e/live/hermes-inference-switch.test.ts
- test/e2e/live/model-router-provider-routed-inference.test.ts
- test/e2e/live/issue-4434-tui-unreachable-inference.test.ts
- test/e2e/live/cloud-onboard.test.ts
- test/e2e/live/credential-sanitization.test.ts
- test/e2e/live/full-e2e.test.ts
- test/e2e/live/agent-turn-latency.test.ts
- test/e2e/live/channels-stop-start-helpers.ts
- test/e2e/live/launchable-smoke.test.ts
- test/e2e/live/gpu-e2e.test.ts
- test/e2e/live/sandbox-rlimits-connect.test.ts
- test/e2e/live/hermes-gpu-startup.test.ts
- test/e2e/live/state-backup-restore.test.ts
- test/e2e/live/channels-add-remove.test.ts
- test/e2e/live/sessions-agents-cli.test.ts
- test/e2e/live/gpu-double-onboard.test.ts
- test/e2e/live/shields-config.test.ts
- test/e2e/live/network-policy.test.ts
- test/e2e/live/double-onboard.test.ts
- test/e2e/live/openclaw-tui-chat-correlation.test.ts
- test/e2e/live/spark-install.test.ts
- test/e2e/live/docs-validation.test.ts
- test/e2e/live/hermes-e2e.test.ts
- test/e2e/live/cron-preflight-inference-local.test.ts
- test/e2e/live/hermes-sandbox-secret-boundary.test.ts
- test/e2e/live/issue-2478-crash-loop-recovery.test.ts
- test/e2e/live/hermes-root-entrypoint-smoke.test.ts
- test/e2e/live/skill-agent.test.ts
- test/e2e/live/token-rotation.test.ts
- test/e2e/live/issue-4462-scope-upgrade-approval.test.ts
- test/e2e/live/kimi-inference-compat.test.ts
- test/e2e/live/snapshot-commands.test.ts
- test/e2e/live/sandbox-survival.test.ts
- test/e2e/live/gateway-health-honest.test.ts
- test/e2e/live/onboard-repair.test.ts
- test/e2e/live/registry-targets.test.ts
- test/e2e/live/concurrent-gateway-ports.test.ts
- test/e2e/live/device-auth-health.test.ts
- test/e2e/live/cloud-inference.test.ts
- test/e2e/live/dashboard-remote-bind.test.ts
- test/e2e/live/openclaw-skill-cli.test.ts
- test/e2e/live/credential-migration.test.ts
- test/e2e/live/sandbox-operations.test.ts
- test/e2e/live/hermes-discord.test.ts
- test/e2e/live/runtime-overrides.test.ts
- test/e2e/live/inference-routing.test.ts
- test/e2e/live/ubuntu-repo-cli-smoke.test.ts
- test/e2e/live/bedrock-runtime-compatible-anthropic.test.ts
- test/e2e/live/hermes-slack-e2e-helpers.ts
- test/e2e/live/openclaw-plugin-runtime-exdev.test.ts
- test/e2e/fixtures/artifacts.ts
- test/e2e/live/common-egress-agent.test.ts
- test/e2e/live/onboard-negative-paths.test.ts
- test/e2e/live/messaging-compatible-endpoint.test.ts
- test/e2e/live/openclaw-inference-switch.test.ts
- test/e2e/live/diagnostics.test.ts
- test/e2e/live/gateway-guard-recovery.test.ts
- test/e2e/live/telegram-injection.test.ts
570942b to
9a3c595
Compare
9a3c595 to
b36e465
Compare
E2E Target Results — ✅ All default jobs passedRun: 28838142470
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some jobs failedRun: 28839471135
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ✅ All default jobs passedRun: 28840988928
|
cv
left a comment
There was a problem hiding this comment.
Hard contributor-compliance gate: commits 86a59c2, 02f8480, and b36e465 are not marked Verified by GitHub. Every PR commit must appear Verified, and maintainers cannot repair or sign contributor history. Please replace them with a clean verified history (or use a fresh branch/PR if the published branch cannot be safely rewritten), then rerun CI.
Maintainer waiver: commit-verification enforcement is waived for known team member jyaunches. Other CI, conflict, automated-review, security, and test gates still apply.
## Summary Add a typed target-evidence facade to the existing artifact sink and migrate live Vitest targets to it. Target metadata/results now retain the same artifact filenames and redaction boundary while receiving a consistent runner, result status, and contract field shape. ## Related Issue Closes NVIDIA#6353 Parent epic: NVIDIA#6346 ## Changes - Add `artifacts.target.declare(...)` and `artifacts.target.complete(...)` with typed extensible metadata/result inputs. - Emit `target.json` and `target-result.json` through the existing redacting `ArtifactSink`. - Supply `runner: "vitest"` centrally and default completed results without an explicit status to `status: "passed"`. - Normalize legacy singular `contract` values to the canonical `contracts` array and reject conflicting or malformed contract fields. - Validate non-empty target IDs and explicit result statuses. - Migrate all 117 live target evidence writes and remove repeated runner fields. - Add focused support tests for emitted files, normalization, validation, redaction, and adoption. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: test-evidence refactor with no user-facing behavior - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — 28 focused artifact, fixture, and redaction assertions passed - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Additional local verification: - `npm run build:cli` - `npm run typecheck` - `npm run typecheck:cli` - `npm run lint` - `npx vitest run --project e2e-support test/e2e/support/e2e-target-evidence.test.ts test/e2e/support/e2e-fixture-context.test.ts test/e2e/support/e2e-redaction-entry.test.ts` --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a typed E2E “target evidence” lifecycle with explicit **declare**/**complete** steps, producing consistent, normalized evidence outputs. * **Bug Fixes** * Improved validation and normalization (non-empty identifiers, non-empty status when provided, enforce single `contract` vs `contracts`, consistent contract array formatting, and automatic result defaults). * Added secret redaction in persisted evidence details. * **Tests** * Migrated many live E2E scenarios to use the typed evidence API. * Added test coverage for evidence normalization/redaction and a safeguard scan to prevent direct JSON target writes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Summary
Add a typed target-evidence facade to the existing artifact sink and migrate live Vitest targets to it. Target metadata/results now retain the same artifact filenames and redaction boundary while receiving a consistent runner, result status, and contract field shape.
Related Issue
Closes #6353
Parent epic: #6346
Changes
artifacts.target.declare(...)andartifacts.target.complete(...)with typed extensible metadata/result inputs.target.jsonandtarget-result.jsonthrough the existing redactingArtifactSink.runner: "vitest"centrally and default completed results without an explicit status tostatus: "passed".contractvalues to the canonicalcontractsarray and reject conflicting or malformed contract fields.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Additional local verification:
npm run build:clinpm run typechecknpm run typecheck:clinpm run lintnpx vitest run --project e2e-support test/e2e/support/e2e-target-evidence.test.ts test/e2e/support/e2e-fixture-context.test.ts test/e2e/support/e2e-redaction-entry.test.tsSigned-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
contractvscontracts, consistent contract array formatting, and automatic result defaults).