Azure / Azure/azure-functions-agents-runtime

Teach the self-review skill lessons from PR #193

Open
#206 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
9
Forks
7
Avg merge
1d 21h
Merged PRs (30d)
20

Description

> Please provide us with the following information:
> ---------------------------------------------------------------

### This issue is for a: (mark with an `x`)
```
- [ ] bug report -> please search issues before submitting
- [x] feature request
- [x] documentation issue or request
- [ ] regression (a behavior that used to work and stopped in a new release)
```

### Minimal steps to reproduce
> Review PR #193 and its 14 resolved review threads, then compare the original implementation with the corrective commits `9dc59be9`, `90d40cb6`, `3fa77d1e`, and `1edce829`.
>
> The PR passed normal lint, type, unit, coverage, and real-host E2E gates, but human and Copilot reviews still found false-positive test oracles, persisted-error leakage, documentation drift, unsafe error formatting, and strict-boundary gaps. These are reusable lessons for a future self-review skill rather than one-off retry-specific checks.

### Any log messages given by the failure
> Representative failure modes discovered during review:
>
> - A cumulative Functions Host log allowed the exhaustion test to pass using records from an earlier workflow.
> - The same workflow's raw private retry marker contained the expected task name, message, and error code, so substring assertions passed even when decoding was broken.
> - `json.dumps()` escaped quotes inside nested `errorMessage` values, so marker searches missed leaked JSON.
> - `raise ... from None` did not prevent Durable from persisting `__context__`; the SDK serializes `ex.__cause__ or ex.__context__` without honoring `__suppress_context__`.
> - Interpolating Pydantic's full `ValidationError` reflected caller-authored `input_value` content.
> - Plain `json.loads()` silently collapsed duplicate retry-envelope keys.

### Expected/desired behavior
> Add a self-review skill or review reference that performs the following checks before a medium+ PR is declared ready.
>
> **1. Establish the intended contract and scope**
>
> - Read the FRD, architecture documentation, PR description, and split rationale before reviewing implementation details.
> - Build an explicit in-scope/out-of-scope list and search the full diff for deferred APIs leaking into code, docstrings, samples, tests, or docs.
> - Verify that the extracted PR is independently usable and does not expose dead public API that only a later PR can consume.
>
> **2. Review persisted and external boundaries as hostile inputs**
>
> - Identify every document crossing a process, replay, storage, JSON, protobuf, or user-authoring boundary.
> - Check strict shape validation, unknown/duplicate key policy, enum/string stability, forward-compatibility rules, and failure behavior for malformed or future-version payloads.
> - Require duplicate-key rejection before JSON object construction where duplicate keys could alter validation semantics.
> - Ensure validation errors returned to users omit raw input values (`errors(include_input=False)` or an equivalent safe formatter).
> - Prefer the repository's boundary-model conventions (for example Pydantic v2 and `StrEnum`) or require an explicit rationale when retaining lower-level wire types.
>
> **3. Verify sanitization at the final serialized boundary**
>
> - Do not stop at the top-level exception message. Inspect the complete persisted failure structure, including `innerFailure`, causes, contexts, stack/properties fields, and backend status/output representations.
> - For Python exception sanitization, inspect both `__cause__` and `__context__`; do not assume `raise ... from None` is sufficient for third-party serializers.
> - Exercise the actual serializer used by the dependency (for this PR, `durabletask.internal.helpers.new_failure_details`) and assert secrets/private markers are absent from the serialized artifact.
> - Treat logs separately from persisted status: logging the raw exception may be intentional operational telemetry, while exposing it in durable user-visible state may violate the contract.
>
> **4. Audit test-oracle provenance and prove negative controls**
>
> - Ask where every asserted value came from and whether an unrelated record, earlier test, nested wrapper, or raw payload contains the same substring.
> - Bound shared/cumulative logs by workflow ID and a capture point immediately before the action under test.
> - Prefer exact semantic equality over independent substring assertions for decoded or normalized contracts.
> - Inspect nested messages directly rather than searching a re-serialized container whose escaping can hide markers.
> - Add a negative control containing all formerly accepted substrings in the wrong representation and prove the assertion rejects it.
> - Add a “guard the guard” check: temporarily run the regression against the pre-fix implementation and record that it fails for the intended reason.
> - Require real E2E tests to report executed test counts and prove they were not skipped.
>
> **5. Verify every execution-path matrix**
>
> - Enumerate behavior dimensions explicitly instead of sampling one path. For PR #193 the matrix was static/dynamic/fan-out × tool/Sub Agent × policy-free/policy-aware × success/terminal/retryable/exhausted.
> - Verify metadata and options together on every dispatch path (for example tags plus retry policy), not in isolated tag-only and retry-only tests.
> - Validate compatibility paths separately: old persisted histories must continue using old dispatch and result envelopes without consulting live configuration.
> - Review replay-sensitive code for wall-clock reads, randomness, mutable global state, live config lookups, unstable ordering, or exception-dependent nondeterminism.
>
> **6. Use real backend evidence to assign ownership**
>
> - When behavior spans application code, SDK, service, and dashboard, inspect the raw protocol/history at each boundary before assigning blame.
> - Compare what the application emitted, what the SDK reconstructed, what the backend persisted, and what the UI rendered.
> - Search upstream source and equivalent SDK implementations for intended behavior.
> - If the defect is upstream, produce a minimal upstream fix/regression and track release consumption separately instead of adding a brittle application workaround.
>
> **7. Cross-check documentation against executable contracts**
>
> - Search all changed docs, FRDs, docstrings, samples, and tests for stale field names, old API names, incorrect persistence claims, overly broad error-code promises, and deferred feature language.
> - Verify examples by constructing/validating them against the implemented model where practical.
> - Keep FRD Decisions logs append-only. Apply newer guidance prospectively; do not consolidate previously reviewed decisions by rewriting history.
> - Keep durable decisions minimal and move SDK choreography or implementation mechanics into the design narrative.
>
> **8. Separate merge requirements from follow-ups**
>
> - Classify findings by concrete risk: blocking correctness/security, should-fix test or contract gaps, or non-blocking convention/refactor work.
> - Fix correctness and security gaps in the current PR.
> - Move broad representation changes, file reorganizations, or documentation-style cleanup into a linked focused issue when they would obscure the behavioral change.
> - Reply to every review thread with either the fixing commit and evidence or a specific rationale plus the follow-up issue.
>
> **9. Suggested skill output**
>
> - Findings grouped by severity with `file:line`, concrete failure mode, and recommended fix.
> - A checked execution-path matrix.
> - Test-oracle provenance notes and negative-control results.
> - Persisted-boundary evidence from a real backend.
> - Code/docs/scope consistency results.
> - A list of accepted fixes versus linked follow-ups.
> - Exact validation counts and confirmation that E2E tests executed without skips.
>
> **Acceptance criteria**
>
> - [ ] The self-review guidance detects cumulative-log contamination and substring false positives.
> - [ ] It requires inspection of nested serialized failures, not only top-level messages.
> - [ ] It checks Python cause/context behavior against the serializer actually used.
> - [ ] It checks external validation output for reflected input values and JSON duplicate keys.
> - [ ] It builds and verifies an execution-path matrix for cross-cutting behavior.
> - [ ] It verifies replay/backward compatibility from persisted input only.
> - [ ] It cross-checks code, samples, tests, docs, docstrings, and FRD claims.
> - [ ] It distinguishes current-PR correctness fixes from focused follow-up refactors.
> - [ ] It requires a pre-fix failing regression or equivalent negative-control proof.
> - [ ] It requires real E2E execution evidence and raw backend-state inspection when the contract crosses an SDK/service boundary.

