Accept capability and limit customization, driven by the policy packs - #10
Conversation
/api/issue could only ever mint the framework preset. A caller asking for an agent that reads but never writes, or one capped at $5 a transaction, got a generic passport: there was nowhere to put either constraint. It now accepts optional `capabilities` and `limits`. Omit both and nothing changes, which is the point -- the preset is what makes a passport useful on day one and remains the default path. Nothing about the OAP vocabulary is written in this repo. spec/aport-policies and spec/aport-spec are submodules pinned by commit, and scripts/build-oap-registry.mjs compiles every active policy pack into functions/lib/generated/oap-registry.json: capability -> min_assurance, limits_required, policy id and version, plus the capability id pattern read out of passport-schema.json rather than restated. 22 capabilities from 21 packs. Supporting a new one is a submodule bump and a regenerate. `--check` fails when the artifact goes stale. The two merge rules, and why they differ: - `limits` DEEP-merges over the preset. Limits nest four levels in practice (payments.charge.currency_limits.USD.max_per_tx), so a shallow merge is not a smaller version of this, it is silent data loss: changing one currency's per-transaction cap would drop that currency's daily cap and every other currency with it. Merging also means a default added here later reaches every passport, including ones minted by callers that never heard of it. - `capabilities` SELECT. A supplied list names the set to grant, because merging can add or replace an entry but never drop one, which would make "read but never write" inexpressible. Each selected entry still keeps its preset params, with the caller's deep-merged over them. Arrays replace rather than concatenate. `allowed_countries: ["US"]` is a narrowing statement, and appending to the preset's list would widen the very thing being restricted. Assurance is NOT enforced here: aport.io validates on the way in. An unrecognised capability id is logged rather than refused, because the framework presets themselves ship ids the registry does not carry (repo.push, web.search) and refusing those would break working mints. validateGrant() exists for a caller that wants the strict check before sending. 13 suites, 148 tests. Fixtures in test/oap.test.ts are selected by property from the submodule rather than by name, so the suite follows the spec instead of pinning a snapshot of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wy45ojceVnqzG6GLx9HJRD
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
aport.id/functions/api/issue.ts
Lines 306 to 309 in 4d527e7
Replace the selected deliverable capability instead of appending
When a request supplies both capabilities: [{ id: "deliverable.task.complete", params: ... }] and the top-level deliverable configuration, the resolver keeps the caller's entry and this unconditional push adds a second entry with the authoritative parameters. The resulting passport request has duplicate capability IDs, and if they are preserved, app/passport/page.tsx uses .find(...) and therefore displays the caller's first parameter set instead of the deliverable block. Remove or replace the existing entry before adding the authoritative one.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function packCapabilities() { | ||
| const out: Array<{ id: string; min_assurance: string; limits_required: string[] }> = []; | ||
| for (const dir of readdirSync(POLICIES)) { |
There was a problem hiding this comment.
In the checked .github/workflows/deploy.yml test job, actions/checkout@v4 is used without enabling submodules, whose documented default is false (checkout v4 usage). Consequently this directory is empty in every fresh CI checkout, FROM_PACKS contains no entries, and the new registry assertions fail on every push and pull request. Configure checkout to fetch the submodules before this filesystem-driven suite runs.
Useful? React with 👍 / 👎.
| if (isPlainObject(value) && isPlainObject(out[key])) { | ||
| out[key] = deepMerge(out[key], value); |
There was a problem hiding this comment.
Because override is public JSON input and out[key] reads inherited properties, a key named __proto__ resolves to Object.prototype and enters this recursive branch. A roughly 70 KB limits object containing about 5,000 nested __proto__ keys exhausts the JavaScript stack, and resolveLimits runs before the endpoint's issuance try block, so the request terminates with an uncaught RangeError. Reject prototype-related keys or use own-property/null-prototype objects with a nesting limit.
Useful? React with 👍 / 👎.
…erable entry Three findings from the review on #10, all of them real. CI never had the policy packs (P1). actions/checkout defaults submodules to false, so spec/ was empty in every fresh checkout: FROM_PACKS came back empty and the OAP suite asserted against nothing on every push. The test job now checks out recursively and runs `registry:check`, so a stale generated registry fails the build rather than deploying. Pulling the submodules in put their own TypeScript into tsc and eslint, which would have turned CI red a different way, so spec/ is excluded from both. deepMerge recursed into Object.prototype (P2). JSON.parse makes `__proto__` an own property, so it reached the merge from a request body: reading `out.__proto__` returned Object.prototype, which is a mergeable object, and a body of nested `__proto__` keys exhausted the stack with a RangeError thrown before the issuance try block. Writing it ran the setter and reparented the object instead of storing a limit. Prototype-reaching keys are now dropped, reads are own-property only, and a depth cap backstops shapes the key list does not cover. The deliverable block appended a duplicate capability (P2). A request carrying both `deliverable` and `deliverable.task.complete` in `capabilities` produced two entries with that id; consumers read it back with `.find`, so the passport card showed the caller's parameters while the policy evaluated the block's. `upsertCapability` replaces in place, which is the invariant resolveCapabilities already held. 157 tests (up from 148), type-check clean, lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wy45ojceVnqzG6GLx9HJRD
Found by an end-to-end audit of the mint path against the real defaults and the real passport schema: the two mint paths disagreed. `identity.manage_roles` is in this build's DEFAULT_CAPABILITIES and in the capability list aport.io publishes, and it fails the OAP passport schema's own id pattern, which allows no underscore. So minting the defaults granted it, while a caller narrowing to a subset that named it had it silently dropped — asking for less than the default could not reproduce the default. The pattern gate is there to stop a caller injecting an id we would not otherwise mint. Selecting one the preset already carries is not that, so a preset id is now allowed through and logged. An id that is neither valid nor in the preset is still dropped. This does not decide whether the spec's pattern or the capability's name is the thing that is wrong — that is aport.io's call, and it is worth raising, because every default passport this endpoint mints today carries an id the published schema rejects. 160 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wy45ojceVnqzG6GLx9HJRD
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51159dfeb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| spec_commit: process.env.APORT_SPEC_COMMIT ?? null, | ||
| capability_id_pattern: capabilityIdPattern(), | ||
| /** Assurance ladder, lowest first. Order is what makes a comparison possible. */ | ||
| assurance_order: ["L0", "L1", "L2", "L3", "L4"], |
There was a problem hiding this comment.
When validateGrant receives the assurance level returned by createBuilderPassport for a KYC-completed passport, it receives L4KYC (and the service also documents L4FIN), but neither value exists in this hard-coded order. meetsAssurance consequently returns false and reports assurance_too_low even for an L0 capability, so callers cannot validate grants using the service's own passport values; normalize the variants or include them at their appropriate level.
Useful? React with 👍 / 👎.
| const strip = (s) => { | ||
| try { | ||
| const { policies_commit, spec_commit, ...rest } = JSON.parse(s); | ||
| return JSON.stringify(rest); | ||
| } catch { | ||
| return s; | ||
| } | ||
| }; | ||
| if (strip(current) !== strip(serialized)) { |
There was a problem hiding this comment.
When either submodule is bumped without changing the extracted behavioral fields, --check removes both commit fields before comparing and therefore accepts an artifact that still names the previous revisions. In addition, the documented registry:build command supplies neither environment variable and rewrites both values to null. Since registryProvenance() exposes these fields specifically to identify the source versions, derive them from the gitlinks or include them in the freshness check rather than discarding them.
Useful? React with 👍 / 👎.
…thority FRAMEWORK_PRESET_ALLOWLIST was a hardcoded copy of the key set of /api/public/framework-passport-presets — checked two lines before this handler asked that same endpoint. A local cache of a remote list, consulted immediately before consulting the remote. It had drifted, and was refusing five presets that exist and work: `github`, `langgraph`, `vercel-ai-sdk`, `autogen` and `Custom`. Asking aport.id to mint a `github` passport returned generic defaults instead of the GitHub Repository Guard preset, because a constant here had not been updated. The lookup decides now. A framework with a preset gets it; one without falls through to the defaults — the same outcome the allowlist produced for an unknown value, without the drift. `sanitizeFrameworks` still bounds length, charset and count, and the narrower /^[A-Za-z0-9-]+$/ still runs before the value is put in a URL path. Those were always the checks doing the work. The new test asserts the property the allowlist should have had: that the preset endpoint is a superset of what bin/agent-guardrails declares installable. It is, today, for all ten. 163 tests, type-check and lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wy45ojceVnqzG6GLx9HJRD
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10c43e5075
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let capabilities = resolveCapabilities(presetCapabilities, body.capabilities); | ||
| const limits: Record<string, any> = resolveLimits(presetLimits, body.limits); |
There was a problem hiding this comment.
For requests without a framework preset, presetLimits comes from the flat DEFAULT_LIMITS object (currency_limits is top-level), while the newly documented customization shape is capability-scoped. Therefore an override such as {"payments.charge":{"currency_limits":{"USD":{"max_per_tx":500}}}} creates a new subtree instead of merging the existing defaults, so payments.charge.currency_limits.USD.daily_cap and the other currencies are absent from the scoped limits sent upstream. This breaks the promised sibling-preserving behavior for generic passports and can discard financial guardrails; normalize the legacy flat defaults or merge them into the capability namespace first.
Useful? React with 👍 / 👎.
| const [presetsRes, installerRes] = await Promise.all([ | ||
| fetch(PRESETS_URL), | ||
| fetch(GUARDRAILS, { headers: { Accept: "application/vnd.github.raw" } }), | ||
| ]); |
There was a problem hiding this comment.
When either external host is unreachable—for example during an offline local run or a transient DNS/TLS outage—fetch rejects and Promise.all throws before the subsequent res.ok guard, failing the entire test suite. The comment says missing network should skip the check, but only HTTP error responses are currently skipped; catch network failures or replace these live requests with deterministic fixtures.
Useful? React with 👍 / 👎.
…r, provenance Four findings on #10, all real. NAMESPACED OVERRIDES LANDED BESIDE THE DEFAULTS, NOT ON THEM (P1). Limits come in two shapes and both are real: DEFAULT_LIMITS is flat, with `currency_limits` at the top level, while the OAP shape a caller sends is scoped per capability. A deep merge sees two unrelated keys, so `{"payments.charge":{"currency_limits": {"USD":{"max_per_tx":500}}}}` produced a namespace containing exactly that and nothing else — no daily cap, no other currency, no allowed countries — while the real defaults sat above it, shadowed. The sibling-preserving behaviour this module exists for was inverted precisely where it matters most, and the values it dropped were the financial ones. A namespace the caller mentions is now seeded from the flat defaults first. Which flat keys belong to a capability is not guessed: `limits_required` in its policy pack says so. Only mentioned namespaces are seeded, because creating them all would rewrite every default mint's limits to prove a point about one. THE ASSURANCE LADDER WAS TYPED OUT, AND WRONG (P2). It read ["L0".."L4"]. The schema permits L0, L1, L2, L3, L4KYC and L4FIN — "L4" is not a value at all — and createBuilderPassport returns L4KYC for a KYC-completed passport. So the most verified passport we can issue failed every capability check, including L0 ones. The ladder now comes from passport-schema.json's enum, and ranking is by the tier in the name so L4KYC and L4FIN compare as the peers they are. The tests had the same hardcoded value and so agreed with the bug; they read it from the registry now. PROVENANCE WAS DISCARDED BY ITS OWN BUILD COMMAND (P2). The commits came from APORT_POLICIES_COMMIT / APORT_SPEC_COMMIT, which the documented `registry:build` does not set — so running it wrote null into both. And `--check` stripped them before comparing, so a submodule bump that changed no rule left the artifact naming the previous revision, which is the one thing these fields exist to get right. Both now come from the submodule gitlinks and are compared. A LIVE TEST FAILED OFFLINE INSTEAD OF SKIPPING (P2). `fetch` rejects on a missing network rather than returning a non-ok response, so the `res.ok` guard was never reached and the suite failed — the opposite of what its comment promised. 174 tests, type-check and lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wy45ojceVnqzG6GLx9HJRD
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
/api/issuecould only ever mint the framework preset. A caller asking for an agent that reads but never writes, or one capped at $5 a transaction, got a generic passport — there was nowhere to put either constraint.It now accepts optional
capabilitiesandlimits. Omit both and nothing changes, which is the point: the preset is what makes a passport useful on day one and remains the default path.Nothing about OAP is written in this repo
spec/aport-policiesandspec/aport-specare submodules pinned by commit.scripts/build-oap-registry.mjscompiles every active policy pack intofunctions/lib/generated/oap-registry.json:plus the capability id pattern read out of
passport-schema.jsonrather than restated. 22 capabilities from 21 packs. Supporting a new capability is a submodule bump and a regenerate — no code change here.--checkfails CI when the artifact goes stale, and the generator refuses to run without the submodules rather than guessing.It also skips non-
activepacks, so a draft cannot silently license a capability, and fails loudly if two active packs claim one capability with different rules.The two merge rules, and why they differ
limitsdeep-merge over the preset. Limits nest four levels in practice —payments.charge.currency_limits.USD.max_per_tx— so a shallow merge is not a smaller version of this, it is silent data loss. Proven:{payments.charge:{currency_limits:{USD:{max_per_tx:500}}}}USD.max_per_txUSD.daily_capEURallowed_countries,blocked_categoriesMerging also means a default added here later reaches every passport, including ones minted by callers that have never heard of it. A replacement would mean every integration silently opted out of every future default.
capabilitiesselect. A supplied list names the set to grant, because merging can add or replace an entry but never drop one — which would make "read but never write" inexpressible. Each selected entry still keeps its preset params, with the caller's deep-merged over them.Arrays replace rather than concatenate.
allowed_countries: ["US"]is a narrowing statement; appending to the preset's list would widen the very thing being restricted. A merge rule that fails open on the security-relevant case is the wrong rule.What is deliberately not enforced here
Assurance is not gated — aport.io validates on the way in. And an unrecognised capability id is logged rather than refused, because the framework presets themselves ship ids the registry does not carry (
repo.push,web.search); refusing those would break working mints to enforce a rule already enforced downstream.validateGrant()exists for a caller that wants the strict check (unknown capability, assurance floor, missing required limits) before sending.Verification
13 suites, 148 tests. Fixtures in
test/oap.test.tsare selected by property from the submodule — the first L0 pack with no limits, the first L3 pack — rather than by name, so the suite follows the spec instead of pinning a snapshot of it. A test hardcodingdata.file.readwould stop testing anything the day it is renamed.Not yet exercised over HTTP. The tests call the resolvers directly; the path through
onRequestPostand its interaction with thedeliverableblock has not been run against a live request. Worth doing before this is relied on.🤖 Generated with Claude Code
https://claude.ai/code/session_01Wy45ojceVnqzG6GLx9HJRD