Fix Fly.io deployment host/port configuration and add regression tests by bokelley · Pull Request #8 · adcontextprotocol/adcp-client

bokelley · 2025-09-11T13:43:57Z

Summary

Fix critical Fly.io deployment issue causing "instance refused connection" errors
Add comprehensive unit tests to prevent configuration regressions
Update CLAUDE.md with deployment requirements and troubleshooting guide

Problem Fixed

The TypeScript server (src/server.ts) was configured to listen on 127.0.0.1:3000 by default instead of 0.0.0.0:8080 required by Fly.io, causing deployment failures with "instance refused connection" errors.

Changes Made

Server Configuration: Updated src/server.ts to use environment-specific host binding
- Production: 0.0.0.0:8080 (required for Fly.io external access)
- Development: 127.0.0.1:8080 (local access only)
Unit Tests: Added test/server-config.test.js with 6 comprehensive tests validating:
- Host binding logic for production vs development environments
- Correct port defaults for Fly.io compatibility
- Environment variable override behavior
Documentation: Enhanced CLAUDE.md with:
- Critical Fly.io deployment requirements section
- Troubleshooting guide for common deployment issues
- Updated deployment checklists with pre/post-deploy verification
Test Scripts: Added npm test and npm run test:server-config commands

Test Plan

Unit tests pass: npm test (6/6 tests passing)
TypeScript compilation: npm run build succeeds
Fly.io deployment: App successfully deployed and accessible
Server logs show correct binding: Server listening at http://0.0.0.0:8080
Health check: curl -I https://adcp-testing.fly.dev returns 200 OK
Fly status: Machine in "started" state with healthy checks

Prevention Measures

This PR implements multiple safeguards to prevent future deployment regressions:

Automated tests that validate configuration before deployment
Clear documentation of requirements and common pitfalls
Step-by-step verification procedures for deployments

🤖 Generated with Claude Code

- Fix server.ts to bind to 0.0.0.0:8080 in production for Fly.io compatibility - Add comprehensive unit tests to prevent deployment configuration regressions - Update CLAUDE.md with critical deployment requirements and troubleshooting guide - Add test scripts to package.json for automated configuration validation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

Addresses review feedback: - Removed standalone Implementation Traps section - Folded accepted traps (adcontextprotocol#1-adcontextprotocol#6) into existing Common Mistakes table - Dropped traps adcontextprotocol#7 (Accept header), adcontextprotocol#8 (Zod bug), adcontextprotocol#9 (pin version) per reviewer guidance

… matrix v4 (#1120) (#1122) * docs(skills): seller-skill behavioral coverage gaps — v6.1 follow-up (#1120) Five patterns surfaced by matrix v4 where LLM-built sales_guaranteed agents failed storyboard scenarios. All fixes are to skills/build-seller-agent/ only (docs-only, no changeset needed). - Pattern 1: add minimum-tool-surface callout after the tools table so LLMs can't omit sync_accounts and cascade-skip 3 storyboard scenarios - Patterns 2+5: add error-code matrix for create_media_buy / update_media_buy — TERMS_REJECTED (unacceptable performance_standards / measurement_terms), PACKAGE_NOT_FOUND (bogus package_id on update); fix misleading prose at line 523 that said "not adcpError" when adcpError is correct - Pattern 3: rewrite updateMediaBuy example to advance pending_creatives → pending_start (start_time future) or active (start_time now/past) when creative_assignments arrive; add key-point #8 - Pattern 4: fix property_list / collection_list field path from pkg.property_list (wrong — no such field on PackageRequest) to pkg.targeting_overlay.property_list (correct per generated types); add storyboard-discrepancy note for adcontextprotocol/adcp upstream fix https://claude.ai/code/session_01LYWfxv9GxrAwXKYVZ1cXGJ * docs(skills): fix pre-PR review blockers — specialism casing, paused guard, list_creatives, BUDGET_TOO_LOW - sales-guaranteed (not sales_guaranteed) in the minimum-tool callout — spec uses kebab-case specialism IDs per CLAUDE.md - remove list_creatives from required surface (it is optional per SalesPlatform interface; only required when seller hosts its own creative library) - add BUDGET_TOO_LOW as distinct row in error matrix (below-floor budget maps to BUDGET_TOO_LOW, not INVALID_REQUEST per error taxonomy) - fix paused → active guard to patch.paused === false (absent paused field means no state change, not resume) - fix status cast from concrete union to string (covers all reachable states) - tighten storyboard-discrepancy note path to packages[].property_list https://claude.ai/code/session_01LYWfxv9GxrAwXKYVZ1cXGJ * chore: changeset for seller-skill #1120 behavioral coverage Skills bundle with the npm package (files: ["skills/**/*"]), so the behavioral-coverage updates need a published patch release. Documents the four concrete deltas: minimum tool surface, error-code matrix, update_media_buy state machine, targeting_overlay nesting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(seller-skill): expert-review fixes — error matrix + type tightness Addressed parallel expert review on PR #1122: - Error-code matrix: added PRODUCT_UNAVAILABLE row (canonical sold-out / no-longer-available rejection per the spec — `schemas/cache/3.0.0/ enums/error-code.json`). Product expert flagged it missing from the guaranteed-rejection paths. - updateMediaBuy state-machine: typed the `status` local + return cast as `MediaBuyStatus` instead of `string`. Imported `MediaBuyStatus` from `@adcp/sdk`. Code reviewer flagged `as string` as type-loose. - sales-guaranteed.md: dropped the "may check the flat path / storyboard is the bug" footnote. Without verified grader logic the hedge reads as a permission slip ("ship broken code, blame the storyboard"). Skill now teaches the spec-correct nested path only — if the grader actually checks a wrong path, that surfaces organically and we file an upstream issue then with evidence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>

Two reviewers flagged convergent and independent findings; addressing all in this commit. **Recursive credential scan** (security M1, the load-bearing fix). `pickWireSpecFields` keeps `ext` and `context` whole because they're wire-spec fields — but a buyer hiding credentials at depth (e.g. `ext.partner.partner_access_token`) survives both `pickWireSpecFields` and a top-level-only `scrubExtensions` filter. Round-4 vector waiting to happen. `scrubExtensions` now recursively scans `ext` and `context` values using the L1 credential-policy default pattern set, dropping credential-shaped keys at any depth. New `recursiveCredentialScan` option (default `true`) lets adopters opt out only when they've validated surviving shapes themselves. Two tests pin the behavior: nested credentials inside allowlisted top-level keys are dropped; top-level credentials are dropped when no allowlist is set. **Scan starts from depth 1 when an allowlist is set, depth 0 when not.** When the adopter explicitly allowlists `scope3_api_key` at the top level, the scan respects that affirmation — wouldn't second- guess by name. When no allowlist runs, the scan walks every depth including top-level. **`__type` phantom in codegen** (code-reviewer #1). Codegen now emits `{ fields: [...], __type: null as unknown as RequestType }` per entry, imported from `../types` (the index that aggregates core.generated + tools.generated). `pickWireSpecFields<K>` return type widened from `WireSafe<Record<string, unknown>>` to `WireSafe<(typeof WIRE_SPEC_FIELDS)[K]['__type']>` — adopters now get strong per-schema narrowing instead of generic Record. **Runtime-frozen arrays** (security L1). Codegen wraps each `fields` array in `Object.freeze` and the outer object too. Test asserts behavioral effect (length unchanged after push attempt) rather than mode-dependent throw. **`inject` JSDoc warning** (security L2). Explicit "values here MUST be storefront-derived, never read from the incoming buyer request." **Migration footgun warning** (security #8). CTX-METADATA-SAFETY.md now spells out: `pickWireSpecFields` ALONE doesn't close round-2/3 vectors. Adopters MUST chain with `scrubExtensions`. Diff-atomic migration recipe included. **`scrubExtensions` constraint widened** to `T extends object`. Was `T extends Record<string, unknown>` which fights `WireSafe<T>` brand. **SI naming fix in codegen.** `si-*` filenames now correctly emit `SI*` PascalCase (uppercase SI prefix matches `json-schema-to- typescript`'s acronym handling). 20/20 wire-safe tests pass (was 17, +3 new), 1287/1287 server suite, format / typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(server): WireSafe<T> brand + pickWireSpecFields (#1529 L2) Closes #1529. Ships the L2 half of the credential-discipline plan designed in `.context/operational-fanout-design.md`. L1 (#1535 credentialPolicy) catches credential-shaped keys at the buyer-facing dispatch boundary. L2 catches STRUCTURAL leakage at the operational fan-out boundary — storefront fan-out code that picks per-target args from a buyer request and forwards them upstream, where the buyer-facing scan has already passed. Three pieces: 1. **`WireSafe<T>` brand** via `unique symbol`. `{...buyerReq}` is `T`, not `WireSafe<T>` — so passing it where `WireSafe<T>` is required is a compile error. The brand is the load-bearing element: code that spreads a buyer request directly cannot satisfy it. 2. **`pickWireSpecFields(req, schemaName)`** — the canonical constructor of `WireSafe<T>`. Strips a buyer request to the AdCP wire-spec field allowlist for `schemaName`, returns a branded value. Drops top-level credential-shaped keys, unknown attacker payload, and anything not in the schema's `properties` list. 3. **`scrubExtensions(safe, options)`** — per-target ext/context scrub. Filters `ext` and `context` (which `pickWireSpecFields` keeps whole because they're wire-spec fields) to a caller- specified key allowlist, and merges in caller-injected values (storefront-resolved credentials/IDs). Returns `WireSafe<T>` so the brand survives the chain. **Codegen.** `scripts/generate-wire-spec-fields.ts` walks `schemas/cache/{version}/**/*-request.json` and emits `src/lib/server/wire-spec-fields.generated.ts` with field arrays per request type. Drift between the helper and the schema is structurally impossible — both are emitted from the same codegen pass. Restricted to fan-out-relevant request types (every mutating tool in `MUTATING_TASKS` + `get_media_buy_delivery`); 29 types total. **Source-compatible with #1530.** `OperationalPlatform` method signatures keep their existing plain-typed parameters (`request: UpdateMediaBuyRequest`). The brand is opt-in at the type level — adopters who use `pickWireSpecFields` get the safety benefit at the picking site (compile error if they accidentally spread buyer args into the variable typed `WireSafe<T>`). Adopters not using the helper see no behavior change. If we want to make the brand mandatory at the operational boundary in a future major bump, that's a v7 change. 17/17 wire-safe tests, 1291/1291 server suite, format / typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(wire-safe): expert review (#1544) Two reviewers flagged convergent and independent findings; addressing all in this commit. **Recursive credential scan** (security M1, the load-bearing fix). `pickWireSpecFields` keeps `ext` and `context` whole because they're wire-spec fields — but a buyer hiding credentials at depth (e.g. `ext.partner.partner_access_token`) survives both `pickWireSpecFields` and a top-level-only `scrubExtensions` filter. Round-4 vector waiting to happen. `scrubExtensions` now recursively scans `ext` and `context` values using the L1 credential-policy default pattern set, dropping credential-shaped keys at any depth. New `recursiveCredentialScan` option (default `true`) lets adopters opt out only when they've validated surviving shapes themselves. Two tests pin the behavior: nested credentials inside allowlisted top-level keys are dropped; top-level credentials are dropped when no allowlist is set. **Scan starts from depth 1 when an allowlist is set, depth 0 when not.** When the adopter explicitly allowlists `scope3_api_key` at the top level, the scan respects that affirmation — wouldn't second- guess by name. When no allowlist runs, the scan walks every depth including top-level. **`__type` phantom in codegen** (code-reviewer #1). Codegen now emits `{ fields: [...], __type: null as unknown as RequestType }` per entry, imported from `../types` (the index that aggregates core.generated + tools.generated). `pickWireSpecFields<K>` return type widened from `WireSafe<Record<string, unknown>>` to `WireSafe<(typeof WIRE_SPEC_FIELDS)[K]['__type']>` — adopters now get strong per-schema narrowing instead of generic Record. **Runtime-frozen arrays** (security L1). Codegen wraps each `fields` array in `Object.freeze` and the outer object too. Test asserts behavioral effect (length unchanged after push attempt) rather than mode-dependent throw. **`inject` JSDoc warning** (security L2). Explicit "values here MUST be storefront-derived, never read from the incoming buyer request." **Migration footgun warning** (security #8). CTX-METADATA-SAFETY.md now spells out: `pickWireSpecFields` ALONE doesn't close round-2/3 vectors. Adopters MUST chain with `scrubExtensions`. Diff-atomic migration recipe included. **`scrubExtensions` constraint widened** to `T extends object`. Was `T extends Record<string, unknown>` which fights `WireSafe<T>` brand. **SI naming fix in codegen.** `si-*` filenames now correctly emit `SI*` PascalCase (uppercase SI prefix matches `json-schema-to- typescript`'s acronym handling). 20/20 wire-safe tests pass (was 17, +3 new), 1287/1287 server suite, format / typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…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>

The #8 edit introduced a formatting drift that failed the format:check lane. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…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>

bokelley merged commit 56d823b into main Sep 11, 2025
1 check passed

bokelley mentioned this pull request Apr 13, 2026

docs: add Implementation Traps section to build-seller-agent skill #502

Merged

bokelley mentioned this pull request Apr 23, 2026

feat: creative-agent ergonomics — render factories, --strict-flags, shape-drift hint #848

Merged

5 tasks

bokelley mentioned this pull request Apr 30, 2026

docs(skills): seller-skill behavioral coverage gaps — 5 patterns from matrix v4 (#1120) #1122

Merged

bokelley mentioned this pull request May 4, 2026

feat(server): WireSafe<T> brand + pickWireSpecFields (#1529 L2) #1544

Merged

5 tasks

andybevan-scope3 added a commit that referenced this pull request Jul 13, 2026

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

8233c57

The #8 edit introduced a formatting drift that failed the format:check lane. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix Fly.io deployment host/port configuration and add regression tests#8

Fix Fly.io deployment host/port configuration and add regression tests#8
bokelley merged 1 commit into
mainfrom
deploy-isn-t-working

bokelley commented Sep 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

bokelley commented Sep 11, 2025

Summary

Problem Fixed

Changes Made

Test Plan

Prevention Measures

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant