Skip to content

docs: canary testing guide + fork-PR preview workflow#1369

Merged
thepagent merged 2 commits into
mainfrom
docs/canary-testing-guide
Jul 12, 2026
Merged

docs: canary testing guide + fork-PR preview workflow#1369
thepagent merged 2 commits into
mainfrom
docs/canary-testing-guide

Conversation

@chaodu-agent

@chaodu-agent chaodu-agent commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

What problem does this solve?

Extracted from #1353 per maintainer scope review, so that PR stays a pure Codex adapter migration. This PR carries the standalone documentation work authored there by @vixenclawsastraagent (credited via Co-authored-by):

  • docs/canary-tests.md (new, 548 lines) — the complete canary-testing workflow: preview builds, safety requirements (isolated bot token, never two instances on one token), acceptance criteria, evidence template, cleanup and rollback drills. The process documented here was exercised end-to-end twice during feat(codex)!: migrate ACP adapter #1353's canary rounds, including catching a real regression that CI could not see.
  • CONTRIBUTING.md — fixes the E2E section's dispatch instructions: fork contributors cannot dispatch PR Preview Build with --ref <branch> (HTTP 403); a maintainer dispatches from main and the workflow resolves the PR head automatically.

Validation

  • Markdown only — no code changes.
  • docs/canary-tests.md cross-references verified against the pr-preview.yml workflow inputs and the feat(codex)!: migrate ACP adapter #1353 canary evidence.
  • README diagrams verified against Cargo.toml unified feature list and docs/adr/unified-binary.md.

Refs #1353.

Extracted from #1353 to keep that PR scoped to the Codex adapter
migration. Content authored on #1353 by @vixenclawsastraagent:

- docs/canary-tests.md: complete canary workflow — preview builds,
  safety requirements, acceptance criteria, evidence template, cleanup,
  and rollback drills. This guide's process was exercised end-to-end
  during #1353's two canary rounds.
- CONTRIBUTING.md: fix the E2E section's dispatch instructions — fork
  contributors cannot dispatch PR Preview Build with --ref <branch>
  (403); a maintainer dispatches from main and the workflow resolves
  the PR head automatically.
- README architecture diagrams: updated to show gateway adapters
  (standalone or embedded unified), WeCom/MS Teams, and the local vs
  AgentCore runtime boundary.

Co-authored-by: vixenclawsastraagent <268079781+vixenclawsastraagent@users.noreply.github.com>
@chaodu-agent

This comment has been minimized.

Keep this PR scoped to the canary-testing process docs (guide +
CONTRIBUTING E2E fix). The README architecture diagram rewrite is an
independent change.
@chaodu-agent chaodu-agent changed the title docs: canary testing guide + fork-PR preview workflow + README architecture docs: canary testing guide + fork-PR preview workflow Jul 11, 2026
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Note

LGTM ✅ — Comprehensive canary-testing guide with correct fork-PR dispatch fix; all cross-references verified against current main.

What This PR Does

Extracts standalone documentation from #1353 so that PR stays a pure Codex adapter migration. Adds a comprehensive canary-testing guide and corrects the fork-contributor dispatch instructions in CONTRIBUTING.md.

How It Works

Pure documentation PR — no code changes:

  • docs/canary-tests.md (548 lines): five-layer canary-testing workflow covering image inspection, ACP protocol smoke, runtime isolation probes, interactive validation, and Discord gateway E2E. Includes security boundaries, acceptance checklists, evidence templates, and cleanup/rollback procedures.
  • CONTRIBUTING.md: corrects the PR Preview Build dispatch from --ref <branch> (HTTP 403 for fork contributors) to --ref main with pr-preview.yml filename, matching the actual workflow behavior.

Findings