### OS and Version?
> N/A. The review lessons are cross-platform; PR #193 was validated on Windows with a real Azure Functions Host, Azurite, and the Durable Task Scheduler emulator.

### Versions
> Source: PR #193, through commit `1edce829`.

### Mention any other details that might be useful

> Key review threads:
>
> - Cumulative log contamination: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3936654366
> - Stale FRD field names: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3936654413
> - Incorrect persisted-idempotency and dispatch API claims: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3936654451
> - Overbroad terminal error-code documentation: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3936654491
> - Deferred decorator API leaked into a docstring: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3936654536
> - Raw marker JSON satisfying decoded substring assertions: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3937758188
> - Reflected Pydantic input values: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3961811657
> - Duplicate JSON keys: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3961811670
> - Persisted-boundary model convention: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3961811673
> - `StrEnum` wire-value convention: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3961811680
> - Source/test mirroring: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3961811685
> - Source docstring scope: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3961811691
> - Minimal durable FRD decisions: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3961811697
> - Escaped nested marker false negative: https://github.com/Azure/azure-functions-agents-runtime/pull/193#discussion_r3961811706
>
> The broader convention-only follow-ups are tracked separately in #205. This issue should focus on teaching the review process how to discover and triage such findings.

> ---------------------------------------------------------------
> Thanks! We'll be in touch soon.

Contributor guide

Open the contributing guide

Research direction

Start by reading PR #193, its corrective commits, and the linked review threads, then compare the requested checks with follow-up issue #205. Identify where the repository's self-review skill or review guidance belongs. Done means the guidance covers the listed acceptance criteria, including boundary inspection, execution-path matrices, negative controls, documentation consistency, and separation of fixes from follow-ups.

Written by the indexing model from the issue text.

Assessment

Tech stack
azure, python
Domain
developer-experience, documentation, testing
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.