diff --git a/features/skill-wiring.feature b/features/skill-wiring.feature index fe5b9f5..1c5a0bb 100644 --- a/features/skill-wiring.feature +++ b/features/skill-wiring.feature @@ -33,6 +33,26 @@ Feature: Skill wiring # Naming a state is allowed and often clearer; defining it elsewhere is the # copy that drifts. All four are checked, not just one. + Scenario: The repository branch sweep is defined once and deferred to + Given clock-in defines the repository-wide branch enumeration + When every other skill is searched for that command + Then none carries it and clock-out references skills/clock-in/SKILL.md + # clock-out needs the same whole-repository view at the end of a session as + # clock-in needs at the start. Copying the command into it is the drift this + # feature exists to catch; the command is read from clock-in so that editing + # it there moves the guard with it. + + Scenario: The branch enumeration covers every branch and reports divergence + Given the two forms of the branch enumeration clock-in defines + When they are inspected for what they produce + Then the checkout form resolves the base branch and counts commits in both directions + And the API form pages through the whole branch list + # The wiring scenario above keeps the command in one place. It does not keep + # it useful: reduced to a list of ref names, or left on the first API page, + # it would still pass while the skill's promise quietly disappeared. Both + # regressions report success while looking at part of the repository, which + # is the one outcome this step exists to prevent. + Scenario: No caller carries the Convergence Check question structure Given the seven question titles read from the Convergence Check itself When every other skill is searched for those titles as headings diff --git a/skills/clock-in/SKILL.md b/skills/clock-in/SKILL.md index b1e69c3..a8a8f77 100644 --- a/skills/clock-in/SKILL.md +++ b/skills/clock-in/SKILL.md @@ -81,6 +81,126 @@ Note anything that contradicts what the topic files will claim. Uncommitted work, a branch that is not the base branch, an open pull request — these are facts about where the last session stopped, and they outrank any file. +Every one of those four commands describes the *current* branch, plus whatever +branches happen to carry an open pull request. A branch with commits and no pull +request is invisible to all four, and that is the ordinary shape of work in +progress. So establish the state of the whole repository as well: + +The enumeration names the base branch rather than assuming it, prints how far +each ref is ahead of and behind it, and ends in one verdict. Listing refs is not +the point — divergence from the base is, and a list of names still leaves the +comparison to whoever reads it. + +```sh +gap="" + +base=$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD) || + { echo "SWEEP: BLOCKED — UNAVAILABLE:base (run git remote set-head origin --auto once)"; exit 1; } +git fetch --quiet --prune origin || + { echo "SWEEP: BLOCKED — UNAVAILABLE:fetch"; exit 1; } +refs=$(git for-each-ref --format='%(refname:short)' refs/heads refs/remotes/origin) || + { echo "SWEEP: BLOCKED — UNAVAILABLE:refs"; exit 1; } +names=$(printf '%s\n' "$refs" | sed 's|^origin/||' | sort -u) || + { echo "SWEEP: BLOCKED — UNAVAILABLE:dedup"; exit 1; } + +report_ref() { + if ! counts=$(git rev-list --left-right --count "$base...$1" 2>/dev/null); then + gap="$gap UNAVAILABLE:divergence($1)"; return + fi + if ! newest=$(git log -1 --format=%cs "$1" 2>/dev/null); then + gap="$gap UNAVAILABLE:date($1)"; return + fi + echo "$1: $(echo "$counts" | awk '{print $2}') ahead," \ + "$(echo "$counts" | awk '{print $1}') behind $base, newest $newest" +} + +for name in $names; do + case "$name" in "${base#origin/}" | origin | HEAD) continue ;; esac + git show-ref --verify --quiet "refs/heads/$name" && has_local=yes || has_local=no + git show-ref --verify --quiet "refs/remotes/origin/$name" && has_remote=yes || has_remote=no + + if [ "$has_local" = yes ] && [ "$has_remote" = yes ]; then + git merge-base --is-ancestor "origin/$name" "$name" 2>/dev/null + case $? in + 0) report_ref "$name" ;; + 1) echo "$name: origin/$name is not contained in local — both listed" + report_ref "$name" + report_ref "origin/$name" ;; + *) gap="$gap UNAVAILABLE:ancestry($name)" ;; + esac + elif [ "$has_local" = yes ]; then + report_ref "$name" + else + report_ref "origin/$name" + fi +done + +if [ -n "$gap" ]; then + echo "SWEEP: BLOCKED —$gap" + exit 1 +fi +echo "SWEEP: CLEAR (base $base)" +``` + +**One entry per branch name, except where that would hide something.** A local +branch and its remote counterpart are the same work in two places only while +`origin/` is an ancestor of the local ref. Then the local one is ahead of +it or equal to it, and reporting that one alone loses nothing. + +When it is not an ancestor — the local branch is behind what was pushed, or the +two have diverged — collapsing them discards exactly the commits this sweep +exists to find. Both refs are listed then, and the pair is named as what it is. +That is a state of the repository rather than a failed query, so it is reported +and does not block. Where only one of the two exists, that one is used. + +Without a checkout, the same question and the same verdict: + +```sh +repo=/ +gap="" + +base=$(gh api "repos/$repo" --jq .default_branch 2>/dev/null) || + { echo "SWEEP: BLOCKED — UNAVAILABLE:default-branch"; exit 1; } +branches=$(gh api --paginate "repos/$repo/branches?per_page=100" --jq '.[].name' 2>/dev/null) || + { echo "SWEEP: BLOCKED — UNAVAILABLE:branch-list"; exit 1; } + +for b in $branches; do + [ "$b" = "$base" ] && continue + if line=$(gh api "repos/$repo/compare/$base...$b" --jq \ + '"\(.ahead_by) ahead, \(.behind_by) behind" + + (if (.commits | length) > 0 + then ", newest \((.commits | last).commit.committer.date[0:10])" + else "" end)' 2>/dev/null); then + echo "$b: $line" + else + gap="$gap UNAVAILABLE:compare($b)" + fi +done + +if [ -n "$gap" ]; then + echo "SWEEP: BLOCKED —$gap" + exit 1 +fi +echo "SWEEP: CLEAR (base $base)" +``` + +`--paginate` is not decoration. Without it the listing stops after the first +page, and a repository with more than a hundred branches reports `SWEEP: CLEAR` +having looked at part of itself — the same silent incompleteness this section +exists to remove, arriving through the door marked "success". The checkout form +has no such limit; `for-each-ref` enumerates everything. + +This is one repository's branches, not other repositories — the multi-project +view still belongs to the private layer. + +**Both forms are fail-closed, and that is not decoration.** A query that did not +run says nothing about the branches, so every fetch is tested on its own and a +failed comparison is collected rather than skipped. Two shapes make this go +wrong quietly and both are avoided above: a `gh api` or `git` command as the left +member of a pipeline, whose failure the pipeline swallows, and a `while read` fed +by a pipe, which runs in a subshell and discards the gaps it collected. A session +that could not list the branches has not established that there were none. + **2. Delegate to the project's own clock-in skill when it has one.** Look for `skills/clock-in/SKILL.md` in the repository being worked in, and apply @@ -119,9 +239,9 @@ here. is often why the plan says what it says. - The issues the plan references, if the plan's next step names any. -**7. Report the delta, then start.** Three things, briefly: where the topic -stands, what the plan's next step is, and what has changed on the base branch -since the file was last touched. If the two disagree, say so and fix the file — +**7. Report the delta, then start.** Four things, briefly: where the topic +stands, what the plan's next step is, what has changed on the base branch since +the file was last touched, and which other refs are ahead of it. If the two disagree, say so and fix the file — it describes now, so a stale statement in it is corrected on sight, not preserved. @@ -129,6 +249,34 @@ preserved. `templates/progress.adoc` and fill the plan **with** the owner, not for them. A plan invented on their behalf is a guess wearing a checklist. +## Say what was read + +This skill claims to start from the repository rather than from memory. A session +that read the issues, the diary and the topic files and a session that +reconstructed them from the conversation produce artifacts that look identical, +so the claim needs a trace or it is only an intention. + +Record, in the layer that outlives the session, which repository artifacts this +session actually fetched — the branch enumeration, the issues read by number, the +topic files, the diary entry, the delegated skills — each with a reference a later +session can re-fetch. `clock-out/SKILL.md` says where that record lands. + +Two rules keep it worth having: + +- **An artifact this session did not fetch does not go in the list.** A plausible + entry written from memory is worse than a short list, because it is the one + thing a later session will trust without checking. +- **A failed fetch is an entry too**, naming what could not be read and why. + +Be clear about how much this is worth. The branch enumeration above is enforced +by a command: run it or do not, the output is either there or it is not. This +record is enforced by review — nothing in the toolkit can tell a truthful list +from an invented one, because the diary form belongs to the project layer and +there is no single path to check. A consuming repository whose diary has a fixed +shape can count the section instead, and one does. Saying that plainly is part of +the rule: a guard that claims more enforcement than it has is the same defect one +level up. + ## The private journal binding A private journal is per user, not per project. Its location must therefore never diff --git a/skills/clock-out/SKILL.md b/skills/clock-out/SKILL.md index 768d859..f09a07c 100644 --- a/skills/clock-out/SKILL.md +++ b/skills/clock-out/SKILL.md @@ -58,6 +58,12 @@ gh pr list --state open gh pr checks ``` +These see the current branch and whatever carries an open pull request. Establish +the state of the whole repository too, with the branch enumeration under step 1 +of `../clock-in/SKILL.md` — that file owns the command, and a second copy here +would get its own chance to drift from it. A branch that moved today and has no +pull request is exactly what a closing report must not miss. + A session does not end tidily just because it stopped. **2. Delegate to the project's own clock-out skill when it has one.** Look for @@ -86,6 +92,12 @@ file, apply steps 3 to 5 against the default layout in the pull requests — not from the session. At clock-out the day is fresh, which is exactly when memory feels reliable enough to skip the check. +The entry carries what this session actually fetched, under the rules in "Say +what was read" in `../clock-in/SKILL.md`: references a later session can +re-fetch, nothing that was not fetched, and every failed fetch named. That list +is what makes the sentence above checkable by a reader instead of merely +asserted. + **5. Say what is open — in the files, not only in the conversation.** The chat is gone tomorrow. A thread that outlives its topic goes to the project's long-lived thread list; one that dies with the topic stays in the progress file. @@ -112,10 +124,15 @@ private layer cannot reconstruct: | `status` | one line, verified against the tools | | `findings` | what was learned that is **not** specific to this project; may be empty | | `evidence` | pull requests, commits, issues — links, not prose | +| `read` | what this session fetched to reach the above, as re-fetchable references | | `threads` | open threads that outlive this project | Rules that make it safe to run more than once: +- **`evidence` and `read` are different sets.** `evidence` is what the day + produced; `read` is what the session fetched to find out. They overlap and are + routinely not the same, and collapsing them loses the only statement that says + the day was reconstructed from the repository. - **Keyed by project and day.** A second clock-out on the same day replaces that project's entry; it never appends a duplicate. - **The private layer owns the day.** It decides which day file the record lands diff --git a/test/skill-wiring.test.mjs b/test/skill-wiring.test.mjs index ad394b8..f354c29 100644 --- a/test/skill-wiring.test.mjs +++ b/test/skill-wiring.test.mjs @@ -138,6 +138,94 @@ test("The Convergence Check result states live in exactly one skill", () => { assert.deepEqual(offenders, []); }); +// The branch enumeration clock-out needs is defined in clock-in. Read it from +// there rather than restating it, for the same reason as above: rename or reword +// the command and the guard follows it. +function canonicalBranchEnumerationBlock() { + const text = read(path.join(skillsDir, "clock-in", "SKILL.md")); + const blocks = [...text.matchAll(/```sh\n([\s\S]*?)```/g)].map((m) => m[1]); + const sweeps = blocks.filter((block) => block.includes("for-each-ref")); + assert.equal( + sweeps.length, + 1, + `expected one branch enumeration in clock-in, found ${sweeps.length}`, + ); + return sweeps[0]; +} + +// The same skill defines a second form for sessions without a checkout. It is +// found by what only it contains, so neither block has to be located by index. +function canonicalApiEnumerationBlock() { + const text = read(path.join(skillsDir, "clock-in", "SKILL.md")); + const blocks = [...text.matchAll(/```sh\n([\s\S]*?)```/g)].map((m) => m[1]); + const forms = blocks.filter((block) => block.includes("/compare/")); + assert.equal( + forms.length, + 1, + `expected one API enumeration in clock-in, found ${forms.length}`, + ); + return forms[0]; +} + +function canonicalBranchEnumeration() { + return canonicalBranchEnumerationBlock() + .split("\n") + .find((line) => line.includes("for-each-ref")) + .trim(); +} + +test("The repository branch sweep is defined once and deferred to", () => { + // Given clock-in defines the repository-wide branch enumeration + const command = canonicalBranchEnumeration(); + + // When every other skill is searched for that command + const copies = skillFiles() + .filter((file) => rel(file) !== path.join("skills", "clock-in", "SKILL.md")) + .filter((file) => read(file).includes(command)) + .map(rel); + + // Then none carries it and clock-out references skills/clock-in/SKILL.md + // + // Both halves matter. Without the first, the command gets pasted into + // clock-out and the two drift; without the second, it gets dropped from + // clock-out entirely and the closing report silently loses the branches + // again — which is the regression this scenario was written for. + assert.deepEqual(copies, []); + const clockOut = path.join(skillsDir, "clock-out", "SKILL.md"); + assert.ok( + skillReferences(clockOut).includes("../clock-in/SKILL.md"), + "clock-out no longer reaches the skill that owns the branch enumeration", + ); +}); + +test("The branch enumeration covers every branch and reports divergence", () => { + // Given the two forms of the branch enumeration clock-in defines + const checkout = canonicalBranchEnumerationBlock(); + const api = canonicalApiEnumerationBlock(); + + // When they are inspected for what they produce + // + // The wiring test above guards where the command lives. It would pass just as + // happily against `git for-each-ref` alone, or against an API listing that + // stops after the first page — both of which report success while having + // looked at part of the repository. So the promise is checked, not the + // location. + const promises = [ + ["checkout form resolves the base branch", checkout, /symbolic-ref/], + ["checkout form counts both directions", checkout, /rev-list --left-right --count/], + ["checkout form labels ahead", checkout, /ahead/], + ["checkout form labels behind", checkout, /behind/], + ["API form pages the whole list", api, /gh api --paginate/], + ]; + const missing = promises + .filter(([, block, pattern]) => !pattern.test(block)) + .map(([label]) => label); + + // Then the checkout form resolves the base branch and counts commits in both + // directions, and the API form pages through the whole branch list + assert.deepEqual(missing, []); +}); + test("The Convergence Check questions are not copied into a caller", () => { // Given the canonical skills under skills/, and the seven question titles read // from the Convergence Check itself