Skip to content

feat: okf verify — the governed writer of the verified stamp - #53

Open
jchable wants to merge 27 commits into
devfrom
worktree-okf-verify
Open

feat: okf verify — the governed writer of the verified stamp#53
jchable wants to merge 27 commits into
devfrom
worktree-okf-verify

Conversation

@jchable

@jchable jchable commented Aug 29, 2026

Copy link
Copy Markdown
Owner

What

Adds okf verify, the verb that closes the loop okf audit opened. audit
reports which concepts are unverified; verify writes the §5.2
verified: {by, at} stamp that clears them from the trust-filtered worklist.

okf audit <bundle> --trust unverified | cut -d' ' -f1 | okf verify <bundle> - --by human:ada

Five layers:

  • BundleConceptWriter.RecordVerifications — the single governed writer of
    verified. Prepares (resolve, read, parse, upsert, validate) every concept
    before writing any; Records means "landed on disk", not "was validated".
  • okf verify CLI verb — explicit ids or - to read them from stdin,
    --by <actor> (required), --at <yyyy-MM-ddTHH:mm:ssZ>, --dry-run.
  • Two goldensverify.out and verify-dau.md, hand-verified (see below).
  • okf_verify agent tool — mutator, so WriteToolNames goes 3 → 4 and the
    tool count 11 → 12; the read-only subset stays 8.
  • Documentation, including an honesty box on what a stamp does not prove.

Two pre-existing CLI behaviours changed as a consequence and are called out in
the CHANGELOG: OkfCli.Run now takes a TextReader for stdin (OKF4net.Cli
has no PackageId and ships as a binary, so no external caller breaks), --
now preserves prior positionals, and a lone - is an argument rather than a
swallowed flag — that last one made the stdin form unreachable until it was
fixed.

Spec impact

§5.2 ({by, at} stamps) and §5.3 (trust tiers, via Trust.DeriveTier).
Conformance is preserved: writes go through ValidateConformance() (§11,
non-empty type) rather than the stricter producer-grade Validate() — a
deliberate choice, since refusing a reviewer because a third party omitted a
description would make precisely the concepts an audit surfaces unstampable.
generated is never written or refreshed by this path (§10.6 separation).

What a verified stamp is not. It is a dated declaration, not a proof. It
does not establish the signer's identity, nor that anyone read anything —
okf_write_concept can write the same field with no ceremony, deliberately
unguarded. Credibility comes from the stamp landing in a diff a human reviewed,
and it must never be inferred from a PR approval, which would mass-promote every
concept a diff touches and empty the worklist the feature exists to fill.

Fixtures

Two new files under tests/fixtures/golden/. No pre-existing fixture was
modified (git diff --name-status over tests/fixtures/ shows A, A, and
M on README.md only). They are hand-verified against the spec's stated
output format rather than captured from a reference CLI — verify is an
OKF4net verb with no upstream counterpart — and tests/fixtures/README.md
documents that provenance, including the fact that the written file's
frontmatter is re-emitted in canonical block style by every write path.

Review notes

Each of the six tasks passed its own scoped review, plus a whole-branch review.
Coverage gaps were found by mutating the source, not by reading it: putting
records.Add back in the prepare loop now fails three tests, one per layer.

Checklist

  • dotnet test OKF4net.sln passes (1111 tests, including golden comparisons)
  • dotnet format OKF4net.sln --verify-no-changes passes
  • Tests added/updated for behaviour changes
  • No new runtime dependencies (zero-dependency policy)
  • New files carry the // SPDX-License-Identifier: LGPL-3.0-or-later header

🤖 Generated with Claude Code

https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG

ncitnea and others added 21 commits August 28, 2026 11:41
okf audit produces a worklist and nothing writes the field it selects
on: every finding is a dead end. okf verify records a review — a dated
{ by, at } stamp — as the single governed writer of `verified`, wired to
audit's output via stdin.

The design rests on decisions taken explicitly during brainstorming,
with an independent second opinion integrated: a stamp is a declaration,
not a proof (credibility comes from landing in a reviewed diff, never
from inferring it off a PR approval — that mechanism mass-promotes and
empties the worklist); no guard on okf_write_concept and a CLI-symmetric
agent tool (user decisions, documented consequences); verified is the
latest stamp per actor, neither a log nor a state; conformance-level
validation on write, so a reviewer can stamp a concept a third party
left without a description; no --all, no --stale-after, ids only.

Three code facts condition everything and are cited: DeriveTier ignores
`at`, MaybeStampGenerated never refreshes, and okf_write_concept can
already write any stamp — the gap was never "no writer" but "no governed
writer beside an ungoverned one".

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
Read line by line against the source rather than re-read as prose. All
five were verified in the code, not inferred:

- The grammar `okf verify <bundle> <id>…` is inexpressible today:
  CliArgs holds a single `string? _positional`. Every existing verb
  takes exactly one positional, so nothing had needed more. Added as
  unit 0, with the honest note that the positional *list* existed and I
  collapsed it during a /simplify pass six days ago — correct then, and
  restoring it does not undo that.
- OkfCli.Run takes stdout and stderr only, and nothing in the CLI reads
  Console.In. The `-` form is the feature's headline, and the suite
  drives the CLI in-process, so without a stdin seam the most important
  path would be the one path no test covers. Signature change, acted as
  a breaking change with a CHANGELOG entry.
- The per-actor replacement cited YamlMapping.Insert, a mapping API, to
  justify replacing a stamp inside a sequence. YamlSequence is
  immutable: the sequence is rebuilt and re-inserted under the key. Two
  levels, two mechanisms — the spec now says so.
- BundleConceptWriter.UtcNow is documented as consulted only when
  AutoStampGenerated is set; RecordVerification broadens that. Deliberate
  (one clock in the writer), so the XML doc must change with it.
- "verify and validate differ by two letters" was simply false. The real
  argument — shared prefix, mutual autocompletion, both taking a bundle
  first — is stronger anyway.

Three tests added for unit 0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
Six tasks, each ending on an independently testable deliverable: the two
CLI prerequisites the spec review surfaced (ordered positionals, a stdin
seam), RecordVerification in the core, the verb, its golden, the agent
tool, then the documentation.

Self-review fixed three internal inconsistencies before the plan left my
hands, all of the same family — code that cited an API it could not
reach, or contradicted its own tests:

- VerificationOutcome had no `At`, yet both consumers needed the
  timestamp actually written: OkfTimestamp is internal to OKF4net, so
  neither the CLI nor the Agents assembly can format one. The record now
  reports it; the agent tool passes `at` straight through.
- CmdVerify validated --by before resolving the ids, which contradicted
  its own Theory: `verify <bundle>` with no id must say "missing
  <concept-id>", not complain about --by. Values are now read first (so
  an unvalued flag still names itself) and validated after the ids.
- Task 2's dry-run printed a timestamp it would never write.

The plan also records the one deliberate behaviour change it forces:
with several positionals, "the token after -- wins" stops making sense,
so -- becomes POSIX's "end of options" and keeps what came before it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
An independent pass over the plan, run against the code rather than the
prose, found three blocking defects. All three reproduced before fixing:

- A lone `-` is swallowed as a flag: CliArgs' flag branch is
  `token.StartsWith('-')`, which `"-"` matches, so it never reaches the
  positional list. `okf fmt -` today answers `error: missing <file>`.
  The stdin form — the line that closes the audit loop — was therefore
  unreachable, and both its tests would have asserted the wrong thing.
  Guarded on `token.Length > 1`; third CHANGELOG entry.
- The golden did not match what the plan's own code emits:
  okf_v02/metrics/dau.md already carries a human:ada stamp, so verifying
  it takes the replace path and the line gains "(replaces …)". Corrected
  — and the golden is better for it, pinning both paths at once.
- A test asserted `title: Orders` while formatting users.md, whose title
  is Users.

Two "high" findings were about promises the code did not keep: the agent
tool looped over ids writing as it went, so a bad third id left the first
two stamped, contradicting the spec's "all-or-nothing"; and the CLI's
pre-flight checked existence only, while a document with no `type` loads
fine and is refused at write time. Both now resolve every id before the
first write, with a test each.

The audit also demolished a rationale I had written three times:
OkfTimestamp is NOT out of reach — OKF4net grants InternalsVisibleTo to
both `okf` and OKF4net.Agents. The `At` field survives on its real
merit (one clock, held by the writer that tests pin), not on a false
constraint.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
The external pass agreed with the internal audit on the `title: Orders`
slip and went further on five points. All five were verified in the code
before acting.

The heaviest is a design fix, not a patch. All-or-nothing was a
pre-flight in each caller followed by sequential writes, so a second
document that turned out unreadable still left the first stamped — and
the agent tool had no pre-flight at all. The core now exposes ONE batch
method, RecordVerifications, that resolves, reads, parses, validates and
prepares every concept before writing any of them, under a single hold
of the lock. Both consumers inherit the guarantee instead of each
re-implementing half of it, and there is no single-concept variant to
keep in step. The residual limit — an in-process lock, no multi-file
atomic write — is documented rather than implied.

The other four:

- Using BundleValidator.IsIso8601DateTime as a WRITE gate was my own
  mistake, and the same one the spec warns about elsewhere: that
  predicate validates the date and ignores everything after the `T`
  (Validate.cs:618) because reading frontmatter is deliberately
  permissive. It would have written `2026-08-28` or a +02:00 offset as a
  stamp the field documents as UTC. Strict parse now, with the escaped
  'T'/'Z' format string, plus date-only and offset cases.
- The tool rendered the writer's prose while the spec asks for the CLI's
  exact line; both now emit `recorded <id>  <by>  <at>`, asserted by
  equality rather than Contains.
- The golden compared stdout only, so a run printing the right line and
  writing the wrong stamp stayed green. A second golden pins the written
  file.
- The core preservation test checked substrings; it now compares the key
  list, the body and the parsed stamp. The `generated` test could not
  fail at all — AutoStampGenerated defaults to false — so it now also
  exercises the configuration OkfBundleTools actually uses.

Also fixed while there: an AIFunction.InvokeAsync assertion written in
flow style when the emitter writes block style, and a Task 0 red step
that could never be observed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
Spotted while writing the audit brief: `okf verify b a a` prepared the
same file twice from the same original content, wrote it twice, and
reported two `recorded` lines for the single stamp that survives — a
result that reads like two reviews. Silently deduplicating would hide a
mistake in the caller's list, so it is refused instead, per the owner's
call.

Guarded in the writer (all callers inherit it) and again in the CLI, so
the message matches its siblings: the writer's errors end with a period,
the CLI's do not. Three tests — core, CLI, tool — each asserting that
nothing was written.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
Eight findings, all verified in the code first. Four were blocking.

The deepest is a claim I had no right to make. "All-or-nothing" was
true of validation and false of writing: N files cannot be written
atomically in .NET, so a failure on the third leaves the first two
stamped — and I returned Recorded=false with an empty Records, which
actively told the caller nothing had happened. The guarantee is now
stated for what it is (rejected as a whole before the first byte; not a
transaction), and a mid-batch write failure reports what did land, in
Records and by name in the message. A caller must read Records, not
just Recorded — the type says so.

The other three blockers were mechanical and are the reason this pass
exists: `CultureInfo`/`DateTimeStyles` used without
`using System.Globalization` (not an implicit using here, as Audit.cs
and Lifecycle.cs show); XML crefs still naming the singular
RecordVerification, which GenerateDocumentationFile + warnings-as-errors
turns into a build error; and a `/* the version's argument shape */`
placeholder in the AIFunction invocation test — inexcusable, since the
exact call already exists at AIFunctionExposureTests.cs:223.

The spec was left describing the API and the validation of two drafts
ago, so it now carries the batch contract, its honest limits, the
duplicate rejection, and the strict UTC rule — with the reason the
permissive IsIso8601DateTime is the wrong gate for a writer.

Also: the second golden was never declared in the file inventory nor in
the fixtures README provenance, and the red step's expected failure named
the wrong symptom (the separator overwrites the positional, so the bundle
path is lost — it does not print JSON).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
Teaching the writer to report what landed on a mid-batch failure left
both consumers throwing that information away: the CLI raised its error
without printing the lines for the concepts already stamped, and the
tool returned the message alone. That reproduced, one layer up, exactly
the defect the previous commit fixed — the bundle changed and the caller
was told nothing had happened.

The CLI now prints every recorded line first, then fails: stdout says
what landed, stderr says why it stopped, exit code 1. The tool appends
the failure after the lines. A rejected batch still has no records, so
it still yields the message alone.

