Skip to content

fix(tui): align model-facing docs and tool gating - #31

Open
asto18089 wants to merge 5 commits into
Pinvou:pinvou3-cleanfrom
asto18089:fix/doc-audit-model-facing
Open

fix(tui): align model-facing docs and tool gating#31
asto18089 wants to merge 5 commits into
Pinvou:pinvou3-cleanfrom
asto18089:fix/doc-audit-model-facing

Conversation

@asto18089

@asto18089 asto18089 commented Aug 27, 2026

Copy link
Copy Markdown

Summary

Model-facing documentation/spec audit fixes, re-verified line by line against the current pinvou3-clean head before changing anything. Four of the six audited findings were confirmed and fixed; two were rejected after re-verification (evidence below). No behavior is changed except where noted (notify method enforcement, finance network gating).

Findings and fixes

1. fleet-manager skill guidance (adjusted, not as audited)

  • Audit claim: codewhale fleet logs/artifacts do not exist because parse_verb (crates/lane/src/control.rs) has no such verbs.
  • Re-verification: parse_verb governs the in-TUI /fleet slash surface. The codewhale fleet CLI (clap FleetCommand, crates/tui/src/lib.rs) really has Inspect/Logs/Artifacts { worker_id } subcommands dispatched to manager.inspect_worker, print_logs, print_artifacts. The original skill text was valid for the CLI.
  • What was actually wrong and is fixed: the skill omitted codewhale fleet resume <run-id> (the orphaned-lease reconcile action, fleet.resume, idempotent), did not mention that stop requires --all, and named no concrete Runtime API evidence endpoints. All three are added; the CLI verb list is otherwise preserved.

2. mcp-builder skill command syntax (rejected — no change)

  • Audit claim: codewhale mcp add my-server --command node --arg server.js / --url ... and codewhale mcp tools do not exist, citing parse_add in crates/tui/src/commands/groups/utility/mcp.rs.
  • Re-verification: that parser is the in-TUI /mcp slash command. The codewhale CLI routes codewhale mcp <args> through the codewhale-cli passthrough (trailing_var_arg) into clap McpCommand (crates/tui/src/lib.rs), which accepts exactly add <name> --command ... --arg ..., add <name> --url ..., and has a real Tools subcommand — the binary's own init output teaches codewhale mcp tools (lib.rs, McpCommand::Init arm). Applying the audit's positional-form fix would have broken the documented CLI syntax. No change made; flagged back to the auditor.

3. verify tool taught a retired name (fixed)

  • crates/tui/src/tools/verify.rs description said "(use run_verifiers)"; run_verifiers is in RETIRED_TOOL_NAMES and cannot dispatch. Now says "(use Run with action=verifiers)".
  • crates/tui/src/tools/verifier.rs background metadata advertised poll_with: ["exec_shell_wait", "task_shell_wait"]; exec_shell_wait is retired. Now ["task_shell_wait"], matching the shell.rs timeout-recovery metadata precedent.
  • The no_advertised_tool_teaches_a_retired_name guard missed this because its builder never registered the verify tool; .with_verify_tool(None, ...) is now in the guard's builder so the whole advertised surface (including verify) is scanned.

4. Bash background timeout semantics (fixed, documentation direction)

  • Verified behavior: the foreground path clamps timeout_ms to 1000-600000, kills at the deadline, and reports TimedOut; the background spawn path passes no kill scheduling, and ShellManager::cleanup retains Running tasks. So background=true tasks are not killed at timeout_ms.
  • The timeout_ms and background schema descriptions now state exactly that, including the clamp and how to bound/stop background work (action=wait timeout, action=cancel). No behavior change. Whether background tasks should become killable at the timeout is left as an upstream discussion item.

5. notify tool ignored method = "off" (fixed, implementation direction)

  • Verified: the tool hardcoded Method::Auto; Off only intercepted when passed in; Auto resolution never read config; the process gate covered only quiet/events. The description and the registration comment in tool_setup both promise method = "off" silences the tool — a user-safety promise, so this was fixed as a bug, not by editing the description.
  • Implementation: settings() now installs the configured [notifications].method process-wide (same bridge and pattern as the NotificationGate), and the notify tool reads configured_method() instead of hardcoding Auto. off returns before any sink write; the tool result stays a success (silent no-op, as documented). Emission was split into emit_model_notify with an injectable sink for tests.
  • Known limitation, shared with the existing gate: the install happens on the first settings() call (turn completion / approval / input / elevation events), so a first-turn notify in a fresh process predates it. Disclosed rather than adding a new config-read path at the tool layer.

6. finance tool bypassed network policy (fixed, implementation direction)

  • Verified: execute ignored _context; no network_policy reference despite the Network capability, so network.default = deny sessions still reached query1.finance.yahoo.com.
  • Implementation: both configured endpoint hosts (quote and chart) are checked up front via NetworkPolicyDecider::evaluate(host, "finance"), so the chart fallback cannot leak. Deny and undecided Prompt fail closed with permission errors in the exact shape used by web_search/speech (including the /network allow <host> hint); no attached policy falls through permissively for back-compat. Description now states the tool is network-policy aware.

Tests

  • cargo test -p codewhale-tui --lib -- tools::finance — 20 passed, including two new fail-closed policy tests (deny-list host with zero mock-server requests; default=prompt approval path).
  • cargo test -p codewhale-tui --lib -- tools::notify tui::notifications — 41 passed, including new: method_off_makes_emission_a_silent_no_op, configured_method_off_silences_the_tool_emission, configured_method_off_still_reports_success_to_the_model, settings_installs_configured_method_from_config, configured_method_bits_round_trip_every_variant.
  • cargo test -p codewhale-tui --lib -- no_advertised_tool_teaches_a_retired_name — 1 passed (guard now scans the verify tool).
  • cargo test -p codewhale-tui --lib -- tools::verifier — 8 passed.
  • cargo test -p codewhale-tui --lib -- tools::shell::tests — 96 passed (existing timeout behavior untouched).
  • cargo test -p codewhale-tui --lib -- skills::system — catalog/generation fixtures unaffected (names unchanged; body-only edit follows the aa4eb12 precedent of no generation bump).
  • cargo fmt --check clean; cargo clippy -p codewhale-tui --lib --all-targets — no new warnings in changed files; cargo check -p codewhale-cli clean.

Fork register

This PR adds fork-distinct behavior (notify method enforcement, finance network gating) and model-facing doc corrections. The parent repo (pinvou-agent) will follow up with the docs/fork-modifications.md register update, the gitlink advance, and any required tag/fingerprint bumps — kept out of this PR intentionally since the register lives in the parent.

DCO sign-off present on every commit.

No-Issue: model-facing documentation and tool-gating audit fixes tracked in the pinvou-agent internal audit; no upstream CodeWhale issue exists for this batch.

The fleet-manager skill listed the real CLI evidence verbs but omitted
'codewhale fleet resume' (the orphaned-lease reconcile action) and did
not say that 'stop' requires '--all'. Add both, name the concrete
Runtime API evidence endpoints (worker inspection, receipt evidence,
event replay) alongside the CLI commands, and list resume in the
post-run receipt action set.

Signed-off-by: asto <asto18089@126.com>
The verify tool description told models to 'use run_verifiers', a name
retired in the v0.9.3 consolidation that cannot dispatch; point at the
canonical 'Run with action=verifiers' form instead. Drop the retired
'exec_shell_wait' from the verifier background metadata poll_with list,
keeping the live 'task_shell_wait'. Extend the
no_advertised_tool_teaches_a_retired_name guard to scan the verify tool
by adding with_verify_tool to its builder.

Signed-off-by: asto <asto18089@126.com>
The timeout_ms schema implied the deadline applies to action=run
generally, but the background spawn path schedules no kill: only the
foreground wait is bounded (deadline kill + TimedOut). State that
explicitly in timeout_ms and background, including the 1000-600000
clamp on the foreground/wait paths and how to bound or stop background
work (action=wait timeout, action=cancel). Documentation aligns with
current behavior; whether background tasks should become killable at
the timeout is left for upstream discussion.

Signed-off-by: asto <asto18089@126.com>
The notify tool description and its registration comment both promise
that [notifications].method = "off" silences the tool, but the tool
hardcoded Method::Auto, so only quiet/events gating applied and a
configured 'off' method still emitted. Install the configured method
process-wide from settings() (same bridge as the NotificationGate) and
have the tool read it; 'off' now returns before any sink write while
the tool result stays a success (silent no-op). Known limitation,
shared with the gate: the install happens on the first settings()
call, so a first-turn notify in a fresh process predates it.

Signed-off-by: asto <asto18089@126.com>
The finance tool declared the Network capability but never consulted
the session NetworkPolicyDecider, so a tightened session
(network.default = deny) still reached query1.finance.yahoo.com. Check
both configured endpoint hosts (quote and chart) before any request,
matching the Web/web_search/speech family: Deny and undecided Prompt
both fail closed with permission errors; no attached policy falls
through permissively for back-compat. Testing both hosts up front
closes the chart-fallback leak. Say so in the description.

Signed-off-by: asto <asto18089@126.com>
@github-actions

Copy link
Copy Markdown

Thanks @asto18089 for taking the time to contribute.

This repository is observing a maintainer-managed PR intake gate in dry-run mode, so this pull request is staying open. This note helps maintainers prepare the allowlist before any enforcement is considered.

Please read CONTRIBUTING.md for the expected contribution shape. A maintainer can grant recurring PR access by commenting /lgtm on a pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant