Skip to content

fix the a2a sdk client import issue#5

Merged
bokelley merged 4 commits into
adcontextprotocol:mainfrom
prateakb:patch-1
Sep 5, 2025
Merged

fix the a2a sdk client import issue#5
bokelley merged 4 commits into
adcontextprotocol:mainfrom
prateakb:patch-1

Conversation

@prateakb

@prateakb prateakb commented Sep 5, 2025

Copy link
Copy Markdown
Contributor

This pull request improves the integration of the A2A SDK client by updating the dependency version and implementing robust, error-tolerant imports in both the main protocol module and the sales agent handlers. If the SDK import fails, the code now logs a clear warning and gracefully falls back to an HTTP-based implementation.

A2A SDK Integration Improvements:

  • Updated the @a2a-js/sdk dependency to version ^0.3.4 in package.json to ensure compatibility with the latest features and fixes.
  • Modified src/protocols.ts to attempt importing the A2AClient from the SDK, with error handling that logs a warning and falls back to an HTTP-based implementation if the import fails.
  • Updated src/sales-agents-handlers-node.js to similarly attempt importing the A2AClient, providing detailed logs for success or failure and defaulting to an HTTP fallback as needed.

Updated A2A client import logic to handle errors gracefully and provide fallback.
Updated A2A client import with error handling and fallback.
@bokelley
bokelley merged commit b9a0b01 into adcontextprotocol:main Sep 5, 2025
1 check passed
tescoboy pushed a commit to tescoboy/adcp-client that referenced this pull request Apr 14, 2026
…tprotocol#3) in Common Mistakes table

Removes traps adcontextprotocol#1, adcontextprotocol#4, adcontextprotocol#5, adcontextprotocol#6 per reviewer feedback:

adcontextprotocol#1 and adcontextprotocol#6 have accuracy issues
adcontextprotocol#4 and adcontextprotocol#5 to be filed as GitHub Issues (may be Addie-specific)
bokelley added a commit that referenced this pull request Apr 15, 2026
… schemas

Replace 40+ individual schema imports and hand-coded TOOL_META.schema
with TOOL_REQUEST_SCHEMAS from #540. Schema and hasAccount are now
derived from the canonical map at registration time.

