docs: clarify existing implementation ADR guidance #116

jercik · 2026-10-04T19:35:13Z

jercik commented

2026-10-04 19:35:13 +00:00

Clarifies the existing ADR guidance identified in #115 by explicitly passing the code-comment rule to the implementer and referring to the drop's ADR without repeating its format limit.

Clarifies the existing ADR guidance identified in [#115](https://code.j4k.dev/j4k-oss/agent-skills/pulls/115) by explicitly passing the code-comment rule to the implementer and referring to the drop's ADR without repeating its format limit.

jercik added 1 commit

2026-10-04 19:35:13 +00:00

docs: clarify existing implementation ADR guidance

commit-msg / commitlint (pull_request) Successful in 16s

Details

Node tests / node:test (pull_request) Successful in 2m9s

Details

Review / Review (pull_request_target) Successful in 6m53s

Details

9624e46c9b

jercik referenced this pull request

2026-10-04 19:35:50 +00:00

fix: verification criteria should refer to the checklist #115

forgejo-actions commented

2026-10-04 19:42:04 +00:00

Review 01M4473NADPV6WDREV56H4QW3G — head add0b1136a7f4f4b840c2a2f3e5bb107a067e07c

Review — j4k-oss/agent-skills @ `df31a6e1cc`

Scope: diff against base tree 8d139933be25
Status: dispatched — coverage complete (5/5 slots terminal)
Facts: current review-wide projection

Computed under:

{
  "abandonment": "abandonment-v1",
  "anchor_recipe": 1,
  "batch_policy": "batch-v1",
  "coverage": "coverage-v3",
  "dispatch_policy": "dispatch-v2",
  "grounder_version": 1,
  "grounding_read_rule": "grounding-read-v1",
  "promotion_policy": "promotion-v1",
  "report": "report-v4",
  "tally": "tally-v1",
  "triage_settle": "triage-settle-v2"
}

Findings (1)

medium — Verification section copies the shared skill’s check set

claim: 01M4487FJJ04VF82SGSZ76D0QK
anchor: skills/reengineer-program/references/implementation.md (snippet)

Check three distinct contracts:

- **Unchanged promises:** compare kept surfaces with their snapshots and run the surviving tests against the rebuilt program.
- **Dropped promises:** demonstrate that each dropped situation is rejected or unrepresentable.
- **Replaced or added promises:** verify against the confirmed design, not the old implementation.

lens: writing-quality · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M4488HASYAGV6Y4GYDTF79W9 · valid: The exact grounded section counts and lists the three verification categories. The reviewer names skills/reengineering/SKILL.md as the defining source and lists its corresponding Retained, Dropped, and Replaced or added promises; the local Unchanged category maps to Retained. No member differs now, but the second complete list can drift when the shared checks change. The prior matching-anchor verdicts identify no concrete safeguard. Point to the shared verification checks and preserve the local snapshot, surviving-test, rejection, and confirmed-design methods without a copied count or set.
disposition: none

The verification section maintains a second definition of the checks, so a future change to the shared reengineering method can leave this implementation guide silently stale. The copy currently agrees, but the reader is directed to a local count and list instead of the skill this workflow calls.

The shared skills/reengineering/SKILL.md says “Use three checks” and defines retained promises, dropped promises, and replaced or added promises. This passage says “Check three distinct contracts” and covers unchanged promises, dropped promises, and replaced or added promises. “Unchanged” corresponds to “retained”; no member appears in only one list.

Point to the verification checks in reengineering/SKILL.md and retain the implementation-specific test instructions without a count or a second list of check names. This preserves the useful verification details while giving the check set one home, as the writing standard’s “One Idea, One Place” rule requires.

I read the full implementation reference, its calling reengineer-program/SKILL.md, and the shared reengineering/SKILL.md. This is a static comparison. The shared skill’s “Use three checks” passage defines the set; if this reference is used without that declared and explicitly invoked dependency, that would refute the proposed consolidation.

Other claims

grounding-pending (0)
ungrounded (0)
rejected (0)
duplicate-of (0)
unadjudicated (1)
- 01M4485X69VH4MM9EZ4F4PJYG1 medium — Implementation reference repeats the parent skill's code-comment exclusions

Coverage

Coverage pass: 01M447YZBGKAJ1N616FWQZYH9E
Accounting: complete
Slot health: healthy

lens	part	arm	unit status	runs	loss
general-bug	whole	default	no-claims	1	no
writing-quality	whole	default	claims-emitted	1	no
test-trimming	whole	default	no-claims	1	no
restated-sets	whole	default	claims-emitted	1	no
project-docs	whole	default	no-claims	1	no

**Review** `01M4473NADPV6WDREV56H4QW3G` — head `add0b1136a7f4f4b840c2a2f3e5bb107a067e07c` # Review — j4k-oss/agent-skills @ df31a6e1cc57 Scope: diff against base tree `8d139933be25` Status: dispatched — coverage complete (5/5 slots terminal) Facts: current review-wide projection Computed under: ```json { "abandonment": "abandonment-v1", "anchor_recipe": 1, "batch_policy": "batch-v1", "coverage": "coverage-v3", "dispatch_policy": "dispatch-v2", "grounder_version": 1, "grounding_read_rule": "grounding-read-v1", "promotion_policy": "promotion-v1", "report": "report-v4", "tally": "tally-v1", "triage_settle": "triage-settle-v2" } ``` ## Findings (1) ### medium — Verification section copies the shared skill’s check set - claim: `01M4487FJJ04VF82SGSZ76D0QK` - anchor: `skills/reengineer-program/references/implementation.md` (snippet) ``` Check three distinct contracts: - **Unchanged promises:** compare kept surfaces with their snapshots and run the surviving tests against the rebuilt program. - **Dropped promises:** demonstrate that each dropped situation is rejected or unrepresentable. - **Replaced or added promises:** verify against the confirmed design, not the old implementation. ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4488HASYAGV6Y4GYDTF79W9` · valid: The exact grounded section counts and lists the three verification categories. The reviewer names skills/reengineering/SKILL.md as the defining source and lists its corresponding Retained, Dropped, and Replaced or added promises; the local Unchanged category maps to Retained. No member differs now, but the second complete list can drift when the shared checks change. The prior matching-anchor verdicts identify no concrete safeguard. Point to the shared verification checks and preserve the local snapshot, surviving-test, rejection, and confirmed-design methods without a copied count or set. - disposition: none > The verification section maintains a second definition of the checks, so a future change to the shared reengineering method can leave this implementation guide silently stale. The copy currently agrees, but the reader is directed to a local count and list instead of the skill this workflow calls. > > The shared skills/reengineering/SKILL.md says “Use three checks” and defines retained promises, dropped promises, and replaced or added promises. This passage says “Check three distinct contracts” and covers unchanged promises, dropped promises, and replaced or added promises. “Unchanged” corresponds to “retained”; no member appears in only one list. > > Point to the verification checks in reengineering/SKILL.md and retain the implementation-specific test instructions without a count or a second list of check names. This preserves the useful verification details while giving the check set one home, as the writing standard’s “One Idea, One Place” rule requires. > > I read the full implementation reference, its calling reengineer-program/SKILL.md, and the shared reengineering/SKILL.md. This is a static comparison. The shared skill’s “Use three checks” passage defines the set; if this reference is used without that declared and explicitly invoked dependency, that would refute the proposed consolidation. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (1) - `01M4485X69VH4MM9EZ4F4PJYG1` medium — Implementation reference repeats the parent skill's code-comment exclusions ## Coverage Coverage pass: 01M447YZBGKAJ1N616FWQZYH9E Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | no-claims | 1 | no | | writing-quality | whole | default | claims-emitted | 1 | no | | test-trimming | whole | default | no-claims | 1 | no | | restated-sets | whole | default | claims-emitted | 1 | no | | project-docs | whole | default | no-claims | 1 | no |

🚀 1

forgejo-actions reviewed

2026-10-04 19:42:05 +00:00

skills/reengineer-program/references/implementation.md Outdated

					
				@ -17,3 +17,3 @@

				Dispatch a fresh implementation sub-agent into a worktree with the old implementation for the chosen scope removed, along with tests of its internals and tests asserting promises the accepted design replaces. Retain build configuration, schemas, fixtures, tests exercising kept surfaces from outside, and exact kept-surface signatures.

				Give the implementer the confirmed design, `CONTEXT.md`, the boundary ADR, binding ADRs in scope, and kept-surface signatures as authority. It may read old code from git only when that is the sole record of an existing external dependency left unspecified by those inputs — a consumer's byte format or stored-data layout, for example. Require it to report each such read and the question it answered. Rationale stays in ADRs, without ADR citations or restatements in code comments.

				Give the implementer the confirmed design, `CONTEXT.md`, the boundary ADR, binding ADRs in scope, and kept-surface signatures as authority. It may read old code from git only when that is the sole record of an existing external dependency left unspecified by those inputs — a consumer's byte format or stored-data layout, for example. Require it to report each such read and the question it answered. Require it to keep rationale in ADRs, without ADR citations or restatements in code comments.

medium — Implementer brief tells the sub-agent to "keep rationale in ADRs", which reads as permission to write ADRs

The changed sentence turns a statement about where rationale lives into an imperative aimed at the implementation sub-agent. A literal implementer that has just chosen between equivalent mechanisms (the next paragraph says "The implementer decides between mechanisms making the same promise") can read "keep rationale in ADRs" as an instruction to record that choice's rationale in a new or edited ADR. That contradicts the skill's model: SKILL.md says naming, layout, and helper boundaries are "settled without ADRs", that "equivalent mechanisms belong to implementation", and that ADRs record user-confirmed decisions; and the repository rule SKILL.md installs makes every binding ADR govern future changes. An implementer-authored ADR would enter the decision record without the user.

The constraint the brief actually needs to carry is negative: do not cite ADRs or restate their rationale in code comments. Proposed correction: "Require it to leave ADR rationale out of code comments: no ADR citations or restatements." This keeps the prohibition the old and new wording both carry and drops the imperative that assigns ADR upkeep to the implementer.

I read all of implementation.md, the diff (the prior text was the statement "Rationale stays in ADRs, without ADR citations or restatements in code comments."), SKILL.md's "Record decisions as they settle" and "Ground the scope" sections, and compare-designs.md. This is static reading; I did not run an implementer with either wording. A trial brief in which the implementer picks between two mechanisms and does or does not create an ADR file would settle it.

lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M446R13V1FQTH68VWQ2Y9SHN of review 01M446KNAEBD8TQNYHWDSGN8TB

**medium** — Implementer brief tells the sub-agent to "keep rationale in ADRs", which reads as permission to write ADRs > The changed sentence turns a statement about where rationale lives into an imperative aimed at the implementation sub-agent. A literal implementer that has just chosen between equivalent mechanisms (the next paragraph says "The implementer decides between mechanisms making the same promise") can read "keep rationale in ADRs" as an instruction to record that choice's rationale in a new or edited ADR. That contradicts the skill's model: SKILL.md says naming, layout, and helper boundaries are "settled without ADRs", that "equivalent mechanisms belong to implementation", and that ADRs record user-confirmed decisions; and the repository rule SKILL.md installs makes every binding ADR govern future changes. An implementer-authored ADR would enter the decision record without the user. > > The constraint the brief actually needs to carry is negative: do not cite ADRs or restate their rationale in code comments. Proposed correction: "Require it to leave ADR rationale out of code comments: no ADR citations or restatements." This keeps the prohibition the old and new wording both carry and drops the imperative that assigns ADR upkeep to the implementer. > > I read all of implementation.md, the diff (the prior text was the statement "Rationale stays in ADRs, without ADR citations or restatements in code comments."), SKILL.md's "Record decisions as they settle" and "Ground the scope" sections, and compare-designs.md. This is static reading; I did not run an implementer with either wording. A trial brief in which the implementer picks between two mechanisms and does or does not create an ADR file would settle it. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M446R13V1FQTH68VWQ2Y9SHN` of review `01M446KNAEBD8TQNYHWDSGN8TB`

👍 1

Fixed in add0b11: the implementer is only required to leave ADR rationale out of code comments, with no ADR citations or restatements; the brief no longer assigns keeping rationale in ADRs to it.

Fixed in add0b11: the implementer is only required to leave ADR rationale out of code comments, with no ADR citations or restatements; the brief no longer assigns keeping rationale in ADRs to it.

jercik marked this conversation as resolved

jercik added 1 commit

2026-10-04 19:43:55 +00:00

fix: keep implementer briefs from assigning ADR maintenance

commit-msg / commitlint (pull_request) Successful in 18s

Details

Node tests / node:test (pull_request) Successful in 2m25s

Details

Review / Review (pull_request_target) Failing after 3m54s

Details

add0b1136a

jercik referenced this pull request

2026-10-04 19:44:54 +00:00

docs: kept surface examples should include formats and stored data #117

jercik commented

2026-10-04 19:45:37 +00:00

Replying to review comment #120269

The summary-only checklist-count claim 01M446R1NDK40C2THD8T3XNMWR is already addressed by merged #115 (f8473dd); that independent change composes with this PR. Tracked the kept-surface example claim 01M446RFYQARJXD9YYVSFAXS4H in #117; its clarification is not claimed fixed in #116 by the unmerged child.

> Replying to review comment #120269 The summary-only checklist-count claim `01M446R1NDK40C2THD8T3XNMWR` is already addressed by merged https://code.j4k.dev/j4k-oss/agent-skills/pulls/115 (f8473dd); that independent change composes with this PR. Tracked the kept-surface example claim `01M446RFYQARJXD9YYVSFAXS4H` in https://code.j4k.dev/j4k-oss/agent-skills/pulls/117; its clarification is not claimed fixed in #116 by the unmerged child.

jercik referenced this pull request

2026-10-04 20:10:23 +00:00

docs: reference shared verification checks #118

jercik commented

2026-10-04 20:11:01 +00:00

Replying to review comment #120269

Tracked the summary-only shared-check-category claim 01M4487FJJ04VF82SGSZ76D0QK in #118, preserving the implementation-specific verification methods. It is not claimed fixed in #116 by the unmerged follow-up.

The unadjudicated medium code-comment-exclusions claim 01M4485X69VH4MM9EZ4F4PJYG1 remains unconfirmed: its body omits the defining source paths and explicitly leaves the future implementer's access to the parent rule unverified. This PR's scoped outcome is to pass that existing prohibition to a fresh implementer. What evidence establishes that the implementer receives or reads the parent rule without the explicit brief instruction? This claim's service adjudication remains outstanding; #116 is not declared complete or merged.

> Replying to review comment #120269 Tracked the summary-only shared-check-category claim `01M4487FJJ04VF82SGSZ76D0QK` in https://code.j4k.dev/j4k-oss/agent-skills/pulls/118, preserving the implementation-specific verification methods. It is not claimed fixed in #116 by the unmerged follow-up. The unadjudicated medium code-comment-exclusions claim `01M4485X69VH4MM9EZ4F4PJYG1` remains unconfirmed: its body omits the defining source paths and explicitly leaves the future implementer's access to the parent rule unverified. This PR's scoped outcome is to pass that existing prohibition to a fresh implementer. What evidence establishes that the implementer receives or reads the parent rule without the explicit brief instruction? This claim's service adjudication remains outstanding; #116 is not declared complete or merged.

jercik commented

2026-10-06 07:46:54 +00:00

Replying to review comment #120269

Closing out this PR without another review round. The inline finding 01M446R13V1FQTH68VWQ2Y9SHN was fixed in add0b11, and the other claims are tracked in #117 and #118 (merged separately). The remaining unadjudicated claim 01M4485X69VH4MM9EZ4F4PJYG1 is a non-blocking wording-duplication concern; it names no defect in the promised behavior and the question above stayed unanswered, so it is left unconfirmed and not carried into a follow-up. The change is two lines of guidance, checks other than the non-required Review job pass, and the branch merges cleanly into current main.

> Replying to review comment #120269 Closing out this PR without another review round. The inline finding `01M446R13V1FQTH68VWQ2Y9SHN` was fixed in add0b11, and the other claims are tracked in #117 and #118 (merged separately). The remaining unadjudicated claim `01M4485X69VH4MM9EZ4F4PJYG1` is a non-blocking wording-duplication concern; it names no defect in the promised behavior and the question above stayed unanswered, so it is left unconfirmed and not carried into a follow-up. The change is two lines of guidance, checks other than the non-required Review job pass, and the branch merges cleanly into current main.

jercik merged commit 370fbaea46 into main

2026-10-06 07:47:03 +00:00

jercik deleted branch docs/clarify-implementation-adr-guidance

2026-10-06 07:47:03 +00:00

jercik referenced this pull request from a commit

2026-10-06 07:47:04 +00:00

docs: clarify existing implementation ADR guidance (#116)

Sign in to join this conversation.

No reviewers

No labels

No milestone