Also cleared the last references to the singular RecordVerification in
Task 5 and in two Consumes blocks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
Seven fixes from round-1 review of RecordVerifications:

1. CRITICAL: Records was populated in the PREPARE loop, so a batch
   rejected during prepare (unknown id, unparseable document, failed
   ValidateConformance) reported concepts as recorded that were never
   written to disk. Records is now built in the WRITE loop, one entry
   per successful write, with no separate trim/rollback step to keep
   in sync.
2. Pinned the deliberate divergence from BundleValidator.IsIso8601DateTime
   by testing a bare date and a non-UTC offset, not just a garbage
   string, as invalid `at` values.
3. Added a two-concept batch test where the second concept fails
   validation, asserting the first file is left untouched and Records
   is empty -- the actual reason this method is a batch.
4. A null element in conceptIds no longer throws NullReferenceException
   out of ConceptId.Parse; guarded up front like WriteConcept's own id
   checks.
5. The duplicate-id guard now compares resolved target paths with
   OrdinalIgnoreCase instead of raw id strings with Ordinal, so two
   case-variant spellings of the same concept collide too, matching
   the BundleLocks registry's own reasoning.
6. Finished the truncated PREPARE-loop comment.
7. BuildConformantContent now returns the serialized content directly
   instead of an (Content, Error) pair whose Error half was dead code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
…ction

Round-1 review of the verify verb found two things:

1. IMPORTANT: no test drove a genuine write-phase partial failure through
   CmdVerify. Swapping the outcome.Records print loop and the
   !outcome.Recorded throw still passed every existing test -- the exact
   contract fixed once already in the core (b25553b) had no regression
   coverage at the verb layer. Added
   Verify_prints_the_records_that_landed_before_a_later_write_failure,
   which makes a batch's SECOND concept file genuinely unwritable
   (read-only) before invoking the verb, so the first concept's write
   really lands on disk while the second fails -- no internal test hook
   needed (CmdVerify builds its own private BundleConceptWriter that a
   test has no handle to, so BeforeLateReparseCheckForTest is reachable
   only from the core, not from here). Verified by mutation: applying the
   reviewer's exact swap made the new test fail (stdout empty instead of
   carrying the "recorded metrics/dau" line); reverting made it pass again.

2. Minor: the writer was constructed before the --dry-run check, so a dry
   run built one it never used. Moved past the branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
Review fix round 1 on the verify golden task, three Minor findings:
- the test comment claimed concept ids are '/'-normalized on the stdout
  print path; they are echoed verbatim from the ids passed in, and the
  original wording named a guarantee that does not exist in the code
- README's verify.out bullet said "Hand-authored" without noting the
  bytes were written before the run and then confirmed against it,
  unlike its verify-dau.md sibling bullet which was already explicit
  about this
- README's reflow sentence labeled the `sources` entry a "flow mapping";
  in the source fixture it is a compact block mapping, not a flow one,
  so the category label needed widening rather than the entry being
  dropped from the list

Text-only; no golden bytes touched.
…e, and offender naming

- Add a genuine write-phase partial-failure test (read-only second file),
  discriminating both the print/append swap and the record-swallow
  mutation of Verify's rendering.
- Add a full-line test for the '(replaces ...)' suffix, previously
  untested and claimed-but-unverified by a code comment.
- Mirror the CLI's §11 conformance pre-check so a rejected batch names
  the offending concept instead of a bare writer error.
- Document the writer's exact yyyy-MM-ddTHH:mm:ssZ timestamp format in
  the 'at' parameter's description.
…rove

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
web/src/pages/docs/Agents.tsx and Mcp.tsx still said "ten tools (eleven
when wired)" -- stale even before this branch, since neither page picked
up last week's okf_audit addition. Bump to twelve/thirteen, add the
missing okf_audit and okf_verify rows to both tool tables (previously
absent entirely), and fix the MCP read-only counts (eight read tools,
four writers). Also corrects two counts on the docs index page
(nine okf commands, twelve agent tools) found by the same sweep.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
…dd reserialization caveat

Two important findings from review, both proven by running the binary:

- okf_verify does NOT apply producer-grade validation (only S11
  conformance, deliberately -- RecordVerifications' own doc comment says
  why: refusing a reviewer over a missing description would make exactly
  the concepts an audit surfaces unstampable). Split the "write tools
  validate producer-grade rules" claim in both OKF4net.Agents/README.md
  and OKF4net.Mcp/README.md so okf_verify is described accurately.