- Eliminates dual source of truth (review item #5)
- hasAccount derived from schema ('account' in schema.shape)
- Tools not in TOOL_META still register if present in TOOL_REQUEST_SCHEMAS
- TOOL_META now only contains response builders and annotations

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request Apr 15, 2026
…lper (#541)

* feat: createAdcpServer — declarative server builder with domain-grouped handlers

Domain registration that wires handler types → input schemas → Zod validation
→ response formatting. Auto-generates get_adcp_capabilities from registered tools.

- Domain-grouped handlers: mediaBuy, signals, creative, governance, accounts,
  eventTracking, sponsoredIntelligence
- resolveAccount middleware: auto-resolves AccountReference on tools with account
  field, returns ACCOUNT_NOT_FOUND on failure
- Response builder auto-application: productsResponse, mediaBuyResponse (with
  revision/confirmed_at defaults), getSignalsResponse, etc.
- Tool annotations: readOnlyHint, destructiveHint, idempotentHint set per tool
- Tool coherence warnings: create_media_buy without get_products, etc.
- Unknown handler key warnings: catches typos like getProduct vs getProducts
- Handler error catching: try/catch returns SERVICE_UNAVAILABLE on unhandled errors
- checkGovernance composable helper: seller calls explicitly in financial handlers
- governanceDeniedError convenience: converts denial to COMPLIANCE_UNSATISFIED

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* chore: add changeset for createAdcpServer

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat: pluggable state store for domain object persistence

AdcpStateStore interface with InMemoryStateStore (default) and
PostgresStateStore (production) implementations. Handlers receive
ctx.store for persisting media buys, accounts, creatives, etc.

- Generic document store: get/put/delete/list by collection + id
- InMemoryStateStore: nested Maps, filtering, pagination
- PostgresStateStore: JSONB table, containment queries, keyset pagination
- Shared across all domain handlers via ctx.store
- getAdcpStateMigration() for postgres table setup

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: state store review fixes — cursor bug, patch, copy semantics, limit cap

- Fix InMemoryStateStore cursor tracking (was broken after filtering)
- Add patch() to interface and both implementations (JSONB || merge for postgres)
- get() returns a shallow copy (prevents mutation of store internals)
- list() returns copies and caps limit at 500
- Validate cursor format in PostgresStateStore
- Add tests for cursor roundtrip, patch, copy semantics

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* docs: update all skills and guides for createAdcpServer framework

Updated 8 SKILL.md files, BUILD-AN-AGENT.md, and llms.txt to reference
createAdcpServer with domain-grouped handlers, ctx.store for state
persistence, and auto-applied response builders.

- Seller, signals, creative, governance, SI, brand rights, retail media,
  generative seller skills all show createAdcpServer patterns
- BUILD-AN-AGENT.md leads with createAdcpServer, demotes createTaskCapableServer
  to advanced/escape-hatch section
- llms.txt Quick Start updated with createAdcpServer example
- Common Mistakes tables updated across all skills
- SDK Quick Reference tables updated across all skills

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: update_media_buy hasAccount → true (adcp#2179)

update_media_buy now has a required account field per adcp#2174.
This enables automated account resolution on the most financially
sensitive mutation tool.

preview_creative flattening (adcp#2175) will be picked up when
schemas are regenerated from the updated spec.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: derive hasAccount from schema, validate governance response, fix JSDoc

- hasAccount no longer hand-coded in TOOL_META — derived from Zod schema
  at init time via 'account' in schema.shape. Eliminates drift bugs
  (get_creative_delivery and get_signals were wrong).
- checkGovernance validates required fields before casting response
- Module JSDoc updated to remove resolveGovernance (now composable helper)
- genericResponse includes tool name instead of "OK"
- Add test for required account field on create_media_buy

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: use TOOL_REQUEST_SCHEMAS as single source of truth for tool schemas

Replace 40+ individual schema imports and hand-coded TOOL_META.schema
with TOOL_REQUEST_SCHEMAS from #540. Schema and hasAccount are now
derived from the canonical map at registration time.

- Eliminates dual source of truth (review item #5)
- hasAccount derived from schema ('account' in schema.shape)
- Tools not in TOOL_META still register if present in TOOL_REQUEST_SCHEMAS
- TOOL_META now only contains response builders and annotations

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: context passthrough in createAdcpServer handlers and capabilities

- Handler wrapper auto-injects params.context into response data before
  response builder wraps it. Handlers don't need to echo context manually.
- get_adcp_capabilities registered with actual schema (not empty {}) so
  context from request is received and echoed.
- Storyboard results: 5/6 signal_marketplace steps pass (remaining failure
  is signal_id null/undefined schema mismatch tracked in #542).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: align TypeScript types with Zod nullish, regenerate schemas

The #1 DX blocker: Zod schemas use .nullish() (null | undefined) but
TypeScript types used ? (undefined only). Every handler echoing
params.context hit type errors.

Fix: generate-types.ts now adds | null to optional TypeScript properties,
matching Zod .nullish() behavior. 25 internal call sites updated with
?? undefined to strip null where needed.

Also fixes ZodIntersection schemas (get_signals, etc.) that lost .shape
after regeneration — extractShape() traverses intersection _def.right.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: remove nullish workaround, fix intersection root cause

Root cause fixes for the two schema generation issues:

1. **Nullish removed**: postProcessForNullish was converting every
   .optional() to .nullish(), making Zod produce null|undefined while
   TypeScript types only had undefined. Removed the post-processor —
   Zod schemas now use .optional() matching TypeScript. Also removed
   alignOptionalWithNullish (TypeScript workaround) and
   postProcessLazyTypeAnnotations (cascading nullish fix).

2. **Intersections fixed**: postProcessRecordIntersections already
   handles most cases. The 5 remaining schemas (get_signals, etc.)
   are now clean z.object() with .shape. Only comply_test_controller
   remains a union (by design — multiple scenario types).

Reverted extractShape() workaround in createAdcpServer — schemas
have .shape directly now.

0 nullish, 2877 optional. Types and schemas match. 2972 tests pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: use GOVERNANCE_DENIED instead of COMPLIANCE_UNSATISFIED

GOVERNANCE_DENIED (adcp#2194) is specific to governance agent denials.
COMPLIANCE_UNSATISFIED is broader (content standards, brand safety).
governanceDeniedError() now uses the correct code.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat: regen schemas from adcp#2194, add preview_creative to createAdcpServer

Pull in adcp#2194: GOVERNANCE_DENIED error code, flatten comply_test_controller,
context/ext on all schemas, require signal_id.

- preview_creative now flat z.object() — added to CreativeHandlers, TOOL_META,
  TOOL_REQUEST_SCHEMAS, and CREATIVE_ENTRIES
- Removed old PreviewCreative variant exports (no longer needed)
- Updated discriminated union tests for flat schema behavior
- signal_id now required on signals (should fix storyboard provenance check)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* chore: fix CI — prettier formatting and regenerate agent docs

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: broken resolveAccount example in seller SKILL.md

resolveAccount callback referenced ctx.store which is not in scope.
Fixed to use stateStore (declared outside createAgent factory).
Added InMemoryStateStore import and declaration to make example
copy-paste correct.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request Apr 25, 2026
Two convergent findings from code-reviewer and dx-expert:

1. governance domain was missing from the cross-domain check despite
   five governance specialisms in the spec. Per protocol-expert: coarse
   domain-level check is fine for v0; per-handler-subgroup mapping
   (createPropertyList → property-lists, calibrateContent →
   content-standards, etc.) is a follow-up. The coarse rule catches
   "governance handlers wired, no claim at all" without false positives
   on legitimate cross-cutting reads.

2. Object.keys(config.creative).length > 0 returned true for
   { listCreativeFormats: undefined } — common in spread patterns like
   { ...maybeHandlers }. Replaced with isWired() helper that filters
   to function-valued keys.

Tests added for both:
- governance handlers without specialism → warning
- governance handlers with property-lists claim → no warning
- empty creative {} → no warning
- creative with all-undefined values → no warning

DX-expert verdict on the runtime-vs-typed trade-off: ship.
"Stop here on cheap CI defenses; pivot to skill slimming."

91 tests pass (4 new). Defense ladder #1#5 complete.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request Apr 25, 2026
* feat(server): cross-domain specialism-declaration runtime check

When a domain handler group (creative, signals, brandRights) is wired
but capabilities.specialisms doesn't include any of that domain's
specialisms, createAdcpServer now logs an error explaining the drift
and pointing at the specialism IDs to add.

Drift class from matrix issue #785: ~30% of "agent built every tool
but storyboard reports no applicable tracks" cases trace to a missing
specialism claim. The conformance runner gates tracks on the claim, so
an agent with working tools but no claim grades as failing silently.

Logged via logger.error (matching the idempotency-disabled precedent)
rather than thrown — middleware-only test harnesses legitimately wire
handlers without declaring specialisms. Production agents will see the
warning in boot logs and conformance failure in the matrix.

mediaBuy is intentionally exempt: its specialism choices
(sales-non-guaranteed vs sales-guaranteed vs sales-broadcast-tv vs
sales-social etc.) are commercially significant and may legitimately
defer the declaration. The build-seller-agent skill already covers
the right declaration in its cross-cutting pitfalls section.

Tests in test/server-create-adcp-server.test.js lock the behavior
for creative, signals, brandRights, and the mediaBuy carve-out.

This is dx-expert priority #5; defenses #1#4 shipped in #945, #957,
#961, #970. Cheap-CI-defense ladder complete with this PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(server): expert-review fixes — governance + handler-detection

Two convergent findings from code-reviewer and dx-expert:

1. governance domain was missing from the cross-domain check despite
   five governance specialisms in the spec. Per protocol-expert: coarse
   domain-level check is fine for v0; per-handler-subgroup mapping
   (createPropertyList → property-lists, calibrateContent →
   content-standards, etc.) is a follow-up. The coarse rule catches
   "governance handlers wired, no claim at all" without false positives
   on legitimate cross-cutting reads.

2. Object.keys(config.creative).length > 0 returned true for
   { listCreativeFormats: undefined } — common in spread patterns like
   { ...maybeHandlers }. Replaced with isWired() helper that filters
   to function-valued keys.

Tests added for both:
- governance handlers without specialism → warning
- governance handlers with property-lists claim → no warning
- empty creative {} → no warning
- creative with all-undefined values → no warning

DX-expert verdict on the runtime-vs-typed trade-off: ship.
"Stop here on cheap CI defenses; pivot to skill slimming."

91 tests pass (4 new). Defense ladder #1#5 complete.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot mentioned this pull request Apr 25, 2026
bokelley pushed a commit that referenced this pull request Apr 28, 2026
…m method map, no-account workaround

- `from-platform.ts` header: replace stale "Wired surface (current
  commit)" block with accurate 6.0 inventory (14 SalesPlatform tools,
  tasks/get, per-server statusChange, all shipped specialisms) and
  move remaining deferred items to a "Still deferred (rc.1+)" note.
- `SalesPlatform` interface: add method-group table (core-required /
  core-one-of-pair / read-feedback / retail-media extensions) so new
  adopters know which subset to implement for a non-retail seller vs
  a full retail-media platform.
- `providePerformanceFeedback` / `listCreativeFormats`: document the
  no-account behaviour for 'explicit'-mode adopters (ctx.account may
  be undefined) with three concrete safe patterns. Pins the rc.1
  resolveAccount refactor in the deferred note of from-platform.ts.

Addresses items #1, #2, #4 from bokelley's round-2 review. Item #3
(file size) explicitly deferred by reviewer. Item #5 (spec/codegen
B13) is spec-side and tracked upstream.

https://claude.ai/code/session_01M78kGbgbYkBddneER6RxNF
bokelley added a commit that referenced this pull request Apr 28, 2026
Salesagent feedback #4 — publishStatusChange envelope tightening:
- caused_by_request_id: optional correlation field. Adopters pass
  ctx.task.id from inside *Task method bodies for HITL-flow trail.
- previous_status: optional state-machine field for transition
  validation. Framework doesn't validate in v6.0; field rides through
  to subscribers who can.

Emma round-1 follow-ups:
- #5 Examples relative imports → @adcp/client/server/decisioning +
  @adcp/client/types. Required adding missing public type exports
  (CreateMediaBuySuccess, UpdateMediaBuySuccess, PricingOption variants,
  MediaChannel, AudienceStatus, MediaBuyStatus, CreativeStatus,
  PublisherPropertySelector, ReportingCapabilities). Examples now
  match the canonical adopter import path.
- #18 AccountNotFoundError thrown from specialism methods now projects
  to ACCOUNT_NOT_FOUND wire envelope (was silently mapping to
  SERVICE_UNAVAILABLE). Adopters are still encouraged to handle
  not-found in resolve() — this is the guardrail.
- #20 typesVersions adds server/decisioning entry so adopters on
  moduleResolution: 'node' get autocomplete on the public subpath.

Migration doc:
- New docs/migration-5.x-to-6.x.md covering: why migrate, merge-seam
  incremental migration with mode table, step-by-step recipe, common
  gotchas, what's deferred to v6.1+. Pairs with existing
  docs/migration-4.x-to-5.x.md.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request Apr 28, 2026
Salesagent feedback #5: get_products conflates catalog lookup (fast
read) with proposal generation (brief-to-pitch creative workflow).
Same wire endpoint, different contracts — buyers calling for fast
catalog filtering against a proposal-mode tenant get a slow response
they didn't expect.

Position: get_products stays sync-only, even after adcp#3392 lands
consolidated Submitted arms for the OTHER 5 HITL tools. The unified
hybrid shape (Success | TaskHandoff) is right when ONE verb has
variable timing — wrong when there are TWO verbs being squished into
one tool name.

Filed [adcp#3407](adcontextprotocol/adcp#3407)
advocating a separate request_proposal tool with explicit
Submitted-only semantics. Until that lands, proposal-mode adopters
surface the eventual products via publishStatusChange on
resource_type: 'proposal'.

Doc updates:
- sales.ts getProducts JSDoc reframed: NOT for proposal generation;
  links adcp#3407
- SKILL "Proposal generation is NOT get_products" subsection with the
  reasoning + adcp#3407 link
- Migration doc carve-out: get_products excluded from "v6.1 unified
  shape extension" list
- Changeset entry documenting the position

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request May 3, 2026
Final review of what's queued for 6.7 ship surfaced four
adopter-relevant changes that weren't in the migration recipe yet:

New recipes (12 → 15):
- #13 createTenantStore — opinionated multi-tenant AccountStore
  builder with built-in fail-closed tenant-isolation gate. Resolves
  the screenshot's "RosterAccountStore (Postgres)" item — Postgres
  was always blocked on adopter persistence layer; the helper is
  the SDK side of that contract.
- #14 state-machine helpers (MEDIA_BUY_TRANSITIONS,
  assertMediaBuyTransition, CREATIVE_ASSET_TRANSITIONS,
  assertCreativeTransition). Drop-in replacements for the local
  status-graph copy three example files were carrying.
- #15 createMediaBuyStore — opt-in targeting_overlay echo on
  get_media_buys for sellers claiming property-lists /
  collection-lists.

Recipe updates:
- #5 — note AccountStore.syncGovernance is now typed (drops the v5
  escape-hatch + `as any` pattern); credentials stripped on emit.
- #10 — add Shape C (createRosterAccountStore) for publisher-curated
  rosters alongside Shape A (InMemoryImplicitAccountStore) and
  Shape B (createOAuthPassthroughResolver). Three-shape table.
- #12 — `createOAuthPassthroughResolver` now also re-exported from
  `@adcp/sdk/server` (the patch landed on main); all three import
  paths work.

What-else-changed additions:
- Account<TCtxMeta> v3 wire alignment — billing_entity (with bank
  stripped on emit), rate_card, payment_terms, credit_limit, setup
  (drives pending_approval → active lifecycle), account_scope,
  governance_agents, reporting_bucket. Promoted from a footnote per
  adtech-product-expert's earlier review.
- multi_id truncation dev warn paired with the existing
  media_buy_ids[] fan-out item.
- Cross-specialism dispatch docs — no ctx.platform.<X> accessor;
  class instance + this OR closure capture.
- composeMethod testing recipes link (docs/recipes/composeMethod-testing.md
  is now merged on main from #1371).
- Sales-social + sales-guaranteed planning surface mocks
  (delivery_estimate / forecast / availability) — adopters can now
  wire Product.forecast against the mocks.
- narrowAccountRef export.
- Storyboard-runner task_completion.<key> capture + webhook fallback.

CLAUDE.md and AGENTS.md blurbs updated from "twelve" to "fifteen"
with the three new helper names called out.

Picked up via merge from main (20 commits since the last sync):
createTenantStore (#1420), createMediaBuyStore (#1424),
state-machine helpers (#1427), createRosterAccountStore + Shape C
docs, syncGovernance typed, Account v3 wire alignment, Account.bank
strip, sales-* planning surface (#1414), task_completion capture
(#1426), runner webhook fallback (#1434), narrowAccountRef (#1428),
composeMethod testing recipes (#1371 merged), oauth-passthrough
re-export, multi-id truncation warn.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
andybevan-scope3 added a commit that referenced this pull request Jul 13, 2026
…guard

- #5: writeDiffFile degrades to a placeholder (not a hard fail) on an oversized
  diff / HTTP 406, so large PRs still get a human-flagged review.
- #6: coverage.md largest-file rule reconciled to the base-SHA checkout — read
  changes from the diff files, cite head line numbers from the diff's + side.
- #8: install-hooks.js no longer clobbers a foreign pre-commit hook (warns +
  prints how to chain the delegator instead).
- #9: WG-constitution fetch gets --max-filesize + a fail-open warning; the
  prompt/findings GITHUB_OUTPUT heredocs use randomized sentinels (was fixed,
  a predictable-sentinel injection risk).
- #10: reviewer prompt no longer claims the delta diff is intersected/trivial-
  filtered (the API compare diff is not); wording now matches reality.
- CI dist guard: .secretariat/ai-review/scripts/check-dist.sh + a workflow that
  rebuilds each node action and fails if committed dist/ is stale — the real
  backstop for the bypassable pre-commit hook.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
andybevan-scope3 added a commit that referenced this pull request Jul 13, 2026
…tion (#2354)

* feat(aao-secretariat): fork Scope3 review action tree (setup/reviewer/arbiter/review) into .github/aao-secretariat

Node/TS GitHub Action set forked from Scope3's Argus review action and adapted
for AdCP: validated findings-json contract, arbiter decides via a constrained
submit_decision enum, auth via the AAO Secretariat App, pull_request_target
head-read handling, and the WG constitution fetched from adcp@main at review time.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(aao-secretariat): add repo-root AAO-SECRETARIAT.md; fence action tree from published package

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(aao-secretariat): cut ai-review.yml over to the forked TS action; retire bash reviewer

Supersedes the bash Argus reviewer (incl. #2338's inline WG-constitution/App-identity
edits, now carried by the TS reviewer composite + orchestrator). Keeps
pull_request_target base-SHA checkout + head fetch + workflow-mod gate.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(aao-secretariat): drop write-capable gh grant from reviewer allowlist

With CLAUDE_CODE_SUBPROCESS_ENV_SCRUB=0 the installation token is in the Bash env;
the `gh api repos/*/issues/*` grant allowed a POST, so fork-PR prompt-injection
could exfiltrate the token via an issue comment. The reviewer posts inline comments
through the MCP tool, not Bash gh, so the grant is unnecessary. Removed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore(aao-secretariat): add empty changeset (no-release; dev-tooling only)

Root-level files (AAO-SECRETARIAT.md, .prettierignore) map to the @adcp/sdk root
package (workspaces includes "."), so `changeset status` requires an entry. This
PR ships no library change, so an empty (no-release) changeset satisfies the gate.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(aao-secretariat): source setup's diff from the GitHub API; drop pull_request_target head fetch

CodeQL flagged actions/untrusted-checkout (critical) on the `git fetch <head>`
step: fetching PR-head code in a privileged pull_request_target workflow. setup
now derives changed files (pulls.listFiles), the delta (compareCommits), and the
diff patches (diff media type) from the GitHub API — matching the pattern in
adcontextprotocol/adcp and this repo's prior reviewer — so the head is never
fetched, checked out, or executed. Removed the fetch step.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(aao-secretariat): drop redundant mergeable reassignment in setup

github-code-quality flagged the initial `let mergeable = true` as always
overwritten. The catch re-assigned the same default; removing it makes the
initial value the transient-failure default, resolving the finding.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* refactor(secretariat): move review action tree to .secretariat/ai-review

Relocates the forked review action tree from .github/aao-secretariat/ to a
root-level, dot-prefixed .secretariat/ai-review/ (Option C: the Secretariat's
ai-review desk). The dot prefix marks it as repo infrastructure (like .github)
and keeps it out of glob-based tooling; root placement matches the actions-repo
layout and simplifies the rebuild hook. Rewires uses:/paths-ignore/mod-gate/
codeql/prettierignore refs. Identity (AAO-SECRETARIAT bot, labels, AAO-SECRETARIAT.md,
input names) is unchanged — the desk vs argus naming call is still pending.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(secretariat): rewire workflow/codeql/prettierignore to .secretariat/ai-review

The tree-move commit missed these three reference files (staging error), leaving
the workflow uses:/paths-ignore/mod-gate pointing at the removed
.github/aao-secretariat path. Point them at .secretariat/ai-review.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore(secretariat): add pre-commit dist-rebuild hook (self-contained; wired via install-hooks.js)

Keeps the setup/arbiter committed dist/ in sync with source: when a staged change
touches a node action's src/build-config, rebuild that action and re-stage its
dist/. Logic lives in .secretariat/ai-review/scripts/precommit.sh so it travels
with the tree (Phase-2 extraction); install-hooks.js installs a thin delegating
pre-commit that calls it. No husky, no core.hooksPath change — existing
commit-msg/pre-push hooks are untouched. Fast no-op for commits that don't touch
the tree.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(secretariat): fail closed on unreliable PR surface; drop write-capable gh grant

Review deep-dive fixes:
- #1/#2: computePrSurfaceFiles now returns {files, reliable}. reliable=false on a
  pulls.listFiles error OR the 3000-file cap; setup fails closed (throws) rather
  than clearing the high-risk/gated gates on an incomplete surface — a transient
  API blip or huge PR can no longer let a security-critical PR auto-approve.
- #3: removed the write-capable `gh api repos/*/contents/*` grant from the reviewer
  allowlist (gh api can't be method-restricted, and with ENV_SCRUB=0 the token is
  in the Bash env → prompt-injection exfil path). The agent now reads changes via
  the diff files + gh pr diff. Also removed # comment lines from inside claude_args
  (that block is passed verbatim to the CLI as args).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(secretariat): deep-review follow-ups (#5,#6,#8,#9,#10) + CI dist guard

- #5: writeDiffFile degrades to a placeholder (not a hard fail) on an oversized
  diff / HTTP 406, so large PRs still get a human-flagged review.
- #6: coverage.md largest-file rule reconciled to the base-SHA checkout — read
  changes from the diff files, cite head line numbers from the diff's + side.
- #8: install-hooks.js no longer clobbers a foreign pre-commit hook (warns +
  prints how to chain the delegator instead).
- #9: WG-constitution fetch gets --max-filesize + a fail-open warning; the
  prompt/findings GITHUB_OUTPUT heredocs use randomized sentinels (was fixed,
  a predictable-sentinel injection risk).
- #10: reviewer prompt no longer claims the delta diff is intersected/trivial-
  filtered (the API compare diff is not); wording now matches reality.
- CI dist guard: .secretariat/ai-review/scripts/check-dist.sh + a workflow that
  rebuilds each node action and fails if committed dist/ is stale — the real
  backstop for the bypassable pre-commit hook.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(ci): set least-privilege permissions on the secretariat dist-check workflow

CodeQL flagged the new workflow for not restricting GITHUB_TOKEN. The job only
checks out, builds, and diffs — add a top-level `permissions: contents: read`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* style(secretariat): prettier-format install-hooks.js

The #8 edit introduced a formatting drift that failed the format:check lane.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants