Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions DECISIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -480,3 +480,9 @@ The claim predicate matters more than any of the handler-level guards above it.
The 202 carries the existing envelope rather than a new one because both alternatives are worse in the same way: the dashboard assigns the response straight into its `ApprovalResponse` store and MCP forwards any 2xx body verbatim, so a second shape under one route is a runtime break with no type error to catch it. The HTTP code plus `execution_mode` is the honest signal, and `ApprovalResponse` has no `status` discriminator of its own to be consistent with — its `status` is the *approval's*.

Moving the tail rather than reimplementing it is what makes "an approved call owes the same things whichever trigger ran it" checkable: the tests assert *counts*, not existence, because a tail that was copied instead of moved passes an existence check and doubles the audit trail. It stays under `routes::approvals` so the `tail → spawn_auto_call → execute_claimed_approval → tail` cycle remains inside one module and `spawn_auto_call` stays private; the boxed `dyn Future` that breaks that cycle sits on the edge that closes it and is untouched.

## D67: A key nothing reads is a warning on every path, and `openapi::ext` is the only thing that knows where each extension is read

**Date**: 2026-08-12
**Decision**: `openapi::lint_extensions` runs on the alias-normalized document at every validation entry point and reports four classes of key the compiler will silently ignore: **`unknown_extension`** (an `x-overslash-*` name nothing reads, with a `closest_match` suggestion), **`misplaced_extension`** (a known name at a position whose extractor does not read it), **`unprefixed_alias_ignored`** (a bare spelling at a position the alias walk does not rewrite), and **`unknown_template_key`** (an unrecognized bare key at a position whose fields we enumerate). Every one is a **warning, never an error, on every path**. Position comes from **`openapi::ext`**, a `READS` matrix of extension × position that every extractor now reads through via `ext::get(obj, pos, ext)`; the accessor carries a `debug_assert!` against the matrix, and `no_extension_getter_bypasses_the_accessor` bans the `obj.get("x-overslash-…")` spelling in `openapi/` production code. Enforcement lives in **`shipped_services_lint_clean`**, which filters on `LINT_CODES` rather than on `warnings.is_empty()` so an unrelated warning can neither disarm nor break the gate. Three positions are deliberately **open-world** for bare keys — request-body and MCP tool-input properties (JSON Schema), a `discovered_tools` snapshot (the MCP wire shape), a platform-action param, and any unrecognized security-scheme `type` — because at those positions the sibling keys are vocabulary we do not own, and a payload field genuinely named `risk` or `template` must not be reported. A position's own declared fields also win over the extension vocabulary, which is what keeps `x-overslash-mcp.auth.provider` — a read field that shares a name with an `oauth2` scheme's `provider` alias — from reading as misplaced. Two normalizer/reader disagreements found while writing the matrix are fixed rather than reported: `APIKEY_HTTP_SEC_ALIASES` is **split** into `APIKEY_SEC_ALIASES` / `HTTP_SEC_ALIASES`, since `extract_http_auth` reads only `default_secret_name` and `label` and generates its own injection template; and `normalize_parameters_in` gains `normalize_body_properties_in`, so an unprefixed `resolve:` in a request body works instead of being a no-op — the HTTP twin of D55's `input_schema` walk, and a **live behaviour change** with zero shipped-template impact (all nine body-property annotations already use the canonical spelling). Path-item level stays un-extended on purpose: `risk:` hoisted out of a method is not a concept, so the lint reporting it is the right outcome. `registry::load_from_dir` logs findings and still loads the template; `template_resolve` lints the *stored* document into the resolution report the catalog already badges; `validate_delta` re-roots a finding's dot-path from the synthetic `paths.{path}.{method}` onto `extensions.actions.{key}.operation`, so it names something the author can find.
**Rationale**: `services/metabase.yaml` carried `response_type: binary` on `export_query` for months and it did nothing — the compiler only *derives* a response type from a `responses:` block, so a large xlsx export was buffered against `max_response_body_bytes` and the only evidence was the absent `prefer_stream` hint. D57 fixed that instance; nothing would have caught the next one. **Neither motivating bug is a misspelling**, and that decided the scope: `response_type` is a `ServiceAction` field name and `resolve:` is an alias, so a lint over `x-overslash-*` names alone would have caught neither, while a closed-world bare-key check alone would have missed `x-overslash-download` on an HTTP operation and every stray key at an open-world position. Each half misses the other's motivating case, which is the argument that four rules is the scope rather than gold-plating. **Warnings, not errors, because the two strict options are both worse than the disease.** An error at `load_from_dir` means the template is *skipped* — a stray key would remove a service, where before it merely removed a field, and that is a strictly larger outage than the bug. An error on create/update would make an already-active stored org or user template un-saveable by its owner, on tenant data no one can survey, over a key that was inert the whole time. Neither is worth it when the population of shipped offenders is empirically zero and a CI gate holds the line for free. That leaves visibility as the real problem, which is why the lenient paths all had to grow a surface: `POST /v1/templates/validate` already rendered warnings in the editor but the header still read "Valid" over them; the draft page rendered `import_warnings` and dropped `validation.warnings` entirely; the layer editor rendered warnings with no `path`. **`template_resolve` is the highest-leverage placement** and the reason the severity question is answerable at all — it is the only thing that ever looks at a row written *before* the lint existed, so the badge count is how the affected population becomes visible without a migration or a grandfathering table, both of which would have preserved the bug class they were protecting. **The accessor is the part that has to survive.** A hand-maintained position table drifts in the direction that matters most: it claims a key is read somewhere it is not, and the lint then blesses the exact no-op it exists to catch — and a name-level source grep cannot see that, because it reconciles *names* while the lint needs *name × position*. It also could not have distinguished `template` on an `apiKey` scheme from `template` on an `http` one, which is a live instance of the bug. Routing reads through `ext::get` makes the matrix a precondition of the reader instead of a description of it, and the mechanism proved itself during this change: PR #550 landed `x-overslash-icon` on `dev` mid-flight, and the guard failed on the unregistered reader immediately rather than shipping a lint that warned about a correct template. The `.get("x-overslash-…")` ban is scoped to reads and deliberately leaves compound dot-paths and message text alone — those are not reads, and routing them through `Ext::key()` would buy nothing and cost legibility. One honest limit, recorded in TECH_DEBT: the matrix's *position* claims rest on review, since only its *name* claims are mechanically checked.
21 changes: 21 additions & 0 deletions SPEC.md
Original file line number Diff line number Diff line change
Expand Up @@ -1220,6 +1220,27 @@ The template YAML is parsed and validated by a pure-Rust linter in `overslash-co
| `yaml_parse` | YAML source could not be parsed (wrapped serde_yaml error) |
| `schema_error` | JSON input (CRUD path) for `auth` or `actions` is structurally malformed |
| `risk_method_mismatch` *(warning)* | read-only HTTP method (GET/HEAD/OPTIONS) is annotated with `risk: write` or `risk: delete` |
| `unknown_extension` *(warning)* | an `x-overslash-*` key nothing in the gateway reads — a typo, or a name that only ever existed in a design doc |
| `misplaced_extension` *(warning)* | a real extension at a position whose extractor does not read it (e.g. `x-overslash-download` on an HTTP operation — it is MCP-only) |
| `unprefixed_alias_ignored` *(warning)* | the bare spelling of an extension at a position the alias normalizer does not rewrite (e.g. `secrets:` under `components`) |
| `unknown_template_key` *(warning)* | an unrecognized non-`x-` key at a position whose fields are enumerated (this is what catches `response_type:` on an operation) |

**Keys nothing reads (D67).** The four warnings above come from
`openapi::lint_extensions`, which runs on the alias-normalized document at every
entry point. They are **warnings on every path, never errors**: an error at
registry load would *skip* the template, and a missing service is worse than an
ignored field, while an error on update would make an already-active stored
template un-saveable. `shipped_services_lint_clean` is what keeps a shipped
template from regressing, and `template_resolve` re-reports them against the
stored document so a row written before the lint existed becomes visible.

Position is authoritative, not just spelling: `openapi::ext` records which
position reads each extension, and every extractor reads through it. Positions
whose sibling keys are vocabulary Overslash does not own — request-body and MCP
tool-input schema properties, a pasted `discovered_tools` snapshot, a
platform-action param, an unrecognized security-scheme `type` — are open-world
for bare keys, so a payload field genuinely named `risk` or `template` is never
reported. Foreign vendor extensions (`x-amazon-*`, `x-ms-*`) are always ignored.

**Grammar notes.** `[optional segment]` in descriptions is **flat only** — nested `[` inside `[...]` is rejected. A `{param}` placeholder inside a description or `[...]` segment must reference a param defined on the same action. The runtime interpolator in `overslash-core::description` uses the same shared grammar primitives (`overslash-core::description_grammar`) as the linter, so "runtime accepts it but linter doesn't" drift is not possible.

Expand Down
23 changes: 23 additions & 0 deletions TECH_DEBT.md
Original file line number Diff line number Diff line change
Expand Up @@ -329,3 +329,26 @@ same treatment, but it is a bigger decision than it looks: the tests are a
separate binary, so a `#[cfg(test)]` helper cannot reach them, and a public
`Config::for_tests()` is a surface worth agreeing on rather than adding in
passing.

## `ext::READS` positions are reviewed, not proven

`crates/overslash-core/src/openapi/ext.rs` records which document position reads
each `x-overslash-*` extension, and D67's lint reads that matrix to decide
whether a key is misplaced. Two of the three drift directions are closed
mechanically: `ext::get`'s `debug_assert!` fails a reader whose position is
missing from the matrix, and `no_extension_getter_bypasses_the_accessor` bans the
raw `obj.get("x-overslash-…")` spelling in `openapi/` production code.

The third is not. A matrix entry claiming a position that no extractor actually
reads would make the lint stay *silent* on precisely the no-op it exists to
catch, and nothing detects that — `ext::get` is never called at the phantom
position, so the assertion never runs. Each entry cites the reader line it
records (`// schemes.rs:95`), which makes it reviewable, and the four known
asymmetries are pinned by `position_asymmetries_are_recorded`. But the guarantee
is "someone checked", not "the compiler checked".

Closing it properly means the readers *enumerating* their positions rather than
naming one per call — e.g. a per-position extractor trait whose implementation
list is the matrix. That is a much larger refactor of `openapi::extract` than
D67 warranted, and the payoff is bounded: the entries are cited, and a phantom
position only costs a missed warning, never a false one.
11 changes: 11 additions & 0 deletions crates/overslash-api/src/services/template_resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,17 @@ pub async fn resolve(
))
},
)?;
// Lint the stored document, not just the compiled definition.
// This is the only place a template that was written *before* the
// lint existed is ever looked at, so it is what makes the
// affected population visible: the findings ride the resolution
// report that the catalog already badges. Warnings only — a
// stored row must not stop resolving over a key nothing reads.
//
// Standalone layers only. A derived layer's stray keys are
// reported by `validate_delta` at write time; repeating them on
// every fold of every layer in a chain would be noise.
warnings.extend(openapi::lint_extensions(&doc));
let (def, _w) = openapi::compile_service(&doc).map_err(|errs| {
AppError::Internal(format!(
"stored openapi for '{}' failed to compile: {errs:?}",
Expand Down
118 changes: 118 additions & 0 deletions crates/overslash-api/tests/template_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,124 @@ async fn validate_accepts_valid_yaml() {
assert!(body["errors"].as_array().unwrap().is_empty());
}

/// The extension lint reaches the endpoint the editor polls, as *warnings* —
/// the document still saves, but nothing it declares is silently dropped.
///
/// Both findings here are the motivating shapes from #539: `response_type` is a
/// real concept in a position nothing reads, and `x-overslash-download` is
/// MCP-only and inert on an HTTP operation.
#[tokio::test]
async fn validate_reports_ignored_declarations_as_warnings_not_errors() {
let pool = common::test_pool().await;
let (base, client, admin_key) = bootstrap(pool).await;

let yaml = r#"
openapi: 3.1.0
info:
title: Svc
key: lint-warn-svc
servers:
- url: https://api.example.com
paths:
/export:
get:
operationId: export_rows
summary: Export rows
risk: read
response_type: binary
x-overslash-download:
url: .url
x-overslash-disclsoe:
- label: Rows
filter: .rows
"#;

let resp = client
.post(format!("{base}/v1/templates/validate"))
.header(auth(&admin_key).0, auth(&admin_key).1)
.header("content-type", "application/yaml")
.body(yaml)
.send()
.await
.unwrap();
assert_eq!(resp.status(), 200);
let body: Value = resp.json().await.unwrap();

// Warnings never affect validity: a key nothing reads must not block a save.
assert_eq!(body["valid"], true, "body: {body}");
assert!(
body["errors"].as_array().unwrap().is_empty(),
"body: {body}"
);

let warnings = body["warnings"].as_array().unwrap();
let by_path = |p: &str| {
warnings
.iter()
.find(|w| w["path"] == p)
.unwrap_or_else(|| panic!("no warning at {p}; warnings: {warnings:?}"))
};

assert_eq!(
by_path("paths./export.get.response_type")["code"],
"unknown_template_key"
);
assert_eq!(
by_path("paths./export.get.x-overslash-download")["code"],
"misplaced_extension"
);
let typo = by_path("paths./export.get.x-overslash-disclsoe");
assert_eq!(typo["code"], "unknown_extension");
assert!(
typo["message"]
.as_str()
.unwrap()
.contains("did you mean `x-overslash-disclose`?"),
"should suggest the real name: {typo}"
);
}

/// A template carrying an ignored key still persists — the lint must not have
/// quietly become a create-time gate.
#[tokio::test]
async fn create_succeeds_despite_ignored_declarations() {
let pool = common::test_pool().await;
let (base, client, admin_key) = bootstrap(pool).await;

let key = format!("lint-ok-{}", &Uuid::new_v4().to_string()[..8]);
let yaml = format!(
r#"
openapi: 3.1.0
info:
title: Lint OK
key: {key}
servers:
- url: https://api.example.com
paths:
/items:
get:
operationId: list_items
summary: List items
risk: read
response_type: json
"#
);

let resp = client
.post(format!("{base}/v1/templates"))
.header(auth(&admin_key).0, auth(&admin_key).1)
.json(&json!({"openapi": yaml}))
.send()
.await
.unwrap();
assert_eq!(
resp.status(),
200,
"an ignored key must not block a save: {}",
resp.text().await.unwrap()
);
}

#[tokio::test]
async fn validate_reports_yaml_parse_error_as_issue_not_400() {
let pool = common::test_pool().await;
Expand Down
Loading
Loading