# Severity Finding Location
1 🟢 Thorough five-layer structure with clear pass criteria at each layer docs/canary-tests.md
2 🟢 Security-conscious: never two instances on one token, credential isolation, no secret logging docs/canary-tests.md:32-36
3 🟢 Cross-references verified: pr-preview.yml workflow exists and confirms --ref main dispatch resolves PR head via API .github/workflows/pr-preview.yml
4 🟢 Linked worked example comment exists (#1353 issuecomment-4947408465)
5 🟢 CONTRIBUTING.md fix is correct — fork PRs get HTTP 403 dispatching with --ref <branch> CONTRIBUTING.md:150-158
Finding Details

🟢 F1: Well-organized layered testing guide

The five-layer approach (image inspection → ACP smoke → isolation → interactive → gateway E2E) is systematic and progressively validates from static to dynamic. Each layer has explicit pass criteria that can be verified independently.

🟢 F2: Security-first canary design

The guide explicitly forbids dual-instance bot tokens, prohibits pasting credentials in logs/PRs, isolates volumes, and uses mode-600 env files. Good operational discipline baked into the process.

🟢 F3: Workflow cross-reference verified

The pr-preview.yml workflow on main confirms the dispatch model: it accepts pr_number as input, runs resolve-pr job to get head ref/repo via API, and checks out that ref. Dispatching from main is correct.

🟢 F4: Worked example link valid

Comment #issuecomment-4947408465 on PR #1353 exists and is accessible. This gives contributors a real-world reference.

🟢 F5: Correct fork-PR dispatch fix

The old --ref <branch> instruction returns HTTP 403 for fork contributors who lack Actions write permission. The fix (--ref main + pr_number input) matches how the workflow resolves the head internally.

Baseline Check
  • PR opened: by chaodu-agent, branch docs/canary-testing-guide
  • Main already has: basic E2E instructions in CONTRIBUTING.md with incorrect --ref <branch> dispatch; no canary-testing doc
  • Net-new value: complete canary-testing guide (548 lines) and corrected fork-PR dispatch instructions
  • Note: PR title mentions "README architecture" but the current diff (HEAD 47d0275) does not include README changes — title is slightly stale
What's Good (🟢)
  • The canary guide is production-quality documentation — it documents a process exercised twice during feat(codex)!: migrate ACP adapter #1353
  • Clear evidence template and acceptance checklist make it reproducible by other contributors
  • Rollback procedures included for both local Docker and Kubernetes deployments
  • The worked-example reference links to real evidence on feat(codex)!: migrate ACP adapter #1353
  • ACP protocol section is agent-agnostic and language-independent
  • Security boundaries (credentials, volumes, bot tokens) are emphasized throughout

5️⃣ Three Reasons We Might Not Need This PR

  1. Docs could live in the wiki — A 548-line testing guide might be better suited for a wiki or external docs site rather than in-tree, reducing repo size for contributors who never canary-test.
  2. Extracted from an open PR — If feat(codex)!: migrate ACP adapter #1353 is abandoned or significantly reworked, the canary guide's worked example and cross-references become stale immediately.
  3. PR title mentions README architecture changes no longer present — The scope narrowed after force-push; the title is mildly misleading for future git-log readers.

Counter: The guide documents a real process exercised end-to-end, in-tree docs are discoverable without wiki access, and the title discrepancy is cosmetic — the PR body accurately describes the current scope.

@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Note

LGTM ✅ — Accurate, battle-tested process documentation; commands and image-tag formats verified against the workflow that was actually exercised on #1353.

What This PR Does

Documents the canary-testing workflow for PRs (5-layer validation from image inspection to Discord gateway E2E) and fixes CONTRIBUTING's E2E dispatch instructions, which previously told fork contributors to use --ref <branch> — a path that fails with HTTP 403.

Findings

# Severity Finding Location
1 🟢 Dispatch instructions match reality: maintainer dispatches from main, workflow resolves the PR head — exactly the procedure used for #1353's three preview builds docs/canary-tests.md §Build
2 🟢 Image-tag documentation (openab:pr<N>-<variant>) matches the produced artifacts (openab:pr1353-codex) §Build
3 🟢 Head-drift guard (verify headRefOid unchanged before trusting a built image) — this exact situation occurred during #1353's canary rounds; the guard is not hypothetical §Build
4 🟢 Security scan clean: no tokens, internal bot IDs, account IDs, or channel IDs; examples use placeholders and isolated-bot/allowlist safety rules
Verified
  • Structure: 16 sections — prerequisites, build, 5 validation layers, per-agent acceptance checklist, report template, cleanup, post-merge canary, worked example
  • grep scans for token patterns (MTQ5…, xoxb-, ghp_, AKIA, internal UIDs/account IDs): zero hits
  • Link style (/docs/canary-tests.md) consistent with the file's existing absolute-path convention
  • CI: all checks pass (docs-only; smoke tests correctly not triggered)
  • The process documented here was executed end-to-end four times on feat(codex)!: migrate ACP adapter #1353, including catching a real regression that CI could not see

Extracted from #1353 per maintainer scope review; original authorship credited via Co-authored-by.

@thepagent thepagent merged commit d89e635 into main Jul 12, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants