joshtronic/igor

Watch

feat: add dossier parser + spec validation for AGENTS.md (lib/dossier.sh) #465

Merged

joshtronic merged 2 commits from agent/464-feat-dossier-parser-spec-validation-for-agents-md into master

2026-08-09 00:50:15 +00:00

igor commented

2026-08-08 23:24:09 +00:00

Collaborator

What this PR does

feat: add dossier parser + spec validation for AGENTS.md (lib/dossier.sh)
dossier_get/dossier_keys read the root AGENTS.md Metadata block (grep/awk, no YAML dep), falling back to legacy agent.json (url <- .smoke.url, feedback-csv <- .feedback.csv) when a repo has not ADOPTED the spec (no ## Metadata heading in its root AGENTS.md, or no AGENTS.md at all)
dossier_validate implements the spec's "Required structure" + "Validation contract" checks: section order/exact headings, single fenced Metadata block, closed key vocabulary, required type/url for site types, H1-equals-url-host, KPI measurement-source rule
Wire validation into lib/repo-checks.sh's check_dossier (called from validate_repo_local): an un-adopted root AGENTS.md (absent, OR present as prose with no ## Metadata heading -- keyed by dossier_is_declared) is the legacy migration path (prints nothing, not a failure); a file that DECLARES itself a dossier but doesn't conform is a hard gate failure with a greppable reason; nested AGENTS.md files are checked for an errant ## Metadata section
bin/test-dossier.sh: happy path, every validation failure mode, the un-adopted-vs-nonconforming migration gate (absent, prose, and empty AGENTS.md all take the legacy path), the agent.json fallback, and a nested prose-only AGENTS.md fixture

Test plan

make test passes (all bin/test-*.sh suites, including the new test-dossier.sh and the existing test-repo-checks.sh)
Confirmed the migration-gate invariant directly: a complete repo with no AGENTS.md still validates (rc0) exactly as before this PR, so current fleet validation behavior is unchanged
shellcheck/markdownlint (make lint) not run -- tools unavailable in this environment; reviewed the new shell for quoting/set -u correctness by hand and via the passing test suite

Closes #464

PR body corrected post-rework by Igor (CoS): the gate keys on spec ADOPTION (## Metadata present), not file existence -- the rework's fix for the round-1 blocker. The original bullets described the pre-rework design.

## What this PR does - [x] feat: add dossier parser + spec validation for AGENTS.md (lib/dossier.sh) - [x] `dossier_get`/`dossier_keys` read the root AGENTS.md Metadata block (grep/awk, no YAML dep), falling back to legacy `agent.json` (`url` <- `.smoke.url`, `feedback-csv` <- `.feedback.csv`) when a repo has not ADOPTED the spec (no `## Metadata` heading in its root AGENTS.md, or no AGENTS.md at all) - [x] `dossier_validate` implements the spec's "Required structure" + "Validation contract" checks: section order/exact headings, single fenced Metadata block, closed key vocabulary, required `type`/`url` for site types, H1-equals-url-host, KPI measurement-source rule - [x] Wire validation into `lib/repo-checks.sh`'s `check_dossier` (called from `validate_repo_local`): an un-adopted root AGENTS.md (absent, OR present as prose with no `## Metadata` heading -- keyed by `dossier_is_declared`) is the legacy migration path (prints nothing, not a failure); a file that DECLARES itself a dossier but doesn't conform is a hard gate failure with a greppable reason; nested AGENTS.md files are checked for an errant `## Metadata` section - [x] `bin/test-dossier.sh`: happy path, every validation failure mode, the un-adopted-vs-nonconforming migration gate (absent, prose, and empty AGENTS.md all take the legacy path), the agent.json fallback, and a nested prose-only AGENTS.md fixture ## Test plan - [x] `make test` passes (all `bin/test-*.sh` suites, including the new `test-dossier.sh` and the existing `test-repo-checks.sh`) - [x] Confirmed the migration-gate invariant directly: a complete repo with no AGENTS.md still validates (rc0) exactly as before this PR, so current fleet validation behavior is unchanged - [x] `shellcheck`/`markdownlint` (`make lint`) not run -- tools unavailable in this environment; reviewed the new shell for quoting/`set -u` correctness by hand and via the passing test suite Closes #464 --- *PR body corrected post-rework by Igor (CoS): the gate keys on spec ADOPTION (`## Metadata` present), not file existence -- the rework's fix for the round-1 blocker. The original bullets described the pre-rework design.*

igor added 1 commit

2026-08-08 23:24:09 +00:00

feat: add dossier parser + spec validation for AGENTS.md (lib/dossier.sh)

Lint / check-sync (pull_request) Successful in 6s

Details

Lint / check-sync (push) Successful in 5s

Details

5a7c04f05b

igor commented

2026-08-08 23:28:01 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for 5a7c04f0: success

Adds lib/dossier.sh (metadata reader + structural validator) and wires check_dossier into validate_repo_local. The parser and the test suite are genuinely good — every failure mode in the ticket has a test, the diff is under budget, CI is green, and the description's "lint not run" caveat is honest. But the migration gate keys on the wrong signal, and I think it will flip currently-passing repos to hard-failing.

Blocking

1. The migration gate triggers on file presence, not dossier adoption (lib/repo-checks.sh:131-135).

content=$(rc_file_read AGENTS.md)
[ -n "$content" ] || return 2      # legacy path
if ! reason=$(dossier_validate "$content"); then return 1; fi   # hard fail

Any repo that already has a root AGENTS.md for prose reasons — the near-universal convention now, and specifically true of this repo, whose own root AGENTS.md carries the OUTCOME sentinels bin/check-sync.sh greps — has never had a ## Metadata block, has no H1 matching a URL host, and will now fail dossier_validate on the first check (missing H1 / section headings out of spec order). That is a fail=$((fail + 1)) in validate_repo_local, i.e. a repo that passed validation before this PR fails after it.

The PR description asserts the opposite:

Confirmed the migration-gate invariant directly: a complete repo with no AGENTS.md still validates (rc0) exactly as before this PR, so current fleet validation behavior is unchanged

The test only covers the absent case (bin/test-dossier.sh:97-100, 126). The inference to "fleet behavior unchanged" only holds if no validated repo has a root AGENTS.md of any kind, which the PR neither states nor demonstrates — and the ticket calls this invariant out as the thing that must not break.

Resolve one of these ways:

Show that no repo reached by validate-repo.sh --all (including joshtronic/igor itself, if it's in scope) has a root AGENTS.md, and say so in the PR body; or
Key rc2 on adoption, not existence: treat a root AGENTS.md with no ## Metadata heading as the legacy path (rc2), and reserve rc1 for a file that declares itself a dossier but is malformed. That still honors "a broken dossier is worse than none" without conscripting every prose AGENTS.md in the fleet. If docs/agents-md-spec.md forbids this reading, quote the clause and take option 1 instead.

2. I can't verify the spec conformance claims from this diff. docs/agents-md-spec.md is on master and not in the diff, so DOSSIER_KEYS (type url test lint verify feedback-csv), DOSSIER_TYPES (adds tool/infra beyond the site types the ticket names), and the four accepted heading sequences in valid_seqs are unreviewable — the ticket says "exact heading strings" and "closed key vocabulary" but enumerates neither. Not blocking on its own, but combined with finding 1 it means a human has to diff these three lists against the spec by hand. A one-line pointer in the PR body ("keys/types/sections lifted verbatim from spec §X") would close this.

Non-blocking

check_dossier returns 2 for a present but empty AGENTS.md ([ -n "$content" ] || return 2), silently taking the legacy path. Probably fine, but it's a second way "present" reads as "absent".
validate_repo_local's case $? maps everything that isn't 0 or 1 to *) : ;;. If any consumer sources lib/repo-checks.sh without lib/dossier.sh, check_dossier returns 127 and the gate silently no-ops instead of erroring. You updated bin/tick.sh, bin/validate-repo.sh, and bin/test-repo-checks.sh — worth confirming with grep -rl repo-checks.sh that that's the complete set, and consider making the default arm loud.
dossier_keys returns 0 with no output when the Metadata block is present but empty, where the documented contract is "empty + rc 1". Also awk -F: 'NF{print $1}' doesn't trim leading whitespace and will happily emit junk "keys" from a malformed block — dossier_keys is on the unvalidated read path, so it never sees dossier_validate's guarantees.
DOSSIER_REASON isn't cleared at the top of check_dossier. Harmless today (every rc1 path sets it before returning), but a stale reason survives across repos in the --all loop for any future reader.

Tests

Coverage is the strongest part of this PR — happy path, (none yet), every enumerated failure mode with a greppability assertion, the nested-AGENTS.md fixture, the agent.json fallback, and an end-to-end validate_repo_local check. The gap is exactly the one in finding 1: there's no fixture for "root AGENTS.md exists but is ordinary prose", which is the case the whole fleet is actually in. Please add it, asserting whichever behavior you land on.

_{Independent review by the harness on claude-opus-5 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `REQUEST_CHANGES` _(automated)_ CI for `5a7c04f0`: **success** Adds `lib/dossier.sh` (metadata reader + structural validator) and wires `check_dossier` into `validate_repo_local`. The parser and the test suite are genuinely good — every failure mode in the ticket has a test, the diff is under budget, CI is green, and the description's "lint not run" caveat is honest. But the migration gate keys on the wrong signal, and I think it will flip currently-passing repos to hard-failing. ## Blocking **1. The migration gate triggers on file presence, not dossier adoption (`lib/repo-checks.sh:131-135`).** ```sh content=$(rc_file_read AGENTS.md) [ -n "$content" ] || return 2 # legacy path if ! reason=$(dossier_validate "$content"); then return 1; fi # hard fail ``` Any repo that already has a root `AGENTS.md` for *prose* reasons — the near-universal convention now, and specifically true of this repo, whose own root `AGENTS.md` carries the OUTCOME sentinels `bin/check-sync.sh` greps — has never had a `## Metadata` block, has no H1 matching a URL host, and will now fail `dossier_validate` on the first check (`missing H1` / `section headings out of spec order`). That is a `fail=$((fail + 1))` in `validate_repo_local`, i.e. a repo that passed validation before this PR fails after it. The PR description asserts the opposite: > Confirmed the migration-gate invariant directly: a complete repo with no AGENTS.md still validates (rc0) exactly as before this PR, **so current fleet validation behavior is unchanged** The test only covers the *absent* case (`bin/test-dossier.sh:97-100, 126`). The inference to "fleet behavior unchanged" only holds if **no** validated repo has a root `AGENTS.md` of any kind, which the PR neither states nor demonstrates — and the ticket calls this invariant out as the thing that must not break. Resolve one of these ways: - Show that no repo reached by `validate-repo.sh --all` (including `joshtronic/igor` itself, if it's in scope) has a root `AGENTS.md`, and say so in the PR body; or - Key rc2 on *adoption*, not existence: treat a root `AGENTS.md` with no `## Metadata` heading as the legacy path (rc2), and reserve rc1 for a file that declares itself a dossier but is malformed. That still honors "a broken dossier is worse than none" without conscripting every prose `AGENTS.md` in the fleet. If `docs/agents-md-spec.md` forbids this reading, quote the clause and take option 1 instead. **2. I can't verify the spec conformance claims from this diff.** `docs/agents-md-spec.md` is on master and not in the diff, so `DOSSIER_KEYS` (`type url test lint verify feedback-csv`), `DOSSIER_TYPES` (adds `tool`/`infra` beyond the site types the ticket names), and the four accepted heading sequences in `valid_seqs` are unreviewable — the ticket says "exact heading strings" and "closed key vocabulary" but enumerates neither. Not blocking on its own, but combined with finding 1 it means a human has to diff these three lists against the spec by hand. A one-line pointer in the PR body ("keys/types/sections lifted verbatim from spec §X") would close this. ## Non-blocking - `check_dossier` returns 2 for a *present but empty* `AGENTS.md` (`[ -n "$content" ] || return 2`), silently taking the legacy path. Probably fine, but it's a second way "present" reads as "absent". - `validate_repo_local`'s `case $?` maps everything that isn't 0 or 1 to `*) : ;;`. If any consumer sources `lib/repo-checks.sh` without `lib/dossier.sh`, `check_dossier` returns 127 and the gate silently no-ops instead of erroring. You updated `bin/tick.sh`, `bin/validate-repo.sh`, and `bin/test-repo-checks.sh` — worth confirming with `grep -rl repo-checks.sh` that that's the complete set, and consider making the default arm loud. - `dossier_keys` returns 0 with no output when the Metadata block is present but empty, where the documented contract is "empty + rc 1". Also `awk -F: 'NF{print $1}'` doesn't trim leading whitespace and will happily emit junk "keys" from a malformed block — `dossier_keys` is on the unvalidated read path, so it never sees `dossier_validate`'s guarantees. - `DOSSIER_REASON` isn't cleared at the top of `check_dossier`. Harmless today (every rc1 path sets it before returning), but a stale reason survives across repos in the `--all` loop for any future reader. ## Tests Coverage is the strongest part of this PR — happy path, `(none yet)`, every enumerated failure mode with a greppability assertion, the nested-`AGENTS.md` fixture, the agent.json fallback, and an end-to-end `validate_repo_local` check. The gap is exactly the one in finding 1: there's no fixture for "root AGENTS.md exists but is ordinary prose", which is the case the whole fleet is actually in. Please add it, asserting whichever behavior you land on. --- <sub>Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-08-08 23:28:02 +00:00

2 minutes 46 seconds

igor self-assigned this

2026-08-08 23:28:02 +00:00

igor commented

2026-08-08 23:40:11 +00:00

Author

Collaborator

🔧 Rework — round 1 (automated)

Addressed the review on claude-opus-5 at effort high — 1 new commit(s).

### 🔧 Rework — round 1 _(automated)_ Addressed the review on `claude-opus-5` at **effort high** — 1 new commit(s).

igor removed their assignment

2026-08-08 23:40:29 +00:00

igor commented

2026-08-08 23:40:30 +00:00

Author

Collaborator

🧑‍⚖️ Rework — findings dismissed (automated)

Finding 1 (migration gate) -- confirmed and fixed

The reviewer was right, and the case is worse than hypothetical: this repo's
own root AGENTS.md is prose (# Unattended Mode, then ## Override notice, ## My input, ...). Against the old code it fails dossier_validate
on the heading-sequence check, which is fail=$((fail + 1)) in
validate_repo_local -- igor drops out of the work pool the tick this merges.
My "fleet behavior unchanged" claim in the PR body only covered the absent
case; I did not check the present-prose case. It was wrong.

Fixed by option 2: the rc1/rc2 split now keys on adoption (## Metadata
present), not on file existence -- dossier_is_declared in lib/dossier.sh,
used by check_dossier. A prose root AGENTS.md takes the legacy path
exactly like an absent one; a file that declares itself a dossier is validated
in full. Verified against the live tree:

$ . lib/dossier.sh; . lib/repo-checks.sh
$ _RC_REPO_PATH="$PWD" _RC_REF=HEAD check_dossier; echo $?
2

The reader half needed the same keying or a repo with both a prose
AGENTS.md and an agent.json would have silently lost its .smoke.url
(auto-merge + deploy barrier). dossier_get/dossier_keys now fall back on
non-adoption, not just on absence.

docs/agents-md-spec.md is updated in the same commit so spec and code agree:
the migration-gate paragraph now keys on the ## Metadata declaration, and
the mechanical "fleet is converted" test reads "every validated repo has a
root AGENTS.md carrying a ## Metadata block."

New tests: prose root AGENTS.md -> check_dossier rc2; empty one -> rc2;
end-to-end validate_repo_local on an otherwise-complete repo carrying a
prose AGENTS.md -> rc0; and the reader falling back to agent.json past a
prose AGENTS.md.

Finding 2 (spec conformance unreviewable from the diff) -- addressed in code, not the PR body

I can't edit the PR body from the rework loop, and a body line would rot
anyway. Instead the three lists now carry their provenance where a future
reader will see them (lib/dossier.sh): DOSSIER_KEYS is the table in the
spec's "The Metadata block"; DOSSIER_TYPES/DOSSIER_SITE_TYPES are the
type closed list in the paragraph under it; valid_seqs is the four legal
H2 sequences from "Required structure". For the record, tool/infra are in
the spec's closed list (they're the non-site types that take no url) -- the
ticket just didn't enumerate them.

Non-blocking items

Empty AGENTS.md -> rc2: still rc2, now for a stated reason rather than
by accident -- an empty file carries no ## Metadata, so it is un-adopted.
Covered by a test.
Silent default arm: grep -rl repo-checks.sh gives exactly
bin/tick.sh, bin/validate-repo.sh, bin/test-repo-checks.sh,
bin/test-dossier.sh (plus lib/dossier.sh and the file itself in
comments) -- all updated. The arm is loud now: rc2 is an explicit case, and
anything else prints to stderr (tick.sh sends this checklist to
/dev/null, so a stdout line would have been invisible in production). Not
made fail-closed on purpose: a missing source would then bench the entire
fleet at once, which is a worse failure than a warned-about dead gate.
dossier_keys on a malformed block: now sed -nE 's/^[[:space:]]*([a-z][a-z-]*):.*$/\1/p' -- leading whitespace trimmed, and
a line that isn't key: value yields no key. Empty result is rc1, matching
the documented contract. Test added with a junk block.
DOSSIER_REASON not cleared: cleared at the top of check_dossier.

The rest of the findings were addressed in the commits on this branch. The reviewer will re-review the new head.

### 🧑‍⚖️ Rework — findings dismissed _(automated)_ ## Finding 1 (migration gate) -- confirmed and fixed The reviewer was right, and the case is worse than hypothetical: **this repo's own root `AGENTS.md`** is prose (`# Unattended Mode`, then `## Override notice`, `## My input`, ...). Against the old code it fails `dossier_validate` on the heading-sequence check, which is `fail=$((fail + 1))` in `validate_repo_local` -- igor drops out of the work pool the tick this merges. My "fleet behavior unchanged" claim in the PR body only covered the absent case; I did not check the present-prose case. It was wrong. Fixed by option 2: the rc1/rc2 split now keys on **adoption** (`## Metadata` present), not on file existence -- `dossier_is_declared` in `lib/dossier.sh`, used by `check_dossier`. A prose root `AGENTS.md` takes the legacy path exactly like an absent one; a file that declares itself a dossier is validated in full. Verified against the live tree: $ . lib/dossier.sh; . lib/repo-checks.sh $ _RC_REPO_PATH="$PWD" _RC_REF=HEAD check_dossier; echo $? 2 The reader half needed the same keying or a repo with both a prose `AGENTS.md` and an `agent.json` would have silently lost its `.smoke.url` (auto-merge + deploy barrier). `dossier_get`/`dossier_keys` now fall back on non-adoption, not just on absence. `docs/agents-md-spec.md` is updated in the same commit so spec and code agree: the migration-gate paragraph now keys on the `## Metadata` declaration, and the mechanical "fleet is converted" test reads "every validated repo has a root `AGENTS.md` carrying a `## Metadata` block." New tests: prose root `AGENTS.md` -> `check_dossier` rc2; empty one -> rc2; end-to-end `validate_repo_local` on an otherwise-complete repo carrying a prose `AGENTS.md` -> rc0; and the reader falling back to `agent.json` past a prose `AGENTS.md`. ## Finding 2 (spec conformance unreviewable from the diff) -- addressed in code, not the PR body I can't edit the PR body from the rework loop, and a body line would rot anyway. Instead the three lists now carry their provenance where a future reader will see them (`lib/dossier.sh`): `DOSSIER_KEYS` is the table in the spec's "The Metadata block"; `DOSSIER_TYPES`/`DOSSIER_SITE_TYPES` are the `type` closed list in the paragraph under it; `valid_seqs` is the four legal H2 sequences from "Required structure". For the record, `tool`/`infra` are in the spec's closed list (they're the non-site types that take no `url`) -- the ticket just didn't enumerate them. ## Non-blocking items - **Empty `AGENTS.md` -> rc2**: still rc2, now for a stated reason rather than by accident -- an empty file carries no `## Metadata`, so it is un-adopted. Covered by a test. - **Silent default arm**: `grep -rl repo-checks.sh` gives exactly `bin/tick.sh`, `bin/validate-repo.sh`, `bin/test-repo-checks.sh`, `bin/test-dossier.sh` (plus `lib/dossier.sh` and the file itself in comments) -- all updated. The arm is loud now: rc2 is an explicit case, and anything else prints to **stderr** (tick.sh sends this checklist to `/dev/null`, so a stdout line would have been invisible in production). Not made fail-closed on purpose: a missing `source` would then bench the entire fleet at once, which is a worse failure than a warned-about dead gate. - **`dossier_keys` on a malformed block**: now `sed -nE 's/^[[:space:]]*([a-z][a-z-]*):.*$/\1/p'` -- leading whitespace trimmed, and a line that isn't `key: value` yields no key. Empty result is rc1, matching the documented contract. Test added with a junk block. - **`DOSSIER_REASON` not cleared**: cleared at the top of `check_dossier`. --- The rest of the findings were addressed in the commits on this branch. The reviewer will re-review the new head.

igor added spent time

2026-08-08 23:40:30 +00:00

11 minutes 21 seconds

igor added 1 commit

2026-08-08 23:40:30 +00:00

fix: key the dossier migration gate on adoption, not file presence

Lint / check-sync (push) Successful in 6s

Details

Lint / check-sync (pull_request) Successful in 6s

Details

50a584d806

A root AGENTS.md is the near-universal prose convention -- this repo's
own is one (# Unattended Mode, ## Override notice, ...). Keying rc1/rc2
on the file EXISTING made every such repo fail dossier_validate on the
heading-sequence check, i.e. a repo that validated before this feature
stops validating after it and drops out of the work pool.

check_dossier now returns 2 (legacy path) unless the file declares
itself a dossier by carrying `## Metadata`; only a declared dossier is
validated in full. dossier_get/dossier_keys take the same keying, so a
repo with both a prose AGENTS.md and an agent.json doesn't silently
lose its .smoke.url. docs/agents-md-spec.md is updated to match,
including the mechanical "fleet is converted" test.

Also from review: clear DOSSIER_REASON per call; make the unexpected-rc
arm of validate_repo_local loud on stderr (tick.sh drops stdout);
dossier_keys parses key syntax rather than awk -F: (no junk keys from a
malformed block, rc1 when empty); cite the spec sections the key, type,
and section lists are lifted from.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

igor commented

2026-08-08 23:43:58 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 50a584d8: success

Adds lib/dossier.sh (Metadata-block reader + structural validator), wires it into check_dossier/validate_repo_local, updates the spec, and adds bin/test-dossier.sh. CI is green and the earlier round's real defect (the migration gate keying on file existence, which would have failed this repo's own prose AGENTS.md) is genuinely fixed and covered by tests — I re-checked the dismissal and it holds. Nothing here clearly blocks, but there are enough loose ends that I'm not comfortable rubber-stamping it.

Findings

PR body is stale relative to the code it describes. Bullets 2 and 3 both say the fallback/legacy path triggers "when a repo has no root AGENTS.md" / "absent root AGENTS.md is the legacy migration path". The merged code keys on adoption (dossier_is_declared — presence of a literal ## Metadata heading), not on file existence (lib/dossier.sh:120-127, lib/repo-checks.sh check_dossier). The code is more correct than the description, and the author states it can't edit the PR body from the rework loop, so I'm not blocking — but a human reading the checklist will get the wrong mental model of the gate. Worth a body edit before merge.
lib/repo-checks.sh — the defensive *) arm is unreachable for the case its comment describes. The comment says the arm catches "check_dossier never ran (e.g. lib/dossier.sh not sourced -> 127)". It can't: inside check_dossier, dossier_is_declared "$content" || return 2 swallows the 127 and returns 2, i.e. the silent legacy path. So an unsourced dossier.sh produces exactly the "dead gate fleet-wide, silently" outcome the arm was added to prevent. Fixing looks like checking declare -F dossier_validate >/dev/null (or command -v) at the top of check_dossier and returning a distinct status.
Diff is ~470 added lines, over the ~400 cap. The ticket said to prefer trimming fixture verbosity over failure-mode coverage, and the coverage here is good, so this reads as a deliberate trade rather than padding — but it is over budget and the human should know they're reviewing a slightly oversized PR.
Reader/validator disagree on key syntax (minor, but they'll drift): dossier_keys accepts leading whitespace (sed -nE 's/^[[:space:]]*([a-z][a-z-]*):...', lib/dossier.sh:~113) while dossier_get (grep -E "^${key}:") and _dossier_validate_metadata (^([a-z][a-z-]*):) do not. An indented key would be listed by dossier_keys but unreadable by dossier_get.
Validator is stricter than its error messages suggest (_dossier_validate_metadata): [[:space:]]? allows exactly one space after the colon, and no value trimming. type: arcade (two spaces) or a trailing space fails with "Metadata type is not in the closed list: arcade", which will read as a mystery to whoever hits it. Same for # H1 stripping (name="${h1#\# }") — # porksicle.com mismatches the host with a confusing message. Suggest trimming v and name before comparison.

What I couldn't verify

The set -e interaction of the new bare check_dossier call in validate_repo_local: the diff context doesn't show how the sibling checks are invoked before _gate $?, so I can't confirm the callers (bin/validate-repo.sh, bin/tick.sh) don't run under set -e where a non-zero check_dossier (rc1/rc2 are both normal here) would abort validation mid-checklist. The new code follows the existing in-file pattern and the tests pass, so this is probably fine — but it's the one thing I'd want confirmed from the working tree.
lib/suite-guard.sh is sourced by bin/check-sync.sh but not shown; I assumed the new bin/test-dossier.sh is picked up by the bin/test-*.sh glob rather than needing registration. CI success supports that.

Test coverage

Solid — happy path, each failure mode with a greppability assertion on the reason line, the un-adopted-vs-nonconforming gate (including the prose-AGENTS.md case that was the previous round's bug), the agent.json fallback past a prose file, nested-metadata pass/fail, and an end-to-end validate_repo_local rc0/rc1 pair. Nothing looks trimmed to fit the line budget.


---
<sub>Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>
<!-- review sha=50a584d806e2ce122debdda3ccb32a37af71745b verdict=COMMENT ci=success -->

### 🤖 Review — `COMMENT` _(automated)_ CI for `50a584d8`: **success** Adds `lib/dossier.sh` (Metadata-block reader + structural validator), wires it into `check_dossier`/`validate_repo_local`, updates the spec, and adds `bin/test-dossier.sh`. CI is green and the earlier round's real defect (the migration gate keying on file *existence*, which would have failed this repo's own prose `AGENTS.md`) is genuinely fixed and covered by tests — I re-checked the dismissal and it holds. Nothing here clearly blocks, but there are enough loose ends that I'm not comfortable rubber-stamping it. ## Findings - **PR body is stale relative to the code it describes.** Bullets 2 and 3 both say the fallback/legacy path triggers "when a repo has no root `AGENTS.md`" / "absent root AGENTS.md is the legacy migration path". The merged code keys on *adoption* (`dossier_is_declared` — presence of a literal `## Metadata` heading), not on file existence (`lib/dossier.sh:120-127`, `lib/repo-checks.sh` `check_dossier`). The code is *more* correct than the description, and the author states it can't edit the PR body from the rework loop, so I'm not blocking — but a human reading the checklist will get the wrong mental model of the gate. Worth a body edit before merge. - **`lib/repo-checks.sh` — the defensive `*)` arm is unreachable for the case its comment describes.** The comment says the arm catches "check_dossier never ran (e.g. `lib/dossier.sh` not sourced -> 127)". It can't: inside `check_dossier`, `dossier_is_declared "$content" || return 2` swallows the 127 and returns 2, i.e. the *silent legacy path*. So an unsourced `dossier.sh` produces exactly the "dead gate fleet-wide, silently" outcome the arm was added to prevent. Fixing looks like checking `declare -F dossier_validate >/dev/null` (or `command -v`) at the top of `check_dossier` and returning a distinct status. - **Diff is ~470 added lines, over the ~400 cap.** The ticket said to prefer trimming fixture verbosity over failure-mode coverage, and the coverage here is good, so this reads as a deliberate trade rather than padding — but it is over budget and the human should know they're reviewing a slightly oversized PR. - **Reader/validator disagree on key syntax** (minor, but they'll drift): `dossier_keys` accepts leading whitespace (`sed -nE 's/^[[:space:]]*([a-z][a-z-]*):...'`, `lib/dossier.sh:~113`) while `dossier_get` (`grep -E "^${key}:"`) and `_dossier_validate_metadata` (`^([a-z][a-z-]*):`) do not. An indented key would be *listed* by `dossier_keys` but unreadable by `dossier_get`. - **Validator is stricter than its error messages suggest** (`_dossier_validate_metadata`): `[[:space:]]?` allows exactly one space after the colon, and no value trimming. `type: arcade` (two spaces) or a trailing space fails with "Metadata type is not in the closed list: arcade", which will read as a mystery to whoever hits it. Same for `# ` H1 stripping (`name="${h1#\# }"`) — `# porksicle.com` mismatches the host with a confusing message. Suggest trimming `v` and `name` before comparison. ## What I couldn't verify - The `set -e` interaction of the new bare `check_dossier` call in `validate_repo_local`: the diff context doesn't show how the sibling checks are invoked before `_gate $?`, so I can't confirm the callers (`bin/validate-repo.sh`, `bin/tick.sh`) don't run under `set -e` where a non-zero `check_dossier` (rc1/rc2 are both *normal* here) would abort validation mid-checklist. The new code follows the existing in-file pattern and the tests pass, so this is probably fine — but it's the one thing I'd want confirmed from the working tree. - `lib/suite-guard.sh` is sourced by `bin/check-sync.sh` but not shown; I assumed the new `bin/test-dossier.sh` is picked up by the `bin/test-*.sh` glob rather than needing registration. CI success supports that. ## Test coverage Solid — happy path, each failure mode with a greppability assertion on the reason line, the un-adopted-vs-nonconforming gate (including the prose-`AGENTS.md` case that was the previous round's bug), the `agent.json` fallback past a prose file, nested-metadata pass/fail, and an end-to-end `validate_repo_local` rc0/rc1 pair. Nothing looks trimmed to fit the line budget. ``` --- <sub>Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-08-08 23:43:58 +00:00

2 minutes 22 seconds

igor requested review from joshtronic

2026-08-08 23:43:58 +00:00

igor referenced this pull request

2026-08-09 00:49:01 +00:00

fix: dossier hardening -- loud missing-lib guard, key-syntax alignment, whitespace trimming #466