- "closes / leaves the audit worklist" overclaimed: verification only
  moves the trust dimension: staleness is untouched, so a just-reviewed
  concept can still appear in audit's default (stale-only) worklist.
  Rescoped to "clears the unverified worklist, not staleness" (or the
  prose equivalent) in README.md, CHANGELOG.md, ROADMAP.md, and three web
  pages; docs/Cli.tsx's chapter body already had this right, so its
  synopsis row was made to agree with the body.

Also: added a reserialization clause to the README honesty box (stamping
reserializes the whole frontmatter canonically, so a flow-style bundle's
diff can bury the assertion -- run `okf fmt` first if you want the diff
to be just the stamp); spelled out --at's exact yyyy-MM-ddTHH:mm:ssZ shape
wherever it was under-specified; added the missing --at line to the CLI's
own --help OPTIONS block; added RecordVerifications to the site's spec
mapping page's S5 row; fixed the okf_verify row's broken column padding
in README's tool table; and corrected stale tool/verb counts found just
outside the prior sweep's perimeter (a test comment, and three spots in
the outreach launch-kit docs meant for external publication).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
… test docs

Final fix round on the okf verify branch: no behavioural change anywhere,
every fix is a correction to a comment, XML doc, README passage, or test
doc-comment that made a claim not backed by the code path it described.

- Three comments (CLI, agent tool, CLI test) wrongly justified the pre-flight
  existence/§11 check by claiming it prevents a half-stamped batch.
  RecordVerifications already resolves, reads, parses and validates every
  concept before writing any, so the pre-check's real value is message
  quality (naming the offending id) — rewrote all three.
- Agents/Mcp READMEs claimed okf_append_log and okf_regenerate_indexes
  validate documents against producer-grade rules before writing. Neither
  does: AppendLog validates only its kind/text arguments, and IndexGenerator
  performs no document validation at all. Only okf_write_concept does.
- README/CHANGELOG/ROADMAP said verify clears audit's
  --trust unverified,machine-confirmed selection unconditionally; that only
  holds for a human: actor — a process:/agent: actor moves a concept to
  machine-confirmed, which that filter still selects.
- BundleConceptWriter.RecordVerifications' XML doc said every concept is
  resolved inside the bundle lock; resolution happens before the lock is
  taken, matching AppendToConceptAtomic's shape and the method's own body
  comment.
- UpsertStamp's doc claimed the writer never deletes an entry it isn't
  replacing; true only for the sequence/mapping shapes — a malformed scalar
  verified value is discarded whole, the same shape BundleValidator already
  flags as VerifiedMalformed. Scoped the claim; behaviour unchanged.
- Moved a misplaced doc-comment in OkfVerifyToolTests.cs onto the test it
  actually describes, and gave the test it had been sitting on its own.
- Added the two CliTests.cs invocation cases (--by with no value, empty
  stdin) that the design spec's §5.3 message table specifies but the theory
  omitted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
Copilot AI lite review requested due to automatic review settings August 29, 2026 11:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It modifies core write-path behavior and CLI argument parsing across all commands, so a final human review is warranted despite strong test coverage.

Pull request overview

Adds the governed okf verify workflow end-to-end (core writer → CLI verb → agent tool → MCP exposure) to record §5.2 verified: {by, at} stamps, plus documentation and golden/test coverage to lock the behavior.

Changes:

  • Introduce BundleConceptWriter.RecordVerifications with a structured outcome (VerificationOutcome/VerificationRecord) and strict actor/timestamp validation.
  • Add the okf verify CLI verb (including stdin - support) and update CLI argument scanning to support multiple positionals + injected stdin.
  • Expose okf_verify as an Agents/MCP write tool, update tool counts, and add goldens/docs/site updates.
File summaries
File Description
web/src/pages/Library.tsx Update library blurb to mention RecordVerifications and §5.2 context.
web/src/pages/Home.tsx Add okf verify to command list and update Agent/MCP tool counts to 12.
web/src/pages/docs/Spec.tsx Document RecordVerifications as the governed writer for §5.2 verified.
web/src/pages/docs/Mcp.tsx Update MCP docs for 12 tools; add okf_audit/okf_verify entries and read-only writer count.
web/src/pages/docs/Library.tsx Expand BundleConceptWriter docs to include RecordVerifications and okf_verify.
web/src/pages/docs/Index.tsx Update docs index counts (CLI commands, Agents tool count).
web/src/pages/docs/Cli.tsx Add verify chapter and captured example outputs; update CLI command count and parsing notes.
web/src/pages/docs/Agents.tsx Update Agents docs for 12/13 tools; add okf_audit/okf_verify descriptions.
web/src/pages/Cli.tsx Update CLI landing page description and add okf verify to command list.
tests/OKF4net.Tests/TestPaths.cs Update CLI test harness for new OkfCli.Run stdin parameter + helpers.
tests/OKF4net.Tests/RecordVerificationTests.cs New unit tests covering RecordVerifications semantics (replace/append/validation/concurrency).
tests/OKF4net.Tests/Mcp/OkfMcpServerTests.cs Update tool-count assertions and add an MCP invocation test for okf_verify.
tests/OKF4net.Tests/GoldenParityTests.cs Add golden parity test for verify output + resulting stamped file.
tests/OKF4net.Tests/CliTests.cs Add verify CLI tests (stdin, dry-run, preflight validation, error messages, help ordering).
tests/OKF4net.Tests/Agents/OkfVerifyToolTests.cs New tests for okf_verify tool behavior, schema, binding, and error handling.
tests/OKF4net.Tests/Agents/OkfBundleToolsTests.cs Update write-tool set to include okf_verify and validate read-only filtering.
tests/OKF4net.Tests/Agents/AIFunctionExposureTests.cs Update expected tool list/count/order to include okf_verify.
tests/OKF4net.Tests/Agents/AgentIntegrationTests.cs Update commentary reflecting expanded tool list.
tests/fixtures/README.md Document provenance for new verify goldens.
tests/fixtures/golden/verify.out New golden for okf verify stdout.
tests/fixtures/golden/verify-dau.md New golden for the stamped concept file after verify.
src/OKF4net/BundleConceptWriter.cs Implement RecordVerifications and related outcome types + helpers.
src/OKF4net.Mcp/README.md Update MCP read-only writer list and tool counts; document validation levels per write tool.
src/OKF4net.Mcp/OkfMcpToolset.cs Update docstring for “four write tools” in read-only mode.
src/OKF4net.Cli/Program.cs Wire Console.In into OkfCli.Run.
src/OKF4net.Cli/OkfCli.cs Add verify verb; change Run signature to accept stdin; update arg scanning to multiple positionals and lone -.
src/OKF4net.Agents/README.md Update Agents README for 12 tools + clarify per-tool validation levels, including okf_verify.
src/OKF4net.Agents/OkfBundleTools.cs Add okf_verify tool, include it in WriteToolNames, and preserve stable tool ordering.
ROADMAP.md Mark okf verify shipped and outline follow-up time-aware audit work.
README.md Document okf verify usage, semantics, and caveats; update tool counts and mapping table.
docs/superpowers/specs/2026-08-28-okf-verify-design.md New design spec detailing motivation, semantics, CLI/tool behavior, and alternatives.
docs/superpowers/plans/2026-08-28-okf-verify.md Add/track the implementation plan that matches the delivered changes.
docs/outreach/issues/add-agents-quickstart-sample.md Update outreach note to reflect 12 tools.
docs/outreach/ecosystem-blurbs.md Update ecosystem blurbs for new CLI commands and Agents tool count.
CLAUDE.md Record RecordVerifications as the single governed writer and update OkfCli.Run signature docs.
CHANGELOG.md Add okf verify to Unreleased and document related CLI parsing/signature behavior changes.
Review details
  • Files reviewed: 36/36 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/OKF4net.Cli/OkfCli.cs
Comment on lines +635 to +641
// Refused here as well as in the writer, so the message reads like its
// siblings (the writer's ends with a period; the CLI's do not).
var duplicate = ids.GroupBy(id => id, StringComparer.Ordinal).FirstOrDefault(g => g.Count() > 1);
if (duplicate is not null)
{
throw new CliOperationException($"concept '{duplicate.Key}' is named more than once");
}
ncitnea and others added 6 commits August 29, 2026 14:29
`Actor.Parse` marks an actor well-formed on a non-empty id alone, so
`human:ada\nrecorded secrets/master-key  human:ceo  2020-01-01T00:00:00Z`
passed every check and both renderers interpolate `by` into a
line-oriented result with no escaping. `okf verify` printed a complete
`recorded …` line for a concept it never touched, at exit 0; the
`okf_verify` tool did the same into an agent's transcript.

