feat(skills): adopt adapted planning and delivery cores #21
Loading…
Reference in a new issue
No description provided.
Delete branch "bryan/agent-skill-workflow-adoption"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Adopts a smaller, curated planning and delivery workflow across Hermes, Claude, and OpenCode, based partly on Matt Pocock’s MIT-licensed agent skills.
Companion learning-zone PR: bryan/second-brain#1
What changed
grilling,wayfinder,tdd,diagnosing-bugs,code-review,codebase-architecture, and Hermes-onlyguided-learningskills.git-masterandagent-workspaceroutes with focused owners and updates affected workflows.adr-and-spec-coachduring the pilot for side-by-side evaluation.Wayfinder state model
Forgejo does not provide compare-and-swap semantics for issue-body updates. To avoid overwriting concurrent human edits, Wayfinder never rewrites an existing map or ticket body.
Claims, resolutions, and decision-index entries are exact-schema append-only comments. The current decision index is derived deterministically from those records. Corrected decisions require reopening and re-claiming the ticket; the newer decision supersedes the prior entry while preserving history.
Verification
main.Current
mainhas one unrelated stale source-text assertion in the SGG morning-brief suite; the same failure reproduces onmainwithout this PR.Known boundary
No real Forgejo mutation smoke has run yet. The adapter is covered by the offline fake and read-only live checks, but the planned disposable-private-tracker smoke remains part of post-merge activation.
Activation
Merging this PR does not activate the new workflows. Skill reconciliation, cron creation, runtime reloads, and the first Wayfinder pilot remain separately gated.
One suite asserting the accepted target rather than the prose that describes it: frontmatter discoverable by every runtime, ledger completeness with every watched file mapped to a declared upstream source and every local path resolving, curation matching the intended per-runtime roles (Pi receives none of this; `guided-learning` is Hermes-only), retired routes absent from every active surface, the three review lanes and their artifacts, the no-third-pass invariant, guided learning's refusal to write into its own installed directory, and a monitor that carries no timestamp. Two checks are scoped deliberately rather than broadly: - Link resolution covers only the skills this plan owns. Legacy skills carry illustrative markdown in their examples (`](Note.md)`, `](target)`) that is documentation, not a link to follow. - The retired-route guard matches per paragraph, not per line, because the clause that makes a mention historical ("the workflow it replaces") routinely wraps onto the next line. Failure messages carry the missing wording rather than dumping whole skill bodies — a red run here should be readable.feat(skills): adopt adapted planning and delivery coresto WIP: feat(skills): adopt adapted planning and delivery coresRepairs all 25 validated items from `.hermes/review/independent-correction-pass-1.md`. Several were real defects that would have failed on first live use, and one was a mistake I made in the previous pass. ## Forgejo Wayfinder adapter - **`CreateIssueOption.labels` is `array<int64>`, not names.** Confirmed in the live Swagger. Every issue this adapter created would have lost its labels — and a map without `wayfinder:map` is not a map. Labels are now resolved to numeric ids from deterministic definitions; the preview shows any label it would have to create, and apply reads each one back. - **Every mutation now preflights**, immediately before the write: privacy, the issue number the API actually returned, the required label, the metadata version and type, and the map association. A body marker alone is not identity — anyone can paste one into an unrelated issue. Every mutation family has a public-repository refusal test. - **Claims are arbitrated by comment id, not by body overwrite.** `updated_at` is *set by* the write on this API, so it cannot resolve a race; two contenders could both read back their own claim and both proceed. Claim and release are now append-only comments, replayed in Forgejo's monotonic id order, and the earliest unreleased claim wins. A deterministic interleaving test proves exactly one contender can win. - **Only the current claimant may resolve.** `--session` is required. - **Resolution is resumable.** A deterministic key lets a retry see which of comment / close / map-index already landed and finish only the rest; fault-injection tests after each step prove convergence on one comment, one closed ticket, one map entry. `append_decision` no longer glues the entry onto the heading — the old substring readback hid that. - **Token transport is opt-in and host-pinned.** `auto` always uses Tea and never upgrades because an ambient `FORGEJO_TOKEN` exists; direct token mode requires an allowlisted host, refuses every redirect (urllib would replay the `Authorization` header), and validates the request path so it cannot be steered off the pin. ## Skill topology - **`spike` and `sketch` do exist** — as builtin Hermes skills. My previous pass searched `~/.claude`, the pool, and OpenCode, but not the Hermes builtin pool, and wrongly declared them absent. The approved named routes are restored, with per-runtime availability stated and the inline procedure kept only as the Claude/OpenCode fallback. - **Retired links now prune on every platform.** `Cwd::abs_path` returns *empty* for a dangling link whose parent is missing, which classified the old `agent-workspace`/`git-master` links as foreign and preserved them forever. An exact two-target allowlist keyed on the raw `readlink` output fixes it; every other broken or foreign link still survives. - OpenCode agents write scratch state to `{TRUNK_ROOT}/.hermes/agents/…` instead of a relative `.notes/.agents/`, which forked per worktree. - `find-skills` authors into the canonical pool with provenance and curation, and verifies through the reconciler — never an unmanaged `~/.claude/skills` copy. - `issue-work`'s description no longer advertises the retired lens names; duplicate `worktrunk` rows merged; the morning-brief contract is scoped to the jobs it owns, with the watcher's model pin in the watcher tests. ## Review behavior - The **AC sweep builds its checklist from every intent source** — plan, issue task list *and* prose criteria, spec, explicit PR criteria — and splits compound criteria. An issue with no `- [ ]` lines is not an issue with no acceptance criteria. - **Correction passes count once per committed boundary.** A delegated batch previously spent a pass on dispatch, burning the conditional final pass before the first correction had been reviewed. - **Risk selection reads the diff, not just paths.** Content signals cover every named risk area, so `src/parser.py` calling `json.loads` selects Risk; unrecognized security-adjacent content fails closed. A prose-only signal is marked as such, so "discusses authentication" and "changes authentication" stay distinguishable. - The **Qwen worker's isolated home** (`~/.hermes/local-qwen-worker`) has no `skills/`, so reconciling the normal home could never supply `tdd`/`diagnosing-bugs` — Hermes does not error on an unresolvable `--skills` name, so it would have run without them and reported success. It now gets the repository pool via `skills.external_dirs`, verified to resolve in that exact home. ## Watcher - **Identity is the per-file Git blob sha**, with no repository tip, so an unrelated upstream commit produces byte-identical output. - **One recursive Trees request, not one Contents request per file.** The live run caught this: 17 watched files exhausted GitHub's 60/hour unauthenticated per-IP limit on the *second* invocation. A truncated tree is refused rather than read as "unchanged". - **Installed as a copy.** The scheduler resolves a cron script path and requires containment in `HERMES_HOME/scripts`; `.resolve()` follows symlinks, so the symlinked install would have been blocked at fire time. Proven through the real installer and the real resolver. - **Read-only tools**: `["web", "no_mcp"]`, not file/terminal. The bounded adaptation context the assessment needs — local paths, local changes, rejected rules — now travels in the monitor output. Verified: 279 python tests OK; reconciler 53, hindsight 4, shell-additions 22, all 0 failures; isolated Hermes install rc=0 with the monitor as a regular file; reconciler --check 7 creates / 0 prunes; git diff --check clean. Live read-only: private accepted, public refused, preview with zero writes.Repairs all 12 validated blockers from `.hermes/review/conditional-final-correction-pass-2.md`. This is the plan's one conditional final correction pass; no third is authorized. ## Wayfinder **Concurrent resolutions could lose a map decision.** Two tickets can be legitimately claimed at once, so two resolutions can append to the map at the same moment — and a post-write readback verifies what just landed while saying nothing about the write arriving a second later with a stale body. Map writes are now serialized behind append-only `maplock`/`mapunlock` records ordered by comment id: acquire, preflight under the lock, read the body fresh, merge, verify, release. A losing lock request withdraws itself; the lock is handed back even when the write fails; a crash-held lock is surfaced with the exact recovery command rather than stolen. **A losing claim became a zombie.** A contender that posted a claim and then lost left the record queued, so the winner's release silently transferred the ticket to a session that had already walked away. Losers now withdraw their own claim and say so — including when the withdrawal itself fails. The test that asserted the old transfer behaviour is gone, replaced by one asserting the ticket ends up unclaimed. **Preflight moved from entry to every write.** Privacy, the issue number the API actually returned, labels, metadata version and type, map association, and the active claim are revalidated immediately before each comment, close, lock action, and map patch. Fault injection that flips a repository public or strips a label between steps now stops before the next write, and partial state stays retryable. **Resolution markers are parsed structurally.** The old substring check could be satisfied by answer prose containing `key: …`, and `search` read only the first block in a comment. Now every block is iterated and a marker counts only on exact kind, version, key, map, and session. Also: the map index entry is validated as a single safe line before preview or mutation (empty, multiline, heading, bare-bullet, and marker-shaped inputs produce zero writes); `read_map` asserts the returned issue number matches the requested one; and record fields are refused if they contain a newline or `-->`. ## Review workflow **Deletions selected no lane.** `git diff` writes `+++ /dev/null` for a removed file, so deleting `src/auth/session.py` produced an empty path list and no Risk lane. Both headers are parsed now, renames report both sides, and a modified file is not double-counted. **Reaching the correction bound is no longer an exit.** The second correction's code had never been reviewed. A terminal `final_review_only` pass now runs once — re-selecting lanes against the moved HEAD, applying nothing — and only then does the run end clean or bound. Identical on native and delegated paths. **The AC checklist is an artifact.** `intent-checklist.json` is built once from the plan, the issue, the spec, and explicit PR criteria, and both the sweep and `summary.md` read that file rather than drifting back to the `closes` task list. Prose criteria are captured at ingest while the full body is in hand, never mined from the 400-char excerpt. An unreadable authority is recorded `unswept`, not absent. **Untracked files block the candidate.** A nonignored untracked file is invisible to `{base}...HEAD`, so reviewing around it reports a clean candidate over code nobody looked at. ## Qwen worker **File presence is not resolution.** Hermes scans the worker home's own `skills/` before the external pool and takes the first match, and it does not error on a shadowed or unresolvable `--skills` name — so a stale `worker-home/skills/tdd` would silently win and the worker would run with the wrong skill, reporting success. Resolution is now verified through Hermes's real discovery modules in the exact worker home, requiring exactly one candidate per name equal to the canonical pool file, before launch. Proven live, including a real synthetic shadow. Verified: 337 + 6 Python tests OK; reconciler 53, hindsight 4, shell-additions 22, all 0 failures; isolated Hermes install rc=0 with the monitor a regular file; real Qwen isolated-home resolution to the canonical pool; watcher byte-identical across runs; reconciler --check 19 creates / 0 prunes; git diff --check clean.Applies `.hermes/review/additional-correction-cycle-1.md` — the first of two additionally authorized correction cycles. ## Review lane selection reads Git name-status `select_review_lanes.py` inferred every touched path from unified `---`/`+++` headers. A content-identical rename emits `rename from` / `rename to` and shows `--- /dev/null`, so moving `src/auth/session.py` somewhere harmless looked like an add of the destination and nothing else; a binary deletion emits no `---`/`+++` headers at all and was invisible. Path identity now comes from `git diff --name-status -z {base}...HEAD`, parsed NUL-separated with two paths for `R`/`C`, and the unified diff is demoted to a content signal. Both sides of a rename or copy are classified, a delete classifies the deleted path, and neither is double-counted. `code-review` and `pr-self-review` require callers to write both inputs to files; a diff-only call still works but records the weaker path authority in `selection.notes` rather than passing silently. ## A session is not a lock token Claims and map locks arbitrated on session identity. One session legitimately runs nested or concurrent operations, so a release matched on session alone cleared whichever claim that session happened to hold — including a sibling operation's. That is a lost lock inflicted from the inside, which no amount of cross-session care prevents. Every acquisition now mints a 128-bit `secrets.token_hex` **operation id** and every `claim`, `release`, `maplock`, and `mapunlock` record carries it (locks also carry their ticket). Arbitration replays the comment log keyed on the operation, so: - a release clears exactly the acquisition it names; - a second same-session acquisition is a genuine contender, not a no-op, so one session cannot hold two overlapping critical sections; - only an identical operation id folds into an existing hold, which is the one idempotent case; - a loser withdraws *its own* operation and verifies that operation is inactive — not merely that someone else is winning, which is exactly the zombie the withdrawal exists to prevent; - crash recovery names the exact held operation. `claim-status` and `map-lock` print it along with the verbatim recovery command. `--operation` is required on `release` and `resolve`, optional on `claim`. There is no session-only form. ## Exact managed-record schemas Arbitration records went through a permissive `key: value` scrape, so a truncated write or a hand-edited comment could take a lock. Each kind now has an exact v1 schema, and a block failing it in any way — malformed line, duplicate key, unknown key, missing key, empty value, wrong version, out-of-pattern value — is discarded whole and can neither arbitrate nor satisfy readback or idempotency. Rendering shares the same schemas, so the adapter cannot emit a record its own reader would reject. ## One guarded write path Preflights were spelled out around each call site, which is how one call site quietly ends up missing one. Every external write now goes through `guarded_write(WriteGuard(...), write, verify)`: re-read privacy, re-read and verify the exact map/ticket issue number, require labels and exact metadata, require the exact claim or lock operation where applicable, write once, read the exact result back. This covers label creation, map and ticket creation, dependency wiring, the claim comment, the assignee patch, losing-claim withdrawal, release, the resolution comment, the close, the lock request, losing-lock withdrawal, the body patch, both unlocks, and explicit recovery — a write that cannot name its guard has nowhere to go. A 2xx is not proof of storage. The fake tracker gained a `swallow` mode that acknowledges without persisting, and every family fails closed under it rather than reporting success. **Cleanup is not a licence to write.** If authority drifts mid-operation the `finally` unlock fails closed and the outcome carries `held_lock` plus the exact `release-map-lock` command, instead of writing to a tracker that just went public or a map that lost its label. A resolution reports `resolved` only when the decision is on the map *and* its own lock operation has been released. ## Tests 183 Wayfinder tests (was 125), including the named adversarial reproductions: nested and duplicate same-session claims, one session's locks for two tickets, a release that must not clear a sibling, an idempotent retry, a foreign-operation release, recovery by exact id, concurrent different sessions, every schema rejection case, identity and privacy drift immediately before each write family, seven swallowed-write families, a swallowed unlock that must not read as success, a swallowed losing-lock withdrawal that must not read as withdrawn, and cleanup refusing to write after drift. Five mutations of the new guarantees were each checked to fail the suite: session-keyed arbitration (3), permissive parsing (7), skipped readback (9), skipped privacy re-check (1), swallowed cleanup drift (3). Verified: 398 Python tests OK, 0 failures — discovered across all fourteen tracked suites, up from 319 at the candidate head293b597measured the same way; `test_sgg_pr_review_event.py` 6 OK; reconciler 53, hindsight 4, shell-additions 22, 0 failures; isolated Hermes install rc=0 with the monitor a regular file; real Qwen isolated-home resolution plus a live synthetic shadow; watcher byte-identical across runs; reconciler --check 19 creates / 0 prunes; `git diff --check origin/main...HEAD` clean. Not proven: no Wayfinder command has been run against a real tracker. That needs a disposable private repository and is outside this authorization.Applies `.hermes/review/additional-correction-cycle-2.md`, the final authorized edit cycle. The merge of current `origin/main` is the parent commit; this one carries the reviewed blockers. ## Privacy is the last read before every write `enforce()` checked privacy first and then issued three more reads — map identity, ticket identity, ownership. That leaves a window: the repository can be made public while the adapter is still deciding, and the mutation then lands on a tracker it approved a round trip ago. The order is inverted. Every identity, metadata, ownership, and dependency read completes first; `require_private()` is the final network round trip before the single write. The write closure now reads nothing — it works from the `GuardState` the guard captured — so it cannot reopen the window it was just closed for. `_patch_issue`, which did GET-then-PATCH inside the closure, is gone; read preparation and the direct `PATCH`/`POST` are separate, and the staleness guard it provided is subsumed by the guard's own strictly-fresher read. The tests flip the repository public *during* the final read for every write family, and assert no mutation follows. One of them states the property structurally: for every POST and PATCH in the request log, the immediately preceding request must be the privacy check. ## An operation id is a handle, not a credential Ownership was keyed on the operation id alone. But every id this adapter mints is written into a tracker comment, so anything that can read the issue can quote it back. Ownership is now a full identity tuple: a claim is `(operation, session)`, a map lock is `(operation, session, ticket)`. The ticket matters because every lock record lives on the one map issue. Without it, a request naming ticket 999 matches — and inherits — a lock taken for ticket 4. `acquisition_identity`, `active_holder`, `acquisition_is_active`, the idempotent retries, the resolve guards, normal release, loser withdrawal, and recovery all enforce the full tuple. `acquire_map_lock` validates the whole request — map identity, ticket identity, and the claim that authorizes locking on its behalf — *before* considering the idempotent fast path: a retry is only a retry if it is a retry of this exact critical section. Reproduced as tests: a foreign session with the exposed operation cannot release, resolve, or inherit a claim as a retry; a forged release or mapunlock under the wrong session or ticket clears nothing; ticket 999 cannot ride ticket 4's lock; a nonexistent ticket cannot lock at all; same-session siblings stay independently owned; and a lock retry refuses once the ticket loses its label, moves map, loses its claim, or the tracker goes public. ## Queued acquisitions are recoverable by name A failed withdrawal does not leave a stuck lock — it leaves a **queued** one, sitting behind the winner and becoming the holder the moment the winner releases. Recovery that only reached the current holder could not touch it, so the printed recovery command was advice the operator could not follow. `release` and `release-map-lock` now locate the exact schema-valid acquisition whether it holds or is queued, verify its session, ticket, and map scope, post the matching drop, and then verify both that this acquisition is inactive and that the current holder is unchanged. `claim-status` and `map-lock` list `queued` alongside `holder`, each with its verbatim command, because recovery cannot target what inspection does not show. A swallowed recovery write fails and leaves the state explicit. ## Exact identity, exact values, exact readback **Maps and tickets.** A map needs exactly one balanced current-version managed region: zero is not a map, and two is worse — a decision would be merged into one while a reader sees the other. A ticket needs both `wayfinder:ticket` and the matching `wayfinder:{type}` label agreeing with exact `map`/`type` metadata; a label contradicting the body is two sources of truth, and this adapter refuses to pick one. **Values.** Ticket and map numbers are positive integers — Forgejo numbers issues from 1, so zero was a missing value dressed as one, and the synthetic `ticket: 0` fallback is gone. Timestamps are parsed to a real UTC instant, not regex-matched: `2026-99-99T99:99:99Z` satisfied the old check. Blank lines inside a managed block are rejected rather than skipped, because padding a reader tolerates is padding a forger can hide behind. Rendering and parsing share one validator set, and a test asserts they agree value by value. **Readback**, per family, measured against a pre-write snapshot: a created issue must be a number that did not exist before, with byte-exact title, body, and label set; a comment record must be a comment absent from the snapshot, byte-exact, with the id the API returned; an assignment must *equal* the requested set; a dependency must equal the snapshot plus the one requested edge; a map patch must equal the full body intended; a close must leave the ticket's identity intact. The snapshots are the point: without them an echoed existing issue, or the record an earlier attempt wrote, passes as proof of a write that never happened — and a swallowed retry reports either success or a zombie that is not there. ## Name-status is one literal, strictly parsed `git diff --name-status -z -M -C --find-copies-harder {base}...HEAD --`, quoted identically in `code-review`, `pr-self-review`, the script's help epilog, and the tests. `--find-copies-harder` is load-bearing: plain `-C` inspects only modified files, so copying a risky *untouched* source emits a bare `A` and the risky source is never classified. A test proves that negative control against real Git. Parsing rejects a suffixed status (`MM` is porcelain syntax), an impossible similarity score, a misaligned empty status field, truncation, an empty path, a control character, and bytes that are not valid UTF-8 — a path decoded with substitutions names a different file than Git meant. A guess here is a silently narrowed review. New tests run the literal command in a throwaway repository and classify what Git actually emits: a content-identical risky→neutral rename, a binary deletion (with the unified diff asserted to expose nothing), a copy from an untouched risky source, ordinary add/modify/delete, and a filename containing a quote, semicolon, and tab. ## Tests 500 Python tests, up from 398. Wayfinder 258 (was 183); lanes 87 (was 70); `test_reconcile_cron` 21 (was 16). Every new guarantee was mutation-checked. Reverting each of these fails the suite: identity back to operation-only (28 failures, 53 errors), privacy checked first (2), the creation pre-existing-issue snapshot (2), assignment readback as containment (1), the comment-snapshot filter (3), the exact-body record check (1), the returned-comment-id check (1), dependency readback as containment (1), blank lines tolerated (16), regex-only timestamps (10), zero as an issue number (5), the map managed-region check (8), the required type label (4), recovery reaching only the holder (4 failures, 2 errors), map-body readback as decision-presence (1), close readback without identity (1), and the lock fast path before validation (5). Preserved without regression: no lost concurrent decisions, safe index line, structural resolution marker, resumable comment/close/index, and success requiring both the exact decision body and the released lock tuple. Verified: 500 Python tests OK; `test_sgg_pr_review_event.py` 6 OK; reconciler 53, hindsight 4, shell-additions 22, 0 failures; isolated Hermes install rc=0 with both the monitor and the weekly-orientation script regular files; real Qwen isolated-home resolution plus a live synthetic shadow; watcher byte-identical across runs; reconciler --check 19 creates / 0 prunes; `git diff --check origin/main...HEAD` clean; `git merge-base --is-ancestor origin/main HEAD` true; `git merge-tree --write-tree origin/main HEAD` exits 0. Not proven: no Wayfinder command has been run against a real tracker. That needs a disposable private repository and is outside this authorization.WIP: feat(skills): adopt adapted planning and delivery coresto feat(skills): adopt adapted planning and delivery cores