📖 Reconcile ADRs with current implementation (#193) - #205
Conversation
Adopt living-document ADR guidance and record dated implementation amendments. Add a repository ADR template and review index, and align the entry point, skill loading, execution outcome, and OpenShell documentation with the current implementation. Assisted-By: GPT-5 Codex <noreply@openai.com> Signed-off-by: Savitha Raghunathan <saveetha13@gmail.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@djzager I took a stab at issue 193, please take a look when you have some time and add your comments. Thanks |
djzager
left a comment
There was a problem hiding this comment.
Overall this is a strong reconciliation pass, and the living-document approach is well-aligned with established ADR practice — the canonical ADR guidance explicitly favors mutable, date-stamped ADRs over strict immutability, and MADR (the ADR-org's own variant) already standardizes an extensible YAML front matter. My comments are all one architectural direction rather than defects: the header metadata is prose, which forces two hand-maintained index/reconciliation tables that will drift from the ADRs. Moving the header into structured front matter (a description/summary plus per-ADR reconciliation fields) would let README.md's index and RECONCILIATION.md's table become generated artifacts, and would give agents a cheap progressive-disclosure hook so they read metadata first and only load the body when relevant. None of this blocks merge on correctness; I'm requesting changes so we can decide the metadata direction before we cement two more hand-maintained tables. Inline details below.
| # ADR NNNN: Title | ||
|
|
||
| **Status:** proposed | accepted | superseded by ADR NNNN | ||
| **Date:** YYYY-MM-DD |
There was a problem hiding this comment.
The living-document approach here is well-aligned with established ADR practice — the canonical ADR guidance explicitly favors mutable ADRs with dated stamps over strict immutability, and MADR (the ADR-org's own variant) already standardizes YAML front matter with an explicitly extensible, optional field set. So we have room to formalize the header as structured metadata without diverging from the standard.
Suggestion: move the header fields into YAML front matter and add two kinds of machine-readable metadata:
- a one-line
description/summary— the progressive-disclosure hook, so a reader (human or agent) can grasp the decision from the front matter and only open the body when it's relevant; - per-ADR reconciliation fields —
last_reviewed:plus animplementation_status:(in-sync / amended / superseded / deferred) and a short note.
With those in place, both README.md's index and RECONCILIATION.md's table become generated artifacts derived from the ADRs themselves, rather than two hand-maintained tables that can drift from the source (this PR even warns about that drift risk). Note this only pays off with a small generator; without one we'd be hand-maintaining YAML plus the tables. The narrative Update (YYYY-MM-DD): amendments stay in the body regardless. Raising as a direction, not a blocker.
There was a problem hiding this comment.
Done — moved the header into YAML front matter with description, last_reviewed, implementation_status, and review_note. Added hack/generate-adr-index.sh to generate both the README index and the RECONCILIATION table from it, wired into make generate-adr-index / make verify-adr-index and run in CI.
| **Status:** proposed | accepted | superseded by ADR NNNN | ||
| **Date:** YYYY-MM-DD | ||
| **Last updated:** YYYY-MM-DD | ||
| **Authors:** Name |
There was a problem hiding this comment.
The template standardizes on plain bold-key lines, but the ADRs remain inconsistent about the header format — e.g. 0013 and 0017 (ask-user) use list-item metadata (- **Status:**) while the rest use plain lines (**Status:**), and 0001 renames Updated → Last updated without a repo-wide sweep. This is precisely the class of drift that prose headers invite and that machine-readable front matter would eliminate (a parser rejects a missing/misnamed field; prose doesn't). If we don't adopt front matter now, it's worth at least normalizing the existing headers to the one form the template blesses, so the convention is enforced by example.
There was a problem hiding this comment.
Fixed by the front matter change above — every ADR now uses the same structured fields instead of prose headers, so there's no more drift between plain-line vs list-item styles or field-name variants like Updated vs Last updated.
| | 0018 | Execution fields on AgentRun, workflow-stage stamping, `Succeeded` terminal condition, and guide scoping are implemented. The ADR remains proposed pending formal acceptance. | | ||
|
|
||
| Future reviews should update this table and, when the ADR is materially | ||
| amended, its `Last updated` field together. A review date alone is not a |
There was a problem hiding this comment.
This file is a second source of truth that duplicates state already living in the ADRs (status, supersession, last-updated) plus a per-ADR review note. This closing paragraph essentially admits the drift hazard — it has to remind maintainers to update the table and the ADR's Last updated field together, which is exactly the manual-sync burden that will eventually be missed.
If the per-ADR reconciliation fields from the front-matter suggestion land (last_reviewed: + implementation_status: + note), this table becomes a generated view and this warning paragraph goes away entirely. Until then, one concrete tightening: the table hard-codes the review date in prose ("Reviewed ... on 2026-08-31") but individual rows don't carry their own review date, so a partial re-review of a single ADR can't be represented without rewriting the header. Consider a per-row Last reviewed column so incremental reviews don't force a global date bump.
There was a problem hiding this comment.
Agreed, this is now a generated view. Each row also has its own last_reviewed date pulled from front matter, so a partial review of one ADR no longer forces a global date bump on the whole table.
| | 0016 | Accepted current state: Gateway CRD, direct Agent Sandbox creation, verification Jobs, and provider-specific credential injection. OpenShell remains deferred. | | ||
| | 0017 (ask-user) | `ask_user` MCP, ACP elicitation forwarding, fail-closed behavior, and resolution frames are implemented in the entry point. The ADR’s status remains proposed pending formal acceptance. | | ||
| | 0017 (catalog layout) | The `catalog/` layout and maintainer `skills/` split are implemented. The ADR remains proposed pending formal acceptance. | | ||
| | 0018 | Execution fields on AgentRun, workflow-stage stamping, `Succeeded` terminal condition, and guide scoping are implemented. The ADR remains proposed pending formal acceptance. | |
There was a problem hiding this comment.
Nit / pre-existing: there are two ADR 0017s, and this index cements the collision by disambiguating them as "0017 (ask-user)" and "0017 (catalog layout)". ADR numbers are meant to be stable unique identifiers — that's what supersession references and cross-links point at. Since this PR is the reconciliation pass, it's a natural moment to renumber one of them (the catalog-layout ADR looks like the later addition) to the next free number and update inbound references, rather than carrying a permanent duplicate. If renumbering is deemed too disruptive, worth a one-line note in README.md documenting that 0017 is intentionally shared so future readers don't treat it as a mistake.
There was a problem hiding this comment.
Renumbered the catalog-layout ADR to 0019 and updated its numbering note. Went with renumbering rather than a footnote since this PR was already touching every ADR.
Convert ADR headers to structured YAML front matter (description, last_reviewed, implementation_status, review_note), generate the README and RECONCILIATION tables from it via hack/generate-adr-index.sh (wired into make and CI), and renumber the duplicate ADR 0017 (catalog layout) to 0019. Addresses PR konveyor#205 review feedback. Assisted-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Savitha Raghunathan <saveetha13@gmail.com>
Adopt living-document ADR guidance and record dated implementation amendments.
Add a repository ADR template and review index, and align the entry point, skill loading, execution outcome, and OpenShell documentation with the current implementation.
Assisted-By: GPT-5 Codex noreply@openai.com