Fix resource scope PR feedback#185
Conversation
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
|
|
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
There was a problem hiding this comment.
Stale comment
Important:
src/registry/index.tsnow treatsscopeOptionalas fullaccount/org/projectsupport. That widens every pre-existingscopeOptionalresource, not just the six multi-scope read resources this PR is trying to fix. A concrete example isscs_component_vulnerabilityinsrc/registry/toolsets/scs.ts: its path builder only has account mode (noartifact_id) and artifact/project mode (artifact_idpresent), butharness_describe/harness_searchwill now advertise and acceptresource_scope="org"viasrc/tools/harness-describe.tsandsrc/tools/harness-search.ts. Because there is no org-only path, that request falls through to the account endpoint instead of failing locally, which breaks the repo's explicit-scope and fail-loud rules.Important:
src/utils/url-parser.tsnow injectsresource_scopethroughapplyUrlDefaults()for every tool, but onlyharness_list,harness_get, andharness_searchdeclare that input.harness_create,harness_update, andharness_deletealso callapplyUrlDefaults(), so write scope now changes implicitly when the caller pastes a URL even though the write-tool schemas never expose or document that behavior. That makes scope resolution depend on the URL parser instead of the tool contract and goes beyond the read-only scope called out intasks/todo.mdfor this change.Sent by Cursor Automation: Sunil On Demand Architecture Review
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
There was a problem hiding this comment.
Stale comment
1 finding remains before this fully matches the repo's scope-contract rules:
src/tools/harness-describe.ts: the new genericscopeHintoverstates omitted-scope behavior fortemplate, which isscopeOptional: trueand does not fall back to configured org/project defaults whenresource_scopeis absent.The two earlier architecture issues from the prior pass look addressed:
scopeOptionalis no longer treated as implicit tri-scope support.- URL-derived
resource_scopeis now limited to the read tools that actually declare it.Sent by Cursor Automation: Sunil On Demand Architecture Review
…escribe The generic scopeHint incorrectly claimed all multi-scope resources fall back to configured org/project defaults when resource_scope is omitted. For scopeOptional resources (e.g. template), org/project are only injected when explicitly passed — no fallback occurs. This fixes the last review finding from the automated architecture review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> AI-Session-Id: b99ebdd3-ebfb-4761-a28b-efcabe820e9a AI-Tool: claude-code AI-Model: unknown
There was a problem hiding this comment.
Stale comment
The earlier
scopeOptional/ URL-derived scope /scopeHintissues look addressed, and the focused verification on this head is green:
pnpm test tests/registry/registry.test.ts tests/tools/tool-handlers.test.ts tests/utils/url-parser.test.tspnpm typecheckpnpm buildOne architecture issue still remains before this fully matches Sunil's scope-contract standards; see the inline comment.
Sent by Cursor Automation: Sunil On Demand Architecture Review
…ismatch Templates use v1 REST path builders that read input.org_id directly, not the registry's query-param-based scope injection. Advertising supportedScopes caused resource_scope='project' writes to build the account-level path with stray orgIdentifier/projectIdentifier params. Templates already support multi-scope via scopeOptional — callers pass org_id/project_id explicitly to control scope, which the path builders consume correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> AI-Session-Id: b99ebdd3-ebfb-4761-a28b-efcabe820e9a AI-Tool: claude-code AI-Model: unknown
There was a problem hiding this comment.
Stale comment
1 important finding remains before this fully matches Sunil's architecture standards.
src/utils/url-parser.ts: template URLs still opt into URL-derivedresource_scope, but this PR intentionally removed templatesupportedScopes. That leaves pasted account-level template URLs outside the runtime scope contract; see the inline comment for the concrete failure path.Verification:
pnpm test tests/registry/registry.test.ts tests/tools/tool-handlers.test.ts tests/utils/url-parser.test.tspnpm typecheckCI snapshot:
build-and-test (20)passedbuild-and-test (22)passed- smoke-test matrix is still running
Sent by Cursor Automation: Sunil On Demand Architecture Review
…h error Account-level template URLs (e.g. /all/settings/templates/my-template) were auto-filling resource_scope='account' via the URL parser, which then failed in getRequestedScope() since template no longer has supportedScopes. Templates use scopeOptional for scope control — the URL parser should not inject resource_scope for them. Added regression test for this path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> AI-Session-Id: b99ebdd3-ebfb-4761-a28b-efcabe820e9a AI-Tool: claude-code AI-Model: unknown
There was a problem hiding this comment.
Stale comment
1 important finding remains before this fully matches Sunil's architecture standards.
src/registry/index.ts: explicit project scope now resolves org/project defaults for query/body injection, butpathBuilderstill receives only the raw input. That leaves template v1 writes on the account path when callers set project scope through the public update tool viaparams.resource_scope; see the inline comment for the concrete reproduction.Assumption:
- I validated the remaining issue on the current head by running the built
harness_updatepath locally with configuredHARNESS_ORG/HARNESS_PROJECTandparams:{ resource_scope: "project", version_label: "v2" }.Verification:
pnpm typecheckpnpm buildpnpm test tests/registry/registry.test.ts tests/tools/tool-handlers.test.ts tests/utils/url-parser.test.tspnpm testThe earlier
scopeOptional, write-tool URL scope, templatescopeHint, and template URL allowlist issues look addressed on this head. CI’sbuild-and-testjobs are passing; the smoke-test matrix is still running.Sent by Cursor Automation: Sunil On Demand Architecture Review
pathBuilder only received raw input, so resource_scope='project' with
config-backed HARNESS_ORG/HARNESS_PROJECT would build the account-level
/v1/templates/... path instead of /v1/orgs/{org}/projects/{proj}/templates/...
Now executeSpec merges resolved org/project into input (gated by the
requested scope level) before calling pathBuilder. This restores full
tri-scope support for templates — supportedScopes and URL_RESOURCE_SCOPE_TYPES
are re-added with regression tests for both project and account scope paths.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AI-Session-Id: b99ebdd3-ebfb-4761-a28b-efcabe820e9a
AI-Tool: claude-code
AI-Model: unknown
There was a problem hiding this comment.
1 important finding remains before this fully matches Sunil's architecture standards.
src/registry/toolsets/templates.ts: addingsupportedScopesmakesresource_scopepart of the template contract across operations, but the public write tools still do not surface a first-classresource_scopeinput. On this head the runtime path works only viaparams.resource_scope, soharness_describe(resource_type="template")now advertises a scope selector that the write-tool schemas do not actually expose.
Verification on the current head:
pnpm install --frozen-lockfilepnpm test tests/registry/registry.test.ts tests/tools/tool-handlers.test.ts tests/utils/url-parser.test.tspnpm typecheckpnpm build
The earlier scope threads are addressed on this head and have been resolved.
Sent by Cursor Automation: Sunil On Demand Architecture Review
| description: "Reusable template definition. Supports list, get, create, update, and delete. Use resource_scope='account' to list or get account-level templates.", | ||
| toolset: "templates", | ||
| scope: "project", | ||
| supportedScopes: ["account", "org", "project"], |
There was a problem hiding this comment.
Adding supportedScopes here widens the documented resource_scope contract for all template operations, not just list/get. The runtime now honors that on writes too, but only through the generic params escape hatch: on this head I reproduced harness_update(resource_type="template", resource_id="my-template", params:{ resource_scope:"project", version_label:"v2" }) emitting the correct project-scoped v1 path.
The problem is that the registered write-tool schemas still do not expose a top-level resource_scope field at all. For example, harness_update only publishes resource_type, resource_id, url, body, org_id, project_id, and params. So harness_describe(resource_type="template") now tells agents to set resource_scope=..., but the only working path is to smuggle that field through params, which breaks the repo's structured-contract rule for agent-facing metadata.
I’d either add explicit resource_scope inputs to the write tools, or keep template supportedScopes/scope guidance limited to the read paths you actually want to advertise.


Description
Fixes follow-up scope feedback from PR #182 by failing loudly for explicit org/project scope without matching IDs/defaults, filtering broad scoped search fan-out to compatible resource types, and preserving the
resource_scopeZod descriptions.Type of Change
Checklist
Verification:
pnpm test tests/registry/registry.test.ts tests/tools/tool-handlers.test.ts tests/utils/url-parser.test.tspnpm typecheckpnpm buildpnpm test