Skip to content

Commit 1e4a631

Browse files
author
claude
committed
Add decisions/ directory with 8 ADRs from Phase A + strategic pivots
Operator's request 2026-05-05: surface decisions from `.codex/DECISIONS.md` (working notes, gitignored) into git history so they're not lost with session context. Pattern adapted from Michael Nygard's ADR convention and the existing `pdurlej/platform/decisions/` directory. Eight numbered ADRs covering Phase A implementation choices and the strategic pivot to dogfood-first: - 0001 Classification namespace = underscore (A1; ADR explains why this resolved a silent bug where `safe_to_remove` always returned `manual_only` regardless of finding properties) - 0002 Baseline JSON validation raises ConfigError (A2; replaces cryptic TypeError from sorted() on mixed-type lists) - 0003 Forgejo runner = ubuntu-latest, not docker:python (A3; live verified on rs2000 runner via PR #2 run id 39) - 0004 Test `normalize()` handles FastMCP dataclass wrapping (A4; dataclass-with-module='types' on 3.11 vs different shape on 3.13) - 0005 Alpha-incremental release strategy (A5; 0.3.0a2/0.1.0a2 alpha retained until Phase B/C land based on dogfood evidence) - 0006 Dogfood-first, Show-HN-later — anti-AI-slop (operator's strategic decision 2026-05-04, founding principle quoted) - 0007 Pyfallow as deterministic code gate, counterpart to platform.exe (identity articulation matching `pdurlej/platform/PLATFORM_CONSTITUTION.md`'s identity articulation for infra) - 0008 Phase B/C execution gated on dogfood evidence (mechanism: 22 Forgejo issues created at #4-#25, unstarted until window closes) Each ADR follows fixed sections: Status / Context / Decision / Consequences / References. Status is `accepted` for all 8. `decisions/README.md` documents the convention (Nygard format), explains the relationship to `.codex/DECISIONS.md` (working memory) vs `decisions/` (project memory), and indexes the 8 ADRs. Identity: this commit is authored as `claude` (claude@noreply.git.pdurlej.com) per the identity-isolation convention adopted from `pdurlej/platform/AGENTS.md` § Identity-isolation.
1 parent 9ec5021 commit 1e4a631

9 files changed

Lines changed: 521 additions & 0 deletions
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
# 0001 — Classification namespace = underscore
2+
3+
**Date:** 2026-05-04
4+
**Status:** accepted
5+
**Phase:** A1
6+
**Authors:** Claude Opus 4.7 (orchestrator), Codex (executor)
7+
8+
## Context
9+
10+
Pre-Phase-A, MCP package `mcp/src/pyfallow_mcp/` carried two parallel namespaces for finding classification:
11+
12+
- **Underscore** (dominant, used in 6+ locations): `auto_safe`, `review_needed`, `blocking`, `manual_only` — defined in `pyfallow.classify.CLASSIFICATION_GROUPS`, used by `Finding.classification`, `Remediation.classification`, `runtime.findings()`, `tools.analyze_diff_impl()`, all `AnalysisResult` field names.
13+
- **Hyphen** (in 2 locations): `safe-auto`, `review-needed`, `manual-only` (no `blocking`!) — used in `mcp/src/pyfallow_mcp/schemas.py::Classification.decision` Literal and four hardcoded strings in `mcp/src/pyfallow_mcp/safety.py:safe_classification`.
14+
15+
This was a silent bug. Agents calling `analyze_diff` (returns `Finding` with `auto_safe`) **and** `safe_to_remove` (returns `Classification` with `safe-auto`) on the same finding got different vocabulary. No schema validation caught it because `FlexibleModel(extra="allow")` masked drift. Audit GLM-5.1 2026-05-03 surfaced as F-11 critical.
16+
17+
Additionally, `mcp/src/pyfallow_mcp/safety.py:34` contained:
18+
19+
```python
20+
return classify_finding(issue).decision == "auto_safe"
21+
```
22+
23+
`classify_finding` returns underscore, but `safe_classification` was setting `decision="safe-auto"` on the Pydantic model. The comparison was a tautology of False — `safe_to_remove` literally never returned `auto_safe`, regardless of finding properties.
24+
25+
## Decision
26+
27+
All MCP classification labels mirror `pyfallow.classify.CLASSIFICATION_GROUPS` underscore namespace: `auto_safe`, `review_needed`, `blocking`, `manual_only`. Single source of truth in core; every other surface (CLI agent-fix-plan, MCP `analyze_diff`, MCP `safe_to_remove`, MCP `Classification.decision`, `Finding.classification`, `Remediation.classification`) renders that source — never duplicates.
28+
29+
`Classification.decision` Literal is extended to include `blocking` (mirroring core) even though `safe_classification` does not currently emit it for `unused-symbol`/`unused-module` rules. Defensive contract: if a future code path uses `Classification` for blocking-class findings, the type system already accepts it.
30+
31+
A drift-detection test suite at `mcp/tests/test_classification_namespace.py` fails fast (CI catches before merge) if any MCP `Literal[...]` diverges from core `CLASSIFICATION_GROUPS`. Includes a canary: `safe_classification` must return `decision="auto_safe"` for a high-confidence clean dead-code finding. Reverting any namespace change → red test.
32+
33+
## Consequences
34+
35+
**Positive:**
36+
- `safe_to_remove` now correctly classifies high-confidence dead code as `auto_safe`. Previously it always returned `manual_only` (silent failure of intended behavior).
37+
- Cross-tool agent code can switch on a single classification namespace.
38+
- Phase B ticket B5 (FlexibleModel hardening) becomes effective: with `extra="forbid"` on contract models + the namespace unified, any future drift gets caught by Pydantic validation, not just the bespoke drift test.
39+
40+
**Negative / breaking:**
41+
- Wire format change. MCP `safe_to_remove` `decision` field now returns underscore values. Pre-`0.3.0a2` pyfallow-mcp was alpha + not on PyPI, so external client impact is local-dev only. CHANGELOG entry documents.
42+
- The hyphen namespace is deprecated entirely. No backwards-compat shim.
43+
44+
**Neutral:**
45+
- Choice of underscore vs hyphen was determined by minimum-diff: 6+ locations were already underscore, 2 were hyphen. Inverting would have been ~3x larger change.
46+
47+
## References
48+
49+
- Implementation: PR #1 (GitHub) / PR #2 (Forgejo) — commit `771c628`, branch `feat/phase-a-ship-blockers`
50+
- Audit: `.codex/audits/glm-engineering-audit-2026-05-03.md` finding F-11 (CRITICAL)
51+
- Drift test: `mcp/tests/test_classification_namespace.py`
52+
- WORKFLOW rule violated by the bug: #11 (single source of truth across transports)
53+
- Live verification on installed package from TestPyPI 0.3.0a2: classification namespace held end-to-end on fresh-venv smoke
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
# 0002 — Baseline JSON validation raises ConfigError
2+
3+
**Date:** 2026-05-04
4+
**Status:** accepted
5+
**Phase:** A2
6+
**Authors:** Claude Opus 4.7 (orchestrator), Codex (executor)
7+
8+
## Context
9+
10+
`src/pyfallow/baseline.py::read_baseline()` previously parsed JSON and returned the raw dict without type validation:
11+
12+
```python
13+
def read_baseline(path: str | Path) -> dict[str, Any]:
14+
return json.loads(Path(path).read_text())
15+
```
16+
17+
Malformed input — most commonly integer fingerprints from a manual edit, merge conflict, or tooling bug — caused a cryptic `TypeError` deep in `compare_with_baseline()` when `sorted()` was called on a mixed-type list. The user saw a stack trace with no indication that the baseline file was malformed.
18+
19+
Audit GLM-5.1 2026-05-03 finding F-17 elevated this from MEDIUM to HIGH because:
20+
1. CI workflows depend on baseline comparison in `--baseline` invocations
21+
2. The error type contract was unstable (TypeError vs ValueError vs random behavior)
22+
3. The user's first signal was a traceback inside pyfallow's internals, not a "your file is malformed" message
23+
24+
## Decision
25+
26+
`read_baseline()` validates the loaded structure via a new `_validate_baseline_shape()` helper, raising `pyfallow.config.ConfigError` (the existing user-input error type) on any contract violation:
27+
28+
- Top level must be a JSON object
29+
- `version` field must exist and be a string
30+
- `fingerprints` field must exist and be a list (legacy format) **or** `issues` field must exist with a list of objects each having a string `fingerprint`
31+
- All fingerprint values must be strings
32+
33+
Error messages include the file path and the specific field at fault. Up to 5 bad indices are listed when validation fails on a list (UX cap to keep errors readable on baselines with many violations).
34+
35+
`json.JSONDecodeError` is also wrapped in `ConfigError` for a consistent error type contract from this entry point.
36+
37+
CLI maps `ConfigError` to exit code 2 (already established convention in pyfallow CLI).
38+
39+
## Consequences
40+
41+
**Positive:**
42+
- Repro from the audit (`{"version": "1.0", "fingerprints": [12345, 67890]}`) now produces a clear error message naming the malformed field at indices `[0, 1]`, exit code 2 — not a traceback.
43+
- All other readers of `baseline` data downstream can assume well-formed input. Removes defensive checks scattered later in the pipeline.
44+
- 5 new tests in `tests/test_baseline.py` provide regression coverage.
45+
46+
**Negative:**
47+
- Consumers that previously caught `TypeError` from `compare_with_baseline()` to detect malformed input must switch to `ConfigError`. Internal tooling only — no external API contract change.
48+
- Adds a small validation pass on every baseline load. Cost is O(n) over fingerprints; negligible at any realistic baseline size.
49+
50+
**Neutral:**
51+
- Baselines with extra top-level fields (forward compat) are still accepted. Strictness is on required fields only.
52+
53+
## References
54+
55+
- Implementation: PR #2 (Forgejo) — commit `7307cd6`, branch `feat/phase-a-ship-blockers`
56+
- Audit: `.codex/audits/glm-engineering-audit-2026-05-03.md` finding F-17 (HIGH; was F-08 MEDIUM in earlier session)
57+
- Tests: `tests/test_baseline.py` — 5 cases (integer fingerprints, missing version, non-object top, invalid JSON, valid baseline accepted)
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
# 0003 — Forgejo runner = ubuntu-latest, not docker:python
2+
3+
**Date:** 2026-05-04
4+
**Status:** accepted
5+
**Phase:** A3
6+
**Authors:** Claude Opus 4.7 (orchestrator), Codex (executor)
7+
8+
## Context
9+
10+
`examples/ci/forgejo-actions.yml` (template shipped to pyfallow users for their own Forgejo CI) specified:
11+
12+
```yaml
13+
jobs:
14+
pyfallow-cleanup:
15+
runs-on: docker
16+
container:
17+
image: python:3.12
18+
```
19+
20+
Live PR test on 2026-05-03 (PR #1, since reverted) confirmed: every job failed at step 1 ("Check out") because `actions/checkout@v4` requires Node.js, and the `python:3.12` Docker image has no Node.js installed.
21+
22+
Additionally, after the live-validation PR was reverted, pyfallow's own Forgejo CI was empty — `.forgejo/workflows/ci.yml` was deleted in the revert. The Forgejo Actions runner on rs2000 was registered but had nothing to run. GitHub CI continued working but Forgejo (operator's primary remote) was effectively dark.
23+
24+
## Decision
25+
26+
Both files use the same shape, mirroring the working pattern from `.github/workflows/ci.yml`:
27+
28+
- `runs-on: ubuntu-latest` (Forgejo runner config maps this label to `catthehacker/ubuntu:act-latest`, which has both Node.js and Python preinstalled)
29+
- No `container:` directive (was the blocker)
30+
- Explicit `actions/setup-python@v5` step to select the desired Python version
31+
32+
Two files updated:
33+
- `examples/ci/forgejo-actions.yml` — single-job template for users
34+
- `.forgejo/workflows/ci.yml` (NEW) — pyfallow's own self-CI, full matrix `["3.11", "3.12", "3.13"]` mirroring GitHub workflow steps
35+
36+
Live verification was a release blocker for closure: PR #2 on Forgejo triggered run id 39 (trigger=pull_request) which completed with `status: success`. A3 is closed with **on-runner evidence**, not just yamllint + assertion test.
37+
38+
## Consequences
39+
40+
**Positive:**
41+
- Forgejo CI works end-to-end on rs2000 runner. Live evidence captured as run id 39.
42+
- GitHub ↔ Forgejo CI parity: same matrix, same steps, same runner-style. Future updates touch both files in lockstep.
43+
- Forgejo PR template now matches what users on Forgejo Actions runners actually need (no GitHub-only assumptions).
44+
45+
**Negative:**
46+
- Drops the `container:` directive even though it would let users specify exact Python images. Tradeoff: runner compatibility > image control. Users who need specific Python versions configure via `actions/setup-python@v5`.
47+
48+
**Neutral:**
49+
- A parallel Codex thread (separate session) produced an alternative refactor for the workflow shape using Forgejo-native action URLs (`https://data.forgejo.org/actions/checkout@v4`), `ubuntu-22.04` (explicit pin), `persist-credentials: false`, plus a Python runner script `scripts/ci/run_python_ci.py`. That work landed in stash (`stash@{0}` on Phase A branch) but was **not needed** for basic CI to work — the simple A3 fix was sufficient. Stash is treated as optional Phase B/C refinement; will be picked up if intensifying CI usage hits GitHub rate limits or if other patterns from the refactor become necessary.
50+
51+
## References
52+
53+
- Implementation: PR #2 (Forgejo) — commit `33061c7`, branch `feat/phase-a-ship-blockers`
54+
- Audit / discovery: live failure on PR #1 (since reverted), 2026-05-03
55+
- Live verification: Forgejo Actions run id 39 (`status: success`, trigger `pull_request`)
56+
- Stash from parallel thread: `stash@{0}: CI refactor work from parallel Codex thread (2026-05-04)` — may inform future Phase B/C ticket
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
# 0004 — Test `normalize()` handles FastMCP dataclass wrapping
2+
3+
**Date:** 2026-05-04
4+
**Status:** accepted
5+
**Phase:** A4
6+
**Authors:** Claude Opus 4.7 (orchestrator), Codex (executor)
7+
8+
## Context
9+
10+
On Python 3.11.14 with `fastmcp` 3.2.4 and `pydantic` 2.13.3, FastMCP's `Client.call_tool()` returns the tool result wrapped in a **dynamically-generated dataclass** named `Root` with `__module__ == "types"`. On Python 3.13.1 the result comes back differently and the bug never surfaced locally.
11+
12+
`mcp/tests/test_mcp.py::normalize()` had branches for:
13+
1. Pydantic BaseModel (uses `model_dump`)
14+
2. fastmcp.* namespace classes (uses `vars()`)
15+
3. dict / list / scalar passthroughs
16+
17+
It had no branch for dataclasses. On 3.11, `normalize()` fell through to `return value`, the test then tried `result["decision"]` on a `Root` instance and got `TypeError: 'Root' object is not subscriptable`.
18+
19+
**9 of 13 MCP tests failed on 3.11.** GitHub CI matrix includes 3.11. We had been pushing a broken matrix for an unknown duration. The audit GLM-5.1 2026-05-03 was run on Python 3.14 and didn't catch this — it surfaced during this orchestrator's verification of the Python 3.11 matrix as part of preparing the Phase A briefs.
20+
21+
## Decision
22+
23+
`normalize()` gains a `dataclasses.is_dataclass(value)` branch (with `not isinstance(value, type)` guard against class objects):
24+
25+
```python
26+
def normalize(value):
27+
if hasattr(value, "model_dump"):
28+
return normalize(value.model_dump(mode="json"))
29+
if dataclasses.is_dataclass(value) and not isinstance(value, type):
30+
return normalize(dataclasses.asdict(value))
31+
if hasattr(value, "__dict__") and value.__class__.__module__.startswith("fastmcp."):
32+
return normalize(vars(value))
33+
if isinstance(value, dict):
34+
return {key: normalize(item) for key, item in value.items()}
35+
if isinstance(value, list):
36+
return [normalize(item) for item in value]
37+
return value
38+
```
39+
40+
Pure test plumbing change. Production code untouched. No Pydantic / FastMCP version pin.
41+
42+
## Consequences
43+
44+
**Positive:**
45+
- 13/13 MCP tests pass on Python 3.11.14 (was 9 failed, 4 passed).
46+
- 13/13 MCP tests still pass on Python 3.13.1 (no regression).
47+
- CI matrix matches declared `requires-python = ">=3.11"`.
48+
- Defensive: any future FastMCP version that returns yet another wrapper type will fail loudly on tests, not silently mask drift.
49+
50+
**Negative:**
51+
- `normalize()` test helper grows by 2 lines. Acceptable.
52+
- Couples test plumbing to FastMCP's runtime serialization details. If FastMCP changes wrapping shape again, helper needs another branch. Tradeoff: kept FastMCP version unpinned (security patches accessible) at the cost of helper maintenance.
53+
54+
**Neutral:**
55+
- Branch order matters: `model_dump` first (Pydantic native), then `is_dataclass`. Pydantic v2 BaseModel is **not** a dataclass per `dataclasses.is_dataclass()`, so the branches are mutually exclusive in practice.
56+
57+
## Open question
58+
59+
Why does FastMCP return a dataclass with `__module__ == "types"` on 3.11 but not on 3.13? Possibly upstream behavior we should file an issue on. **Not a blocker for pyfallow** — the helper handles either shape — but worth tracking. Out of scope for A4; deferred to Phase B/C if relevant.
60+
61+
## References
62+
63+
- Implementation: PR #2 (Forgejo) — commit `527865e`, branch `feat/phase-a-ship-blockers`
64+
- Discovery: this orchestrator's local verification setup, 2026-05-04 ~01:30 (using `uv venv --python 3.11` and reproducing 9-failed result)
65+
- Reproducer venv: `/tmp/pyfallow-py311` (transient; recreate via `uv venv --python 3.11 /tmp/pyfallow-py311 && /tmp/pyfallow-py311/bin/python -m ensurepip && /tmp/pyfallow-py311/bin/python -m pip install -e ".[dev]" -e ./mcp`)
66+
- Type signature of the wrapper observed: `class Root` with `__module__ == "types"`, `dataclasses.is_dataclass(value) == True`, `vars()` returns ordered field dict
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
# 0005 — Alpha-incremental release strategy
2+
3+
**Date:** 2026-05-04
4+
**Status:** accepted
5+
**Phase:** A5
6+
**Authors:** Claude Opus 4.7 (orchestrator), Codex (executor), operator (`pdurlej`) decision-maker
7+
8+
## Context
9+
10+
After Phase A landed (5 atomic commits A1-A5), pyfallow needed a release decision before TestPyPI publish. Pre-Phase-A versions were `pyfallow 0.3.0-alpha.1` and `pyfallow-mcp 0.1.0-alpha.1`. Phase A introduced a **breaking change** to MCP wire format: `safe_to_remove.decision` switched from hyphen to underscore namespace (per ADR 0001). Whatever release strategy we choose has to bump versions.
11+
12+
Three options considered:
13+
14+
- **(α) Alpha-incremental:** bump to `pyfallow 0.3.0a2` and `pyfallow-mcp 0.1.0a2`, retain alpha tag
15+
- **(β) Skip-to-stable:** publish `0.3.0` and `0.1.0` non-alpha
16+
- **(γ) Minor bump (still alpha):** `0.4.0a1` / `0.2.0a1`, bigger semver delta to signal breaking change
17+
18+
## Decision
19+
20+
**(α) Alpha-incremental.** Versions bumped to `0.3.0a2` and `0.1.0a2` (PEP 440 normalized form of the previous `0.3.0-alpha.X` style).
21+
22+
Rationale: a stable tag (option β) before Phase B would mis-sell the analyzer's state. Phase B has HIGH-severity engineering findings still unfixed (Tarjan recursion crash, walrus operator FP, two confirmed framework FPs on SQLAlchemy and async generators, MCP root sandboxing, etc.) — see Phase B issues #4-#15. Calling Phase A "stable" while those known defects ride on top would either lie to users or force an immediate `0.3.1` patch sprint. Neither is healthy.
23+
24+
Option γ (minor bump) was rejected because the breaking change is contained to MCP `safe_to_remove.decision` field, and `pyfallow-mcp` was not yet on PyPI — there are no external clients to migrate. The semver "breaking change" weight is mostly performative in this case. Alpha label communicates instability well enough.
25+
26+
Subsequent releases follow the same pattern (a3, a4, ...) until Phase B/C land based on dogfood evidence (per ADR 0008), at which point the next release drops the alpha tag as `0.3.0` stable. That tag transition is operator's deliberate decision, not automated.
27+
28+
## Consequences
29+
30+
**Positive:**
31+
- TestPyPI publish completed cleanly. URLs:
32+
- https://test.pypi.org/project/pyfallow/0.3.0a2/
33+
- https://test.pypi.org/project/pyfallow-mcp/0.1.0a2/
34+
- Fresh-venv install smoke (Python 3.12.12) passed: `pyfallow --version`, `pyfallow analyze`, `pyfallow-mcp --help` all worked. A1 invariant verified live on installed package.
35+
- README and changelog can honestly say "alpha — fixes incoming during dogfood window."
36+
37+
**Negative:**
38+
- Production PyPI publish is intentionally not done yet. Users wanting to install pyfallow from pypi.org will get the stale `0.1.0` (whoever previously held the name; pdurlej confirmed account ownership). Workaround: pin from TestPyPI per `docs/dogfood.md` until `0.3.0` stable lands.
39+
40+
**Neutral:**
41+
- BW vault item for TestPyPI token (`test.pypi.org`, custom field `API token`) was retrieved by orchestrator with operator's explicit BW_SESSION authorization in chat. Production PyPI publish remains operator's manual click; orchestrator does not handle the prod-PyPI flow.
42+
43+
## References
44+
45+
- Implementation: PR #2 (Forgejo) — commit `11c0a31`, branch `feat/phase-a-ship-blockers`
46+
- TestPyPI upload: orchestrator night shift 2026-05-04, BW item `test.pypi.org`
47+
- Smoke verification: PR #1 + PR #2 comments by `claude` user
48+
- Phase B issues #4-#15 (Forgejo) — what's blocking the alpha→stable transition

0 commit comments

Comments
 (0)