fix(verify): stop feature verification reporting false green - #1392
Conversation
A verify-features run with four failing Tier 2 checks reported "COMPLETED — 11 passed, 0 failed". Four independent defects produced that, and each is closed here. 1. Every step set failOnError: false, so a non-zero exit scored as a pass. Tiers still use it (all tiers should run even after one fails), but they are no longer the verdict: each check now appends pass/fail/skip to checks.jsonl and a `verdict` step aggregates it into verdict.json. 2. Nothing aggregated tier results into a run verdict. `verdict` plus a terminal `enforce-verdict` now do, and main() exits non-zero. 3. main() discarded the runner result. It now reads verdict.json and fails closed when that file is missing. 4. applyReliabilityDefaults force-enables strategy "retry" with a repair agent for any workflow declaring agents, so a failing verification gate was handed to an agent that edited the tree until it passed. onError is now "continue", which opts out of that path. The run also verified agent-relay 10.0.0 against an 11.3.0 checkout — it logged "Update available: 10.0.0 -> 11.3.0" and then reported `relay node deadletters` as unknown, a real command in 11.x. A `provenance` step now fails on CLI/repo version drift. A `capabilities` probe turns unrunnable checks (no cloud login, no provider CLI) into SKIPs with a stated cause rather than product failures, and `verdict` fails the run when a tier produced no records at all, so a crashed tier cannot look clean. Coverage now spans tiers 1-6 and all six critical paths, including a real MCP JSON-RPC handshake asserting the required tool set (previously just `relay mcp --help`). On failure the run posts to Slack, files a GitHub issue, attempts a fix on a branch, and opens a draft PR. An integrity gate refuses a branch that reduced the number of verification call sites, left the repo on main, or has no root cause on record. Harness-level breakage escalates to NightCTO via the CloudEvidenceSummary v1 contract; run and per-failure events go to PostHog. Adds scripts/audit-feature-manifest.mjs and workflows/audit-feature-manifest.ts to answer whether the feature map is current. manifest-contract.test.ts asserts a hand-maintained list, so it cannot notice a newly added command; the audit derives the surface from recursive `--help` and `tools/list` instead. Its first run found `fleet spawn` and `fleet release` shipping undocumented, and therefore unverified — both are now in the manifest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both workflows now post through @relayflows/slack-primitive pinned to its
cloud-relay runtime, which proxies /api/v1/slack/post-message and delivers
via the workspace's configured Slack integration (the Nango connection).
Pinning the runtime is load-bearing. selectRuntime() prefers cloud-relay but
falls back to a local SLACK_BOT_TOKEN runtime that talks to slack.com
directly, so leaving it on auto meant a stray bot token in the environment
silently changed the delivery route. Pinned, a missing CLOUD_API_* pair
raises auth_token_missing instead. Verified: with SLACK_BOT_TOKEN set and
CLOUD_API_* absent, delivery fails loudly rather than falling back.
Also fixes the endpoint. The previous cloud fallback POSTed to
/api/v1/integrations/slack/messages, which does not exist; the primitive's
actual path is /api/v1/slack/post-message. Confirmed against a stub server:
POST /api/v1/slack/post-message, bearer CLOUD_API_TOKEN, body
{channel, text, unfurlLinks, unfurlMedia}.
Default channel is now the ID C0AEKNLDNKW rather than a #name. The
cloud-relay runtime does not implement channel resolution, so an ID is the
only form guaranteed to resolve. Override with VERIFY_SLACK_CHANNEL /
AUDIT_SLACK_CHANNEL.
An undelivered alert still echoes its full payload to the run log, so a
delivery failure can never read as a delivered alert.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR changes webhook creation to use ChangesFeature Verification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AuditWorkflow
participant AuditScript
participant Manifest
participant ManifestEditor
participant VerifyWorkflow
AuditWorkflow->>AuditScript: Derive CLI and MCP surfaces
AuditScript->>Manifest: Compare live and documented entries
AuditScript-->>AuditWorkflow: Return clean, drift, or audit failure
AuditWorkflow->>ManifestEditor: Apply autofix when enabled
ManifestEditor-->>AuditWorkflow: Write manifest update summary
VerifyWorkflow->>VerifyWorkflow: Run checks and generate verdict
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72539f7b88
ℹ️ 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".
| elif [ "$CLI_VERSION" = "$REPO_VERSION" ]; then | ||
| echo " PASS cli-matches-repo ($CLI_VERSION)" | tee -a "$LOG" | ||
| record "$TIER" "cli-matches-repo" pass "" |
There was a problem hiding this comment.
When the globally installed relay and the checkout both report the same package version—as normally happens for feature branches between releases—this records a pass even though every subsequent check still invokes the global binary and may exercise none of the branch's changes. The provenance gate should require the CLI path/build to belong to this checkout, or invoke the checkout's built entry point directly, rather than treating version equality as proof of provenance.
Useful? React with 👍 / 👎.
| run_check "message file upload" \ | ||
| "RELAY_AGENT_TOKEN='$TOKEN_A' relay message file upload '$CHANNEL' '$UPLOAD_FILE'" "." |
There was a problem hiding this comment.
Whenever Tier 4 reaches this check, it invokes the command with the channel as the positional path and the fixture as an unexpected second positional argument, so a working upload implementation is reported as failing. The command definition at packages/cli/src/cli/commands/message.ts:240-245 requires upload <path> --channel <channel>; this check needs to pass $UPLOAD_FILE first and $CHANNEL through --channel.
Useful? React with 👍 / 👎.
| - If the file does not exist, or its "verdict" field is "PASS", there is NOTHING | ||
| to fix. Write "No failures — nothing to fix." to | ||
| ${ARTIFACTS}/fix-summary.md and STOP IMMEDIATELY. Do not read other files, do | ||
| not create a branch, do not edit anything, do not run any commands. | ||
| - Only if "verdict" is "FAIL" do you continue with the rest of this task. |
There was a problem hiding this comment.
When VERIFY_AUTOFIX=0 and the verdict is FAIL, only file-issue observes the flag; this agent task checks only the verdict, so it still creates a branch, edits and commits code, and the downstream integrity and open-pr steps can still push and open a draft PR. Include the autofix setting in this early-stop contract (and gate the downstream path) so the documented opt-out actually disables the fix/PR workflow.
Useful? React with 👍 / 👎.
| - If it does not contain exactly \`1\`, there is NOTHING to do. Write "No drift — | ||
| nothing to update." to ${ARTIFACTS}/update-summary.md and STOP IMMEDIATELY: | ||
| do not create a branch, edit files, or run commands. | ||
| - Only if it contains \`1\` do you continue. |
There was a problem hiding this comment.
When AUDIT_AUTOFIX=0 and drift is found, file-issue exits successfully without filing an issue, but this task proceeds solely because audit-exit.txt contains 1; it can therefore create a branch and commit a manifest update, after which open-pr can push it. Make the editor and all downstream mutation steps stop when autofix is disabled, not just when the audit is clean.
Useful? React with 👍 / 👎.
| const drift = undocumentedCommands.length + staleCommands.length + undocumentedMcp.length + staleMcp.length; | ||
|
|
||
| const report = { | ||
| ok: drift === 0, | ||
| auditable: true, |
There was a problem hiding this comment.
If relay mcp crashes, times out, or never returns tools/list while the CLI manifest has no other drift, mcpTools is null, both MCP drift arrays become empty, and drift remains zero; the script then reports ok: true, prints MANIFEST_CLEAN, and exits 0 despite explicitly saying MCP drift was not checked. Treat an underivable MCP surface as audit failure/exit 2 so a broken half of the audit cannot produce a clean result.
Useful? React with 👍 / 👎.
| const staleCommands = [...documentedCommands].filter((cmd) => { | ||
| const parts = cmd.split(' '); | ||
| for (let i = parts.length; i >= 1; i--) { | ||
| if (cliLeafSet.has(parts.slice(0, i).join(' '))) return false; | ||
| } |
There was a problem hiding this comment.
When a documented subcommand is removed but its parent group remains—for example, fleet spawn disappears while fleet still exists—this loop finds the parent prefix in cliLeafSet and declares the manifest entry non-stale. Because deriveCliLeaves() deliberately adds every parent group, almost any removed child command is hidden; positional-dispatch exceptions need to be identified explicitly rather than allowing any surviving prefix to validate an arbitrary documented suffix.
Useful? React with 👍 / 👎.
| gated_check cloud "cloud status" "relay cloud status" "." | ||
| gated_check cloud "cloud schedules" "relay cloud schedules" "." | ||
| gated_check cloud "cloud logs" "relay cloud logs 2>&1 || true" "." |
There was a problem hiding this comment.
In a cloud-authenticated environment, both checks omit the required <runId> defined at packages/cli/src/cli/commands/cloud.ts:897-925: cloud status therefore always records a failure, while cloud logs masks the parser failure with || true and records a false pass because the error text matches .. Use a real fixture run ID, or record these as explicit skips when no run exists.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
workflows/audit-feature-manifest.ts (2)
490-491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScheduled runs will file a duplicate issue every week while drift persists.
Nothing checks for an already-open drift issue, and the title is run-ID-unique so GitHub won't dedupe either. Consider searching for an open issue with a stable label/marker first and commenting on it instead of creating a new one.
♻️ Sketch
+EXISTING=$(gh issue list --state open --label manifest-drift --limit 1 --json url --jq '.[0].url' 2>/dev/null || true) +if [ -n "$EXISTING" ]; then + gh issue comment "$EXISTING" --body-file "$ARTIFACTS/issue-body.md" >/dev/null 2>&1 || true + echo "$EXISTING" > "$ARTIFACTS/issue-url.txt" + echo "ISSUE_UPDATED: $EXISTING" + exit 0 +fi + ISSUE_URL=$(gh issue create --title "Feature manifest drift: ${RUN_ID}" \ - --body-file "$ARTIFACTS/issue-body.md" 2>&1 | grep -oE 'https://[^ ]+' | head -1 || true) + --label manifest-drift --body-file "$ARTIFACTS/issue-body.md" 2>&1 | grep -oE 'https://[^ ]+' | head -1 || true)🤖 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 `@workflows/audit-feature-manifest.ts` around lines 490 - 491, Update the issue-reporting flow around ISSUE_URL to search for an existing open drift issue using a stable label or marker before invoking gh issue create. If one exists, reuse its URL and add the current drift details as a comment; only create a new issue when none is found, and ensure newly created issues receive the stable identifier.
185-190: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTwo full audit invocations double the CLI walk.
Each invocation spawns
--helpfor every node of the command tree plus an MCP handshake, so this pays that cost twice against a 900s workflow timeout — and the two runs can disagree (the second's exit code is discarded via|| true, soaudit.jsonmay not describe the run whose exit code drives everything downstream). Consider running once with--jsonand rendering the human-readable log from the JSON, or having the script emit both in one pass.🤖 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 `@workflows/audit-feature-manifest.ts` around lines 185 - 190, Update the audit workflow to invoke scripts/audit-feature-manifest.mjs only once, preserving a single authoritative exit code and report. Have that invocation produce both machine-readable JSON for audit.json and a human-readable audit.txt, either by using the script’s combined-output support or by rendering the human-readable log from the captured JSON; remove the second invocation and discarded exit-code path.workflows/verify-features.ts (1)
1128-1135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
TOOL_COUNTcounts quote characters and is never used.
grep -o '"' | wc -lis not a tool count and nothing reads the variable. Drop it, or derive a real count from the parsedtoolsarray incp4-mcp.mjsand record it in the reason.♻️ Proposed cleanup
if node "${ARTIFACTS}/cp4-mcp.mjs" > "$MCP_OUT" 2>/dev/null && grep -q '"ok":true' "$MCP_OUT"; then - TOOL_COUNT=$(grep -o '"' "$MCP_OUT" | wc -l | tr -d ' ') # Assert the tools we depend on are actually listed, not just that some🤖 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 `@workflows/verify-features.ts` around lines 1128 - 1135, Remove the unused TOOL_COUNT assignment from the MCP verification block, including its quote-counting pipeline; keep the existing required-tool checks in the loop unchanged.
🤖 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 `@scripts/audit-feature-manifest.mjs`:
- Line 325: Update the audit result and exit-status logic around
undocumentedMcp, staleMcp, and the final manifest status so a null result from
deriveMcpTools() is treated as unauditable rather than clean. Ensure the script
exits with the existing non-clean audit status (such as 2) when mcpDerivable is
false, while preserving normal drift detection for successfully derived MCP
data; only allow a clean exit when the MCP surface was derivable or an explicit
opt-out flag is supported.
- Around line 192-204: Guard the child.stdin writes in the send helper used by
the initialize and tools/list requests against broken or destroyed streams.
Attach an error handler or otherwise safely suppress EPIPE and
ERR_STREAM_DESTROYED, ensuring stdin failures are handled through the existing
audit failure path and preserve the intended exit status 2 rather than causing
an uncaught exception.
In `@workflows/audit-feature-manifest.ts`:
- Around line 346-397: Guard JSON.parse in both the slack-alert builder at
workflows/audit-feature-manifest.ts:346-397 and the file-issue builder at
workflows/audit-feature-manifest.ts:430-433. When audit.json is missing, empty,
truncated, or invalid, preserve the notification flow by emitting a minimal
drift-detected message for Slack and a minimal issue body containing the exit
code and artifact link, instead of allowing the Node scripts to fail or produce
empty output.
- Around line 638-651: Strengthen the integrity gate in the workflow block
around the existing scripts/audit-feature-manifest.mjs check to validate the
measured surface as well as the audit script. Detect changes to the CLI command
definitions and manifest-contract.test.ts expectation list, mark INTEGRITY=fail,
and exit before accepting MANIFEST_CLEAN when either measuring surface is
modified.
In `@workflows/verify-features.ts`:
- Around line 161-170: Update record in workflows/verify-features.ts:161-170 to
remove all ASCII control characters, including DEL, from the reason before
escaping and truncating it. Update the verdict-reading logic in
workflows/verify-features.ts:1264-1298 to count JSON.parse failures and add a
reason such as “N unparseable ledger line(s)” so malformed ledger entries affect
the run verdict rather than being silently discarded.
- Around line 637-645: Update the "node tail (bounded)" check so only exit
status 0 or timeout status 124 is accepted; reject other nonzero statuses such
as command or broker failures while preserving the existing timeout-success
behavior.
- Around line 1012-1019: Update the CP1 lifecycle assertion using the relay node
status command so it verifies that the complete status output does not contain
“running,” rather than succeeding when any individual line lacks it. Replace the
grep-based inverted match with a negated match while preserving the existing CP1
counter and pass/fail handling.
- Around line 1211-1217: Restrict the cleanup loops around the relay agent and
channel listing commands to harness-owned names only: match complete tokens
beginning with the expected vf- or cp[0-9] prefix, rather than matching those
substrings anywhere in output. Prefer including the current ${SUFFIX} in the
pattern so relay agent remove and relay channel archive only affect objects
created by this run.
- Around line 527-568: Initialize the shell command with the shared
${ENV_DEFAULTS} content before the probe function and capability checks execute.
Ensure the defaults are applied before probe slack evaluates CLOUD_API_URL and
CLOUD_API_TOKEN, while preserving the existing provider_ and provider_any probe
behavior.
---
Nitpick comments:
In `@workflows/audit-feature-manifest.ts`:
- Around line 490-491: Update the issue-reporting flow around ISSUE_URL to
search for an existing open drift issue using a stable label or marker before
invoking gh issue create. If one exists, reuse its URL and add the current drift
details as a comment; only create a new issue when none is found, and ensure
newly created issues receive the stable identifier.
- Around line 185-190: Update the audit workflow to invoke
scripts/audit-feature-manifest.mjs only once, preserving a single authoritative
exit code and report. Have that invocation produce both machine-readable JSON
for audit.json and a human-readable audit.txt, either by using the script’s
combined-output support or by rendering the human-readable log from the captured
JSON; remove the second invocation and discarded exit-code path.
In `@workflows/verify-features.ts`:
- Around line 1128-1135: Remove the unused TOOL_COUNT assignment from the MCP
verification block, including its quote-counting pipeline; keep the existing
required-tool checks in the loop unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 357a2055-1fac-477f-af3a-624864fde413
📒 Files selected for processing (7)
.agentworkforce/agents/relay-feature-guardian/manifest-contract.test.ts.agentworkforce/features/manifest.yaml.claude/skills/verify-features.md.gitignorescripts/audit-feature-manifest.mjsworkflows/audit-feature-manifest.tsworkflows/verify-features.ts
| # 'relay node tail' streams until interrupted; bound it and accept a timeout | ||
| # kill as success. Without the bound this step hangs until the run times out. | ||
| run_check "node tail (bounded)" \ | ||
| "timeout 5 relay node tail >/dev/null 2>&1; [ \$? -le 124 ] && echo tail-ok" "tail-ok" | ||
|
|
||
| echo "" | tee -a "$LOG" | ||
| echo "Tier 2 result: $PASS passed, $FAIL failed" | tee -a "$LOG" | ||
| # Redeliver against an empty queue is a no-op, so this exercises the command | ||
| # path without mutating real delivery state. | ||
| run_check "node redeliver --all (empty queue)" \ | ||
| "relay node redeliver --all 2>&1 || true" "." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
[ $? -le 124 ] treats real failures as success.
Only 0 (clean exit) and 124 (timeout kill) should count. Exit 1 (unknown command, broker unreachable) is -le 124, so a removed or broken relay node tail still records PASS — the precise class of false green this file targets.
🐛 Proposed fix
run_check "node tail (bounded)" \
- "timeout 5 relay node tail >/dev/null 2>&1; [ \$? -le 124 ] && echo tail-ok" "tail-ok"
+ "timeout 5 relay node tail >/dev/null 2>&1; _rc=\$?; { [ \$_rc -eq 0 ] || [ \$_rc -eq 124 ]; } && echo tail-ok" "tail-ok"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 'relay node tail' streams until interrupted; bound it and accept a timeout | |
| # kill as success. Without the bound this step hangs until the run times out. | |
| run_check "node tail (bounded)" \ | |
| "timeout 5 relay node tail >/dev/null 2>&1; [ \$? -le 124 ] && echo tail-ok" "tail-ok" | |
| echo "" | tee -a "$LOG" | |
| echo "Tier 2 result: $PASS passed, $FAIL failed" | tee -a "$LOG" | |
| # Redeliver against an empty queue is a no-op, so this exercises the command | |
| # path without mutating real delivery state. | |
| run_check "node redeliver --all (empty queue)" \ | |
| "relay node redeliver --all 2>&1 || true" "." | |
| # 'relay node tail' streams until interrupted; bound it and accept a timeout | |
| # kill as success. Without the bound this step hangs until the run times out. | |
| run_check "node tail (bounded)" \ | |
| "timeout 5 relay node tail >/dev/null 2>&1; _rc=\$?; { [ \$_rc -eq 0 ] || [ \$_rc -eq 124 ]; } && echo tail-ok" "tail-ok" | |
| # Redeliver against an empty queue is a no-op, so this exercises the command | |
| # path without mutating real delivery state. | |
| run_check "node redeliver --all (empty queue)" \ | |
| "relay node redeliver --all 2>&1 || true" "." |
🤖 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 `@workflows/verify-features.ts` around lines 637 - 645, Update the "node tail
(bounded)" check so only exit status 0 or timeout status 124 is accepted; reject
other nonzero statuses such as command or broker failures while preserving the
existing timeout-success behavior.
Ran the pipeline for real against the 11.3.0 build with a live broker, cloud
login, and all four provider CLIs: 90 passed, 3 failed, 17 skipped, exit 1.
Fixes below are the workflow's own bugs that the run exposed.
- grep -q under `set -o pipefail`: -q exits at the first match, the upstream
writer takes SIGPIPE, and the pipeline reports failure. A demonstrably
RUNNING broker probed as absent. Fixed at 10 piped call sites, including
run_check itself, where it was a latent false-FAIL generator for any check
whose command produced long output.
- The capabilities step read $CLOUD_API_URL without seeding it, so `set -u`
killed it mid-probe and every provider_* flag went unwritten. Tier 5/6
gating and the NightCTO provider escalation both read a truncated file.
- Token extraction matched any 20+ char string, so a failed registration
("agent already exists") yielded the agent's own NAME as its token and every
downstream call failed for unrelated reasons. Now parses the JSON token
field. Fixed at 7 sites.
- VERIFY_AUTOFIX=0 gated file-issue but not attempt-fix, so the documented off
switch did not stop an agent from editing the tree. The flag is now written
to autofix.env and honoured by attempt-fix and fix-integrity.
- Three checks contradicted signatures the manifest already documented
correctly: `message file upload <path> --channel`, `cloud status <runId>`
(now a stated skip — no disposable run is made), and
`integration webhook create <url>`.
- CP5 asserted the sentinel against the launch response, but `workflow run` is
asynchronous and returns status "running". It now polls the log for the
sentinel and asserts a completed sync, as critical-path 5 documents.
- Added a post-removal assertion in tier 3: `agent remove` reporting success
was assumed to mean the agent was gone. It is not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes what the live verification run found, and one thing that run could not
see because it was measuring a stale build.
## integration webhook create never worked
`POST /v1/webhooks` accepts `{ channel, name? }` and RETURNS the url. The
command took a `<url>` argument and sent `{ url, event }`, so the required
channel was never sent and every invocation failed with "channel is required".
`url` is not a request field at all, and `event` belongs to subscriptions.
- CLI now takes `<channel>` with an optional `--name`, matching create-inbound,
which posts to the same endpoint with the same schema.
- `RelayCreateWebhookInput` corrected from `{ url, event? }` to
`{ channel, name? }`. Callers passing url/event were already failing.
- Verified against the live API: create returns a real webhookId/url/token, and
the workflow's create -> trigger -> delete lifecycle now passes.
## The audit was reading a stale dist
`packages/cli/dist` was older than `packages/cli/src`, so the audit derived a
surface missing 16 shipped commands and reported MANIFEST_CLEAN across several
runs — the same class of error as the sandbox verifying CLI 10.0.0 against an
11.3.0 checkout, and the reason it went unnoticed.
Now documented, so they are covered rather than invisible: `agent me`,
`agent presence`, the `cloud integration` tree (catalog, connect, connections,
disconnect) and the `cloud room` tree (session, revoke-session, members,
remove-member, invite, invites, accept, revoke-invite). Two new manifest
categories, each mapped to a verification procedure. 180 -> 194 features.
The audit now warns when dist is older than src, so a stale build cannot
produce a falsely clean result again.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
scripts/audit-feature-manifest.mjs (2)
192-204: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
child.stdinstill unguarded against EPIPE (unresolved from prior review).
send()writes tochild.stdinwith no error handler; a broken pipe (spawn failure, orfinish()'s ownchild.kill()) can still surface as an uncaught exception outsidemain().catch(...), exiting 1 instead of the intended 2, andworkflows/audit-feature-manifest.tswould then treat the broken audit as confirmed drift.🛡️ Proposed fix
+ child.stdin.on('error', () => finish(null)); + const send = (msg) => child.stdin.write(`${JSON.stringify(msg)}\n`);🤖 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 `@scripts/audit-feature-manifest.mjs` around lines 192 - 204, Guard the `send` helper in the audit request flow against `child.stdin` write failures, including EPIPE caused by spawn failure or `child.kill()`, so they are captured by the existing `main().catch(...)` path and produce the intended exit code 2. Attach appropriate stdin error handling and propagate the failure through `send` without changing the JSON-RPC messages or audit behavior.
381-381: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMCP-null still allows
MANIFEST_CLEAN/exit 0 (unresolved from prior review).When
deriveMcpTools()resolvesnull,undocumentedMcpis forced to[]; combined with no CLI drift,driftcan be 0 and the script exits 0, contradicting the header contract that a broken audit must not read as clean.🤖 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 `@scripts/audit-feature-manifest.mjs` at line 381, Update the audit flow around deriveMcpTools and undocumentedMcp so a null MCP result is treated as an audit failure rather than converted to an empty list. Ensure MANIFEST_CLEAN and exit 0 are unreachable when MCP derivation returns null, while preserving normal undocumented-tool handling for non-null results.workflows/verify-features.ts (3)
1279-1285: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCleanup pattern still broad enough to touch unrelated workspace state (unresolved from prior review).
grep -oE '(vf|cp[0-9])[A-Za-z0-9_-]*'still matches those substrings anywhere in the listed output, so a pre-existing agent/channel containingvforcp<digit>can be removed/archived by this cleanup step.🤖 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 `@workflows/verify-features.ts` around lines 1279 - 1285, Restrict the cleanup loops for relay agents and channels to only match resources created by this workflow, rather than matching vf or cp<digit> substrings anywhere in command output. Update the grep patterns or parsing in both cleanup blocks so names are matched against the complete expected resource-name format and unrelated pre-existing resources are never removed or archived.
161-170: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLedger integrity gap persists (unresolved from prior review).
recordstill only flattens\n\r\t; raw control/ANSI bytes from CLI output can land inside the JSON string. The verdict reader (1337-1354) still dropsJSON.parsefailures silently without counting them, so a corrupted ledger line can vanish rather than fail the run.Also applies to: 1337-1354
🤖 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 `@workflows/verify-features.ts` around lines 161 - 170, Harden the ledger path by updating record() to sanitize all control and ANSI escape bytes before writing JSON, not only newline, carriage-return, and tab characters. Update the verdict reader around JSON.parse to treat parse failures as recorded failures instead of silently discarding corrupted lines, ensuring malformed ledger entries cannot disappear without affecting the run result.
645-646: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
[ $? -le 124 ]still treats real failures as success (unresolved from prior review).Exit 1 (e.g. "unknown command", broker unreachable) satisfies
-le 124, so a removed/brokenrelay node tailstill records PASS.🤖 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 `@workflows/verify-features.ts` around lines 645 - 646, Update the “node tail (bounded)” check in run_check so only a successful command exit or timeout status 124 produces tail-ok; ensure exit codes such as 1 for command or connectivity failures cause the check to fail rather than pass.
🧹 Nitpick comments (1)
workflows/verify-features.ts (1)
904-942: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWebhook token failure isn't fail-fast, unlike the rest of the file's established pattern.
If
WEBHOOK_TOKENextraction fails (empty),relay channel createfor$WEBHOOK_CHANNELis swallowed via|| true, and the subsequentintegration webhook createwould then likely fail for an unrelated reason (missing channel) rather than reflecting a real webhook defect — misattributing the failure. Tier 3/4 already establish the pattern of checking token emptiness up front andskip_check-ing the dependent checks with a clear reason; this block doesn't follow it.🤖 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 `@workflows/verify-features.ts` around lines 904 - 942, Update the webhook setup around WEBHOOK_TOKEN and WEBHOOK_CHANNEL to validate that token extraction succeeded before creating the channel or running dependent integration checks. When WEBHOOK_TOKEN is empty, skip the webhook list, subscription, create, trigger, and delete checks with a clear token-failure reason, while preserving cleanup and the existing cloud-capability handling; only proceed with channel creation and webhook operations when the token is present.
🤖 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 @.agentworkforce/features/manifest.yaml:
- Around line 848-849: Update the description for the “relay integration webhook
create” CLI entry in the manifest to describe it as registering an inbound
webhook, while preserving the existing command and channel behavior.
In `@CHANGELOG.md`:
- Line 8: Update the changelog’s top-level heading from “[Unreleased - Patch]”
to the standard “[Unreleased]” format, keeping release type labels reserved for
versioned release sections.
In `@scripts/audit-feature-manifest.mjs`:
- Around line 247-300: Update the audit’s exit-status decision to treat a
non-null staleBuildWarning() as a non-clean result, even when drift === 0.
Ensure stale builds fail closed or otherwise use the existing unauditable status
consumed by the workflow, while preserving current exit behavior for genuine
drift and clean, current builds.
In `@workflows/verify-features.ts`:
- Around line 1052-1057: Correct the down-state assertion in the CP1
verification flow after `relay node down` so it succeeds only when the complete
status output contains no “running” match, rather than when any individual line
lacks it. Update the `relay node status` pipeline around the final `grep` while
preserving the existing CP1 increment behavior and pipefail-safe redirection.
---
Duplicate comments:
In `@scripts/audit-feature-manifest.mjs`:
- Around line 192-204: Guard the `send` helper in the audit request flow against
`child.stdin` write failures, including EPIPE caused by spawn failure or
`child.kill()`, so they are captured by the existing `main().catch(...)` path
and produce the intended exit code 2. Attach appropriate stdin error handling
and propagate the failure through `send` without changing the JSON-RPC messages
or audit behavior.
- Line 381: Update the audit flow around deriveMcpTools and undocumentedMcp so a
null MCP result is treated as an audit failure rather than converted to an empty
list. Ensure MANIFEST_CLEAN and exit 0 are unreachable when MCP derivation
returns null, while preserving normal undocumented-tool handling for non-null
results.
In `@workflows/verify-features.ts`:
- Around line 1279-1285: Restrict the cleanup loops for relay agents and
channels to only match resources created by this workflow, rather than matching
vf or cp<digit> substrings anywhere in command output. Update the grep patterns
or parsing in both cleanup blocks so names are matched against the complete
expected resource-name format and unrelated pre-existing resources are never
removed or archived.
- Around line 161-170: Harden the ledger path by updating record() to sanitize
all control and ANSI escape bytes before writing JSON, not only newline,
carriage-return, and tab characters. Update the verdict reader around JSON.parse
to treat parse failures as recorded failures instead of silently discarding
corrupted lines, ensuring malformed ledger entries cannot disappear without
affecting the run result.
- Around line 645-646: Update the “node tail (bounded)” check in run_check so
only a successful command exit or timeout status 124 produces tail-ok; ensure
exit codes such as 1 for command or connectivity failures cause the check to
fail rather than pass.
---
Nitpick comments:
In `@workflows/verify-features.ts`:
- Around line 904-942: Update the webhook setup around WEBHOOK_TOKEN and
WEBHOOK_CHANNEL to validate that token extraction succeeded before creating the
channel or running dependent integration checks. When WEBHOOK_TOKEN is empty,
skip the webhook list, subscription, create, trigger, and delete checks with a
clear token-failure reason, while preserving cleanup and the existing
cloud-capability handling; only proceed with channel creation and webhook
operations when the token is present.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f820774-e7e1-4722-b33b-335d886d56b9
📒 Files selected for processing (8)
.agentworkforce/features/manifest.yamlCHANGELOG.mdpackages/cli/src/cli/commands/integration-webhook-create.test.tspackages/cli/src/cli/commands/integration.tspackages/sdk/src/__tests__/integrations.test.tspackages/sdk/src/messaging/types.tsscripts/audit-feature-manifest.mjsworkflows/verify-features.ts
| and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | ||
|
|
||
| ## [Unreleased] | ||
| ## [Unreleased - Patch] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the standard [Unreleased] heading.
Unreleased must not embed a release type; reserve SemVer versions for released sections.
Proposed fix
-## [Unreleased - Patch]
+## [Unreleased]As per coding guidelines, CHANGELOG.md must “maintain the correctly leveled [Unreleased] heading.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## [Unreleased - Patch] | |
| ## [Unreleased] |
🤖 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 `@CHANGELOG.md` at line 8, Update the changelog’s top-level heading from
“[Unreleased - Patch]” to the standard “[Unreleased]” format, keeping release
type labels reserved for versioned release sections.
Source: Coding guidelines
| /** | ||
| * Warn when the built CLI is older than the sources it is derived from. | ||
| * | ||
| * The audit can only see what the built CLI exposes, so a stale `dist` makes it | ||
| * under-report and print MANIFEST_CLEAN while real commands go undocumented. | ||
| * That is exactly what happened here: a stale dist hid 16 shipped commands | ||
| * (`agent me`, `agent presence`, the whole `cloud room` and `cloud integration` | ||
| * trees) across several audit runs that all reported clean. | ||
| * | ||
| * @returns A warning string, or null when the build is current or unknowable. | ||
| */ | ||
| function staleBuildWarning() { | ||
| const distEntry = join(repoRoot, 'packages/cli/dist/cli/index.js'); | ||
| const srcDir = join(repoRoot, 'packages/cli/src'); | ||
|
|
||
| let distMtime; | ||
| try { | ||
| distMtime = statSync(distEntry).mtimeMs; | ||
| } catch { | ||
| return null; // auditing a published CLI, not this checkout | ||
| } | ||
|
|
||
| let newest = 0; | ||
| const walk = (dir) => { | ||
| let entries; | ||
| try { | ||
| entries = readdirSync(dir, { withFileTypes: true }); | ||
| } catch { | ||
| return; | ||
| } | ||
| for (const entry of entries) { | ||
| const full = join(dir, entry.name); | ||
| if (entry.isDirectory()) { | ||
| if (entry.name !== 'node_modules' && entry.name !== 'dist') walk(full); | ||
| continue; | ||
| } | ||
| if (!entry.name.endsWith('.ts') || entry.name.includes('.test.')) continue; | ||
| try { | ||
| newest = Math.max(newest, statSync(full).mtimeMs); | ||
| } catch { | ||
| // unreadable file; ignore | ||
| } | ||
| } | ||
| }; | ||
| walk(srcDir); | ||
|
|
||
| if (newest === 0 || newest <= distMtime) return null; | ||
| const minutes = Math.round((newest - distMtime) / 60_000); | ||
| return ( | ||
| `packages/cli/dist is ${minutes} minute(s) older than packages/cli/src. ` + | ||
| `This audit only sees what the BUILT CLI exposes, so newer commands are invisible ` + | ||
| `and a "clean" result understates drift. Rebuild first: npm run build --workspace=agent-relay` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists and size =="
wc -l scripts/audit-feature-manifest.mjs workflows/audit-feature-manifest.ts 2>/dev/null || true
echo "== relevant function and exit points =="
sed -n '220,320p' scripts/audit-feature-manifest.mjs
printf '\n--- exit/staleBuild usages ---\n'
rg -n "staleBuildWarning|staleBuild|process\.exit|audit-feature-manifest|mjs" scripts scripts workflows 2>/dev/null || true
echo "== workflow snippets =="
if [ -f workflows/audit-feature-manifest.ts ]; then
sed -n '1,240p' workflows/audit-feature-manifest.ts
fiRepository: AgentWorkforce/relay
Length of output: 25982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '360,460p' scripts/audit-feature-manifest.mjs
printf '\n--- workflow audit result logic relevant lines ---\n'
sed -n '780,835p' workflows/audit-feature-manifest.ts
printf '\n--- staleBuild warning references in workflow artifacts/usages ---\n'
rg -n "staleBuildWarning|staleBuild|audit\.json|drift|exit code|exitCode" workflows/audit-feature-manifest.ts workflows 2>/dev/null || trueRepository: AgentWorkforce/relay
Length of output: 15655
Surface stale builds with the audit exit semantics
staleBuildWarning() is only printed and included in audit.json; when drift === 0 the script still exits 0. The workflow treats exit 0 as clean and ignores staleBuildWarning, so a stale dist can pass code-only gates even though the audit no longer reliably reflects the CLI surface.
One option: fail closed (or mark unauditable) without breaking existing drift paths
- process.exit(drift === 0 ? 0 : 1);
+ if (staleBuild && !process.env.VERIFY_ALLOW_STALE_BUILD) {
+ process.exit(2);
+ }
+ process.exit(drift === 0 ? 0 : 1);🤖 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 `@scripts/audit-feature-manifest.mjs` around lines 247 - 300, Update the
audit’s exit-status decision to treat a non-null staleBuildWarning() as a
non-clean result, even when drift === 0. Ensure stale builds fail closed or
otherwise use the existing unauditable status consumed by the workflow, while
preserving current exit behavior for genuine drift and clean, current builds.
Codex and CodeRabbit findings on #1392. Every item is a real hole in code this PR added; each is fixed and exercised. Audit script: - A version match is not provenance. Between releases a globally installed CLI reports the same version as the checkout, so the old gate passed while every check ran the global binary. It now resolves the binary and requires it to live inside this checkout; outside the checkout fails, and a non-git tree is a stated SKIP rather than a pass. - An underivable MCP surface now exits 2. Previously mcpTools === null forced both MCP drift arrays empty, so a broken probe printed MANIFEST_CLEAN and exited 0 — half an audit reported as a clean one. - Stale detection required only that SOME prefix of a documented command exist. Since deriveCliLeaves records every parent group, removing `fleet spawn` left `fleet` and hid the removal. It now requires the exact leaf, with positional-dispatch parents (`telemetry`) named explicitly. - child.stdin now has an error handler. A broken pipe raised an uncaught 'error' — not a rejection, so it escaped main().catch and exited 1, which the audit workflow reads as confirmed drift and acts on. verify-features: - The ledger stripped only \n\r\t, so ANSI/control bytes made a line unparseable and the verdict reader dropped it silently — a failing check could vanish. Control bytes are stripped, and unparseable lines are now counted and fail the run. - `[ $? -le 124 ]` accepted exit 1, so a broken `node tail` scored PASS. Only 0, 124 and 137 now count. - CP1's down-assertion used `grep -v`, which succeeds when ANY line lacks the pattern; a multi-line status still saying running satisfied it. Negated over the whole output instead. - Cleanup matched "vf"/"cp<digit>" anywhere in the listing and could remove unrelated workspace agents and channels. Now anchored to this run's SUFFIX. - `cloud logs` masked a missing <runId> with `|| true` and recorded a false pass off the error text; it is a stated skip like `cloud status`. audit workflow: - AUDIT_AUTOFIX=0 only stopped file-issue. The editor still branched and committed, and open-pr could push it. The flag is written to autofix.env and honoured by the editor and the integrity gate. - Both audit.json consumers parsed it unguarded, though it is written under `|| true` and can be truncated — drift with no alert. Both now degrade to a stated "report unreadable" message. - The integrity gate only protected the audit script. It now asserts the whole changed-file set is the manifest alone, so drift cannot be resolved by deleting a command or relaxing the contract test's expectations. manifest: the webhook entry described an outbound webhook; it is inbound. Not applied: CodeRabbit asked for a plain `[Unreleased]` changelog heading. CLAUDE.md requires the pending release level in the heading (`[Unreleased - Patch]`), so the existing form is correct for this repo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — all of these were real. Addressed in CodexP1 · Verify the checkout rather than matching only its version — Correct, and this is the one that mattered most: between releases a global P1 · Pass the file path and channel flag to upload — Already fixed in P1 · Honor VERIFY_AUTOFIX before launching the fixer — Already fixed in P1 · Honor AUDIT_AUTOFIX before launching the editor — Was still open; fixed the same way. P1 · Fail when the MCP surface cannot be derived — Agreed, and it's the exact failure mode this PR exists to stop. P1 · Require the full CLI leaf when detecting stale entries — Right, and worse than described: since P2 · Supply run IDs to cloud status and logs — CodeRabbitGuard Ledger integrity gap — Fixed on both sides.
Cleanup pattern too broad — Fixed; teardown is now anchored to this run's Unguarded Integrity gate only protects the audit script — Agreed; asserting one file was the wrong shape. It now requires the entire changed-file set to be the manifest alone, so drift can't be resolved by deleting a command or relaxing Describe this as an inbound webhook — Fixed. Not appliedUse the standard Re-verified after the changes: typecheck clean, both workflows |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/audit-feature-manifest.mjs (1)
437-490: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale-build warning still doesn't affect the exit code (unresolved from prior review).
staleBuildWarning()is computed and included in the report/JSON, but Line 489 exits0/1purely ondrift, ignoring it. The downstream workflow (workflows/audit-feature-manifest.ts) treats exit0as "manifest is clean" and skips Slack/issue filing, so a staledistbuild can pass every gate even though the derived CLI surface may not reflect current source. This was flagged in a prior review round and, unlike the sibling EPIPE and MCP-null fixes in this same commit, does not appear to have been addressed here.♻️ One option: fail closed (or mark unauditable) on a stale build
+ if (staleBuild && !process.env.VERIFY_ALLOW_STALE_BUILD) { + process.exit(2); + } process.exit(drift === 0 ? 0 : 1);🤖 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 `@scripts/audit-feature-manifest.mjs` around lines 437 - 490, Update the audit result and exit-status logic around staleBuild and the final process.exit call so a stale build cannot produce a successful manifest audit. Treat staleBuild as a failure or otherwise mark the audit unauditable, ensure the human-readable status reflects that outcome, and preserve clean success only when drift is zero and no stale-build condition exists.
🤖 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 `@workflows/verify-features.ts`:
- Around line 1396-1405: Update the ledger parsing flow in
workflows/verify-features.ts to retain every nonblank line, rather than
filtering only lines whose trimmed content starts with “{”. Pass each retained
line through JSON.parse so truncated or corrupted records increment
malformedLines and cannot be silently omitted.
- Around line 1097-1101: Update the CP1 status check in the relay node
verification flow so a failed relay node status command does not increment CP1
as “broker down.” Require relay node status to complete successfully before
evaluating whether its output lacks “running,” or explicitly accept only the
documented down-state exit code while treating other failures as errors.
---
Outside diff comments:
In `@scripts/audit-feature-manifest.mjs`:
- Around line 437-490: Update the audit result and exit-status logic around
staleBuild and the final process.exit call so a stale build cannot produce a
successful manifest audit. Treat staleBuild as a failure or otherwise mark the
audit unauditable, ensure the human-readable status reflects that outcome, and
preserve clean success only when drift is zero and no stale-build condition
exists.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 394920c0-7fed-484e-9ab9-fee654c66d75
📒 Files selected for processing (4)
.agentworkforce/features/manifest.yamlscripts/audit-feature-manifest.mjsworkflows/audit-feature-manifest.tsworkflows/verify-features.ts
| # "grep -v" succeeds when ANY line lacks the pattern, so a multi-line status | ||
| # that still says running satisfied it. Negate the match over the whole output. | ||
| if ! relay node status --state-dir "$CP1_STATE" 2>&1 | grep -i running >/dev/null; then | ||
| CP1=$((CP1 + 1)) | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not count a failed status command as “broker down.”
Line 1099 negates the entire pipeline, so an errored relay node status with no running text increments CP1. Require a successful status call before accepting the down assertion (or explicitly handle its documented down exit code).
Proposed fix
-if ! relay node status --state-dir "$CP1_STATE" 2>&1 | grep -i running >/dev/null; then
+CP1_STATUS=$(relay node status --state-dir "$CP1_STATE" 2>&1)
+CP1_STATUS_RC=$?
+if [ "$CP1_STATUS_RC" -eq 0 ] &&
+ ! printf '%s\n' "$CP1_STATUS" | grep -i -- running >/dev/null; then
CP1=$((CP1 + 1))
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # "grep -v" succeeds when ANY line lacks the pattern, so a multi-line status | |
| # that still says running satisfied it. Negate the match over the whole output. | |
| if ! relay node status --state-dir "$CP1_STATE" 2>&1 | grep -i running >/dev/null; then | |
| CP1=$((CP1 + 1)) | |
| fi | |
| # "grep -v" succeeds when ANY line lacks the pattern, so a multi-line status | |
| # that still says running satisfied it. Negate the match over the whole output. | |
| CP1_STATUS=$(relay node status --state-dir "$CP1_STATE" 2>&1) | |
| CP1_STATUS_RC=$? | |
| if [ "$CP1_STATUS_RC" -eq 0 ] && | |
| ! printf '%s\n' "$CP1_STATUS" | grep -i -- running >/dev/null; then | |
| CP1=$((CP1 + 1)) | |
| fi |
🤖 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 `@workflows/verify-features.ts` around lines 1097 - 1101, Update the CP1 status
check in the relay node verification flow so a failed relay node status command
does not increment CP1 as “broker down.” Require relay node status to complete
successfully before evaluating whether its output lacks “running,” or explicitly
accept only the documented down-state exit code while treating other failures as
errors.
| const raw = fs | ||
| .readFileSync(ledgerPath, 'utf8') | ||
| .split('\n') | ||
| .filter((line) => line.trim().startsWith('{')); | ||
| for (const line of raw) { | ||
| try { | ||
| checks.push(JSON.parse(line)); | ||
| } catch { | ||
| malformedLines += 1; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Parse every nonblank ledger line.
The { filter silently drops truncated or otherwise corrupted nonblank records before JSON.parse, leaving malformedLines at zero. A lost failing record can still yield a PASS when its tier has other valid entries.
Proposed fix
.readFileSync(ledgerPath, 'utf8')
.split('\n')
- .filter((line) => line.trim().startsWith('{'));
+ .filter((line) => line.trim() !== '');🤖 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 `@workflows/verify-features.ts` around lines 1396 - 1405, Update the ledger
parsing flow in workflows/verify-features.ts to retain every nonblank line,
rather than filtering only lines whose trimmed content starts with “{”. Pass
each retained line through JSON.parse so truncated or corrupted records
increment malformedLines and cannot be silently omitted.
CI caught two tests in relaycast-groups.test.ts still driving `webhook create <url> --event`, which now fails with "unknown option '--event'". My mistake: I changed a public CLI signature and verified only the tests I had touched instead of the full suite. - relaycast-groups.test.ts: both cases moved to `<channel> [--name]`. - verify/procedures.md documented the same broken invocation, and built the whole webhook procedure on an outbound model the API does not have — it registered CAPTURE_URL as a delivery destination. `POST /v1/webhooks` is inbound only: it takes the channel and returns the URL external services post to. Corrected, and the surrounding note now states the inbound flow. Full suite green: 1644 passed, 16 skipped. (integration-relayfile-contract flaked once under full-suite ordering and passed standalone and on re-run; it references nothing in this change.) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Why
A
verify-featuresrun reportedCOMPLETED — 11 passed, 0 failedwhile Tier 2 had four failing checks. This makes that impossible, then makes a failure actionable.The false green had three causes, all fixed
failOnError: false, so a non-zero exit scored as a pass. Tiers still use it (all tiers should run even after one fails), but they are no longer the verdict — each check appendspass/fail/skiptochecks.jsonland averdictstep aggregates it.verdict+ a terminalenforce-verdictnow do, andmain()exits non-zero.main()discarded the runner result. It now readsverdict.jsonand fails closed when that file is missing.A fourth defect nobody asked about
applyReliabilityDefaultsin@relayflows/coreforce-enablesstrategy: "retry"with a repair agent for any workflow declaring agents. On the pasted run,tier1-cli-healthfailed, thereporteragent was handed the gate, edited two files, and it passed. A verification workflow whose assertions an agent can rewrite until they pass measures nothing. Fixed withonError('continue'), the documented opt-out, confirmed via the built config (errorHandling: {"strategy":"continue"}).The run also verified the wrong CLI
The sandbox ran agent-relay 10.0.0 against an 11.3.0 checkout — it logged
Update available: 10.0.0 → 11.3.0, then reportedrelay node deadlettersas unknown. That is a real command in 11.x (registerCoreCommands(node, …),packages/cli/src/cli/commands/core.ts:394). A whole run measured a stale published CLI and produced a red herring. Aprovenancestep now fails on CLI/repo version drift.Honest accounting
cloud whoami/cloud sessionwere scored as product failures when the sandbox simply wasn't cloud-authed. Acapabilitiesprobe now turns unrunnable checks into SKIPs with a stated cause — never a pass, never a fail.verdictalso fails the run when a tier produced zero records, so a tier that crashes before its first check cannot masquerade as clean.What happens on failure now
Slack alert → GitHub issue → fix attempt on a branch → draft PR with re-verified evidence. An integrity gate refuses a branch that reduced verification call sites, left the repo on
main, or has no root cause on record — a fixer's cheapest path to green is always to weaken the assertion.Harness-level breakage (no verdict, tier not run, broker never started, no provider CLI) escalates to NightCTO via the
CloudEvidenceSummary v1contract. Run and per-failure events go to PostHog, posted directly rather than through the CLI's telemetry client — that one is opt-in product telemetry keyed to a real user.Slack goes through
@relayflows/slack-primitivepinned to its cloud-relay runtime (/api/v1/slack/post-message, the workspace's Slack integration). Pinning matters: auto-detection falls back to a localSLACK_BOT_TOKENruntime, so a stray token silently changed the route. This also fixes a wrong endpoint — the previous fallback used/api/v1/integrations/slack/messages, which does not exist.Coverage
Tiers 1–6 and all six critical paths, including a real MCP JSON-RPC handshake asserting the required tool set (previously just
relay mcp --help).Is the feature map current? No — and the old check couldn't tell
manifest-contract.test.tsasserts a hardcoded list of ~110 commands, so a newly added command is absent from both the manifest and the expectation list and passes silently.scripts/audit-feature-manifest.mjsderives the surface instead (recursive--help+tools/list).Its first run found
fleet spawnandfleet releaseshipping undocumented, therefore unverified. Both added; audit reportsMANIFEST_CLEAN.workflows/audit-feature-manifest.tsruns this on a schedule with the same Slack/issue/PR/NightCTO/PostHog treatment. Exit 2 (audit broke) is deliberately distinct from exit 0 — a broken audit must never read as a clean manifest.Verification
Ran and confirmed:
Validation: PASS.sh -n.enforce-verdictexits 0/1/2 correctly.recordcalls (a bug I found and fixed mid-review: the counters lived in the callers, so multi-step checks logged0 passed, 0 failedwhile the ledger held real results).C0AEKNLDNKW. WithSLACK_BOT_TOKENset andCLOUD_API_*absent it fails loudly instead of falling back.Not verified: no live end-to-end run — the tiers need a broker, workspace, cloud auth, and provider CLIs. The
gh issue create/gh pr createpaths were deliberately not executed, since that would file real issues.Deliberate omissions
relay-feature-guardian. (There is contrary precedent at line 168.)cloud run,fleet spawn, OAuth logins) are recorded as explicit SKIPs with reasons rather than attempted, so the coverage hole is visible instead of hidden.🤖 Generated with Claude Code
Live end-to-end run (replaces the earlier "not verified" caveat)
Ran the whole pipeline for real against the 11.3.0 build with a live broker, an authenticated cloud session, and all four provider CLIs present. Isolated working directory; the repo checkout was never touched.
Result: 90 passed, 3 failed, 17 skipped → verdict FAIL,
enforce-verdictexit 1.Tier 6 and CP3 spawned, held, resumed, and released a real Claude agent. CP4 completed a real MCP JSON-RPC handshake. CP5 ran a real local workflow and asserted the sentinel plus a completed sync.
relay node deadletterspasses against 11.3.0, confirming the original sandbox failure was purely the stale 10.0.0 CLI — exactly what the newprovenancestep now catches.The run found 3 real product bugs
Each reproduced independently, outside the harness, with clean fixtures. Left failing rather than worked around.
relay message file uploadcannot succeed. Three distinct errors as the invocation was corrected, ending at server-sideInvalid attachments: file ids must exist in workspace and be complete. Also,--helpdocuments--textas defaulting to"", but the API rejects empty text withtext is required— the documented default cannot work.relay integration webhook create <url>returnschannel is required, but--helpexposes no--channeloption. The command as documented cannot succeed.relay agent removefails for any agent that has sent a message, leaking a raw SQL error:Failed query: delete from "agents" where "agents"."id" = ?. Exit code is correctly 1, but the message is an internal query dump. This silently accumulated 16 test identities in the workspace during these runs, while every tier still reported clean — so tier 3 now asserts the agent is actually gone after removal instead of assuming it.relay fleet configalso fails withRelaycastMessagingClient.workspace.fleetNodes requires @relaycast/sdk with the workspace fleet nodes API, which looks like a dependency/capability gap rather than a code defect.And 8 bugs in this workflow, now fixed
grep -qunderset -o pipefail—-qexits at the first match, the upstream writer takes SIGPIPE, and the pipeline reports failure. A demonstrably RUNNING broker probed as absent. Fixed at 10 piped sites includingrun_check, where it was a latent false-FAIL generator for any long-output check.capabilitiesdied mid-probe — it read$CLOUD_API_URLwithout seeding it, soset -ukilled the step and everyprovider_*flag went unwritten, silently degrading tier 5/6 gating.tokenfield (7 sites).VERIFY_AUTOFIX=0did not stop the fixer — it gatedfile-issuebut notattempt-fix, so the documented off switch still let an agent edit the tree.message file upload <path> --channel,cloud status <runId>,integration webhook create <url>. Good evidence the manifest is worth keeping accurate.workflow runis async and returnsstatus: "running", so the sentinel could never appear there. It now polls the log as critical-path 5 documents.Still not exercised
Slack delivery was verified against a stub server (correct path, bearer auth, channel
C0AEKNLDNKW) rather than live — this machine has a cloud session but noCLOUD_API_URL/CLOUD_API_TOKENin the environment, so theslackcapability probes 0. Thegh issue create/gh pr createpaths were deliberately not executed, since that would file real issues.Note: these runs left ~16 test agents in the Default workspace that cannot be deleted because of product bug 3 above.
Bugs fixed (follow-up to the live run)
integration webhook create— fixed herePOST /v1/webhooksaccepts{ channel, name? }and returns the url. The command took a<url>argument and sent{ url, event }, so the required channel was never sent and every call failed withchannel is required.urlis not a request field at all;eventbelongs to subscriptions.<channel>with an optional--name, matchingcreate-inbound— which posts to the same endpoint with the same schema (relay.js:428-429), making them true aliases.RelayCreateWebhookInputcorrected from{ url, event? }to{ channel, name? }. Anyone passingurl/eventwas already failing at runtime, so this breaks no working code.url/eventare never sent.webhookId/url/token, and the workflow's create → trigger → delete lifecycle now passes. Tier 5 went from 12p/2f to 15p/1f.The audit had been reading a stale build
packages/cli/distwas older thanpackages/cli/src, so the audit derived a surface missing 16 shipped commands and printedMANIFEST_CLEANacross several runs. Same class of error as the sandbox verifying CLI 10.0.0 against an 11.3.0 checkout — and the reason it went unnoticed.Now documented:
agent me,agent presence, thecloud integrationtree (catalog, connect, connections, disconnect) and thecloud roomtree (session, revoke-session, members, remove-member, invite, invites, accept, revoke-invite). Two new manifest categories, each mapped to a verification procedure. 180 → 194 features.The audit now warns when
distis older thansrc, so a stale build cannot yield a falsely clean result again.Filed, not fixable in this repo
relay message file uploadcannot succeed.serializeAttachmentInputsJSON-stringifies{type,path}and sends it where a file id is required; the file is never uploaded. The API needs a three-step presigned upload and@relaycast/sdkexposes no files API, so the SDK must gain one first.relay agent removefails for any agent with message history (likely an FK) and leaksFailed query: delete from "agents" where "agents"."id" = ?. Server-side; the CLI only callsagents.delete.relay fleet configfails on@relaycast/sdkmissing the workspace fleet-nodes API.All three reproduced independently with clean fixtures. Their checks are left failing rather than skipped, so the run stays red until they are fixed.