From 51c61ddbd6f773272b94ad983d57f3ecf0b87fe0 Mon Sep 17 00:00:00 2001 From: user Date: Mon, 13 Jul 2026 18:52:23 -0400 Subject: [PATCH] adds ux bits, spec review skill --- Makefile | 20 ++ scripts/setup-gateway-cli.sh | 29 +- skills/review/spec-gap-analysis/SKILL.md | 137 ++++++++ .../review/spec-gap-analysis/evals/evals.json | 47 +++ skills/review/spec-review/SKILL.md | 149 ++++++++ skills/review/spec-review/evals/evals.json | 47 +++ specs/index.spec.md | 1 + specs/platform/runner.spec-gaps.md | 254 ++++++++++++++ specs/standards/specs/specs.spec.md | 323 ++++++++++++++++++ 9 files changed, 992 insertions(+), 15 deletions(-) create mode 100644 skills/review/spec-gap-analysis/SKILL.md create mode 100644 skills/review/spec-gap-analysis/evals/evals.json create mode 100644 skills/review/spec-review/SKILL.md create mode 100644 skills/review/spec-review/evals/evals.json create mode 100644 specs/platform/runner.spec-gaps.md create mode 100644 specs/standards/specs/specs.spec.md diff --git a/Makefile b/Makefile index ce477e17c..2962575a2 100755 --- a/Makefile +++ b/Makefile @@ -1424,6 +1424,26 @@ kind-setup-openshell-cli: check-kubectl _kind-require-cluster ## Auto-discover t echo "$(COLOR_BLUE)▶$(COLOR_RESET) Found openshell-gateway in: $$NAMESPACES"; \ ./scripts/setup-gateway-cli.sh $$NAMESPACES +kind-stop-openshell-cli: ## Stop openshell gateway port-forwards + @STOPPED=0; \ + for pidfile in $(KIND_PF_DIR)/openshell-pf-*.pid; do \ + [ -f "$$pidfile" ] || continue; \ + NS=$$(basename "$$pidfile" .pid | sed 's/^openshell-pf-//'); \ + PID=$$(cat "$$pidfile"); \ + if ps -p "$$PID" >/dev/null 2>&1; then \ + kill "$$PID" 2>/dev/null || true; \ + echo " Stopped openshell port-forward for $$NS (PID $$PID)"; \ + STOPPED=1; \ + fi; \ + rm -f "$$pidfile"; \ + rm -f "$(KIND_PF_DIR)/openshell-pf-$$NS.log"; \ + done; \ + if [ "$$STOPPED" -eq 1 ]; then \ + echo "$(COLOR_GREEN)✓$(COLOR_RESET) Openshell port-forwards stopped"; \ + else \ + echo "$(COLOR_YELLOW)No active openshell port-forwards found$(COLOR_RESET)"; \ + fi + kind-clean: kind-down ## Alias for kind-down e2e-clean: kind-down ## Alias for kind-down (backward compatibility) diff --git a/scripts/setup-gateway-cli.sh b/scripts/setup-gateway-cli.sh index 7702f9a9e..34dd3f9df 100755 --- a/scripts/setup-gateway-cli.sh +++ b/scripts/setup-gateway-cli.sh @@ -23,15 +23,18 @@ set -e NAMESPACES=("${@:-tenant-a}") CERT_BASE="$HOME/.config/openshell/gateways" +PF_DIR="/tmp/ambient-code" PF_PIDS=() GW_PORTS=() -cleanup() { - for pid in "${PF_PIDS[@]}"; do - kill "$pid" 2>/dev/null || true - done -} -trap cleanup EXIT +mkdir -p "$PF_DIR" + +for pidfile in "$PF_DIR"/openshell-pf-*.pid; do + [ -f "$pidfile" ] || continue + OLD_PID=$(cat "$pidfile") + kill "$OLD_PID" 2>/dev/null || true + rm -f "$pidfile" +done for NS in "${NAMESPACES[@]}"; do GW_NAME="$NS" @@ -54,15 +57,15 @@ for NS in "${NAMESPACES[@]}"; do # Start port-forward on :0 (kernel picks a free port), capture the assigned port kubectl port-forward -n "$NS" statefulset/openshell-gateway ":8080" \ - >/tmp/pf-${NS}.log 2>&1 & + >"$PF_DIR/openshell-pf-${NS}.log" 2>&1 & PF_PID=$! PF_PIDS+=($PF_PID) # Wait for kubectl to print the assigned port PORT="" for attempt in $(seq 1 30); do - if [ -s "/tmp/pf-${NS}.log" ]; then - PORT=$(grep -oE 'Forwarding from 127\.0\.0\.1:[0-9]+' "/tmp/pf-${NS}.log" | grep -oE '[0-9]+$' | head -1) + if [ -s "$PF_DIR/openshell-pf-${NS}.log" ]; then + PORT=$(grep -oE 'Forwarding from 127\.0\.0\.1:[0-9]+' "$PF_DIR/openshell-pf-${NS}.log" | grep -oE '[0-9]+$' | head -1) if [ -n "$PORT" ]; then break fi @@ -77,6 +80,7 @@ for NS in "${NAMESPACES[@]}"; do continue fi + echo "$PF_PID" > "$PF_DIR/openshell-pf-${NS}.pid" GW_PORTS+=("$PORT") # Register the gateway. Remove first if it already exists. @@ -143,9 +147,4 @@ for NS in "${NAMESPACES[@]}"; do done echo "" echo "Port-forwards are running in the background (PIDs: ${PF_PIDS[*]})." -echo "Press Ctrl-C to stop them, or run: kill ${PF_PIDS[*]}" -echo "" - -# Keep port-forwards alive until interrupted -trap - EXIT -wait +echo "Stop with: make kind-stop-openshell-cli" diff --git a/skills/review/spec-gap-analysis/SKILL.md b/skills/review/spec-gap-analysis/SKILL.md new file mode 100644 index 000000000..b36c50807 --- /dev/null +++ b/skills/review/spec-gap-analysis/SKILL.md @@ -0,0 +1,137 @@ +--- +name: spec-gap-analysis +description: > + Validate a spec against the actual codebase. Reads a *.spec.md file, searches + for its implementation and test coverage, then writes a *.spec-gaps.md report + alongside it. Run this on every spec before a release, after major refactors, + or when you suspect spec drift. Triggers on: "gap analysis", "spec gaps", + "validate spec", "spec coverage", "what's missing", "audit spec", "find gaps", + "spec vs implementation", "untested requirements", "spec drift". +--- + +# Spec Gap Analysis + +Validate one spec file against implementation and tests. Writes a sibling `*.spec-gaps.md`. + +## Usage + +```text +/spec-gap-analysis specs/platform/control-plane.spec.md +/spec-gap-analysis specs/security/rbac-enforcement.spec.md +``` + +## User Input + +```text +$ARGUMENTS +``` + +## Steps + +### Phase 1 — Parse the Spec + +Read the spec file. Extract every requirement (statements using SHALL, MUST, SHOULD, MAY) and every scenario (GIVEN/WHEN/THEN blocks). Assign IDs: R1, R2, ... for requirements; S1, S2, ... for scenarios. Note which components the spec touches — use `CLAUDE.md` `## Structure` to map domains to source directories. + +Summarize: "This spec covers N requirements and M scenarios across components X, Y, Z." + +### Phase 2 — Search the Implementation + +For each requirement, grep the codebase for key terms (function names, env vars, API paths, K8s resource kinds, error strings). Read matching files. Classify each requirement: + +- **Implemented** — code matches the spec +- **Partial** — some aspects present, others missing +- **Deviation** — implemented differently than spec describes +- **Missing** — no implementation found + +Also note **Impl Extras** — implementation behavior the spec doesn't cover. + +### Phase 3 — Search the Tests + +For each requirement, grep test directories for the same terms. Read matching test files. Classify coverage: + +- **Full** — all scenarios have corresponding tests +- **Partial** — some scenarios tested +- **None** — no tests found + +### Phase 4 — Classify Gaps + +Produce four gap types: + +- **G-type (Implementation Gap)** — spec requirement with no or partial implementation +- **T-type (Test Gap)** — implemented but untested. Priority: CRITICAL for security, HIGH for core paths, MEDIUM for secondary, LOW for edge cases +- **D-type (Drift)** — spec and implementation disagree. Needs human decision: update spec or fix code +- **E-type (Undocumented)** — implementation exists but spec doesn't describe it. Note for spec update consideration + +### Phase 5 — Propose Tests + +For each T-type gap, propose concrete test names and what they validate. Group by test file. Follow the project's existing test conventions. + +### Phase 6 — Write the Report + +Write to `.replace('.spec.md', '.spec-gaps.md')`. Follow the output format below. + +## Output Format + +```markdown +# — Gap Analysis + +**Date:** YYYY-MM-DD +**Spec:** `` +**Components:** +**Tests:** + +--- + +## Methodology + + +## Requirement Coverage Matrix + + < /dev/null | ID | Requirement | Impl Status | Test Coverage | Priority | +|----|-------------|-------------|---------------|----------| + +## Implementation Gaps (G-type) + +### G1: +- **Requirement:** R<n> +- **Spec says:** <quote> +- **Current state:** <description> +- **Risk:** <impact> + +## Test Gaps (T-type) + +### T1: <title> +- **Requirement:** R<n> +- **Implementation:** <file:line> +- **Current tests:** None / partial +- **Risk:** CRITICAL / HIGH / MEDIUM / LOW +- **Proposed tests:** `test_<name>` — <validates what> + +## Drift (D-type) + +### D1: <title> +- **Spec says:** <quote> +- **Implementation does:** <actual behavior> +- **Resolution:** Update spec / Fix code + +## Undocumented Behavior (E-type) + +### E1: <title> +- **Implementation:** <file:line> +- **Behavior:** <description> +- **Recommendation:** Add to spec / Remove / Keep internal + +## Summary +- **Requirements:** N total (N impl, N partial, N missing) +- **Test coverage:** N full, N partial, N none +- **Top risks:** <ranked list> +``` + +## Heuristics + +- Security requirements (auth, RBAC, tokens, paths) are always CRITICAL priority. +- SHALL/MUST with no test = T-type gap, always. +- Deviations need human judgment — flag, don't fix. +- Read actual code. Never guess from file names. +- One spec at a time. Run repeatedly for the full suite. +- Favor explanation over rigidity — if a requirement is ambiguous, say so rather than forcing a classification. diff --git a/skills/review/spec-gap-analysis/evals/evals.json b/skills/review/spec-gap-analysis/evals/evals.json new file mode 100644 index 000000000..fca2a8ecc --- /dev/null +++ b/skills/review/spec-gap-analysis/evals/evals.json @@ -0,0 +1,47 @@ +[ + { + "input": "run a gap analysis on specs/platform/control-plane.spec.md", + "expected_tool_call": "Skill", + "expected_args": { + "skill": "spec-gap-analysis", + "args": "specs/platform/control-plane.spec.md" + }, + "description": "Triggers gap analysis on a specific platform spec" + }, + { + "input": "what gaps exist in the runner spec?", + "expected_tool_call": "Skill", + "expected_args": { + "skill": "spec-gap-analysis", + "args": "specs/platform/runner.spec.md" + }, + "description": "Triggers gap analysis when user asks about spec coverage" + }, + { + "input": "validate spec coverage for RBAC enforcement", + "expected_tool_call": "Skill", + "expected_args": { + "skill": "spec-gap-analysis", + "args": "specs/security/rbac-enforcement.spec.md" + }, + "description": "Triggers gap analysis for a security spec" + }, + { + "input": "find untested requirements in the SSO spec", + "expected_tool_call": "Skill", + "expected_args": { + "skill": "spec-gap-analysis", + "args": "specs/security/sso-authentication.spec.md" + }, + "description": "Triggers gap analysis focused on test coverage" + }, + { + "input": "spec gaps for the UI architecture", + "expected_tool_call": "Skill", + "expected_args": { + "skill": "spec-gap-analysis", + "args": "specs/ui/architecture.spec.md" + }, + "description": "Triggers gap analysis for a UI spec" + } +] diff --git a/skills/review/spec-review/SKILL.md b/skills/review/spec-review/SKILL.md new file mode 100644 index 000000000..f67580a23 --- /dev/null +++ b/skills/review/spec-review/SKILL.md @@ -0,0 +1,149 @@ +--- +name: spec-review +description: > + Review a spec file for adherence to the project's spec authoring standards. + Validates structure, RFC 2119 keyword usage, scenario completeness, dependency + declarations, registry entry, and reconcilability. Use before merging any new + or modified spec. Triggers on: "review spec", "spec review", "check spec", + "validate spec format", "spec standards", "is this spec valid", "spec quality". +--- + +# Spec Review + +Validate a spec file against the spec authoring standards defined in `specs/standards/specs/specs.spec.md`. Produces a structured review with pass/fail verdicts per check and actionable feedback. + +## Usage + +```text +/spec-review specs/platform/session-timeout.spec.md +/spec-review specs/security/new-rbac-policy.spec.md +``` + +## User Input + +```text +$ARGUMENTS +``` + +## Before Anything Else + +Load the spec authoring standards: + +1. Read `specs/standards/specs/specs.spec.md` +2. Read `specs/index.spec.md` (Spec Registry) +3. Read the target spec file provided in `$ARGUMENTS` + +If no file path is provided, ask the user which spec to review. + +## Steps + +### Phase 1 — Structural Validation + +Check the spec against the required document structure. For each item, classify as PASS, FAIL, or WARN: + + < /dev/null | # | Check | Severity | Rule | +|---|-------|----------|------| +| S1 | Single H1 heading exists | FAIL | Exactly one `#` heading | +| S2 | Introductory prose follows H1 | FAIL | At least one paragraph before any `##` section | +| S3 | `## Requirements` section exists | FAIL | Mandatory section | +| S4 | Requirements use `### Requirement: <Name>` format | FAIL | Every H3 under Requirements must match pattern | +| S5 | Every requirement has at least one `#### Scenario:` | FAIL | No requirement without a scenario | +| S6 | `## Terminology` present if domain-specific terms introduced | WARN | Terms used but not defined | +| S7 | `## Dependencies` present if cross-spec references exist | WARN | References other specs without declaring dependency | +| S8 | `## Migration` present if changing existing behavior | FAIL | Behavioral change without migration plan | +| S9 | `## Design Decisions` present if non-obvious choices made | WARN | Complex design without rationale | +| S10 | File naming follows `<descriptive-title>.spec.md` | FAIL | Kebab-case with `.spec.md` extension | +| S11 | File placed in correct domain directory | WARN | Path matches `specs/{domain}/` | + +### Phase 2 — Semantic Validation + +Check the content quality of requirements and scenarios: + +| # | Check | Severity | Rule | +|---|-------|----------|------| +| M1 | RFC 2119 keywords used in requirements | FAIL | Every requirement must contain at least one normative keyword | +| M2 | Keywords appear in UPPERCASE | WARN | Lowercase "shall" or "must" in normative context | +| M3 | No implementation details in requirements | FAIL | Internal function names, class names, SQL queries, library choices | +| M4 | Requirements describe observable behavior | FAIL | Inputs, outputs, error codes, state transitions — not internals | +| M5 | Scenarios are concrete and testable | FAIL | No vague preconditions or outcomes | +| M6 | Scenarios follow Given/When/Then format | FAIL | `- GIVEN`, `- WHEN`, `- THEN` with optional `- AND` | +| M7 | Every GIVEN has a corresponding WHEN and THEN | FAIL | Incomplete scenario structure | +| M8 | No reserved term collisions | WARN | Check against Ambient domain model terms | + +### Phase 3 — Reconcilability Validation + +Check that an autonomous agent can use this spec for gap analysis and code generation: + +| # | Check | Severity | Rule | +|---|-------|----------|------| +| R1 | Spec Registry entry exists in `specs/index.spec.md` | FAIL | Every spec must be registered | +| R2 | Registry entry has all required fields | FAIL | Path, Domain, Primary Entities, Components, Depends On | +| R3 | Component scope declared | FAIL | Agent must know where to search | +| R4 | Dependencies match cross-spec references in body | WARN | Body references a spec not listed in Dependencies or Registry | +| R5 | No circular dependencies | FAIL | Check the dependency graph for cycles | +| R6 | Scenarios can derive test assertions | WARN | Each THEN clause maps to a verifiable assertion | +| R7 | Migration table covers all affected consumers | WARN | Verify completeness against Component scope | + +### Phase 4 — Cross-Reference Integrity + +| # | Check | Severity | Rule | +|---|-------|----------|------| +| X1 | All relative markdown links resolve to existing files | FAIL | No broken links | +| X2 | Section-level links resolve to existing headings | WARN | Link targets exist in referenced file | +| X3 | Specs referenced in Dependencies section exist | FAIL | No phantom dependencies | + +### Phase 5 — Generate Review Report + +```markdown +# Spec Review: <Spec Title> + +**Date:** YYYY-MM-DD +**Spec:** `<path>` +**Reviewer:** spec-review skill + +## Verdict: PASS | FAIL | WARN + +## Results + +### Structural Validation +| Check | Result | Detail | +|-------|--------|--------| + +### Semantic Validation +| Check | Result | Detail | +|-------|--------|--------| + +### Reconcilability Validation +| Check | Result | Detail | +|-------|--------|--------| + +### Cross-Reference Integrity +| Check | Result | Detail | +|-------|--------|--------| + +## Summary +- **Passed:** N/M checks +- **Failed:** N checks (must fix) +- **Warnings:** N checks (should fix) +- **Blockers:** <list of FAIL items> + +## Recommendations +<actionable fixes for each FAIL and WARN> +``` + +### Verdict Rules + +- **PASS** — zero FAILs +- **FAIL** — one or more FAILs +- **WARN** — zero FAILs, one or more WARNs (reported as PASS with advisory) + +Present the report to the user. If the verdict is FAIL, offer to help fix the issues. + +## Heuristics + +- **Read the spec, not just the headings.** Structurally valid specs can have semantic problems. +- **Implementation detail is the most common antipattern.** Authors instinctively write how they plan to build something. Redirect to observable behavior. +- **Migration completeness is the highest-value check.** Missing consumers cause cascading failures during reconciliation waves. +- **Registry entry is a hard gate.** Without it, `/reconcile` cannot discover or order the spec. +- **Don't flag style preferences as failures.** Prose quality and heading capitalization are not structural violations. +- **Favor explanation over rejection.** When a check fails, explain why the standard exists and what specific change would fix it. diff --git a/skills/review/spec-review/evals/evals.json b/skills/review/spec-review/evals/evals.json new file mode 100644 index 000000000..dd705296b --- /dev/null +++ b/skills/review/spec-review/evals/evals.json @@ -0,0 +1,47 @@ +[ + { + "input": "/spec-review specs/platform/session-timeout.spec.md", + "expected_tool_call": "Skill", + "expected_args": { + "skill": "spec-review", + "args": "specs/platform/session-timeout.spec.md" + }, + "description": "Direct invocation with slash command and spec path" + }, + { + "input": "review the new credential rotation spec for standards compliance", + "expected_tool_call": "Skill", + "expected_args": { + "skill": "spec-review", + "args": "review the new credential rotation spec for standards compliance" + }, + "description": "Natural language trigger for spec review" + }, + { + "input": "check if this spec is valid: specs/security/new-rbac-policy.spec.md", + "expected_tool_call": "Skill", + "expected_args": { + "skill": "spec-review", + "args": "specs/security/new-rbac-policy.spec.md" + }, + "description": "Validation request with spec path" + }, + { + "input": "does the runner spec follow our spec standards?", + "expected_tool_call": "Skill", + "expected_args": { + "skill": "spec-review", + "args": "does the runner spec follow our spec standards?" + }, + "description": "Quality question about existing spec" + }, + { + "input": "spec quality check on specs/ui/work-tracking-dashboard.spec.md", + "expected_tool_call": "Skill", + "expected_args": { + "skill": "spec-review", + "args": "specs/ui/work-tracking-dashboard.spec.md" + }, + "description": "Alternative trigger phrase with spec path" + } +] diff --git a/specs/index.spec.md b/specs/index.spec.md index 225d47ab2..cea15f368 100644 --- a/specs/index.spec.md +++ b/specs/index.spec.md @@ -55,3 +55,4 @@ Machine-readable index for autonomous reconciliation (`/reconcile` skill). | `standards/control-plane/conventions.spec.md` | standards | - | CP | - | | `standards/platform/cross-cutting.spec.md` | standards | - | ALL | - | | `standards/security/security.spec.md` | standards | - | ALL | - | +| `standards/specs/specs.spec.md` | standards | Spec, Requirement, Scenario | ALL | - | diff --git a/specs/platform/runner.spec-gaps.md b/specs/platform/runner.spec-gaps.md new file mode 100644 index 000000000..95bd04cca --- /dev/null +++ b/specs/platform/runner.spec-gaps.md @@ -0,0 +1,254 @@ +# Runner Gap Analysis: Spec vs Implementation + +**Date:** 2026-07-11 +**Spec Reference:** `specs/platform/runner.spec.md` (2026-07-05) +**Runner Source:** `components/runners/ambient-runner/ambient_runner/` +**Test Suite:** `components/runners/ambient-runner/tests/` (46 test files) + +--- + +## Methodology + +1. Read the runner spec end-to-end (810 lines covering startup, bridge, gRPC transport, SSE tap, credentials, MCP servers, OpenShell sandbox) +2. Mapped every spec section to actual source files, read all 60+ implementation files +3. Inventoried all 46 existing test files and their coverage +4. Cross-referenced live pod failure logs (`exec process exited with code 1` in `ambient-control-plane-5b755b8db7-4vlpj`) with the OpenShell gateway exec path +5. Identified spec requirements that have no test, implementation deviations from spec, and runtime-observable gaps + +--- + +## A. Spec-vs-Implementation Feature Parity + +| Spec Feature | Spec Section | Status | Implementation File(s) | Notes | +|---|---|---|---|---| +| FastAPI app + uvicorn | Overview | **Match** | `app.py`, `main.py` | | +| `GET /events/{thread_id}` SSE tap | §SSE Tap | **Match** | `endpoints/events.py` | Filters `MESSAGES_SNAPSHOT`, heartbeat 30s, close on `RUN_FINISHED`/`RUN_ERROR` | +| `GET /events/{thread_id}/wait` | — | **Impl extra** | `endpoints/events.py` | Polling fallback not in spec — defensive addition for slow listener startup | +| `POST /` AG-UI run | §Two Message Paths | **Match** | `endpoints/run.py` | With `grpc_push_middleware` fan-out | +| `POST /model` | §Design Decisions | **Match** | `endpoints/model.py` | Lock, reject-if-generating, between-run queue event | +| `POST /interrupt` | §Bridge Layer | **Match** | `endpoints/interrupt.py` | | +| `GET /health` | §Bridge Layer | **Match** | `endpoints/health.py` | | +| `GET /capabilities` | §Bridge Layer | **Match** | `endpoints/capabilities.py` | Dynamic feature scan of registered routes | +| `GET /repos`, `POST /repos/add`, `POST /repos/remove` | — | **Impl extra** | `endpoints/repos.py` | Runtime repo management, not in spec | +| `POST /workflow` | — | **Impl extra** | `endpoints/workflow.py` | Runtime workflow change, not in spec | +| `GET /mcp/status` | — | **Impl extra** | `endpoints/mcp_status.py` | MCP server diagnostics | +| `GET /content/*` | — | **Impl extra** | `endpoints/content.py` | File/git operations, replaces Go content sidecar | +| `GET /tasks`, `POST /tasks/{id}/stop` | — | **Impl extra** | `endpoints/tasks.py` | Background task management | +| `POST /feedback` | — | **Impl extra** | `endpoints/feedback.py` | Langfuse score creation | +| gRPC `WatchSessionMessages` listener | §gRPC Transport | **Match** | `bridges/claude/grpc_transport.py` | Resume-by-seq, backoff 1s→30s, `UNAUTHENTICATED` reconnect | +| `GRPCMessageWriter` | §gRPC Transport | **Deviation** | `bridges/claude/grpc_transport.py` | Spec says accumulate `MESSAGES_SNAPSHOT` and extract text. Impl uses `TEXT_MESSAGE_START/CONTENT/END` buffering — functionally equivalent, simpler | +| `PushSessionMessage` push | §gRPC Transport | **Match** | `_session_messages_api.py` | Hand-rolled protobuf, UNAUTHENTICATED retry | +| `PushSessionEvent` push | — | **Impl extra** | `_session_events_api.py` + `middleware/event_compressor.py` | Compressed event push not in spec | +| CP token (RSA-OAEP) | §Token Authentication | **Match** | `_grpc_client.py` | Encrypt session ID, fetch from CP `/token` | +| `get_bot_token()` priority | §Token Authentication | **Match** | `platform/utils.py` | CP cache → file mount → env var | +| `AGUI_TOKEN` middleware | §AGUI_TOKEN Session Auth | **Match** | `app.py` | `secrets.compare_digest()`, exempt `/health` and `/healthz` | +| Bridge ABC | §Bridge Layer | **Match** | `bridge.py` | `capabilities`, `run`, `interrupt` + lifecycle hooks | +| `ClaudeBridge` | §Claude Bridge | **Match** | `bridges/claude/bridge.py` | Session isolation, per-turn lifecycle, `mark_dirty()` | +| `SessionManager` + `SessionWorker` | §Session Isolation | **Match** | `bridges/claude/session.py` | Persistent reader, between-run queue, `--resume` via JSON | +| Deferred `_setup_platform()` | §First-Run Platform Setup | **Match** | `bridges/claude/bridge.py` | 8-step setup per spec | +| Workspace resolution | §Workspace Resolution | **Match** | `platform/workspace.py` | Workflow → multi-repo → default | +| `SESSION_CONFIG_PATH` | §Workspace Resolution | **Match** | `platform/config.py` | Validates abs, exists, is_dir; enables skills | +| Credential sidecar isolation | §Credential Management | **Match** | `platform/auth.py` | Sidecar mode skips env population | +| Git operations via MCP | §Git Operations | **Match** | `platform/prompts.py` | System prompt instructs MCP tools; no `git push` from runner | +| MCP server assembly | §MCP Servers | **Match** | `bridges/claude/mcp.py` | All server types present | +| `acp` MCP server fallback | §MCP Servers | **Match** | `bridges/claude/backend_tools.py` | Only registered when `AMBIENT_MCP_URL` unset | +| OpenShell file-mode sandbox | §OpenShell Sandbox | **Match** | `standard-claude-wrapper.sh` | Supervisor dispatch with stale netns cleanup | +| OpenShell gateway mode | §Gateway Mode | **Match** | `Dockerfile.openshell`, `openshell-claude-wrapper.sh`, `entrypoint.sh` | | +| Inference routing (`ACP_OPENSHELL_INFERENCE`) | §Runner-Side Inference Routing | **Match** | `bridges/claude/auth.py` | Sets `ANTHROPIC_BASE_URL=https://inference.local`, proxy vars, clears Vertex flags | + +--- + +## B. Identified Gaps + +### B1. Gaps with Missing or Zero Test Coverage + +| # | Gap | Risk | Affected File(s) | Current Test Coverage | +|---|-----|------|-------------------|----------------------| +| **G1** | `AGUI_TOKEN` session auth middleware — prevents cross-session attacks via `X-Ambient-Session-Token` header | **CRITICAL** — security boundary | `app.py:264-283` | **Zero tests** | +| **G2** | SSE event stream end-to-end through OpenShell gateway `ExecSandbox` path | **HIGH** — the exact failure path from pod logs | `endpoints/events.py`, `grpc_transport.py`, `entrypoint.sh` | **Zero integration tests** for gateway-specific SSE delivery | +| **G3** | OpenShell inference routing auth setup (`setup_sdk_authentication` when `ACP_OPENSHELL_INFERENCE=true`) — env var population for proxy/TLS | **HIGH** — makes inference work through the OpenShell proxy | `bridges/claude/auth.py:86-125` | **Zero tests** | +| **G4** | Content endpoint path traversal validation — validates paths stay within `WORKSPACE_PATH` | **HIGH** — security | `endpoints/content.py` | **Zero explicit security tests** | +| **G5** | `RESUME_AFTER_SEQ` gRPC listener resume filtering — prevents duplicate Claude turns on pod restart | **MEDIUM** — correctness on restart | `bridges/claude/grpc_transport.py` | **Zero targeted tests** | +| **G6** | `mark_dirty()` session ID preservation across adapter rebuild — ensures `--resume` survives MCP config changes | **MEDIUM** — session continuity | `bridges/claude/bridge.py` | **Zero tests** | +| **G7** | `PermissionError` retry in `_handle_user_message` — token refresh + retry once | **MEDIUM** — gRPC transport resilience | `bridges/claude/grpc_transport.py` | **Zero tests** | +| **G8** | `GRPCMessageWriter` edge cases — empty text, very large text, mid-stream error flush | **MEDIUM** — data integrity | `bridges/claude/grpc_transport.py` | Partial (basic consume tested, no edge cases) | +| **G9** | Event compressor → `SessionEventsAPI` push integration — combined path through `grpc_push_middleware` | **MEDIUM** — event delivery | `middleware/grpc_push.py`, `middleware/event_compressor.py`, `_session_events_api.py` | Partial (unit tests exist separately, no integration test) | +| **G10** | `openshell-claude-wrapper.sh` recursion guard and env setup | **HIGH** — guards infinite recursion, sets proxy/TLS env | `openshell-claude-wrapper.sh` | **Zero tests** (shell script) | +| **G11** | `entrypoint.sh` ndots patching — resolv.conf rewrite for musl libc DNS | **MEDIUM** — the exact ndots:5 issue from pod logs | `entrypoint.sh` | **Zero tests** (shell script) | +| **G12** | SSE heartbeat keepalive (30s timeout → `: heartbeat\n\n`) | **LOW** — correctness under slow consumers | `endpoints/events.py` | **Zero tests** | +| **G13** | `POST /model` between-run event delivery to SSE consumers | **LOW** — model switch notification | `endpoints/model.py`, `bridges/claude/session.py` | Partial (structural test exists, no SSE integration) | +| **G14** | `STOP_ON_RUN_FINISHED` — calls `os._exit(0)` after run completes | **LOW** — one-shot session mode | `bridges/claude/grpc_transport.py` | **Zero tests** | + +### B2. Runtime-Observable Gaps (from pod failure at 2026-07-11T11:11:45Z) + +| # | Observation | Root Cause Hypothesis | Evidence | +|---|---|---|---| +| **R1** | `exec process exited with code 1` — runner exec failed 2s after token issuance | Entrypoint or uvicorn startup crashed inside the sandbox. Possible causes: missing deps at `/runner/ambient-runner`, Python venv not activated, proxy misconfiguration blocking pip/import, or ndots issue preventing DNS resolution for gRPC channel setup. | CP log: `"failed to start runner exec"` at 11:11:45, sandbox `session-3gm4lrutcm9dtery27bmsxffwx4` | +| **R2** | `pr-reviewer` session (`3GM9fQ9KNznJnQlXCsgMbuk0x5R`) never appeared in the watch stream | gRPC watch stream reconnection gap or the session was created during a stream interruption. The watch reconnected once at 11:01:46 with 935ms backoff. If `pr-reviewer` was created during a subsequent reconnect gap, the event could be lost. | CP log: zero events matching session ID `3GM9fQ9KNznJnQlXCsgMbuk0x5R` across entire log | +| **R3** | ndots:5 pod recreation dance — sandbox pod started with ndots:5 despite CR patch | OpenShell gateway controller creates the pod from the sandbox CR spec before the CP's ndots:1 patch propagates to the pod template. The CP detected the mismatch at 11:11:09 and deleted the pod for recreation. | CP log: `"sandbox pod has ndots:5, deleting for recreation from patched CR"` | + +### B3. Spec Deviations (Non-Gap — Implementation Differs from Spec Text) + +| # | Spec Says | Implementation Does | Impact | +|---|-----------|---------------------|--------| +| **D1** | `GRPCMessageWriter` accumulates `MESSAGES_SNAPSHOT` events, keeps only the latest, extracts assistant text on `RUN_FINISHED` or `RUN_ERROR` | Writer uses `TEXT_MESSAGE_START/CONTENT/END` event buffering — appends `delta` on CONTENT, pushes accumulated text on END | **None** — functionally equivalent, simpler implementation. Spec should be updated. | +| **D2** | SSE queue size: 100 | Queue is `asyncio.Queue()` with no explicit maxsize in the events endpoint (unbounded). The `_active_streams` queue pre-registered in lifespan also has no explicit size. | **Low risk** — could grow under pathological conditions but practical runs are bounded | +| **D3** | Spec lists `AGUI_PORT` default as `8001` in the overview diagram but env var table says `0.0.0.0:8001` | `app.py` defaults `AGUI_HOST=0.0.0.0` and `AGUI_PORT=8000`. `entrypoint.sh` (OpenShell image) forces port `8001`. | **Potential confusion** — spec and non-OpenShell default disagree on port number | + +--- + +## C. Proposed Test Plan + +### C1. Security Tests — CRITICAL + +**File:** `tests/test_agui_token_middleware.py` + +| Test | Validates | +|------|-----------| +| `test_token_correct_passes_through` | Valid token in `X-Ambient-Session-Token` header → request succeeds | +| `test_token_incorrect_returns_401` | Wrong token → 401 Unauthorized | +| `test_token_absent_returns_401` | Missing header → 401 Unauthorized | +| `test_health_exempt_with_token_set` | `GET /health` succeeds without token when `AGUI_TOKEN` is set | +| `test_healthz_exempt_with_token_set` | `GET /healthz` succeeds without token when `AGUI_TOKEN` is set | +| `test_no_token_env_no_middleware` | `AGUI_TOKEN` unset → all requests pass without header | +| `test_timing_safe_comparison` | Verify `secrets.compare_digest` is used (not `==`) | +| `test_non_health_paths_require_token` | `/events/x`, `/`, `/model`, `/interrupt`, `/capabilities` all require token | + +**File:** `tests/test_content_path_traversal.py` + +| Test | Validates | +|------|-----------| +| `test_path_within_workspace_allowed` | Valid path under `WORKSPACE_PATH` succeeds | +| `test_path_traversal_dot_dot_rejected` | `../../etc/passwd` → 400/403 | +| `test_symlink_escape_rejected` | Symlink pointing outside workspace → rejected | +| `test_absolute_path_outside_workspace_rejected` | `/etc/shadow` → rejected | +| `test_null_byte_injection_rejected` | Path with `%00` → rejected | + +### C2. AG-UI SSE Event Stream Tests — HIGH + +**File:** `tests/test_sse_event_stream_e2e.py` + +| Test | Validates | +|------|-----------| +| `test_events_endpoint_streams_text_message_events` | Full text message lifecycle (START → CONTENT → END) delivered via SSE | +| `test_events_endpoint_filters_messages_snapshot` | `MESSAGES_SNAPSHOT` events never reach the SSE consumer | +| `test_events_endpoint_closes_on_run_finished` | Stream terminates after `RUN_FINISHED` event | +| `test_events_endpoint_closes_on_run_error` | Stream terminates after `RUN_ERROR` event | +| `test_events_endpoint_heartbeat_on_timeout` | `: heartbeat\n\n` sent after 30s of no events | +| `test_events_endpoint_queue_cleanup_on_disconnect` | Queue removed from `_active_streams` after client disconnects | +| `test_events_endpoint_concurrent_consumers` | Two consumers on the same `thread_id` both receive events | +| `test_events_wait_endpoint_polls_until_queue_appears` | `/wait` variant waits up to timeout for queue registration | +| `test_events_wait_endpoint_returns_404_on_timeout` | `/wait` returns 404 if queue never appears | +| `test_events_endpoint_with_grpc_listener_fanout` | gRPC listener `_handle_user_message` → bridge.run() → SSE tap receives events | + +### C3. gRPC Transport Tests — HIGH + +**File:** `tests/test_grpc_message_writer_extended.py` + +| Test | Validates | +|------|-----------| +| `test_multi_fragment_text_accumulation` | 10+ `TEXT_MESSAGE_CONTENT` deltas concatenated correctly | +| `test_empty_text_message` | `TEXT_MESSAGE_START` → `TEXT_MESSAGE_END` with no CONTENT → pushes empty string | +| `test_large_text_accumulation` | 100KB+ of content → single push with full text | +| `test_push_error_flushes_buffered_text` | `push_error()` pushes any buffered text before the error | +| `test_push_failure_logged_not_raised` | gRPC push failure → logged at warning, no exception propagated | +| `test_interleaved_tool_and_text_events` | Tool events between text messages don't corrupt text accumulation | + +**File:** `tests/test_grpc_listener_resume.py` + +| Test | Validates | +|------|-----------| +| `test_resume_after_seq_skips_old_messages` | Messages with `seq <= RESUME_AFTER_SEQ` are filtered out | +| `test_resume_after_seq_processes_new_messages` | Messages with `seq > RESUME_AFTER_SEQ` trigger `bridge.run()` | +| `test_time_based_resume_filtering` | Messages before the 5-second lookback cutoff are skipped | +| `test_permission_error_triggers_token_refresh_and_retry` | `PermissionError` in `_handle_user_message` → refresh token → retry once | +| `test_stop_on_run_finished_exits` | `STOP_ON_RUN_FINISHED=true` → `os._exit(0)` after run completes | + +### C4. OpenShell Gateway Mode Tests — HIGH + +**File:** `tests/test_openshell_inference_routing.py` + +| Test | Validates | +|------|-----------| +| `test_inference_routing_sets_anthropic_base_url` | `ANTHROPIC_BASE_URL` → `https://inference.local` | +| `test_inference_routing_sets_https_proxy` | `HTTPS_PROXY` → `http://10.200.0.1:3128` | +| `test_inference_routing_sets_ssl_cert_file` | `SSL_CERT_FILE` → `/etc/openshell-tls/openshell-ca.pem` | +| `test_inference_routing_sets_requests_ca_bundle` | `REQUESTS_CA_BUNDLE` → same CA path | +| `test_inference_routing_sets_node_extra_ca_certs` | `NODE_EXTRA_CA_CERTS` → same CA path | +| `test_inference_routing_clears_vertex_flags` | `USE_VERTEX` and `CLAUDE_CODE_USE_VERTEX` removed from env | +| `test_inference_routing_returns_placeholder_api_key` | Returns `("inference-routing", False, model)` | +| `test_inference_routing_default_model` | Model defaults to `claude-sonnet-4-6` | +| `test_inference_routing_disabled_when_env_unset` | Falls through to Vertex/Anthropic path when `ACP_OPENSHELL_INFERENCE` absent | +| `test_inference_routing_disabled_for_false_values` | `ACP_OPENSHELL_INFERENCE=false` → not enabled | + +**File:** `tests/test_openshell_wrapper.sh` (bash/bats) + +| Test | Validates | +|------|-----------| +| `test_wrapper_dispatches_to_supervisor_when_enabled` | `OPENSHELL_ENABLED=true` → execs `/openshell-sandbox` | +| `test_wrapper_dispatches_direct_when_disabled` | `OPENSHELL_ENABLED` unset → execs Claude directly | +| `test_wrapper_recursion_guard_prevents_infinite_loop` | Second invocation hits guard file → execs `--bare` directly | +| `test_wrapper_sets_home_sandbox` | `HOME=/sandbox` in wrapper environment | +| `test_wrapper_cleans_stale_netns` | Stale `/var/run/netns/sandbox-*` entries cleaned before supervisor launch | + +### C5. Bridge Lifecycle Tests — MEDIUM + +**File:** `tests/test_mark_dirty_session_preservation.py` + +| Test | Validates | +|------|-----------| +| `test_mark_dirty_preserves_session_ids` | `_saved_session_ids` snapshot taken before manager shutdown | +| `test_mark_dirty_triggers_adapter_rebuild` | `_adapter` and `_ready` cleared → next `run()` calls `_setup_platform()` | +| `test_mark_dirty_restores_session_ids_after_rebuild` | Session IDs restored in new `SessionManager` after rebuild | +| `test_mark_dirty_concurrent_with_active_run` | `mark_dirty()` during an active run → run completes, rebuild happens after | + +**File:** `tests/test_model_switch_integration.py` + +| Test | Validates | +|------|-----------| +| `test_model_switch_emits_between_run_event` | `ambient:model_switched` custom event placed on worker between-run queue | +| `test_model_switch_rejected_during_generation` | Returns 422 when session worker lock is held | +| `test_model_switch_updates_env_var` | `LLM_MODEL` env var updated, `LLM_MODEL_VERTEX_ID` cleared if Vertex | +| `test_model_switch_triggers_mark_dirty` | `bridge.mark_dirty()` called after env update | + +### C6. Event Compressor + Push Integration — MEDIUM + +**File:** `tests/test_grpc_push_integration.py` + +| Test | Validates | +|------|-----------| +| `test_text_message_compressed_to_single_push` | START + N×CONTENT + END → single `PushSessionEvent` with `event_count=N+2` | +| `test_tool_call_compressed_to_single_push` | TOOL_CALL_START + N×TOOL_CALL_ARGS + TOOL_CALL_END → single push | +| `test_standalone_events_pushed_individually` | `RUN_STARTED`, `TOOL_CALL_RESULT` → individual pushes with `event_count=1` | +| `test_flush_on_stream_end` | Incomplete accumulation flushed when stream ends without END event | +| `test_dual_push_session_messages_and_events` | Each event pushed to both `session_messages` and `session_events` APIs | +| `test_no_op_when_grpc_unconfigured` | `AMBIENT_GRPC_URL` unset → events pass through with zero gRPC calls | + +--- + +## D. Summary + +### Spec Coverage + +All major spec features are implemented. The implementation has grown beyond the spec in useful ways (event compression via `PushSessionEvent`, `/wait` endpoint, `content`/`tasks`/`repos`/`workflow`/`feedback` endpoints, runtime model switching). + +### Key Risk Areas (ranked) + +1. **AGUI_TOKEN middleware** (G1) — zero tests for a security boundary that prevents cross-session attacks +2. **Content endpoint path traversal** (G4) — zero explicit security tests for file access validation +3. **OpenShell inference routing auth** (G3) — zero tests for the env var population that makes inference work through the proxy +4. **SSE event stream through gateway exec** (G2) — the exact path that failed in the pod logs +5. **OpenShell wrapper recursion guard** (G10) — shell script with zero test harness, guards infinite recursion +6. **gRPC listener resume** (G5) — zero targeted tests for the duplicate-turn prevention mechanism (`RESUME_AFTER_SEQ`) +7. **`mark_dirty()` session ID preservation** (G6) — zero tests for the `--resume` survival path across MCP rebuilds + +### Pod Failure Analysis + +The `hello-world` session failure at 11:11:45 (`exec process exited with code 1`) is in the gateway `ExecSandbox` path. The control plane successfully created the sandbox, configured OPA policy, and called `ExecSandbox` with entrypoint `/runner/entrypoint.sh`, but the process inside the sandbox died within 2 seconds. Root cause candidates: + +- Python venv not properly activated inside the sandbox (path mismatch between `Dockerfile.openshell` and `entrypoint.sh`) +- Proxy misconfiguration blocking Python imports or gRPC channel setup +- ndots issue (patched but possibly re-emerging) preventing DNS resolution for `ambient-api-server` during startup +- Missing runtime dependency in the OpenShell sandbox image + +The `pr-reviewer` session never appearing in the watch stream is a separate issue: either the gRPC watch stream had a reconnection gap that dropped the event, or the session creation occurred during a period where the listener was not connected. diff --git a/specs/standards/specs/specs.spec.md b/specs/standards/specs/specs.spec.md new file mode 100644 index 000000000..43e77999c --- /dev/null +++ b/specs/standards/specs/specs.spec.md @@ -0,0 +1,323 @@ +# Spec Authoring Standards + +Cross-cutting standards for writing specifications that serve as machine-readable desired-state declarations for autonomous agentic reconciliation. + +## Purpose + +Specs are the source of truth for desired system behavior. Code is the actual state. The development lifecycle reconciles the two through automated gap analysis, wave planning, and code generation. This standard defines how to write specs that agents can reliably parse, diff, and reconcile — enabling hands-free software delivery from intent to production. + +## Context: Spec-Driven Development in an Agentic SDLC + +Spec-driven development inverts the traditional workflow. Instead of humans writing code and documenting it afterward, specs declare the desired end state and autonomous agents reconcile code to match. This model draws from three established disciplines: + +1. **Declarative reconciliation** (Kubernetes controllers) — desired state vs. actual state, with a control loop that converges the two. Specs are the CustomResource; code is the running system. +2. **Design by contract** (Eiffel, DbC) — preconditions, postconditions, and invariants expressed as behavioral contracts rather than implementation instructions. +3. **Behavior-Driven Development** (BDD/Gherkin) — Given/When/Then scenarios as executable specifications that double as acceptance tests. + +The agentic SDLC adds a fourth dimension: **machine parseability**. Specs must be structured consistently enough that an agent can extract requirements, build a dependency graph, search for implementations, classify gaps, and generate code — all without human intervention. + +### Industry Standards Referenced + +| Standard | Application | +|----------|-------------| +| RFC 2119 (Key words for use in RFCs) | Requirement strength keywords: SHALL, MUST, SHOULD, MAY | +| IEEE 830 (Software Requirements Specifications) | Requirement attributes: unambiguous, complete, consistent, verifiable, traceable | +| Gherkin (Cucumber BDD) | Scenario format: Given/When/Then with And/But | +| RFC 6648 (Deprecating X- prefix) | No invented prefixes; use established terminology | +| OpenAPI 3.x | API contract co-evolution with specs | +| Kubernetes Declarative Model | Desired state + reconciler pattern | + +## Requirements + +### Requirement: Spec File Naming + +Every spec file SHALL use the naming convention `<descriptive-kebab-case-title>.spec.md` and SHALL be placed in `specs/{domain}/` where domain is one of the established capability domains (platform, security, ui, cli, standards). + +New domains MAY be introduced when existing domains become too broad, but SHALL NOT be created preemptively. + +#### Scenario: New platform feature spec + +- GIVEN a developer writes a spec for session timeout behavior +- WHEN the spec is placed in the repository +- THEN the filename is `session-timeout.spec.md` +- AND the file is located at `specs/platform/session-timeout.spec.md` + +#### Scenario: Domain boundary decision + +- GIVEN a spec covers both API behavior and RBAC policy +- WHEN the primary concern is authorization enforcement +- THEN the spec is placed in `specs/security/` +- AND cross-references to the platform data model use relative links + +### Requirement: Document Structure + +Every spec SHALL contain the following top-level sections in order: + +1. `# Title` — a single H1 heading +2. Introductory paragraph(s) — prose overview of what this spec covers and why +3. `## Terminology` (if domain-specific terms are introduced) — definitions of terms used in the spec +4. `## Dependencies` (if cross-spec dependencies exist) — references to other specs this depends on +5. `## Requirements` — one or more `### Requirement: <Name>` subsections +6. `## Migration` (if changing existing behavior) — consumer impact table and amendment list +7. `## Design Decisions` (if non-obvious choices were made) — decision/rationale table + +Optional metadata MAY appear immediately after the H1: + +```markdown +**Date:** YYYY-MM-DD +**Status:** Active | Draft +**Issue:** [#N](url) +``` + +#### Scenario: Minimal valid spec + +- GIVEN an author creates a new spec +- WHEN the spec contains an H1, introductory prose, and at least one Requirement with at least one Scenario +- THEN the spec is structurally valid + +#### Scenario: Spec missing Requirements section + +- GIVEN an author submits a spec for review +- WHEN the spec contains no `## Requirements` section +- THEN the spec is rejected as structurally invalid + +### Requirement: Requirement Format + +Each requirement SHALL be a level-3 heading under `## Requirements` using the format `### Requirement: <Descriptive Name>`. The body SHALL contain one or more declarative statements using RFC 2119 keywords (SHALL, MUST, SHOULD, MAY) to express the strength of each behavioral obligation. + +Requirements SHALL describe observable behavior — inputs, outputs, error conditions, and constraints. Requirements SHALL NOT describe internal implementation details (class names, function signatures, library choices). + +**Quick test:** if the implementation can change without changing externally visible behavior, it does not belong in the requirement. + +#### Scenario: Well-formed requirement + +- GIVEN a requirement states "The API server SHALL return 404 when a session does not exist" +- WHEN an agent parses this requirement +- THEN the agent can identify the subject (API server), the obligation (SHALL), and the observable behavior (return 404) +- AND the agent can search for implementation code that handles this case + +#### Scenario: Requirement with implementation detail + +- GIVEN a requirement states "The `handleGetSession` function SHALL query PostgreSQL using a LEFT JOIN" +- WHEN a reviewer evaluates this requirement +- THEN it is flagged as containing implementation details (function name, SQL join type) +- AND the author is asked to rewrite as observable behavior + +### Requirement: Scenario Format + +Every requirement SHALL have at least one scenario. Scenarios SHALL follow the Given/When/Then format with each clause on its own line, prefixed with a dash and the keyword in uppercase: + +```markdown +#### Scenario: <Descriptive Name> + +- GIVEN <precondition> +- AND <additional precondition> +- WHEN <action or event> +- THEN <expected observable outcome> +- AND <additional outcome> +``` + +Scenarios SHALL be concrete enough to derive automated tests. A scenario that cannot be validated through testing or explicit observation is too vague. + +#### Scenario: Testable scenario + +- GIVEN a scenario states "GIVEN credential A is bound to project P, WHEN a session starts in project P, THEN credential A is injected" +- WHEN an agent reads this scenario +- THEN the agent can derive a test: create a binding, start a session, assert credential presence + +#### Scenario: Untestable scenario rejected + +- GIVEN a scenario states "GIVEN the system is configured correctly, WHEN something happens, THEN it works" +- WHEN a reviewer evaluates this scenario +- THEN it is rejected for vagueness +- AND the author is asked to specify concrete preconditions, actions, and assertions + +### Requirement: RFC 2119 Keyword Discipline + +Specs SHALL use RFC 2119 keywords consistently and exclusively for requirement strength: + +| Keyword | Meaning | Agent interpretation | +|---------|---------|---------------------| +| SHALL / MUST | Absolute requirement. Deviation is a gap. | G-type gap if missing, T-type gap if untested | +| SHALL NOT / MUST NOT | Absolute prohibition. Presence is a gap. | G-type gap if violated | +| SHOULD | Recommended. Exceptions require documented rationale. | Reported but not blocking | +| MAY | Optional. Implementation is at discretion. | Informational only | + +These keywords SHALL appear in UPPERCASE when used normatively. Lowercase "should" or "must" in prose does not carry normative weight. + +#### Scenario: Keyword strength determines gap severity + +- GIVEN a spec states "The system SHALL validate input" +- AND no validation code exists +- WHEN gap analysis runs +- THEN a G-type gap is classified with at minimum HIGH priority + +#### Scenario: SHOULD violation reported but not blocking + +- GIVEN a spec states "The system SHOULD cache responses" +- AND no caching code exists +- WHEN gap analysis runs +- THEN the gap is reported as an advisory, not a blocker + +### Requirement: Dependency Declaration + +Every spec that depends on concepts, entities, or behaviors defined in another spec SHALL declare those dependencies explicitly, either in a `## Dependencies` section or in the Spec Registry (`specs/index.spec.md`). + +Dependencies enable topological ordering for safe reconciliation — a spec's requirements cannot be implemented until its dependencies are satisfied. + +#### Scenario: Spec with undeclared dependency + +- GIVEN a spec references "credential bindings" without depending on `credential-binding.spec.md` +- WHEN a reviewer evaluates the spec +- THEN the missing dependency is flagged +- AND the author is asked to add it to the Dependencies section and the Spec Registry + +#### Scenario: Circular dependency detected + +- GIVEN spec A depends on spec B +- AND spec B depends on spec A +- WHEN the dependency graph is constructed +- THEN the circular dependency is flagged as a structural error +- AND one of the specs must be refactored to break the cycle + +### Requirement: Spec Registry Entry + +Every spec SHALL have a corresponding entry in `specs/index.spec.md` in the Spec Registry table. The entry SHALL include: + +| Field | Description | +|-------|-------------| +| Path | Relative path from `specs/` | +| Domain | Capability domain (platform, security, ui, cli, standards) | +| Primary Entities | Domain objects this spec governs | +| Components | Which system components are affected (API, SDK, BE, CLI, CP, FE, Runner, MCP) | +| Depends On | Other specs this spec depends on (by short name) | + +#### Scenario: New spec without registry entry + +- GIVEN a new spec is created at `specs/platform/session-timeout.spec.md` +- WHEN the spec is submitted for review +- AND no corresponding entry exists in `specs/index.spec.md` +- THEN the review flags the missing registry entry as a blocker + +### Requirement: Component Scope Declaration + +Every spec SHALL identify which components it affects. This enables agents to know which source directories to search during gap analysis and which build/test targets to verify after reconciliation. + +Component identifiers follow the established set: API, SDK, BE, CLI, CP, FE, Runner, MCP. + +#### Scenario: Spec with clear component scope + +- GIVEN a spec declares `Components: CP, Runner` +- WHEN an agent performs gap analysis +- THEN the agent searches only `components/ambient-control-plane/` and `components/runners/ambient-runner/` +- AND build verification targets only those components + +### Requirement: Migration Path Completeness + +Any spec that changes existing behavior SHALL include a `## Migration` section with two subsections: + +1. **Existing consumers** — a table listing every consumer of the changed behavior, its current behavior, and the required change +2. **Specs requiring amendment** — a table listing other specs that must be updated to remain consistent + +#### Scenario: Behavioral change without migration section + +- GIVEN a spec changes the credential resolution algorithm +- WHEN the spec omits a Migration section +- THEN the review flags it as incomplete +- AND the author must enumerate all consumers (control plane, sidecars, runner, UI) + +#### Scenario: Complete migration table + +- GIVEN a spec includes a Migration section +- AND every known consumer is listed with current behavior and required change +- WHEN an agent reads the migration table +- THEN the agent can plan implementation waves that update all consumers + +### Requirement: Observable Behavior Only + +Specs SHALL describe externally observable behavior — what the system does — not how it does it internally. Acceptable spec content includes: API responses, error codes, state transitions, user-visible outcomes, security constraints, data schemas, and protocol contracts. + +Unacceptable spec content includes: internal function names, class hierarchies, specific SQL queries, library or framework choices, and code structure. + +#### Scenario: Spec with acceptable code examples + +- GIVEN a spec includes a protobuf definition showing the wire format of a gRPC message +- WHEN a reviewer evaluates the code example +- THEN it is acceptable because it describes a protocol contract (externally observable) + +#### Scenario: Spec with unacceptable implementation detail + +- GIVEN a spec states "The `SessionRepository` class SHALL use a `sync.Map` for caching" +- WHEN a reviewer evaluates this requirement +- THEN it is flagged as over-specified implementation detail +- AND the author is asked to restate as observable behavior (e.g., "concurrent session lookups SHALL be safe") + +### Requirement: Design Decision Documentation + +When a spec makes non-obvious design choices, those choices SHALL be documented in a `## Design Decisions` section using a table with at minimum `Decision` and `Rationale` columns. + +Design decisions are critical for agents and future authors to understand why a particular approach was chosen, preventing unnecessary re-litigation of settled questions. + +#### Scenario: Design decision with rationale + +- GIVEN a spec chooses hierarchical resolution over flat lookup for credential binding +- WHEN the Design Decisions table documents this choice +- THEN future agents and authors understand the rationale +- AND do not propose alternative approaches that were already considered and rejected + +### Requirement: Living Document Maintenance + +Specs SHALL be maintained as living documents. When system behavior changes, the spec SHALL be updated to reflect the new desired state. Specs SHALL NOT be archived, superseded, or moved to a historical directory. A spec that no longer reflects desired behavior SHALL be deleted, not deprecated. + +#### Scenario: Feature removed from system + +- GIVEN a feature described in a spec is permanently removed +- WHEN the desired state no longer includes this feature +- THEN the spec file is deleted from the repository +- AND the Spec Registry entry is removed from `specs/index.spec.md` + +#### Scenario: Feature behavior changed + +- GIVEN a spec describes credential resolution as flat lookup +- AND the system now uses hierarchical resolution +- WHEN the desired state changes +- THEN the spec is amended in place to describe hierarchical resolution +- AND a changelog note is added to the header metadata + +### Requirement: Cross-Reference Integrity + +Specs SHALL use relative markdown links when referencing other specs. Links SHALL point to specific sections when referencing a particular requirement or concept. + +#### Scenario: Valid cross-reference + +- GIVEN a spec references the token-reader role defined in `credential-binding.spec.md` +- WHEN the reference is written +- THEN it uses the format `[credential-binding](../security/credential-binding.spec.md#requirement-credential-token-reader-grant-lifecycle)` + +#### Scenario: Broken cross-reference detected + +- GIVEN a spec links to a heading that does not exist in the target spec +- WHEN a reviewer checks cross-reference integrity +- THEN the broken link is flagged for correction + +### Requirement: Spec Size and Decomposition + +A spec that exceeds approximately 300 words of prose (excluding scenarios, tables, and code blocks) or covers multiple distinct capabilities SHOULD be decomposed into multiple spec files within a containing directory. + +#### Scenario: Oversized spec covering multiple concerns + +- GIVEN a single spec file covers data model, API behavior, RBAC policy, and UI rendering +- WHEN a reviewer evaluates the scope +- THEN the spec is flagged for decomposition into domain-appropriate files + +## Design Decisions + +| Decision | Rationale | +|----------|-----------| +| RFC 2119 keywords as normative vocabulary | Industry standard, unambiguous, machine-parseable. Agents can classify gap severity directly from keyword strength | +| Given/When/Then scenario format | Maps directly to automated tests. Agents can generate test scaffolding from scenarios without interpretation | +| Mandatory Spec Registry entry | Enables topological ordering for reconciliation waves. Without registry entries, agents cannot determine safe execution order | +| Observable behavior only | Specs that describe internals create false gaps when implementation changes. Observable-only specs remain stable across refactors | +| Living documents over versioned archives | Archived specs create ambiguity about current desired state. Single source of truth eliminates staleness | +| Migration path requirement | The most common spec gap is forgetting to update existing consumers. Mandatory migration tables force completeness | +| Component scope declaration | Bounds the search space for gap analysis. Without scope, agents must search the entire codebase for every requirement |