Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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