Skip to content

chore: baseline golden files + CI guardrails (PR #0) - #48

Merged
sagi5060 merged 2 commits into
devfrom
chore/golden-baselines-ci-guardrails
Aug 4, 2026
Merged

chore: baseline golden files + CI guardrails (PR #0)#48
sagi5060 merged 2 commits into
devfrom
chore/golden-baselines-ci-guardrails

Conversation

@sagi5060

@sagi5060 sagi5060 commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

PR #0 of the three-ring refactor: the safety net that must exist before any refactor code
is written. Three things, nothing else.

  1. Golden wire baselines (tests/golden/) — byte-level snapshots of the current
    HTTP/SSE surface, recorded against the real serve.py app via TestClient, replayed
    on every make test.
  2. import-linter guardrails (.importlinter) — one contract that is true today, plus
    the agentdeck.core contract staged for Phase 1.
  3. CI — runs the suite, the golden replay (twice, in separate processes) and
    lint-imports on every PR.

Zero behaviour change.

Production files touched

None. No file under agentdeck/ is modified — the determinism seam of Deliverable 1.2
turned out to be unnecessary. The scripted model is injected entirely from the test
fixture:

monkeypatch.setattr("agentdeck.agents.runners.base.OpenAIProvider", ScriptedProvider)

BaseRunner.from_agent already builds its provider by name, and RunConfig.model is a
string, so the SDK resolves the model through model_provider.get_model(...) — the fake
lands at the real agents.models.interface.Model boundary with no production hook.

Full diff: .github/workflows/ci.yml, .pre-commit-config.yaml, Makefile,
pyproject.toml (one pinned dev dep), CHANGELOG.md, .importlinter, tests/golden/.
473 changed lines excluding snapshots.

What is recorded

Eleven cases, each stored as HTTP <status> + the three headers the app sets deliberately

  • the raw body, byte for byte:

/health · POST /agents/{name}/chat · the same ?stream=true (every SSE frame,
separators, terminal done frame) · 422 missing field · 404 unknown agent ·
POST /workflows/{name} · the same ?stream=true (node updates + done) · streamed run
that pauses on interrupt() (interrupt frame in place of done) · GET .../pending ·
POST .../resume · pending once answered.

Normalization rules

None. Nothing is rewritten, masked or sorted on the way into a snapshot. Everything
variable is pinned at the source instead: the scripted model's response ids, item ids,
token counts and created_at are constants; session_id / thread_id / input state are
literals in the capture; conftest._PINNED_ENV neutralises the settings knobs that would
otherwise reach outside the test (Redis sessions, Langfuse export, the sqlite
checkpointer); no endpoint on this surface emits a timestamp. date / server /
content-length are not recorded — an omission of transport noise, documented in
tests/golden/README.md, not a rewrite of the body.

The scripted turn sequence is: turn 1 → function_call to the fixture agent's
lookup_slot tool, turn 2 → three text deltas. The tool call reaches the wire only as
usage.requests == 2, because serve.py forwards text deltas and drops structural
events — itself part of the recorded contract.

Stability

test_capture_is_stable_across_runs captures twice against two independent app instances
in one process and asserts byte equality. CI additionally runs pytest tests/golden twice
in separate processes.

Red-test evidence

Added import fastapi to agentdeck/errors.py, ran the gate, reverted:

$ .venv/bin/lint-imports
Contracts: 0 kept, 1 broken.

----------------
Broken contracts
----------------

errors imports no engine or surface
-----------------------------------

agentdeck.errors is not allowed to import fastapi:

-   agentdeck.errors -> fastapi (l.33)

$ echo $?
1

The staged core contract is present but commented out — import-linter errors on unknown
source modules, so it cannot be enabled before agentdeck.core exists. Phase 1 activates
it by uncommenting the block.

Re-capturing goldens

make golden    # AGENTDECK_GOLDEN_UPDATE=1 pytest tests/golden -q

Deliberate only. A failing replay is a wire change to read, not to re-record; put the
snapshot diff in the PR that changes it. Note that pre-commit's whitespace fixers now skip
tests/golden/snapshots/ — they were rewriting the recordings.

Notes

  • make check gains lint-imports; make test already covers the replay suite because
    it runs all of tests/.
  • Pre-existing, out of scope: pytest tests/ reports its summary in ~31 s but the process
    then lingers for minutes before exiting. Reproduced identically with
    --ignore=tests/golden, i.e. without any of this PR's tests, so it is not introduced
    here (likely the same thing behind c46ebda's "suite hangs on the GitHub runner").

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings August 4, 2026 17:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a “safety net” ahead of the refactor by snapshotting the current HTTP/SSE surface as byte-level goldens, and wiring enforcement into local gates and CI (plus import-linter guardrails).

Changes:

  • Add a golden-wire test suite (tests/golden/) that captures and replays byte-exact HTTP/SSE responses against the real serve.py app using a scripted model.
  • Add import-linter contracts and wire them into make check and CI (lint-imports).
  • Update CI and pre-commit to run/guard these checks (including double-process golden replay) and to avoid whitespace fixers rewriting golden snapshots.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
.github/workflows/ci.yml Runs import-linter and golden replay twice in CI to ensure process-stable snapshots.
.importlinter Adds forbidden-import contract for agentdeck.errors (engine/surface-free), with staged future agentdeck.core contract commented.
.pre-commit-config.yaml Excludes golden snapshot recordings from whitespace fixers.
CHANGELOG.md Documents the new golden baselines + import-linter guardrail additions under Unreleased.
Makefile Adds lint-imports and golden targets; wires lint-imports into make check.
pyproject.toml Adds import-linter to dev dependencies.
tests/golden/README.md Documents what’s recorded, why, and how to re-record snapshots.
tests/golden/conftest.py Provides a pinned-env + scripted-provider TestClient fixture over the fixture project.
tests/golden/fake_model.py Implements a deterministic scripted Agents SDK Model + provider to stabilize wire output.
tests/golden/test_golden_wire.py Captures 11 HTTP/SSE exchanges and asserts they match committed golden snapshots.
tests/golden/fixture_project/.agentdeck/agents/greeter/agent.py Fixture agent used by the golden suite (tool-call + text).
tests/golden/fixture_project/.agentdeck/workflows/approval_flow/workflow.py Fixture workflow covering interrupt/pending/resume path (durable=True).
tests/golden/fixture_project/.agentdeck/workflows/echo_flow/workflow.py Fixture workflow covering “done” path with deterministic node updates.
tests/golden/snapshots/01_health.http Golden snapshot: /health.
tests/golden/snapshots/02_chat.http Golden snapshot: non-stream chat response.
tests/golden/snapshots/03_chat_stream.http Golden snapshot: streamed chat SSE frames + done.
tests/golden/snapshots/04_chat_missing_field.http Golden snapshot: 422 missing field response.
tests/golden/snapshots/05_agent_unknown.http Golden snapshot: 404 unknown agent response.
tests/golden/snapshots/06_workflow.http Golden snapshot: non-stream workflow response.
tests/golden/snapshots/07_workflow_stream.http Golden snapshot: streamed workflow SSE node updates + done.
tests/golden/snapshots/08_interrupt_stream.http Golden snapshot: streamed workflow interrupt frame.
tests/golden/snapshots/09_pending.http Golden snapshot: pending while paused.
tests/golden/snapshots/10_resume.http Golden snapshot: resume response.
tests/golden/snapshots/11_pending_after_resume.http Golden snapshot: pending after resume.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +47 to +53
if UPDATE:
for name, body in recorded.items():
(SNAPSHOTS / name).write_bytes(body)
return
for name, body in recorded.items():
assert body == (SNAPSHOTS / name).read_bytes(), f"wire changed: {name}"
assert sorted(recorded) == sorted(p.name for p in SNAPSHOTS.iterdir())
Safety net for the planned three-ring refactor. No production code changes.

- tests/golden/: byte-level snapshots of the HTTP/SSE surface (11 cases:
  health, chat, streamed chat, 422/404, workflow run, streamed run, interrupt
  stream, pending, resume, pending-after-resume) captured against the real
  serve.py app. Determinism comes from a scripted agents.models.Model
  injected in place of OpenAIProvider by the test fixture, so no field on the
  wire is variable and no normalization is applied.
- .importlinter: forbidden contract keeping agentdeck.errors free of
  agents/langgraph/fastapi/redis, plus the agentdeck.core contract staged
  (commented) for Phase 1.
- CI runs lint-imports and the golden suite twice; make gains golden and
  lint-imports targets, and check includes both.
- pre-commit whitespace fixers skip tests/golden/snapshots/ — they rewrite
  byte-exact recordings.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sagi5060
sagi5060 force-pushed the chore/golden-baselines-ci-guardrails branch from b5f18e9 to bca7459 Compare August 4, 2026 17:27
- pin APP_CONFIG_PATH to the packaged config.default.yaml and
  AGENTDECK_RUNNER_MAX_TURNS: .env / config.yaml resolve from the package's repo
  root, not the cwd, so chdir alone could not neutralize them. README's env
  claim reworded to match.
- record the 500 and SSE-error paths (cases 12/13) via a new BoomFlow fixture
  whose node raises a secret-shaped SkillError, plus a test asserting neither
  recording echoes the message — the one wire contract with a security property.
- make golden now deletes snapshots no case produces, so a renamed case cannot
  leave an orphan that fails the next plain run.
- CI replays the goldens once in a second process instead of twice.
- .gitattributes marks snapshots binary so no checkout can rewrite line endings;
  pytest pythonpath makes the tests/golden imports explicit; one-line reason on
  the import-linter pin.

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

sagi5060 commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Review addressed in a9e6cdc. Point-by-point:

1. README over-claimed env isolation — fixed. You're right that REPO_ROOT makes .env and config.yaml package-relative, so chdir never touched them. Pinned APP_CONFIG_PATH to the packaged config.default.yaml rather than a missing path — that neutralizes a repo-root config.yaml while keeping the shipped defaults rather than silently falling back to pydantic field defaults — plus AGENTDECK_RUNNER_MAX_TURNS=30 for the env-var vector, which wins over YAML either way. README now states the real boundary and says to add a key to _PINNED_ENV rather than normalize its effect away.

2. 500 handler — covered. New BoomFlow fixture (one node raising SkillError("stderr: AGENTDECK_TOKEN=sk-do-not-leak")) and two cases:

12_workflow_error.http        HTTP 500  {"detail":"internal error"}
13_workflow_error_stream.http HTTP 200  event: error / data: {"error": "SkillError"}

Took the streaming counterpart too, since the in-band error frame carries the same property for one extra line. test_failures_never_echo_the_error_message asserts the secret is absent from both — the snapshot alone would pass if a refactor started echoing a different message. 01_health.http re-recorded for the new workflow name. Left the other gaps (skills, second turn on one session, timers) out — PR #0 is the net, not full coverage.

3. make golden orphans — fixed. Update mode now unlinks snapshots no case produces before writing. Moving the assertion above the branch would have made a rename un-recordable, which is the wrong end to fix.

4. Staged contract — keeping it. It's an explicit deliverable of this PR's spec ("the prepared future contract ... included but disabled/commented with a note that Phase 1 activates it"). I agree it's dead config; the disagreement is with the spec, not the review, so I'm not dropping it unilaterally. Happy to delete it if you'd rather the PR body carry the intent alone.

5. Triple golden run — trimmed to one extra separate-process replay, with a comment saying why that one isn't redundant with test_capture_is_stable_across_runs.

6. Nits — all taken: one-line reason on the == pin (matching the openai pin convention), .gitattributes with tests/golden/snapshots/** -text, pythonpath = ["tests/golden"], and the README now names a dependency bump as a legitimate reason for the bytes to move (incl. the queued httpx2 migration the TestClient warning is about).

Re-verified: pytest tests/golden -q → 3 passed · pytest tests/ -q → 79 passed · lint-imports → 1 kept, 0 broken · ruff check + format clean.

One note on CI: no workflow run triggered on the original push — only Copilot Code Review. It ran and passed after the rebase (25s), so the check job is green on the current head; worth a glance at whether first-push runs need approval on this repo.

@sagi5060
sagi5060 merged commit bc2394f into dev Aug 4, 2026
1 check passed
@sagi5060
sagi5060 deleted the chore/golden-baselines-ci-guardrails branch August 4, 2026 17:38
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