feat(cli): round-tables subcommand group with watch daemon

[dundas]

Summary

Adds admp round-tables to the CLI — a full CRUD subcommand group plus a daemon-style watch loop.

• create — POST /api/round-tables
• list — GET /api/round-tables with optional --status filter
• get — full session + thread dump
• speak — contribute a message
• resolve — close with outcome (facilitator only)
• watch — daemon loop: polls on interval, emits new entries, fires --on-speak hook, auto-exits on resolved/expired

watch daemon design

Modeled on the Teleportation daemon pattern:

• Sleep loop to avoid drift on slow polls
• Cursor = lastLength (thread is append-only, no server cursor needed)
• Transient poll errors: warn + continue
• --on-speak : /bin/sh per entry, entry JSON piped to stdin, fire-and-forget
• --json mode: clean event stream on stdout, warnings on stderr
• SIGTERM/SIGINT: entries-seen summary, exit 0
• Auto-exit: resolved -> 0, expired -> 1

Test plan

• admp round-tables --help shows all 6 subcommands
• bun run build passes with zero errors
• admp round-tables list against live server
• admp round-tables watch polls and prints new entries
• --on-speak hook fires per entry
• --json emits clean event stream
• Ctrl+C exits 0 with summary

Generated with Claude Code

[claude]

Code Review

Overall this is a clean, well-structured addition. The daemon design is thoughtful (signal handling, cursor-based replay avoidance, transient-error resilience) and the help text / examples are genuinely useful. A few issues worth fixing before merge:

---

Bug: entries_seen reports total thread length, not new entries

In the signal handler lastLength is initialised to rt.thread.length (the pre-existing entry count), so the summary is off for any session that already had entries when watch started.

// current (misleading)
entries_seen: lastLength   // includes entries that existed before watch started

// fix: capture initial length before the loop
const initialLength = rt.thread.length;
// ... then in handleSignal:
entries_seen: lastLength - initialLength

---

validateRtId duplicates existing shared validators

cli/src/validate.ts already exports validateMessageId and validateGroupId, both with the identical ^[\w\-]+$ regex. The existing groups.ts imports from there. round-tables.ts introduces a local copy instead.

Prefer adding validateRoundTableId to validate.ts and importing it — keeps the validation surface in one place and makes it consistent with the established pattern.

---

Missing automated tests

CLAUDE.md requires: "Add or update tests for behavior changes." The PR test plan is entirely manual checkboxes. There are existing unit-test files in cli/src/ (client.test.ts, config.test.ts, etc.) that show the pattern.

At a minimum, unit tests for:

• validateRtId (valid IDs pass, path-traversal attempts are rejected)
• the interval parser in watch (boundary values: 499 → error, 500 → ok)
• entries_seen calculation in the signal handler (the bug above)

---

fireHook: child.stdin is not null-checked

With stdio: ['pipe', ...] Commander will always set stdin, but TypeScript types stdin as Writable | null. If for any reason the spawn fails silently before the 'error' event fires, child.stdin.write(...) will throw uncaught.

if (child.stdin) {
  child.stdin.write(JSON.stringify(entry) + '\n');
  child.stdin.end();
}

---

No ceiling on consecutive poll errors

The watch loop warns and continues forever on persistent errors (e.g. server goes away, token expires). After N consecutive failures it should either exit with a non-zero code or at least print a louder warning. Without this, --on-speak pipelines will silently stall indefinitely with no actionable signal to the operator.

---

Minor nits

• --on-speak hooks accumulate unboundedly: multiple entries can arrive in a single poll; each spawns an independent /bin/sh process fire-and-forget. Worth documenting this in help text, or at least noting that hooks are concurrent and unordered.
• --no-exit-on-close semantics: the flag name implies "do not exit when closed", but the default (exitOnClose: true) is never explicitly stated in the description text. A note like (default: exit on resolve/expire) in the help text would help.
• Column widths in list: the TOPIC column is hard-coded to padEnd(42) but truncation happens at 40 + … (1 char) = 41 chars. The padding width is fine but a comment explaining the off-by-one would prevent future confusion.

---

Summary

Severity	Issue
Bug	entries_seen off-by-initial-thread-length
Convention	validateRtId should live in validate.ts
Required (CLAUDE.md)	No automated tests
Low	child.stdin null-safety
Low	No consecutive-error limit in poll loop
Nit	Hook concurrency / --no-exit-on-close docs

Happy to help with any of these if useful.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e409465a19

ℹ️ 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".

[chatgpt-codex-connector]

Handle hook stdin EPIPE to avoid crashing watch loop

Writing each entry to child.stdin without handling stream errors can terminate the entire watch process when the hook exits before reading stdin (for example --on-speak 'true' or other short-lived commands). In that case Node emits EPIPE on the stdin socket, and because no error listener is attached to child.stdin, the process crashes instead of continuing to poll.

Useful? React with 👍 / 👎.

[greptile-apps]

Greptile Summary

Adds admp round-tables CLI subcommand group with full CRUD operations (create, list, get, speak, resolve) and a watch daemon for real-time monitoring. The implementation follows established CLI patterns from other commands like groups and outbox, with proper signature authentication, input validation, and JSON mode support throughout.

Key additions:

• CRUD commands map cleanly to /api/round-tables endpoints with appropriate validation
• Watch daemon implements polling loop with configurable interval, cursor-based tracking (lastLength), graceful signal handling, and optional hook execution per new entry
• Hook mechanism (--on-speak) pipes entry JSON to /bin/sh commands in fire-and-forget mode
• Auto-exit behavior on resolved (exit 0) or expired (exit 1) status when --exit-on-close is enabled

Integration notes:

• All commands properly require agent_id, secret_key, and base_url from config
• ID validation uses [\w\-]+ regex matching server-side expectations
• Participant parsing splits CSV input and filters empty strings
• Poll errors are transient (warn + continue) rather than fatal

Confidence Score: 4/5

• Safe to merge with minimal risk
• Code follows established patterns, has comprehensive error handling, and properly integrates with existing API endpoints. The watch daemon design is sound with appropriate signal handling and cursor tracking. Minor edge case exists if server violates append-only thread assumption, but this is a design contract violation rather than a CLI bug.
• No files require special attention

Important Files Changed

Filename	Overview
cli/src/index.ts	Adds import and registration for round-tables command module, following existing pattern
cli/src/commands/round-tables.ts	Implements complete CRUD operations plus watch daemon for Round Tables with proper validation, error handling, and signal management

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as admp watch
    participant API as /api/round-tables/:id
    participant Hook as --on-speak command

    User->>CLI: admp round-tables watch rt_123 --on-speak 'handler'
    CLI->>API: GET initial state
    API-->>CLI: RoundTable + thread
    Note over CLI: Initialize cursor = thread.length<br/>Skip existing entries
    CLI->>User: Print watch banner

    loop Poll every intervalMs
        CLI->>CLI: sleep(intervalMs)
        CLI->>API: GET current state
        API-->>CLI: RoundTable + thread
        
        alt New entries detected
            Note over CLI: newEntries = thread.slice(lastLength)
            loop For each new entry
                CLI->>User: printEntry(entry)
                CLI->>Hook: spawn /bin/sh -c handler<br/>pipe entry JSON to stdin
                Hook-->>CLI: (fire-and-forget)
            end
            Note over CLI: lastLength = thread.length
        end
        
        alt status !== 'open' && exitOnClose
            CLI->>User: printClosure(status, outcome)
            CLI->>CLI: exit(status === 'resolved' ? 0 : 1)
        end
    end

    User->>CLI: Ctrl+C (SIGINT)
    CLI->>User: Print summary (entries_seen)
    CLI->>CLI: exit(0)

Loading

_{Last reviewed commit: e409465}

[claude]

Code Review

Overall this is a well-structured addition. The daemon design is thoughtful, the input validation is thorough, and the JSON mode support is consistent throughout. A few issues to address before merging.

---

Bug: child.stdin null dereference risk

In fireHook (cli/src/commands/round-tables.ts), child.stdin is typed as Writable | null. While stdio: pipe guarantees it is non-null at runtime, the compiler may flag it and an explicit guard is good practice. Add a check before writing to child.stdin.

---

Bug: Entries seen reports total thread length, not entries observed

In the watch signal handler, lastLength is initialised to rt.thread.length (the existing entries explicitly skipped at startup). The summary therefore overstates entries seen. For example if there were 5 entries before watching and 3 new ones arrived, it prints 8 instead of 3.

Fix: track an initialLength at startup and subtract it in the summary:
const initialLength = rt.thread.length;
let lastLength = initialLength;
// ... in signal handler:
console.log(' Entries seen: ' + (lastLength - initialLength));

---

Security note: --on-speak shell hook

fireHook passes the operator-supplied command string directly to /bin/sh -c. This is intentional, and the entry data is safely piped to stdin rather than interpolated into the command string, so there is no server-data injection vector. Worth a brief note in docs/AGENT-GUIDE.md that --on-speak executes arbitrary shell commands.

---

Auth middleware change is correct and backward-compatible

Switching from authenticateAgent to authenticateHttpSignature in src/routes/round-tables.js is the right call. authenticateHttpSignature verifies Ed25519 signatures when the Signature header is present and falls back to the legacy authenticateAgent path when absent, so no existing callers are broken.

---

client.ts host header fix is correct

url.hostname strips the port; url.host preserves it for non-standard ports. Required for HTTP Signature verification where the signed Host value must match exactly (RFC 7230 section 5.4). Good catch.

---

No backoff on transient poll errors

On a sustained outage the daemon hammers the server at the full --interval rate. A simple capped exponential backoff (e.g. double sleep up to ~60 s, reset on success) would be kinder to a degraded server. Not a blocker, but worth a follow-up issue.

---

Thread cap not surfaced in watch mode

The service caps threads at 200 entries. Once that cap is reached, the watch loop keeps polling but will never see new entries (and will not auto-exit unless the session closes). Adding a warning when current.thread.length equals 200 would help users understand why activity appears to have stopped.

---

Test coverage

Server-side API coverage in server.test.js looks solid (13+ cases covering the full lifecycle, auth, participant caps, and edge cases). There are no CLI-level tests for the new subcommands. The watch daemon signal-handling and hook-firing logic would benefit most from testing. This matches existing patterns (no other commands have unit tests), so not a merge blocker, but worth tracking.

---

Minor nits

• running = false in the signal handler directly before process.exit(0) is redundant.
• --interval validates a floor (500 ms) but has no ceiling. A maximum (e.g. one hour) with a warning would prevent accidental misconfiguration.
• Hardcoded column widths in list (padEnd(42) for topic) will misalign on topics containing multi-byte Unicode. Low priority.

[claude]

Code Review

Overall this is a solid addition — the daemon loop design is thoughtful (cursor-based, error backoff, clean signal handling), and the url.hostname → url.host fix in client.ts is a genuine bug fix for non-standard ports. A few things worth addressing before merge:

---

Bug: ANSI codes ignore NO_COLOR / output.ts helpers

printEntry, printClosure, and the watch banner all use raw escape codes (\x1b[36m, \x1b[32m, etc.) instead of the color helpers in output.ts. The project already has green(), yellow(), cyan(), bold(), dim() that gate on NO_COLOR. This command will output colors even when NO_COLOR=1 is set, breaking the contract respected by every other command.

// round-tables.ts — should use helpers from output.ts instead of raw escapes
console.log(`[${ts}] \x1b[36m${entry.from}\x1b[0m`);         // ← bypass NO_COLOR
console.log(rt.status === 'resolved' ? '\x1b[32m✓ ...' ...); // ← same issue

---

Bug: Thread-cap warning fires on every poll cycle

if (lastLength >= 200 && newEntries.length === 0 && current.status === 'open') {
  warn('Thread is at the 200-entry cap — no new messages can be added...');
}

Once the cap is hit this will warn on every poll interval indefinitely. Add a boolean flag (let capWarned = false) so it fires at most once.

---

Code duplication: validateRtId belongs in validate.ts

The project already has validate.ts with validateMessageId and validateGroupId — identical regex, same calling convention. validateRtId is a third copy of the same function in a new file. Adding validateRoundTableId to validate.ts and importing it keeps the pattern consistent and avoids a third drift point.

---

Dead code: running flag is never set to false

let running = true;
// ...
while (running) {
  // ...
  // `running` is never assigned false anywhere in the function
}

The loop exits exclusively via process.exit(). The running variable and the redundant if (!running) break are confusing (they imply an orderly exit path that doesn't exist). Either wire up running = false as a graceful shutdown mechanism, or remove the flag and use while (true).

---

Auth migration: inconsistency across routes (informational)

The round-tables routes now correctly use authenticateHttpSignature. For reference, groups.js and outbox.js still use the legacy authenticateAgent. This PR doesn't need to fix those, but it's worth noting that authenticateHttpSignature falls back to authenticateAgent when no Signature header is present (see auth.js:237), so the migration is backward-compatible.

---

Minor: fireHook hardcodes /bin/sh

spawn('/bin/sh', ...) will fail silently on Windows. If Windows support is ever a goal, process.platform === 'win32' ? 'cmd' : '/bin/sh' or { shell: true } would be more portable. Not a blocker for a server-side CLI, just worth documenting.

---

Test coverage

The PR description's test plan is all manual. Given the daemon loop has non-trivial state (cursor management, cap detection, error counting), unit tests for the watch logic would be valuable — especially the consecutive-error backoff and the cap-warning behavior.

---

Summary of required changes:

1. Use output.ts color helpers in printEntry, printClosure, and the watch banner
2. Add a capWarned guard so the 200-entry warning fires once
3. Move validateRtId → validate.ts as validateRoundTableId
4. Remove or properly wire the dead running flag

[dundas]

All four issues from the review have been addressed in commit fd355ed (pushed to main after merge):

1. NO_COLOR / raw ANSI escapes — Replaced all raw \x1b[...] codes in printEntry, printClosure, and the watch banner with local helpers (cyan, green, yellow, bold, dim) that check 'NO_COLOR' in process.env — the same pattern used by output.ts. NO_COLOR=1 is now respected throughout.

2. Thread-cap warning repeating — Added let capWarned = false before the poll loop. The 200-entry warning now fires at most once per watch session.

3. validateRtId duplicated — Moved to cli/src/validate.ts as validateRoundTableId (same pattern as validateGroupId). All five call sites in round-tables.ts updated to import and use it.

4. Dead running flag — Removed let running = true, the while (running) condition, and the if (!running) break guard. Loop is now while (true) — all exits go through process.exit() which is the only actual exit path.

Build clean (33 modules), tests passing ✓