fix(cli): make help and single-step summaries readable - #279
Conversation
Session-Id: 01a08ba4-4473-7903-b7c3-e778edb9b523
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
Review swarm: maintainabilityMaintainability Review: PR #279PR Title: fix(cli): plug the shakedown-found holes in the --help / verbs surface SummaryThis PR makes three changes to
Maintainability AssessmentFinding 1: The USAGE join change is unexplained and breaks the implicit contractLocation: Issue: The diff changes The USAGE constant is an array of 7 usage lines. Before this change, joining with space would create a single long line — which is semantically wrong for usage help but would have been visible as broken output. The change to newline fixes the display bug, but:
Risk: In six months, someone fixing another CLI bug will not know if the newline join is load-bearing or if some callers expect a different format. The dual emission paths (direct stdout vs. inputFailureReport) create an implicit contract that is nowhere written. What's missing: A comment stating "USAGE is emitted verbatim in --help handling and embedded in invalid_invocation reports; the newline join controls both formats." Finding 2: The --help early return bypasses parseArgs, creating a semantic inconsistencyLocation: Issue: The new --help handling runs before if (args.length === 1 && (args[0] === '--help' || args[0] === '-h')) {
io.stdout(USAGE);
return 0;
}This early return only works for The semantic inconsistency:
A maintainer reading this in six months will wonder: is The code does not answer these questions. The only hint is the structure itself, which a reader must reverse-engineer: "it checks length === 1, so multi-arg --help must be intentionally excluded." Risk: Future maintainer tries to add subcommand-specific help (e.g., What's missing:
Finding 3: The pluralization fix is correct but the broader output contract is unclearLocation: Issue: The change from function emitRunReport(execution: RunExecution, json: boolean, io: CliIo): void {
const { report } = execution;
emitDiagnostics(report.diagnostics, io);
if (json) {
io.stdout(JSON.stringify(report));
return;
}
// ... human-readable formatting
}When The contract question: What happens if const completed = report.completedSteps === undefined ? '' : ` (${report.completedSteps} ${report.completedSteps === 1 ? 'step' : 'steps'})`;This checks
A maintainer looking at a run report that says The broader issue:
Why does Risk: In six months, a maintainer debugging why a parked run shows What's missing: A comment: "completedSteps and completionReason are optional; they are omitted from output when undefined. status defaults to 'unknown' rather than being omitted because every run has a lifecycle state (running/succeeded/failed/parked), but the report may not have captured it yet." Finding 4: The --help addition does not gate itself against the spawn/data-dir machineryLocation: Issue: The new --help block is an unconditional early return at the top of Compare this to the explicit daemon-free guarantee for // Deliberately daemon-free (kernel/DAEMON-LIFECYCLE.md §4). `checkFlow` is
// a pure compile-and-preflight that opens no socket, and the parser
// refuses `--data-dir` on `check`, so there is no data dir to attach to.
// `flows check` keeps working with no daemon, no relayflowd binary and no
// data directory at all -- a property worth keeping, not an omission.The The failure mode: Someone refactors Risk: In six months, when the CLI gains a telemetry system or crash reporter that initializes early, --help will start sending telemetry pings. A user running What's missing: A comment at line 73: "Help must remain side-effect-free: no daemon spawn, no socket open, no state reads. This early return is structural, not a micro-optimization." Boundary Issues Not Addressed by This PRThese are pre-existing gaps that this PR does not introduce but makes more visible:
Tests That Would Fail If the Behavior BrokeThis is the critical maintainability question. Looking at the changes:
Implication: All three changes are unguarded by tests. A future refactor of Missing Failure HandlingNone. The changes are purely presentational (help formatting, pluralization). The error paths are unchanged, and the early --help return explicitly avoids new failure modes by skipping all execution logic. Comments That Assert What the Code Does Not DoNone in this PR. However, the pre-existing comment at lines 88-92 ("Deliberately daemon-free") is an example of the correct pattern: it asserts a property the code must preserve even though the code does not explicitly enforce it. The --help block would benefit from the same treatment. VerdictThe changes are clean micro-fixes that improve user experience. The maintainability risk is not in the changes but in the lack of specification around them:
A stranger reading this code in six months will be able to trace what it does, but not why certain choices were made or what properties must hold. That is the classic maintainability gap: the code is correct, but its invariants are invisible. Per AGENTS.md and RFC-0001 covenant 1 ("easy to read"), a technical founder should be able to read this code and say what it does. They can. But they cannot say what it must continue to do (newlines in USAGE? exit 0 for --help? side-effect-free help?), and that is the specification gap that makes safe changes harder. RecommendationThe changes improve the CLI's user-facing behavior. The core logic is sound. The missing piece is specification of intent: comments stating the contracts and tests guarding the behavior. In a codebase building toward RFC-0001's nine gates, where "fail closed" (covenant 2) and "no silent fallbacks" are constitutional, a --help path that is accidentally side-effect-free is a lower-grade risk than, say, a credential writeback that silently swallows errors. But the pattern matters: implicit correctness does not scale. The PR is shippable as-is from a correctness standpoint. From a maintainability standpoint, it leaves the --help surface underspecified. A follow-up adding:
...would close the gap. REVIEW_PASSED |
Review swarm: historyPR #279 — history reviewVerdict: PASS. No blocking finding through the requested history lens. Reviewed head: Does this change fit the story?Yes. The three hunks in
Review environment and limitsThe initial literal command The requested This is a static history review, not a runtime correctness or test-suite signoff. Captured evidenceCommandgit rev-parse HEAD HEAD^Captured output: Exit code: 0. Commandcat .review-target/pr.jsonCaptured output: Exit code: 0. Commandgit log --oneline -40Captured output: Exit code: 0. Commandgit show --format=fuller --stat HEADCaptured output: Exit code: 0. Commandgit diff HEAD^ HEAD -- packages/sdk/src/cli.tsCaptured output: Exit code: 0. Commandgit diff HEAD^ HEAD -- packages/sdk/src/cli.ts | cmp - .review-target/pr.diffCaptured output: Exit code: 0. Commandgit diff --exit-code HEAD -- packages/sdk/src/cli.tsCaptured output: Exit code: 0. Commandgit log --follow --oneline -- packages/sdk/src/cli.tsCaptured output: Exit code: 0. Commandgit log --follow --oneline -G '\-\-help|\-h' HEAD^ -- packages/sdk/src/cli.tsCaptured output: Exit code: 0. Commandgit show 5bbbe6ec --format=short -- packages/sdk/src/cli.tsCaptured output: Exit code: 0. Commandsed -n '3019,3043p' ops/DRIVE-LOG.mdCaptured output: Exit code: 0. Commandsed -n '7117,7138p' ops/DRIVE-LOG.mdCaptured output: #249 branch commit 2026-09-09 15:14:28 +0200 Exit code: 0. Commandsed -n '/### 2026-09-10 10:43Z/,/### 2026-09-10 11:56Z/p' ops/DRIVE-LOG.mdCaptured output: Exit code: 0. Commandcat ops/NEXT.md ops/DIRECTIVES.mdCaptured output: test -n "$CLOUD_API_URL" grep -c CLOUD_API_ACCESS_TOKEN_EXPIRES_AT README.md # already 0 bash -n .github/workflows/scripts/swarm-post.sh && python3 -c "import yaml; yaml.safe_load(open('.github/workflows/review-swarm.yml'))" && grep -i "whitelist|github.event.pull_request.user.login" .github/workflows/review-swarm.yml || echo "No author whitelist found (GOOD)" git status --porcelain Exit code: 0. Transcript whitespace check
REVIEW_PASSED |
Review swarm: structureNo fresh transcript was produced for run |
Review swarm: FAILED
Cloud run: |
kjgbot
changed the title
maintainability lens — PASSMaintainability review — PR #279 (
|
history lens — PASSBlockers: none. PR #279 passes the HISTORY lens’s three rejection criteria.
Concerns: No blocker-level concern. The PR adds no automated regression tests for the new help branch or singular wording; that is a coverage consideration outside this lens’s permitted rejection criteria. Notes: I ran REVIEW_PASSED |
structure lens — MISSING |
|
🎯 review-swarm: FAILED (M:pass H:pass S:missing) Lens transcripts posted as sibling comments above. |
* feat(surface): github.check_run and github.issue_comment triggers Regenerated `packages/surface/src/triggers/github.ts` from the core GitHub mapping in AgentWorkforce/relayfile-adapters#279, which declares `check_run` and `issue_comment` as action-bearing webhook keys. The surface gains `github.check_run(action?)` and `github.issue_comment(action?)` — the two events a PR reviewer needs for merge-on-green and comment-driven directives — and `providerEventTypes.github` lists them, so `flows check` admits a subscription to either instead of refusing it as unpublished. Generated with the adapters checkout's `packages/core/mappings` alone, which is byte-for-byte what the published `@relayfile/adapter-core` tarball will carry, so `generate-triggers.mjs --check` reproduces these files once the SDK's pinned adapter-core is bumped to the release that contains #279. Until that bump the check refuses, by design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(sdk): flows deploy --on github:events=pull_request The GitHub source setting `events` (AgentWorkforce/cloud#3772) selects which records wake a listener: `issues` (default) or `pull_request`. The CLI validates it client-side like the other settings and the doc describes the pull-request run's input. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(sdk): pin @relayfile/adapter-core 0.5.25 The release carrying relayfile-adapters#279 (`check_run` and `issue_comment` webhook keys). `generate-triggers.mjs` against the installed tarball reproduces the committed trigger modules byte for byte, and `--check` passes again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(sdk): send the github events setting as Cloud's lowercase enum Validation was case-insensitive but the caller's spelling was serialized, so `events=PULL_REQUEST` passed the CLI and failed at Cloud (Devin). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Relayflow Lead <lead@relayflows.local> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…bhooks (#456) * feat(surface): github.check_run and github.issue_comment triggers Regenerated `packages/surface/src/triggers/github.ts` from the core GitHub mapping in AgentWorkforce/relayfile-adapters#279, which declares `check_run` and `issue_comment` as action-bearing webhook keys. The surface gains `github.check_run(action?)` and `github.issue_comment(action?)` — the two events a PR reviewer needs for merge-on-green and comment-driven directives — and `providerEventTypes.github` lists them, so `flows check` admits a subscription to either instead of refusing it as unpublished. Generated with the adapters checkout's `packages/core/mappings` alone, which is byte-for-byte what the published `@relayfile/adapter-core` tarball will carry, so `generate-triggers.mjs --check` reproduces these files once the SDK's pinned adapter-core is bumped to the release that contains #279. Until that bump the check refuses, by design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(sdk): flows deploy --on github:events=pull_request The GitHub source setting `events` (AgentWorkforce/cloud#3772) selects which records wake a listener: `issues` (default) or `pull_request`. The CLI validates it client-side like the other settings and the doc describes the pull-request run's input. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(sdk): pin @relayfile/adapter-core 0.5.25 The release carrying relayfile-adapters#279 (`check_run` and `issue_comment` webhook keys). `generate-triggers.mjs` against the installed tarball reproduces the committed trigger modules byte for byte, and `--check` passes again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(surface): trigger namespaces for every relayfile adapter with webhooks `scripts/generate-triggers.mjs` read only the mapping YAML that @relayfile/adapter-core bundles, and that was the two core fallbacks, so `flow().on(...)` could subscribe to Slack and GitHub and nothing else, although relayfile ingests events from 47 providers. The generator now applies three sources per provider: the core fallback mappings, each adapter's own mapping (`mappings/adapters/` in the package since relayfile-adapters#280, or `packages/<adapter>/` in a checkout) which supersedes the fallback as a whole, and the trigger catalog (`@relayfile/adapter-core/triggers`, fed by every adapter's `supportedEvents()`) for providers with no `webhooks:` block. Mapping-backed providers keep payload-aware signatures (`github.pull_request(action?)`); catalog-backed ones get `(filter?)`. Result: 47 namespaces, 502 events; `github` and `slack` are supersets of what #446 generated, so `--on github:events=` and the six fallback GitHub events are unchanged. Also: a reserved-namespace guard (`webhook`, `flow`, `schedule`, …), a generated `PROVIDERS.md` table covered by `--check`, and hyphenated ids mapped to identifiers (`azure_blob`, `google_drive`) with upstream spelling kept in the lowered filter. Merge condition: adapter-core published with relayfile-adapters#280 and the SDK pin bumped; until then `generate-triggers.mjs --check` (and its test) report github.ts/gitlab.ts/index.ts/PROVIDERS.md drift against 0.5.25 — 45 of the 47 providers already generate identically from the catalog it ships. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(surface): satisfy the strict test tsconfig in the all-providers trigger test Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(surface): union catalog events into mapping-backed providers Devin on #456: a provider with any `webhooks:` block lost every event the catalog listed but the mapping did not — gitlab kept 8 of the 53 it delivers, so ingress would refuse the other 45 and `flows check` had no namespace for them. The mapping describes payload shape for some events, never the delivered set; `supportedEvents()` does. Catalog events are now unioned into every provider with the plain `(filter?)` signature; a mapping-declared event keeps its signature. Where two upstream names mangle to one identifier (slack publishes both `reaction.added` and `reaction_added`) the mapping-declared event owns the method and the other remains in `providerEventTypes`, subscribable via `webhook(provider, { provider, type })` and listed in PROVIDERS.md. 47 providers, 570 events (was 502): gitlab 53, github 26, slack 21. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(surface): action-qualified events take a plain filter, never a second action Cursor on #456: `pull_request_edited(action?)` pinned the type to `pull_request.edited` and still accepted an action, a dual vocabulary that is easy to misuse. Only an aggregate event whose mapping extracts `action` takes one now (`pull_request`, `check_run`, `issue_comment`). Both spellings stay, because two ingresses deliver them: the aggregate form is what raw GitHub and the local receiver carry, the action-qualified form is what relayfile's Cloud ingress normalizes to; the README says which to use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Relayflow Lead <lead@relayflows.local> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
flows --helpandflows -hnow print one usage form per line to stdout and exit 0; unknown options still exit 2. Single-step summaries now say(1 step)instead of(1 steps).Reproduced during the launch shakedown on main a42ca16. This small PR is based directly on main; it does not modify #268/#269 branches.
Validation on this change (full captured output in the shakedown report):
cargo test -p relayflowdexited 0. Final literal output:Note
Low Risk
Small UX-only changes to help handling and run output text; no auth, execution, or parsing behavior beyond the new early help exit.
Overview
Improves the
flowsCLI help and run output polish found during launch shakedown.--help/-h: Invokingflowswith only--helpor-hnow prints the usage block to stdout (one subcommand per line) and exits 0. Invalid invocations still emit the same usage via the existing refusal path and exit 2.Usage formatting: The shared
USAGEstring is joined with newlines instead of spaces, so help and error messages list each command on its own line.Run summary: Completed-run lines use correct singular/plural —
(1 step)whencompletedSteps === 1, otherwise(N steps).Reviewed by Cursor Bugbot for commit 1790921. Bugbot is set up for automated code reviews on this repo. Configure here.