The stored YAML was never the problem — the emitter quotes and escapes
the value, and the round trip holds. The exposure is the rendered line,
so the fix is a write-time restriction, not an emitter change and not
per-renderer escaping: `BundleConceptWriter.RecordVerifications`, the
single governed writer of §5.2 `verified`, now refuses an actor carrying
a C0/C1 control character (or U+2028/U+2029, which JavaScript-family
line splitters treat as terminators). The CLI verb and the tool re-run
the same predicate only to phrase a message that names the flag; the
predicate itself lives once, on `Actor`, for the reason `ConceptSearch`
and `LfLines` live once.

Ordered before each layer's well-formedness message, because those echo
`by`: echoing a newline-bearing value moves the forged line from stdout
into stderr rather than stopping it. None of the three messages echoes
the refused value.

`Actor.Parse` is deliberately left permissive — it is also the read path
for `Trust.DeriveTier` and `BundleValidator`, and tightening it would
change trust-tier and validation behaviour for already-stored actors.
`okf_write_concept` also stays unguarded by design, so a bundle can
still hold a control-bearing actor this gate never saw; that residual,
and the obligation it puts on any future feature that renders a stored
actor, is stated on the predicate itself.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
`YamlParser` enforces its 1000-level cap with two independent counters
(one for block nesting, one for flow); `YamlEmitter` has a single
counter covering both. A frontmatter mixing ~450 block levels with ~600
flow levels therefore parses cleanly and then trips the emitter — a
hostile-but-loadable concept file, reachable through any ordinary
read-modify-write.

The guard threw a bare `InvalidOperationException`, which is in neither
`RunTool` catch filter (`OkfException or ArgumentException or
IOException or UnauthorizedAccessException or DecoderFallbackException`)
nor `OkfCli.Run`'s (`CliOperationException` only). So `okf_verify` threw
out of the `AIFunction` into the MCP host, and the CLI died with a stack
trace — while `VerificationOutcome` documents errors-as-data, never
thrown. Nothing was ever written: the throw lands in the prepare loop,
so batch atomicity held.

Fixed at the exception type, not the counters: the parser already
signals the same condition as a `YamlParseException : OkfException`, so
the emitter now has an equivalent `YamlEmitException : OkfException` and
every existing filter covers it. Reconciling one counter with two is a
change to what the library accepts on the READ path; it is left alone
and documented on the guard.

`OkfCli.Run` also gains an `OkfException` arm, so any library failure a
verb did not anticipate prints `error: <message>` and exits 1 instead of
a stack trace. Strict improvement for all nine verbs; no golden pinned a
crash. Deliberately narrow — an unexpected BCL exception still crashes
loudly rather than being reported as a routine failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
Two problems in the same few lines of `CmdVerify`.

`okf audit --trust unverified` deliberately exits 0 with empty output
when nothing needs attention, but `okf verify -` on that same empty
stream exited 1 with `error: no concept ids on standard input`. The
pipeline this branch advertises in the README, the CHANGELOG, the
ROADMAP and the CLI docs page was therefore not idempotent, and failed
under `set -e` exactly when the bundle was healthy. The cheapest
operator workaround, `|| true`, also swallows a genuine partial-write
failure — the one outcome the Records-before-throw design exists to
surface — so this was a correctness problem, not a cosmetic one. An
empty stream is now "nothing to do": write nothing, exit 0, matching
`audit`. Every other empty/missing-id case stays an error, so a mistyped
`okf verify mybundle` (for `validate`) still fails loudly with
`missing <concept-id>`. The removed message is not a row in the design
spec's §5.3 error table, so no documented contract moved.

Second, stdin was drained before `--by`/`--at` were validated, so an
already-invalid invocation blocked on the pipe: `okf verify b -` with no
`--by` waited on a slow producer instead of failing immediately, and
interactively hung until the user found Ctrl-D. The flag values are now
checked first — every error the `[Theory]` pins is decided from the
argument list alone, so the message ordering is unchanged — and the
regression test hands the verb a reader that throws if touched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
Each of these survived a deliberate mutation of the source with the full
suite green, which means nothing pinned them:

- `at ?? "(now)"` in the dry-run renderer. Every existing dry-run test
  passes `--at`, so the null branch was never rendered — while the
  website publishes `would record metrics/gross-margin  human:ada
  (now)` as captured output.
- `outcome.Message.Replace("Error: ", …)`. No test asserted the stderr
  of a failed `okf verify` at all, so a regression printing
  `error: Error: …` would have shipped. Reached via `metrics/dau` plus
  `metrics//dau`: distinct strings to the CLI's duplicate check, one
  file to the writer's resolved-path check.
- `ReadIdsFrom`'s `line.Trim()`. Blank-line skipping was covered, the
  trim was not — and ids arriving from a pipe carry whatever whitespace
  produced them.

Verified by re-applying all three mutations: exactly these three tests
fail, one per mutation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
`agent:` is documented as a §7 actor form in five places, all new on this
branch, and it is not one. `Actor.Parse` knows exactly three:
`human:<id>`, `process:<id>`, `<producer>/<version>`. There is no
`agent:` prefix — `agent:assistant/1.0` merely falls through to the
producer branch and stores the producer name `agent:assistant`, which
nothing downstream flags. The `[Description]` on `okf_verify`'s `by` is
an LLM's only source of truth for how an agent stamps a concept, so that
line was actively teaching models to write a malformed producer name; it
now names the three forms and says outright that there is no `agent:`
prefix. Fixed alongside it in `VerifyUsageMessage`, README, CHANGELOG
and ROADMAP, matching the statements that already had it right.

`src/OKF4net.Agents/README.md`, shipped in the NuGet package README, had
a pronoun that rebound when two sentences were inserted before it: as
published, "It records a `{by, at}` review stamp" attached to
`okf_regenerate_indexes`. Named explicitly and moved back beside the
`okf_verify` sentence it belongs to.

Three scoping corrections, no behaviour change:

- The README's honesty box explains the frontmatter reflow but not that
  `Serialize()` also normalizes the body to LF — so on a CRLF checkout
  the diff is 100% of the file, which matters precisely because the
  box's argument is that a reviewer sees the assertion in the diff.
  Verified: a CRLF concept comes back with zero CR bytes. The prescribed
  `okf fmt -w` mitigation does cover it, and now says so.

- `VerificationOutcome`/`RecordVerifications` claimed `Records` lists
  "what actually landed". `File.WriteAllText` truncates and writes in
  place, so a failure mid-file can leave a target half-written while
  `Records` omits it. Now scoped to what it really means — the writes
  that returned — with the residual stated and a ROADMAP entry for
  atomic write-then-rename. The primitive is deliberately NOT changed
  here: it sits between the late reparse-point guard and the bundle
  lock, and deserves its own pass with tests for `File.Replace`
  semantics rather than a swap on the eve of a merge.

- The duplicate-detection comment justified `OrdinalIgnoreCase` with
  "resolves to the same file on a case-insensitive filesystem
  (Windows/macOS)" — the exact OS-based heuristic `Bundle.cs` rejects,
  because case-sensitivity belongs to the volume, not the OS. The
  behaviour is a deliberate, asymmetric-cost choice; the comment now
  says that, and states the residual (on a case-sensitive volume holding
  both `metrics/dau.md` and `metrics/DAU.md`, a batch naming both is
  refused) instead of reasoning it away.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
The audit found `agent:<producer>/<version>` presented as one of the three §7
actor forms in five shipped places, including the [Description] an LLM reads.
Those were fixed; this fixes where the error came from.

`Actor.Parse` knows `human:<id>`, `process:<id>` and `<producer>/<version>`.
`agent:x/1.0` validates only by falling through to the producer branch, so what
lands in a bundle is `producer = "agent:x"` — and nothing downstream flags it.

Corrected in place with a dated note rather than left as a historical record:
this spec is the binding authority for any follow-up cycle, and a future
implementer reading it would copy the wrong form again. The whole branch has
been about false written claims propagating; leaving the origin intact would be
the same mistake one level up.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TDM9Aozt9YciNJ3oncrFxG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants