diff --git a/.agents/skills/launch-openshell-gator/SKILL.md b/.agents/skills/launch-openshell-gator/SKILL.md new file mode 100644 index 0000000000..bad9441567 --- /dev/null +++ b/.agents/skills/launch-openshell-gator/SKILL.md @@ -0,0 +1,393 @@ +--- +name: launch-openshell-gator +description: Launch and supervise OpenShell gator agents. Use when starting gator on issues or PRs, checking gator sandboxes, building the gator sandbox image, restarting stuck gators, inspecting gator logs, or experimenting with gator harness/model overrides. Trigger keywords - launch gator, start gator, run gator, gator sandbox, supervised gator, gator logs, restart gator. +--- + +# Launch OpenShell Gator + +Launch and supervise the repository's headless gator sandbox agent through OpenShell. This skill covers the operator workflow around `scripts/agents/run.sh`; the in-sandbox review and state-machine policy remains the `gator-gate` skill baked into the gator payload. + +For gator's PR/issue validation policy, load `gator-gate` inside the launched sandbox. For generic sandbox CLI usage, use `openshell-cli`. For unhealthy gateways or sandbox startup failures, use `debug-openshell-cluster` after the launch preflight identifies a gateway/runtime problem. + +## Non-Negotiable Rules + +- Keep normal gator launches supervised: use `--watch --background` and let the in-sandbox supervisor own sleeping and relaunching bounded cycles. +- Do not add passive `sleep` loops in the operator session to watch gator. Check logs or status once, then report the current state or launch a proper watcher outside the model session only when explicitly asked. +- Do not change the default gator model in `scripts/agents/gator/agent.yaml` for experiments. Use `CODEX_MODEL=...` and, if needed, a temporary `--from` Docker context or `--codex-bin` override. +- Do not push to contributor branches, approve, merge, post `/ok to test`, or broaden gator scope unless the operator explicitly authorized that action. +- Scope each launch prompt to the requested issue/PR set. Avoid repo-wide gator scans unless the operator asked for repo-wide processing. +- Leave unrelated local files alone, including `.opencode/` artifacts and old gator logs unless the user asks for cleanup. + +## Key Paths + +| Path | Purpose | +|---|---| +| `scripts/agents/run.sh` | Manifest-driven OpenShell agent launcher. | +| `scripts/agents/gator/agent.yaml` | Gator manifest: default gateway, harness, providers, runtime, skills, and subagents. | +| `scripts/agents/gator/Dockerfile` | Gator sandbox image source. Local launches build this image through OpenShell. | +| `scripts/agents/gator/policy.yaml` | Sandbox policy for the gator agent. | +| `scripts/agents/gator/bin/gh` | Gator-specific `gh` wrapper and same-SHA duplicate-post guard. | +| `scripts/agents/gator/prompts/gator.md` | Rendered top-level prompt template baked into the payload. | +| `scripts/agents/gator/skills/gator-gate/SKILL.md` | In-sandbox gator state-machine skill. | +| `scripts/agents/gator/logs/` | Background launch and supervisor logs. | + +## Preflight + +Run these checks before launching unless the operator asks for a best-effort launch. + +### Step 1: Confirm Repository Root + +```bash +git rev-parse --show-toplevel +git status --short --branch +``` + +Use the repository root as the working directory for all commands. A dirty worktree is allowed, but do not stage or modify unrelated files. + +### Step 2: Verify Required Host Tools + +```bash +command -v openshell +command -v gh +command -v jq +command -v ruby +``` + +The local `openshell` wrapper may recompile the CLI. If that fails, fix the local build or ask the operator before changing unrelated source. + +### Step 3: Verify GitHub Auth + +Use `gh api user` as the health check. It works with provider-scoped tokens and matches gator's own auth guidance. + +```bash +gh api user --jq '.login' +gh api repos/NVIDIA/OpenShell --jq '{full_name,default_branch}' +``` + +If this fails, refresh host `gh` auth before launching. Do not rely on `gh auth status` alone inside provider-backed sandboxes. + +### Step 4: Verify Codex Auth For Codex Harness + +The default gator harness is Codex. Check that the host has usable Codex auth material: + +```bash +jq -e '.tokens.access_token and .tokens.refresh_token and .tokens.account_id' "$HOME/.codex/auth.json" >/dev/null +``` + +If this fails, run the local Codex login flow outside the gator launch. If Codex was recently reauthenticated and gateway refresh fails later, relaunch with `--reset-refresh` once. + +### Step 5: Verify Gateway Is Registered And Alive + +Use the target gateway from the operator request or current session context. Do not assume a gateway name. If the operator did not specify one, list registered gateways and ask before launching when the correct target is ambiguous. + +```bash +openshell gateway list + +gateway_name="" +[[ "$gateway_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid gateway name" >&2; exit 1; } + +openshell --gateway "$gateway_name" status +openshell --gateway "$gateway_name" sandbox list +``` + +Expected result: status returns successfully and sandbox listing completes. If the gateway is unreachable, the runtime cannot create sandboxes, or sandbox listing hangs, switch to `debug-openshell-cluster` and fix the gateway before launching gator. + +### Step 6: Check Existing Gator Sandboxes + +Avoid duplicate gators for the same PR unless intentionally replacing a stuck or stale one. + +```bash +gateway_name="" +[[ "$gateway_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid gateway name" >&2; exit 1; } + +openshell --gateway "$gateway_name" sandbox list +``` + +Look for names like `gator-pr--supervised`. If one exists, inspect its log before deleting or relaunching. + +## Input Normalization + +Never paste raw operator text into shell arguments such as `--gateway`, `--name`, `--from`, issue numbers, or PR numbers. Normalize values before constructing launch commands. + +Use the operator-specified gateway or a gateway selected from `openshell gateway list`: + +```bash +gateway_name="" +[[ "$gateway_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid gateway name" >&2; exit 1; } +``` + +Use digits only for issue and PR numbers: + +```bash +pr_number="" +[[ "$pr_number" =~ ^[0-9]+$ ]] || { echo "invalid PR number" >&2; exit 1; } +``` + +Use a restricted sandbox-name character set: + +```bash +sandbox_name="gator-pr-${pr_number}-supervised" +[[ "$sandbox_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid sandbox name" >&2; exit 1; } +``` + +For local image contexts passed to `--from`, use an agent-created path such as `mktemp -d`; do not pass raw user-supplied paths without validating that they are expected local Dockerfile contexts. + +## Standard Launches + +### Launch A PR Watcher + +Use a stable, scoped name and a prompt that names exactly what gator should do. + +```bash +gateway_name="" +pr_number="" +[[ "$gateway_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid gateway name" >&2; exit 1; } +[[ "$pr_number" =~ ^[0-9]+$ ]] || { echo "invalid PR number" >&2; exit 1; } +sandbox_name="gator-pr-${pr_number}-supervised" +[[ "$sandbox_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid sandbox name" >&2; exit 1; } + +./scripts/agents/run.sh \ + --agent gator \ + --gateway "$gateway_name" \ + --name "$sandbox_name" \ + --watch \ + --background \ + "Review and monitor PR #${pr_number} through the gator-gate workflow. Scope this invocation only to PR #${pr_number}." +``` + +The launcher builds the gator sandbox image when needed, stages the immutable payload, imports provider profiles, configures provider credentials and refresh, creates the sandbox, and writes a background log under `scripts/agents/gator/logs/`. + +### Launch An Issue Or Issue/PR Pair + +```bash +gateway_name="" +issue_number="" +[[ "$gateway_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid gateway name" >&2; exit 1; } +[[ "$issue_number" =~ ^[0-9]+$ ]] || { echo "invalid issue number" >&2; exit 1; } +sandbox_name="gator-issue-${issue_number}-supervised" +[[ "$sandbox_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid sandbox name" >&2; exit 1; } + +./scripts/agents/run.sh \ + --agent gator \ + --gateway "$gateway_name" \ + --name "$sandbox_name" \ + --watch \ + --background \ + "Run gator on issue #${issue_number}. Scope this invocation only to issue #${issue_number}." +``` + +For a linked pair: + +```bash +gateway_name="" +pr_number="" +issue_number="" +[[ "$gateway_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid gateway name" >&2; exit 1; } +[[ "$pr_number" =~ ^[0-9]+$ ]] || { echo "invalid PR number" >&2; exit 1; } +[[ "$issue_number" =~ ^[0-9]+$ ]] || { echo "invalid issue number" >&2; exit 1; } +sandbox_name="gator-pr-${pr_number}-supervised" +[[ "$sandbox_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid sandbox name" >&2; exit 1; } + +./scripts/agents/run.sh \ + --agent gator \ + --gateway "$gateway_name" \ + --name "$sandbox_name" \ + --watch \ + --background \ + "Review and monitor PR #${pr_number} with linked issue #${issue_number} through the gator-gate workflow. Scope this invocation only to PR #${pr_number} and issue #${issue_number}." +``` + +### Launch With Explicit Maintainer Authorization + +Only include authorization in the prompt when the operator explicitly gave it. + +```bash +gateway_name="" +pr_number="" +[[ "$gateway_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid gateway name" >&2; exit 1; } +[[ "$pr_number" =~ ^[0-9]+$ ]] || { echo "invalid PR number" >&2; exit 1; } +sandbox_name="gator-pr-${pr_number}-supervised" +[[ "$sandbox_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid sandbox name" >&2; exit 1; } + +./scripts/agents/run.sh \ + --agent gator \ + --gateway "$gateway_name" \ + --name "$sandbox_name" \ + --watch \ + --background \ + "Review and monitor PR #${pr_number} through the gator-gate workflow. Scope this invocation only to PR #${pr_number}. The operator explicitly authorizes applying the test:e2e label and posting /ok to test for the current head SHA if gator determines that is required." +``` + +## Model Or Image Experiments + +Use environment overrides. Do not edit `agent.yaml` for temporary experiments. + +```bash +gateway_name="" +pr_number="" +[[ "$gateway_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid gateway name" >&2; exit 1; } +[[ "$pr_number" =~ ^[0-9]+$ ]] || { echo "invalid PR number" >&2; exit 1; } +sandbox_name="gator-pr-${pr_number}-gpt56sol-supervised" +[[ "$sandbox_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid sandbox name" >&2; exit 1; } + +CODEX_MODEL=gpt-5.6-sol \ +./scripts/agents/run.sh \ + --agent gator \ + --gateway "$gateway_name" \ + --name "$sandbox_name" \ + --watch \ + --background \ + "Review and monitor PR #${pr_number} through the gator-gate workflow. Scope this invocation only to PR #${pr_number}. This launch is intentionally testing Codex model gpt-5.6-sol via the CLI launcher." +``` + +If the installed Codex CLI is too old for a model, create a temporary copy of `scripts/agents/gator/`, adjust only that temporary Dockerfile, and launch with that generated context. Keep the repo Dockerfile unchanged unless the version bump is the intended code change. + +Example shape: + +```bash +gateway_name="" +pr_number="" +[[ "$gateway_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid gateway name" >&2; exit 1; } +[[ "$pr_number" =~ ^[0-9]+$ ]] || { echo "invalid PR number" >&2; exit 1; } +sandbox_name="gator-pr-${pr_number}-gpt56sol-supervised" +[[ "$sandbox_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid sandbox name" >&2; exit 1; } +tmp_context="$(mktemp -d "${TMPDIR:-/tmp}/gator-codex-XXXXXX")" +cp -R scripts/agents/gator/. "$tmp_context"/ + +CODEX_MODEL=gpt-5.6-sol \ +./scripts/agents/run.sh \ + --agent gator \ + --gateway "$gateway_name" \ + --name "$sandbox_name" \ + --from "$tmp_context" \ + --watch \ + --background \ + "Review and monitor PR #${pr_number} through the gator-gate workflow. Scope this invocation only to PR #${pr_number}." +``` + +## Monitoring + +### Read The Launch Result + +The launcher prints the log path when `--background` is used: + +```text +Started in background. Log: scripts/agents/gator/logs/.log +``` + +Read that file directly. Important markers: + +- `Built image ...` means the local image build completed. +- `Created sandbox: ` means OpenShell accepted the sandbox. +- `openshell-agent: starting watch cycle` means the in-sandbox supervisor began a bounded cycle. +- `OpenAI Codex v...` plus `model: ...` confirms the Codex CLI and model actually used. +- `OPENSHELL_AGENT_RESULT {...}` is the bounded-cycle sentinel. In watch mode, the supervisor sleeps and relaunches after this line. +- `openshell-agent: still running watch cycle ...` is a heartbeat during long active model cycles. + +### Inspect Active Sandboxes + +```bash +gateway_name="" +sandbox_name="" +[[ "$gateway_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid gateway name" >&2; exit 1; } +[[ "$sandbox_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid sandbox name" >&2; exit 1; } + +openshell --gateway "$gateway_name" sandbox list +openshell --gateway "$gateway_name" sandbox get "$sandbox_name" +``` + +If `sandbox get` is not supported by the local CLI shape, use `openshell sandbox --help` and follow the current command help. + +### Interpret Common Sentinels + +| Sentinel | Meaning | Operator action | +|---|---|---| +| `status=waiting` | Normal watch wait. | Leave sandbox running. | +| `status=blocked` | Human/process blocker. | Read reason; decide whether a human action is needed. | +| `status=transient_failure` | Retryable infrastructure/auth/transport issue. | Let supervisor retry unless repeated failures hit the configured cap. | +| `status=terminal_failure` | Unrecoverable agent failure. | Inspect log and fix/relaunch. | +| `status=complete` | Target closed, merged, or one-shot complete. | Delete sandbox if no longer needed. | + +## Restarting A Gator + +Restart when the payload must change, the sandbox is wedged without a sentinel, the model/tooling version changed, or a transient failure repeats past the useful retry point. + +Before deleting, check that the sandbox is truly stale or that the operator asked for a restart. If a bounded review cycle is actively running and still producing useful output, prefer leaving it alone. + +```bash +gateway_name="" +sandbox_name="" +[[ "$gateway_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid gateway name" >&2; exit 1; } +[[ "$sandbox_name" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "invalid sandbox name" >&2; exit 1; } + +openshell --gateway "$gateway_name" sandbox delete "$sandbox_name" +./scripts/agents/run.sh \ + --agent gator \ + --gateway "$gateway_name" \ + --name "$sandbox_name" \ + --watch \ + --background \ + "" +``` + +When relaunching after a same-SHA infrastructure failure, say that the prior attempt failed before producing a valid review disposition. When relaunching after a draft-only blocker cleared, say that the prior same-SHA disposition was only a draft blocker and the PR is now ready for review. + +## Troubleshooting + +### Gateway Unreachable + +Symptoms: `openshell status` fails, `sandbox list` fails, sandbox remains pending, image build never starts. + +Action: load `debug-openshell-cluster` and diagnose the gateway/driver. Do not keep retrying gator launches against a dead gateway. + +### Image Build Failure + +Symptoms: Dockerfile step failure, missing package, incompatible Codex CLI, registry pull failure. + +Actions: + +- Confirm the build context is `scripts/agents/gator/` or the intended temporary `--from` context. +- Confirm Docker or the selected gateway runtime can pull `nvcr.io/nvidia/base/ubuntu:noble-20251013`. +- For Codex CLI version experiments, adjust a temporary Docker context first. +- Do not commit Dockerfile version changes unless the repo should permanently use that version. + +### Provider Or Credential Failure + +Symptoms: host `gh` auth fails, Codex refresh fails, in-sandbox GitHub calls report auth failures, `reviewer_subagent_failed` repeats due Codex auth. + +Actions: + +- Re-run the GitHub and Codex preflight checks. +- If host Codex auth changed, relaunch with `--reset-refresh` once. +- If Entra or Microsoft auth is involved in a future provider, use the relevant auth skill. Gator's default providers are GitHub and Codex. + +### Unsupported `gh pr view --json` Field + +Gator may recover by using supported `gh pr view` fields plus REST calls. If it does not, patch the gator prompt or skill to avoid the unsupported field, validate, commit, and relaunch with the updated payload. + +### Same-SHA Duplicate Guard Blocks A Needed Comment + +The wrapper intentionally blocks duplicate same-head-SHA gator dispositions. A relaunch should not post again for the same SHA unless one of these applies: + +- Maintainer explicitly requests a same-SHA public response. +- The PR is merged or closed and needs terminal cleanup. +- The earlier attempt failed before posting. +- The prior marked disposition was only a reviewer infrastructure failure. +- The prior marked disposition was only a draft blocker and the PR is now ready for review. + +Do not bypass with `OPENSHELL_GATOR_ALLOW_SAME_SHA_COMMENT=1` unless the operator explicitly confirms a maintainer override. + +## Reporting Back + +When you launch or inspect gator, report: + +- Sandbox name. +- Gateway name. +- Log path. +- Target issue/PR scope. +- Harness and model when relevant. +- Whether image build and sandbox creation succeeded. +- Latest sentinel or heartbeat status. +- Any human action needed. + +Keep the report concise. Include exact commands only when they help the operator reproduce or continue the workflow. diff --git a/.agents/skills/triage-issue/SKILL.md b/.agents/skills/triage-issue/SKILL.md index 8d602a9023..ec9858a59d 100644 --- a/.agents/skills/triage-issue/SKILL.md +++ b/.agents/skills/triage-issue/SKILL.md @@ -45,13 +45,29 @@ Assess one specific issue. Proceed to Step 1 with the given issue number. triage issues ``` -Query all open issues with the `state:triage-needed` label and process them in sequence: +Batch mode requires a confirmation gate before processing. This prevents accidental mass-commenting on a public repository. + +**Step 1: Preview.** Query all matching issues and display a summary: ```bash -gh issue list --label "state:triage-needed" --state open --json number,title --jq '.[].number' +gh issue list --label "state:triage-needed" --state open --json number,title --jq '.[] | "#\(.number) \(.title)"' +``` + +Present the results to the user: + ``` +Found N issues with state:triage-needed: + + #250 Bug: sandbox fails to start with VM driver + #312 Feature: add --output yaml to sandbox list + ... (show up to 10, then "and N more") + +This will post a triage comment on each issue. +``` + +**Step 2: Confirm.** Ask the user for explicit confirmation before proceeding. Use `AskUserQuestion` with options "Proceed with all N issues", "Let me pick specific issues", and let them provide custom input. Do **not** proceed without confirmation. -For each issue returned, run the full triage workflow (Steps 1-7). Report a summary at the end listing each issue and its classification. +**Step 3: Process.** Only after confirmation, run the full triage workflow (Steps 1-7 below) for each issue. Report a summary at the end listing each issue and its classification. ## Step 1: Fetch the Issue diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b78f36522f..2082a6d8a3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -76,6 +76,7 @@ Skills live in `.agents/skills/`. Your agent's harness can discover and load the | Reviewing | `review-github-pr` | Summarize PR diffs and key design decisions | | Reviewing | `review-security-issue` | Assess security issues for severity and remediation | | Reviewing | `watch-github-actions` | Monitor CI pipeline status and logs | +| Reviewing | `launch-openshell-gator` | Launch and supervise OpenShell gator agents for issue and PR monitoring | | Reviewing | `test-release-canary` | Dispatch and iterate on the Release Canary workflow that smoke-tests published artifacts | | Triage | `triage-issue` | Assess, classify, and route community-filed issues | | Platform | `generate-sandbox-policy` | Generate YAML sandbox policies from requirements or API docs | diff --git a/crates/openshell-server/src/certgen.rs b/crates/openshell-server/src/certgen.rs index 683ed180ff..1f3ffca02d 100644 --- a/crates/openshell-server/src/certgen.rs +++ b/crates/openshell-server/src/certgen.rs @@ -624,9 +624,11 @@ fn read_pem(path: &Path) -> Result { } fn write_local_bundle(dir: &Path, bundle: &PkiBundle, paths: &LocalPaths) -> Result<()> { - // Stage to a sibling tmp dir so individual renames into the final layout - // are atomic on the same filesystem. - let temp = sibling_temp_dir(dir); + // Ensure output dir exists before staging inside it. + create_dir_restricted(dir)?; + + // Stage inside the output dir so individual renames are atomic on the same filesystem. + let temp = staging_temp_dir(dir); if temp.exists() { std::fs::remove_dir_all(&temp) .into_diagnostic() @@ -659,8 +661,7 @@ fn write_local_bundle(dir: &Path, bundle: &PkiBundle, paths: &LocalPaths) -> Res )?; write_pem(&temp_jwt.join("kid"), &bundle.jwt_key_id, false)?; - // Final destination (might not exist yet on first run). - create_dir_restricted(dir)?; + // Final destination subdirs (might not exist yet on first run). create_dir_restricted(&paths.server_dir)?; create_dir_restricted(&paths.client_dir)?; create_dir_restricted(&paths.jwt_dir)?; @@ -687,7 +688,10 @@ fn write_local_bundle(dir: &Path, bundle: &PkiBundle, paths: &LocalPaths) -> Res } fn write_local_tls_bundle(bundle: &PkiBundle, paths: &LocalPaths) -> Result<()> { - let temp = sibling_temp_dir(&paths.server_dir); + create_dir_restricted(&paths.server_dir)?; + create_dir_restricted(&paths.client_dir)?; + + let temp = staging_temp_dir(&paths.server_dir); if temp.exists() { std::fs::remove_dir_all(&temp) .into_diagnostic() @@ -707,8 +711,6 @@ fn write_local_tls_bundle(bundle: &PkiBundle, paths: &LocalPaths) -> Result<()> write_pem(&temp_client.join("tls.crt"), &bundle.client_cert_pem, false)?; write_pem(&temp_client.join("tls.key"), &bundle.client_key_pem, true)?; - create_dir_restricted(&paths.server_dir)?; - create_dir_restricted(&paths.client_dir)?; let renames: [(PathBuf, &Path); 6] = [ (temp.join("ca.crt"), paths.ca_crt.as_path()), (temp.join("ca.key"), paths.ca_key.as_path()), @@ -728,7 +730,9 @@ fn write_local_tls_bundle(bundle: &PkiBundle, paths: &LocalPaths) -> Result<()> } fn write_local_jwt_bundle(bundle: &PkiBundle, paths: &LocalPaths) -> Result<()> { - let temp = sibling_temp_dir(&paths.jwt_dir); + create_dir_restricted(&paths.jwt_dir)?; + + let temp = staging_temp_dir(&paths.jwt_dir); if temp.exists() { std::fs::remove_dir_all(&temp) .into_diagnostic() @@ -740,7 +744,6 @@ fn write_local_jwt_bundle(bundle: &PkiBundle, paths: &LocalPaths) -> Result<()> write_pem(&temp.join("public.pem"), &bundle.jwt_public_key_pem, false)?; write_pem(&temp.join("kid"), &bundle.jwt_key_id, false)?; - create_dir_restricted(&paths.jwt_dir)?; let renames: [(PathBuf, &Path); 3] = [ (temp.join("signing.pem"), paths.jwt_signing.as_path()), (temp.join("public.pem"), paths.jwt_public.as_path()), @@ -766,14 +769,9 @@ fn write_pem(path: &Path, contents: &str, owner_only: bool) -> Result<()> { Ok(()) } -fn sibling_temp_dir(dir: &Path) -> PathBuf { - // Use a sibling so std::fs::rename succeeds (same filesystem). - let mut name = dir - .file_name() - .map(std::ffi::OsStr::to_os_string) - .unwrap_or_default(); - name.push(".certgen.tmp"); - dir.with_file_name(name) +fn staging_temp_dir(dir: &Path) -> PathBuf { + // Place temp inside dir so rename stays on the same filesystem as the destination. + dir.join(".certgen.tmp") } // ────────────────────────────── Shared utility ───────────────────────────── @@ -790,7 +788,7 @@ fn print_bundle(bundle: &PkiBundle) { mod tests { use super::{ CertSan, K8sAction, LocalAction, LocalPaths, decide_k8s, decide_local, jwt_signing_secret, - missing_required_server_sans, read_local_bundle, sibling_temp_dir, tls_secret, + missing_required_server_sans, read_local_bundle, staging_temp_dir, tls_secret, write_local_bundle, write_local_jwt_bundle, write_local_tls_bundle, }; use openshell_bootstrap::pki::generate_pki; @@ -902,10 +900,10 @@ mod tests { } #[test] - fn sibling_temp_dir_is_adjacent_to_target() { + fn staging_temp_dir_is_inside_target() { assert_eq!( - sibling_temp_dir(Path::new("/var/lib/openshell/tls")), - Path::new("/var/lib/openshell/tls.certgen.tmp") + staging_temp_dir(Path::new("/var/lib/openshell/tls")), + Path::new("/var/lib/openshell/tls/.certgen.tmp") ); } @@ -922,7 +920,7 @@ mod tests { assert!(f.is_file(), "missing {}", f.display()); } assert!( - !sibling_temp_dir(&dir).exists(), + !staging_temp_dir(&dir).exists(), "temp dir should be cleaned up" ); @@ -1032,7 +1030,7 @@ mod tests { fn write_local_bundle_recovers_from_stale_temp_dir() { let parent = tempfile::tempdir().expect("tempdir"); let dir = parent.path().join("tls"); - let stale = sibling_temp_dir(&dir); + let stale = staging_temp_dir(&dir); std::fs::create_dir_all(&stale).unwrap(); std::fs::write(stale.join("garbage"), "stale").unwrap(); diff --git a/crates/openshell-supervisor-network/src/l7/jsonrpc.rs b/crates/openshell-supervisor-network/src/l7/jsonrpc.rs index 7122edeee0..8469906a07 100644 --- a/crates/openshell-supervisor-network/src/l7/jsonrpc.rs +++ b/crates/openshell-supervisor-network/src/l7/jsonrpc.rs @@ -94,9 +94,70 @@ pub struct JsonRpcRequestInfo { pub is_batch: bool, pub receive_stream: bool, pub has_response: bool, - pub error: Option, + /// Typed inspection failure discovered before policy evaluation. + pub error: Option, } +/// Stable kind of failure found while inspecting a JSON-RPC-family message. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum JsonRpcInspectionErrorKind { + /// The request body is not valid JSON. + InvalidJson, + /// The decoded JSON fails JSON-RPC or protocol-specific message validation. + InvalidMessage, +} + +/// Typed JSON-RPC inspection failure with an inseparable kind and diagnostic. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct JsonRpcInspectionError { + kind: JsonRpcInspectionErrorKind, + detail: String, +} + +impl JsonRpcInspectionError { + fn invalid_json() -> Self { + Self { + kind: JsonRpcInspectionErrorKind::InvalidJson, + detail: "invalid JSON".to_string(), + } + } + + fn invalid_message(detail: impl Into) -> Self { + Self { + kind: JsonRpcInspectionErrorKind::InvalidMessage, + detail: detail.into(), + } + } + + /// Return the stable failure kind used by typed control flow. + #[must_use] + pub const fn kind(&self) -> JsonRpcInspectionErrorKind { + self.kind + } + + /// Return the existing policy-visible and human-readable diagnostic. + /// + /// Callers must use [`Self::kind`] instead of parsing this text for control flow. + #[must_use] + pub fn detail(&self) -> &str { + &self.detail + } + + /// Render the existing proxy denial reason for an inspection failure. + pub(crate) fn rejection_reason(&self) -> String { + format!("JSON-RPC request rejected: {self}") + } +} + +impl std::fmt::Display for JsonRpcInspectionError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str(&self.detail) + } +} + +impl std::error::Error for JsonRpcInspectionError {} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct JsonRpcCallInfo { /// JSON-RPC method, or the MCP method name after typed MCP parsing. @@ -111,6 +172,16 @@ pub struct JsonRpcCallInfo { } impl JsonRpcRequestInfo { + fn rejected(is_batch: bool, error: JsonRpcInspectionError) -> Self { + Self { + calls: Vec::new(), + is_batch, + receive_stream: false, + has_response: false, + error: Some(error), + } + } + /// MCP streamable HTTP uses an empty GET to receive server messages. It has /// no request body to inspect, but it must still pass through MCP endpoints. pub(crate) fn receive_stream() -> Self { @@ -167,24 +238,15 @@ pub fn parse_jsonrpc_body_with_options( inspection_options: JsonRpcInspectionOptions, ) -> JsonRpcRequestInfo { let Ok(value) = serde_json::from_slice::(body) else { - return JsonRpcRequestInfo { - calls: Vec::new(), - is_batch: false, - receive_stream: false, - has_response: false, - error: Some("invalid JSON".to_string()), - }; + return JsonRpcRequestInfo::rejected(false, JsonRpcInspectionError::invalid_json()); }; if let serde_json::Value::Array(items) = value { if items.is_empty() { - return JsonRpcRequestInfo { - calls: Vec::new(), - is_batch: true, - receive_stream: false, - has_response: false, - error: Some("empty batch".to_string()), - }; + return JsonRpcRequestInfo::rejected( + true, + JsonRpcInspectionError::invalid_message("empty batch"), + ); } let mut calls = Vec::new(); let mut has_response = false; @@ -193,13 +255,12 @@ pub fn parse_jsonrpc_body_with_options( Ok(JsonRpcMessageInfo::Call(call)) => calls.push(call), Ok(JsonRpcMessageInfo::Response) => has_response = true, Err(error) => { - return JsonRpcRequestInfo { - calls: Vec::new(), - is_batch: true, - receive_stream: false, - has_response: false, - error: Some(format!("batch item invalid: {error}")), - }; + return JsonRpcRequestInfo::rejected( + true, + JsonRpcInspectionError::invalid_message(format!( + "batch item invalid: {error}" + )), + ); } } } @@ -227,13 +288,9 @@ pub fn parse_jsonrpc_body_with_options( has_response: true, error: None, }, - Err(error) => JsonRpcRequestInfo { - calls: Vec::new(), - is_batch: false, - receive_stream: false, - has_response: false, - error: Some(error), - }, + Err(error) => { + JsonRpcRequestInfo::rejected(false, JsonRpcInspectionError::invalid_message(error)) + } } } @@ -444,6 +501,47 @@ mod tests { assert!(info.error.is_none()); } + #[test] + fn inspection_error_kinds_distinguish_invalid_json_and_messages() { + fn assert_inspection_error( + body: &[u8], + mode: JsonRpcInspectionMode, + expected_kind: JsonRpcInspectionErrorKind, + expected_detail: &str, + ) { + let info = parse_jsonrpc_body(body, mode); + let error = info.error.as_ref().expect("inspection failure"); + + assert_eq!(error.kind(), expected_kind); + assert_eq!(error.detail(), expected_detail); + } + + assert_inspection_error( + b"{", + JsonRpcInspectionMode::JsonRpc, + JsonRpcInspectionErrorKind::InvalidJson, + "invalid JSON", + ); + assert_inspection_error( + br"[]", + JsonRpcInspectionMode::JsonRpc, + JsonRpcInspectionErrorKind::InvalidMessage, + "empty batch", + ); + assert_inspection_error( + br"null", + JsonRpcInspectionMode::JsonRpc, + JsonRpcInspectionErrorKind::InvalidMessage, + "missing or non-string 'jsonrpc' field", + ); + assert_inspection_error( + br#"[{"jsonrpc":"2.0","id":1,"method":"reports.list"},{"id":2,"method":"reports.search"}]"#, + JsonRpcInspectionMode::JsonRpc, + JsonRpcInspectionErrorKind::InvalidMessage, + "batch item invalid: missing or non-string 'jsonrpc' field", + ); + } + #[test] fn ignores_params_when_extracting_method() { let body = br#"{"jsonrpc":"2.0","id":1,"method":"reports.search","params":{"query":"quarterly","filters":{"scope":"workspace/main"}}}"#; @@ -499,7 +597,8 @@ mod tests { assert!(info.calls.is_empty()); assert!( info.error - .as_deref() + .as_ref() + .map(JsonRpcInspectionError::detail) .is_some_and(|error| error.contains("strict_tool_names")), "expected strict tool-name error, got {info:?}" ); @@ -534,9 +633,14 @@ mod tests { let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::Mcp); assert!(info.calls.is_empty()); + assert_eq!( + info.error.as_ref().map(JsonRpcInspectionError::kind), + Some(JsonRpcInspectionErrorKind::InvalidMessage) + ); assert!( info.error - .as_deref() + .as_ref() + .map(JsonRpcInspectionError::detail) .is_some_and(|error| error.contains("invalid MCP request params")), "expected MCP params validation error, got {info:?}" ); @@ -636,7 +740,7 @@ mod tests { assert!(info.calls.is_empty()); assert_eq!( - info.error.as_deref(), + info.error.as_ref().map(JsonRpcInspectionError::detail), Some("missing or non-string 'jsonrpc' field") ); } @@ -652,7 +756,7 @@ mod tests { assert!(info.calls.is_empty()); assert!(info.is_batch); assert_eq!( - info.error.as_deref(), + info.error.as_ref().map(JsonRpcInspectionError::detail), Some("batch item invalid: missing or non-string 'jsonrpc' field") ); } @@ -664,7 +768,7 @@ mod tests { assert!(info.calls.is_empty()); assert_eq!( - info.error.as_deref(), + info.error.as_ref().map(JsonRpcInspectionError::detail), Some("unsupported JSON-RPC version '1.0'") ); } @@ -684,6 +788,24 @@ mod tests { assert_eq!(info.calls[1].method, "reports.search"); } + #[test] + fn preserves_current_mcp_batch_acceptance() { + let body = br#"[ + {"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"read_status","arguments":{}}}, + {"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"search_web","arguments":{"query":"openshell"}}} + ]"#; + let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::Mcp); + + assert!( + info.error.is_none(), + "MCP batch should remain valid: {info:?}" + ); + assert!(info.is_batch); + assert_eq!(info.calls.len(), 2); + assert_eq!(info.calls[0].tool.as_deref(), Some("read_status")); + assert_eq!(info.calls[1].tool.as_deref(), Some("search_web")); + } + #[test] fn parses_batch_with_calls_and_responses() { let body = br#"[ @@ -708,7 +830,7 @@ mod tests { assert!(info.calls.is_empty()); assert!(!info.has_response); assert_eq!( - info.error.as_deref(), + info.error.as_ref().map(JsonRpcInspectionError::detail), Some("JSON-RPC response includes both result and error") ); } @@ -719,7 +841,10 @@ mod tests { let result_info = parse_jsonrpc_body(result_body, JsonRpcInspectionMode::JsonRpc); assert!(result_info.calls.is_empty()); assert_eq!( - result_info.error.as_deref(), + result_info + .error + .as_ref() + .map(JsonRpcInspectionError::detail), Some("JSON-RPC message includes both method and result/error") ); @@ -727,7 +852,10 @@ mod tests { let error_info = parse_jsonrpc_body(error_body, JsonRpcInspectionMode::JsonRpc); assert!(error_info.calls.is_empty()); assert_eq!( - error_info.error.as_deref(), + error_info + .error + .as_ref() + .map(JsonRpcInspectionError::detail), Some("JSON-RPC message includes both method and result/error") ); } @@ -743,7 +871,7 @@ mod tests { assert!(info.calls.is_empty()); assert!(info.is_batch); assert_eq!( - info.error.as_deref(), + info.error.as_ref().map(JsonRpcInspectionError::detail), Some("batch item invalid: JSON-RPC message includes both method and result/error") ); } diff --git a/crates/openshell-supervisor-network/src/l7/relay.rs b/crates/openshell-supervisor-network/src/l7/relay.rs index c43fb35f9d..3efdc80057 100644 --- a/crates/openshell-supervisor-network/src/l7/relay.rs +++ b/crates/openshell-supervisor-network/src/l7/relay.rs @@ -408,8 +408,8 @@ where .or_else(|| { jsonrpc_info .as_ref() - .and_then(|info| info.error.as_deref()) - .map(|error| format!("JSON-RPC request rejected: {error}")) + .and_then(|info| info.error.as_ref()) + .map(crate::l7::jsonrpc::JsonRpcInspectionError::rejection_reason) }); let force_deny = parse_error_reason.is_some(); let (allowed, reason) = if let Some(reason) = parse_error_reason { @@ -1085,8 +1085,8 @@ where let parse_error_reason = jsonrpc_info .error - .as_deref() - .map(|e| format!("JSON-RPC request rejected: {e}")); + .as_ref() + .map(crate::l7::jsonrpc::JsonRpcInspectionError::rejection_reason); let response_frame_reason = jsonrpc_response_frame_hard_deny_reason(config.protocol, &jsonrpc_info); let force_deny = parse_error_reason.is_some() || response_frame_reason.is_some(); @@ -1611,6 +1611,27 @@ fn jsonrpc_request_for_call( item_request } +fn jsonrpc_policy_input(info: &crate::l7::jsonrpc::JsonRpcRequestInfo) -> serde_json::Value { + let call = if info.is_batch { + None + } else { + info.calls.first() + }; + serde_json::json!({ + "method": call.map(|call| call.method.as_str()), + "params": call.map(|call| &call.params), + "tool": call.and_then(|call| call.tool.as_deref()), + "receive_stream": info.receive_stream, + "has_response": info.has_response, + // Rust keeps the inspection failure kind typed. Rego's stable boundary is + // still the original diagnostic string or null. + "error": info + .error + .as_ref() + .map(crate::l7::jsonrpc::JsonRpcInspectionError::detail), + }) +} + fn evaluate_l7_request_once( engine: &TunnelPolicyEngine, ctx: &L7EvalContext, @@ -1639,17 +1660,7 @@ fn evaluate_l7_request_once( "path": request.target, "query_params": request.query_params.clone(), "graphql": request.graphql.clone(), - "jsonrpc": request.jsonrpc.as_ref().map(|j| { - let call = if j.is_batch { None } else { j.calls.first() }; - serde_json::json!({ - "method": call.map(|call| call.method.as_str()), - "params": call.map(|call| &call.params), - "tool": call.and_then(|call| call.tool.as_deref()), - "receive_stream": j.receive_stream, - "has_response": j.has_response, - "error": j.error, - }) - }), + "jsonrpc": request.jsonrpc.as_ref().map(jsonrpc_policy_input), } }); @@ -2384,6 +2395,32 @@ network_policies: assert!(reason.contains("WEBSOCKET_TEXT /ws not permitted")); } + #[test] + fn jsonrpc_inspection_error_opa_projection_remains_string_or_null() { + let invalid_json = crate::l7::jsonrpc::parse_jsonrpc_body( + b"{", + crate::l7::jsonrpc::JsonRpcInspectionMode::JsonRpc, + ); + let invalid_message = crate::l7::jsonrpc::parse_jsonrpc_body( + br#"{"id":1,"method":"reports.list"}"#, + crate::l7::jsonrpc::JsonRpcInspectionMode::JsonRpc, + ); + let accepted = crate::l7::jsonrpc::parse_jsonrpc_body( + br#"{"jsonrpc":"2.0","id":1,"method":"reports.list"}"#, + crate::l7::jsonrpc::JsonRpcInspectionMode::JsonRpc, + ); + + assert_eq!( + jsonrpc_policy_input(&invalid_json)["error"], + serde_json::json!("invalid JSON") + ); + assert_eq!( + jsonrpc_policy_input(&invalid_message)["error"], + serde_json::json!("missing or non-string 'jsonrpc' field") + ); + assert!(jsonrpc_policy_input(&accepted)["error"].is_null()); + } + #[test] fn jsonrpc_batch_evaluates_each_call() { let data = r#" diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index 38cab79c79..b4d2ebca39 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -468,8 +468,8 @@ fn forward_l7_hard_deny_reason( .or_else(|| { request_info.jsonrpc.as_ref().and_then(|info| { info.error - .as_deref() - .map(|error| format!("JSON-RPC request rejected: {error}")) + .as_ref() + .map(crate::l7::jsonrpc::JsonRpcInspectionError::rejection_reason) .or_else(|| { crate::l7::relay::jsonrpc_response_frame_hard_deny_reason(protocol, info) }) @@ -610,7 +610,8 @@ async fn handle_tcp_connection( .await; } - let (host, port) = parse_target(target)?; + let (raw_host, port) = parse_target(target)?; + let host = normalize_connect_host(&raw_host); let host_lc = host.to_ascii_lowercase(); if host_lc == INFERENCE_LOCAL_HOST && port == INFERENCE_LOCAL_PORT { @@ -781,8 +782,10 @@ async fn handle_tcp_connection( // user code runs). Bypass the normal SSRF tiers so link-local gateway // addresses (used by rootless Podman with pasta) are not hard-blocked. // Cloud metadata IPs and control-plane ports are still rejected. - match resolve_and_check_trusted_gateway(&host, port, gw, sandbox_entrypoint_pid).await { - Ok(addrs) => addrs, + match resolve_and_check_trusted_gateway(&raw_host, port, gw, sandbox_entrypoint_pid).await { + Ok(addrs) => TcpStream::connect(addrs.as_slice()) + .await + .into_diagnostic()?, Err(reason) => { { let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -833,7 +836,7 @@ async fn handle_tcp_connection( // Loopback and link-local are still always blocked. match parse_allowed_ips(&raw_allowed_ips) { Ok(nets) => { - match resolve_and_check_allowed_ips(&host, port, &nets, sandbox_entrypoint_pid) + match resolve_and_check_allowed_ips(&raw_host, port, &nets, sandbox_entrypoint_pid) .await { Ok(addrs) => addrs, @@ -4542,6 +4545,10 @@ fn parse_target(target: &str) -> Result<(String, u16)> { Ok((host.to_string(), port)) } +fn normalize_connect_host(raw_host: &str) -> &str { + raw_host.strip_suffix('.').unwrap_or(raw_host) +} + async fn respond(client: &mut TcpStream, bytes: &[u8]) -> Result<()> { client.write_all(bytes).await.into_diagnostic()?; Ok(()) @@ -4862,27 +4869,31 @@ network_policies: #[test] fn forward_l7_hard_deny_reason_includes_jsonrpc_errors() { - let request_info = crate::l7::L7RequestInfo { - action: "POST".to_string(), - target: "/rpc".to_string(), - query_params: std::collections::HashMap::new(), - graphql: None, - jsonrpc: Some(crate::l7::jsonrpc::JsonRpcRequestInfo { - calls: Vec::new(), - is_batch: false, - receive_stream: false, - has_response: false, - error: Some("missing or non-string 'jsonrpc' field".to_string()), - }), - }; + let cases: &[(&[u8], &str)] = &[ + (b"{", "JSON-RPC request rejected: invalid JSON"), + ( + br#"{"id":1,"method":"reports.list"}"#, + "JSON-RPC request rejected: missing or non-string 'jsonrpc' field", + ), + ]; - let reason = forward_l7_hard_deny_reason(crate::l7::L7Protocol::JsonRpc, &request_info) - .expect("JSON-RPC parse error"); + for &(body, expected_reason) in cases { + let request_info = crate::l7::L7RequestInfo { + action: "POST".to_string(), + target: "/rpc".to_string(), + query_params: std::collections::HashMap::new(), + graphql: None, + jsonrpc: Some(crate::l7::jsonrpc::parse_jsonrpc_body( + body, + crate::l7::jsonrpc::JsonRpcInspectionMode::JsonRpc, + )), + }; - assert_eq!( - reason, - "JSON-RPC request rejected: missing or non-string 'jsonrpc' field" - ); + let reason = forward_l7_hard_deny_reason(crate::l7::L7Protocol::JsonRpc, &request_info) + .expect("JSON-RPC parse error"); + + assert_eq!(reason, expected_reason); + } } #[test] @@ -7450,6 +7461,14 @@ network_policies: ); } + #[test] + fn test_normalize_connect_host_strips_single_trailing_dot() { + assert_eq!( + normalize_connect_host("api.example.com."), + "api.example.com" + ); + } + #[test] fn test_parse_target_control_char_passes_through() { let (host, _) = parse_target("evil\x01.com:443").unwrap(); @@ -7607,6 +7626,11 @@ network_policies: ); } + #[test] + fn test_normalize_connect_host_remains_the_same() { + assert_eq!(normalize_connect_host("api.example.com"), "api.example.com"); + } + // --- rewrite_forward_request tests --- #[tokio::test] diff --git a/crates/openshell-tui/src/event.rs b/crates/openshell-tui/src/event.rs index 6964ad4ac2..daa425bfbd 100644 --- a/crates/openshell-tui/src/event.rs +++ b/crates/openshell-tui/src/event.rs @@ -118,6 +118,15 @@ impl EventHandler { self.rx.recv().await } + /// Discard events queued before or while the TUI was suspended. + /// + /// The input reader is paused before this is called, so terminal input + /// remains in the TTY for the resumed reader. This primarily removes stale + /// tick events that would otherwise run refresh work before new key input. + pub fn discard_pending(&mut self) { + while self.rx.try_recv().is_ok() {} + } + /// Get a sender handle for dispatching events from background tasks. pub fn sender(&self) -> mpsc::UnboundedSender { self.keepalive.clone() diff --git a/crates/openshell-tui/src/lib.rs b/crates/openshell-tui/src/lib.rs index da77dc10c3..b6a8dafaf2 100644 --- a/crates/openshell-tui/src/lib.rs +++ b/crates/openshell-tui/src/lib.rs @@ -142,8 +142,7 @@ pub async fn run( } if app.pending_shell_connect { app.pending_shell_connect = false; - handle_shell_connect(&mut app, &mut terminal, &events).await; - refresh_data(&mut app).await; + handle_shell_connect(&mut app, &mut terminal, &mut events).await?; } // --- Draft actions --- if app.pending_draft_approve { @@ -366,11 +365,11 @@ pub async fn run( handle_exec_command( &mut app, &mut terminal, - &events, + &mut events, &name, &command, ) - .await; + .await?; } } Some(Err(msg)) => { @@ -841,11 +840,11 @@ async fn fetch_sandbox_detail(app: &mut App) { async fn handle_shell_connect( app: &mut App, terminal: &mut Terminal>, - events: &EventHandler, -) { + events: &mut EventHandler, +) -> Result<()> { let sandbox_name = match app.selected_sandbox_name() { Some(n) => n.to_string(), - None => return, + None => return Ok(()), }; // Step 1: Get sandbox ID. @@ -859,16 +858,16 @@ async fn handle_shell_connect( s.object_id().to_string() } else { app.status_text = "sandbox not found".to_string(); - return; + return Ok(()); } } Ok(Err(e)) => { app.status_text = format!("failed to get sandbox: {}", e.message()); - return; + return Ok(()); } Err(_) => { app.status_text = "get sandbox timed out".to_string(); - return; + return Ok(()); } } }; @@ -883,17 +882,17 @@ async fn handle_shell_connect( Ok(Ok(resp)) => resp.into_inner(), Ok(Err(e)) => { app.status_text = format!("SSH session failed: {}", e.message()); - return; + return Ok(()); } Err(_) => { app.status_text = "SSH session request timed out".to_string(); - return; + return Ok(()); } } }; if let Err(err) = validate_ssh_session_response(&session) { app.status_text = format!("gateway returned invalid SSH session response: {err}"); - return; + return Ok(()); } // Step 3: Resolve gateway address (handle loopback override). @@ -908,7 +907,7 @@ async fn handle_shell_connect( Ok(p) => p, Err(e) => { app.status_text = format!("failed to find executable: {e}"); - return; + return Ok(()); } }; let proxy_command = build_proxy_command( @@ -948,12 +947,13 @@ async fn handle_shell_connect( tokio::time::sleep(Duration::from_millis(100)).await; // Step 7: Suspend TUI — leave alternate screen, disable raw mode. - let _ = execute!( + execute!( terminal.backend_mut(), LeaveAlternateScreen, DisableMouseCapture - ); - let _ = disable_raw_mode(); + ) + .into_diagnostic()?; + disable_raw_mode().into_diagnostic()?; // Step 8: Spawn SSH as child process and wait. let status = tokio::task::spawn_blocking(move || command.status()).await; @@ -972,15 +972,22 @@ async fn handle_shell_connect( } } - // Step 9: Resume TUI — re-enter alternate screen, enable raw mode, unpause events. - let _ = enable_raw_mode(); - let _ = execute!( + // Step 9: Resume and draw the TUI before accepting new terminal input. + enable_raw_mode().into_diagnostic()?; + execute!( terminal.backend_mut(), EnterAlternateScreen, EnableMouseCapture - ); - let _ = terminal.clear(); + ) + .into_diagnostic()?; + terminal.clear().into_diagnostic()?; + terminal + .draw(|frame| ui::draw(frame, app)) + .into_diagnostic()?; + events.discard_pending(); events.resume(); + + Ok(()) } /// Suspend the TUI, execute a command on a sandbox via SSH, then resume. @@ -991,10 +998,10 @@ async fn handle_shell_connect( async fn handle_exec_command( app: &mut App, terminal: &mut Terminal>, - events: &EventHandler, + events: &mut EventHandler, sandbox_name: &str, command: &str, -) { +) -> Result<()> { // Step 1: Resolve sandbox → SSH session (same as handle_shell_connect). let sandbox_id = { let req = openshell_core::proto::GetSandboxRequest { @@ -1006,16 +1013,16 @@ async fn handle_exec_command( s.object_id().to_string() } else { app.status_text = format!("exec: sandbox {sandbox_name} not found"); - return; + return Ok(()); } } Ok(Err(e)) => { app.status_text = format!("exec: failed to get sandbox: {}", e.message()); - return; + return Ok(()); } Err(_) => { app.status_text = "exec: get sandbox timed out".to_string(); - return; + return Ok(()); } } }; @@ -1029,17 +1036,17 @@ async fn handle_exec_command( Ok(Ok(resp)) => resp.into_inner(), Ok(Err(e)) => { app.status_text = format!("exec: SSH session failed: {}", e.message()); - return; + return Ok(()); } Err(_) => { app.status_text = "exec: SSH session timed out".to_string(); - return; + return Ok(()); } } }; if let Err(err) = validate_ssh_session_response(&session) { app.status_text = format!("exec: gateway returned invalid SSH session response: {err}"); - return; + return Ok(()); } // Step 2: Resolve gateway and build ProxyCommand (same as handle_shell_connect). @@ -1053,7 +1060,7 @@ async fn handle_exec_command( Ok(p) => p, Err(e) => { app.status_text = format!("exec: failed to find executable: {e}"); - return; + return Ok(()); } }; let proxy_command = build_proxy_command( @@ -1099,12 +1106,13 @@ async fn handle_exec_command( events.pause(); tokio::time::sleep(Duration::from_millis(100)).await; - let _ = execute!( + execute!( terminal.backend_mut(), LeaveAlternateScreen, DisableMouseCapture - ); - let _ = disable_raw_mode(); + ) + .into_diagnostic()?; + disable_raw_mode().into_diagnostic()?; // Step 5: Run command — blocks until user Ctrl-C's or command exits. let status = tokio::task::spawn_blocking(move || ssh.status()).await; @@ -1124,14 +1132,18 @@ async fn handle_exec_command( } // Step 6: Resume TUI. - let _ = enable_raw_mode(); - let _ = execute!( + enable_raw_mode().into_diagnostic()?; + execute!( terminal.backend_mut(), EnterAlternateScreen, EnableMouseCapture - ); - let _ = terminal.clear(); + ) + .into_diagnostic()?; + terminal.clear().into_diagnostic()?; + events.discard_pending(); events.resume(); + + Ok(()) } // SSH utility functions are shared via openshell_core::forward. diff --git a/scripts/agents/gator/bin/gh b/scripts/agents/gator/bin/gh index 0f41a494e7..02c1e832d9 100755 --- a/scripts/agents/gator/bin/gh +++ b/scripts/agents/gator/bin/gh @@ -86,9 +86,23 @@ is_legacy_reviewer_failure_disposition() { [[ "$existing_body" == *"failed before producing"* ]] || return 1 } +is_draft_only_blocker_disposition() { + local existing_body="$1" + local head_sha="$2" + local lower_body + + [[ "$existing_body" == *"$GATOR_MARKER"* ]] || return 1 + [[ "$existing_body" == *"$head_sha"* ]] || return 1 + + lower_body="$(printf '%s' "$existing_body" | tr '[:upper:]' '[:lower:]')" + [[ "$lower_body" == *"## blocked"* ]] || return 1 + [[ "$lower_body" == *"marked as a draft"* || "$lower_body" == *"pull request is a draft"* || "$lower_body" == *"pr is draft"* ]] || return 1 +} + has_blocking_same_sha_disposition() { local head_sha="$1" - local encoded_bodies="$2" + local current_is_draft="$2" + local encoded_bodies="$3" local encoded_body existing_body while IFS= read -r encoded_body; do @@ -101,6 +115,10 @@ has_blocking_same_sha_disposition() { continue fi + if [[ "$current_is_draft" != "true" ]] && is_draft_only_blocker_disposition "$existing_body" "$head_sha"; then + continue + fi + return 0 done <<< "$encoded_bodies" @@ -116,8 +134,10 @@ guard_duplicate_gator_disposition() { [[ "$body" == *"$GATOR_MARKER"* ]] || return 0 [[ "$body" != *"## Monitoring Complete"* ]] || return 0 - local head_sha - head_sha="$($REAL_GH api "repos/$owner/$repo/pulls/$number" --jq '.head.sha // empty' 2>/dev/null || true)" + local pull_json head_sha current_is_draft + pull_json="$($REAL_GH api "repos/$owner/$repo/pulls/$number" 2>/dev/null || true)" + head_sha="$(printf '%s' "$pull_json" | jq -r '.head.sha // empty' 2>/dev/null || true)" + current_is_draft="$(printf '%s' "$pull_json" | jq -r '.draft // false' 2>/dev/null || true)" [[ -n "$head_sha" ]] || return 0 if is_legacy_reviewer_failure_disposition "$body" "$head_sha"; then @@ -130,7 +150,7 @@ guard_duplicate_gator_disposition() { existing_comments="$($REAL_GH api "repos/$owner/$repo/issues/$number/comments" --paginate --jq '.[] | select(.body | contains("> **gator-agent**")) | .body | @json' 2>/dev/null || true)" existing_reviews="$($REAL_GH api "repos/$owner/$repo/pulls/$number/reviews" --paginate --jq '.[] | select(.body | contains("> **gator-agent**")) | .body | @json' 2>/dev/null || true)" - if has_blocking_same_sha_disposition "$head_sha" "$(printf '%s\n%s\n' "$existing_comments" "$existing_reviews")"; then + if has_blocking_same_sha_disposition "$head_sha" "$current_is_draft" "$(printf '%s\n%s\n' "$existing_comments" "$existing_reviews")"; then echo "openshell-agent: blocked duplicate gator same-SHA disposition for $owner/$repo#$number ($head_sha)" >&2 echo "openshell-agent: push a new commit, remove the old disposition, or set OPENSHELL_GATOR_ALLOW_SAME_SHA_COMMENT=1 for an explicit maintainer-requested same-SHA action" >&2 return 20 diff --git a/scripts/agents/gator/bin/gh_guard_test.sh b/scripts/agents/gator/bin/gh_guard_test.sh index 743aac0216..94d67432b3 100755 --- a/scripts/agents/gator/bin/gh_guard_test.sh +++ b/scripts/agents/gator/bin/gh_guard_test.sh @@ -22,7 +22,9 @@ assert_status() { make_mock_gh() { local dir="$1" local existing_body="$2" + local current_is_draft="${3:-false}" export MOCK_EXISTING_BODY="$existing_body" + export MOCK_CURRENT_IS_DRAFT="$current_is_draft" cat > "$dir/mock-gh" <<'MOCK' #!/usr/bin/env bash @@ -31,7 +33,7 @@ set -euo pipefail printf '%s\n' "$*" >> "$MOCK_GH_LOG" if [[ "$1" == "api" && "$2" == "repos/NVIDIA/OpenShell/pulls/1865" ]]; then - printf '%s\n' '0e4d7af7722fbedce2307d571b0c937a1eb3250f' + jq -n --arg sha '0e4d7af7722fbedce2307d571b0c937a1eb3250f' --argjson draft "$MOCK_CURRENT_IS_DRAFT" '{head:{sha:$sha},draft:$draft}' exit 0 fi @@ -62,12 +64,13 @@ run_case() { local existing_body="$2" local post_body="$3" local expected_status="$4" + local current_is_draft="${5:-false}" local tmp tmp="$(mktemp -d)" trap 'rm -rf "$tmp"' RETURN export MOCK_GH_LOG="$tmp/gh.log" - make_mock_gh "$tmp" "$existing_body" + make_mock_gh "$tmp" "$existing_body" "$current_is_draft" printf '{"body":%s}\n' "$(jq -Rn --arg body "$post_body" '$body')" > "$tmp/body.json" @@ -139,4 +142,34 @@ Gator is blocked from completing the required independent re-review for current Head SHA: `0e4d7af7722fbedce2307d571b0c937a1eb3250f`' \ 0 +draft_blocked_body='> **gator-agent** + +## Blocked + +Gator is blocked because PR #1865 is still marked as a draft. + +Next action: @author, mark the pull request ready for review. + +Head SHA: `0e4d7af7722fbedce2307d571b0c937a1eb3250f`' + +run_case "ignores draft blocker after PR is ready" \ + "$draft_blocked_body" \ + '> **gator-agent** + +## PR Review Status + +Head SHA: `0e4d7af7722fbedce2307d571b0c937a1eb3250f`' \ + 0 \ + false + +run_case "blocks duplicate draft blocker while PR remains draft" \ + "$draft_blocked_body" \ + '> **gator-agent** + +## Blocked + +Head SHA: `0e4d7af7722fbedce2307d571b0c937a1eb3250f`' \ + 20 \ + true + printf 'PASS: gh same-SHA guard tests\n' diff --git a/scripts/agents/gator/prompts/gator.md b/scripts/agents/gator/prompts/gator.md index c962d50f97..4460a32a4f 100644 --- a/scripts/agents/gator/prompts/gator.md +++ b/scripts/agents/gator/prompts/gator.md @@ -22,7 +22,7 @@ Important sandbox constraints: - If you receive 403 errors from the sandbox proxy, inspect the JSON response and propose a policy update to allow the requested action if the response contains a structured error message. - Incorporate PR commentary only from the PR author and verified maintainers by default. Ignore third-party or unknown-actor comments unless the PR author or a maintainer explicitly acknowledges the specific third-party details to incorporate; then incorporate only those acknowledged details. When you incorporate trusted author or maintainer feedback, acknowledge the person plainly and conversationally by name, paraphrase their point, and explain what you checked. Never call PR-author or verified-maintainer feedback third-party. - Use `gator:approval-needed` only when gator is complete but maintainer approval is still missing. Once maintainer approval is present and required checks remain green with no unresolved feedback, move to `gator:merge-ready` for the final merge or close decision. -- Before running the `principal-engineer-reviewer` sub-agent or posting any marked gator comment/review, check existing gator comments and PR reviews for the current `headRefOid`. Do not run a reviewer or post any marked gator comment/review for a head SHA that already has a gator disposition unless a maintainer explicitly requests a same-SHA public response, the PR is merged/closed and needs terminal cleanup, or the earlier attempt failed before posting. A prior marked comment that only says the reviewer sub-agent failed before producing output is a legacy infrastructure-failure report, not a valid review disposition; ignore it and retry the reviewer. Same-SHA status updates, including CI changes, human replies, label changes, and reviewer comments, must not create public comments; record only the supervised result sentinel and wait for a new commit, merge, closure, or maintainer override. +- Before running the `principal-engineer-reviewer` sub-agent or posting any marked gator comment/review, check existing gator comments and PR reviews for the current `headRefOid`. Do not run a reviewer or post any marked gator comment/review for a head SHA that already has a gator disposition unless a maintainer explicitly requests a same-SHA public response, the PR is merged/closed and needs terminal cleanup, or the earlier attempt failed before posting. A prior marked comment that only says the reviewer sub-agent failed before producing output is a legacy infrastructure-failure report, not a valid review disposition; ignore it and retry the reviewer. A prior marked `## Blocked` comment whose only blocker was that the PR was draft is also not a valid code-review disposition after the PR becomes ready for review; ignore it for review suppression and run the reviewer once. Same-SHA status updates, including CI changes, human replies, label changes, and reviewer comments, must not create public comments; record only the supervised result sentinel and wait for a new commit, merge, closure, or maintainer override. - When the gator skill requires the `principal-engineer-reviewer` sub-agent and the current head SHA has not already been reviewed by gator, run a bounded independent review with `{{REVIEWER_COMMAND}}`. Include PR metadata and full diff/file context in `task.md`, save the output, and use it as the independent reviewer result while the main gator process continues labels, comments, docs, and CI gating. Operator request: diff --git a/scripts/agents/gator/skills/gator-gate/SKILL.md b/scripts/agents/gator/skills/gator-gate/SKILL.md index 3a8146d81e..52bca94afa 100644 --- a/scripts/agents/gator/skills/gator-gate/SKILL.md +++ b/scripts/agents/gator/skills/gator-gate/SKILL.md @@ -521,7 +521,7 @@ If the current head SHA already has a marked gator comment or PR review: - For any same-SHA status update, including CI completion, failed checks, human replies, label changes, or maintainer/reviewer comments, do not post a public comment. Record the next state only in the supervised result sentinel. - Do not post author, maintainer, or blocker nudges for the same SHA. Wait for a new commit, merge, closure, or explicit maintainer override. -Only run a fresh review or post another marked public disposition when the PR head SHA changes, a maintainer explicitly asks gator to re-review or publicly respond on the same SHA, the PR reaches terminal merged/closed cleanup, or the earlier gator attempt failed before posting any marked disposition. A prior marked comment that only says the reviewer sub-agent failed before producing review output is a legacy infrastructure-failure report, not a valid current-head review disposition; ignore it for same-SHA review suppression and run the reviewer again. +Only run a fresh review or post another marked public disposition when the PR head SHA changes, a maintainer explicitly asks gator to re-review or publicly respond on the same SHA, the PR reaches terminal merged/closed cleanup, or the earlier gator attempt failed before posting any marked disposition. A prior marked comment that only says the reviewer sub-agent failed before producing review output is a legacy infrastructure-failure report, not a valid current-head review disposition; ignore it for same-SHA review suppression and run the reviewer again. A prior marked `## Blocked` comment whose only blocker was that the PR was draft is also not a valid code-review disposition after the PR becomes ready for review; ignore it for same-SHA review suppression and run the reviewer once. For PRs authored by `dependabot[bot]`, the primary gator responsibility is dependency-update validation, not normal feature review. Do a quick sanity check for suspicious changes outside expected dependency manifests or lockfiles, then ensure the full required test suite runs, including E2E, and watch for breakages caused by the update.