Repository navigation
Fix two check scripts, test the sync step, correct agent-facing docs (#6957) - #3575
Merged
Merged
Conversation
…(#6957) * Pin check-base-skills on non-object manifests and a partial --write Two tests that fail on the current script, committed before the fixes: - A marketplace manifest whose top level is null, an array or a string should be a finding with exit 1, as invalid JSON already is. Today null crashes the check with exit 2, and the other values are read as an empty plugin list, so the finding blames a skill instead of the manifest. - --write must write nothing when it cannot read a tracked link it would remove. Today it rewrites AGENTS.md, CLAUDE.md and .gitignore and adds links first, then exits 77 saying it could not examine the repository. * Report a non-object marketplace manifest as a finding readMarketplace let a top-level null through to a property read, so the check crashed with exit 2, and it read a number, string, boolean or array as an empty plugin list, so the finding blamed a skill. A manifest whose top level is not a JSON object is now a finding naming the manifest, the way invalid JSON and a non-list "plugins" already are. With --write, a malformed manifest now exits 1 instead of 2. The sync step tolerates exit 1, so the sync PR opens and its required check shows the finding, as it does for every other refusal. Retired test: "a crash during the check exits 2 saying the check stopped before a verdict". It checked that an unanticipated error exits 2 with the stopped-before-verdict message, and the null manifest was its only trigger; with this fix it went red because that input is now a finding. No other check-phase crash path exists: git, read and lstat errors all become NOT-RUN (exit 77). The surviving check is "a crash during --write, after it rewrote files, exits 2 ...", which runs the same top-level handler: on a scratch copy it went red for each of three planted handler defects (exit code 2 changed to 1, the rethrow removed, the message dropped) and green unmutated. Recorded gap: a future crash path specific to the check phase. Inventory: one test removed, three assertions added to an existing test. * Read check-base-skills' removal candidates before --write writes writeTree read the state of each tracked .claude/skills link it might remove only after it had rewritten AGENTS.md, CLAUDE.md and .gitignore and created the named links. A read error there, such as ENAMETOOLONG from a tracked name longer than the filesystem allows, threw NOT-RUN and exited 77 on a partly rewritten tree, without printing what it rewrote. Every other --write refusal returns before the first write. The pre-check now reads the removal candidates' link states with the named links' states, and the write phase reuses those states instead of reading them again. A write or link call that still fails takes the exit 2 path, which says rewritten files stay rewritten. The new test's trigger, a 300-byte tracked name, was observed on macOS (APFS); on Linux (ext4, NAME_MAX 255) the same ENAMETOOLONG is expected, and CI's script-tests run checks it. * Test the skills sync step's mapping of check-base-skills exits The sync job's "Rewrite the base-skills copies and links" step runs `node scripts/check-base-skills.mjs --write || [ "$?" -eq 1 ]`. Its contract: exit 0 or 1 (clean, or a refusal or findings after a write) lets the job go on to open the sync PR, and 77 (NOT-RUN) or 2 (a crash) stops it. No test ran that text, so an edit to `|| true` or to the bare command would change what the job admits without anything going red. The new case reads the step from the parsed workflow, runs its run: text under `bash -e` (the runner's shell when neither the step, the job nor the workflow sets one, which it also asserts) against a stub script for statuses 0, 1, 77 and 2, checks the stub ran with --write, and asserts which statuses let the step succeed. On a scratch copy of the workflow it went red for the `|| true` mutant (status 77 made the step exit 0) and for the bare command (status 1 made the step exit 1). * Pin check-pre-push-contract's protected check to the commits it judges Two tests that fail on the current checker, committed before the fix: - The protected-artifact check takes its change list from the commits since the merge base, but read the protected list and the change marker from the working tree. A marker entry that exists only in the working tree must not cure a committed edit to a protected file, and an uncommitted edit to the list must not be judged. - An uncommitted change to a protected path is named in a note as not yet judged, so a clean local run is not read as covering it. * Read check-pre-push-contract's protected list and marker at HEAD protectedFindings took its change list from the commits since the merge base but read config/protected-oracle-artifacts.json and config/pre-push-contract.CHANGES.md from the working tree. A local run could therefore pass commits that CI refuses (an entry present only in the working tree cured a committed edit) and refuse an uncommitted edit to the list. FR-30 of the gate-placement spec judges the diff, so the check now reads both files at HEAD, the state its diff comes from. It also adds a note naming uncommitted changes to protected paths or the marker, which it does not judge. CI's verdict is unchanged: its checkout has no uncommitted changes to these files when the check runs. The change-marker entry records the transition. * Delete two test comments that state stale facts Both assertions stay as they are; only the prose comments go, since the repository's comment policy (eng:no-comments) deletes a stale prose comment a change touches rather than rewriting it. - scripts/cross-harness/generate.test.ts: "~78 KiB combined corpus" was root AGENTS.md plus Open Knowledge's when #1141 wrote it (38377 + 39950 = 78327 bytes). Today's largest root-plus-subtree pair is 60492 bytes, and the caps allow at most 72768, so the > 78000 floor still holds. - scripts/check-review-submitted.test.mjs: a queue entry waiting on a required check that never reports does not wait forever. The ruleset's status check timeout (check_response_timeout_minutes, 60 in ruleset 15121147) removes it with checks_timed_out. The assertion's own message still says why the trigger matters. The deleted comment also noted that the regex is anchored at trigger indentation so a mention inside a YAML comment cannot satisfy it. * Correct public/agents/AGENTS.md for agents-private and its mirror - Build and pre-push name the agents-private route, `pnpm build:agents` and `pnpm check:agents` from its root, and why: inside public/agents, `pnpm build` and `pnpm check` first run that package's prebuild and precheck hooks (`pnpm install --frozen-lockfile`), and that install moves its links into a second store (CI_RUNBOOK.md, TS2883 entry). The root scripts turn pre/post hooks off. The feature checklist now points at Verification instead of naming `pnpm check`. - The lockfile procedure re-installs with `pnpm install:all` from the agents-private root after restoring each conflicted lockfile: the root lockfile and public/agents/pnpm-lock.yaml both carry public/agents importers, and install:all rewrites both. - `feature/` becomes `feat/`, the prefix both repositories use. In agents-private the branch goes in a new worktree, since the main checkout must not switch branches, and every worktree command runs from the root so `../name` lands beside the checkout, not under public/. - `prd` becomes `spec`, if it is installed: agents-private deleted its prd skill in #2017, spec produces the PRD, and the mirror ships no skills. - The checklist's "see" pointer names the heading that holds that guidance; the heading it named no longer exists. A typo (feautre) and a list item that did not parse are fixed. - The documentation example's outer fence is four backticks, so GitHub stops rendering "Debugging Commands" and its subsection as code. - AI Coding Assistant Guidance keeps what is true in both repositories: CLAUDE.md is a symlink to AGENTS.md, and AGENTS.md may load in part or not at all. The mirror ships no .agents, .codex, .claude or plugin skills, and in agents-private every entry under public/agents/.agents/ skills and .codex/skills is a link, which for a team skill resolves to a synced copy the next sync overwrites. A cold write-skill review checked every changed line; the PR body records its findings and how each was resolved. The file goes from 39,575 to 39,591 bytes, under its 39,600 pin. * Correct two provenance notes in agents-md-caps.tsv - Root row: codex exec records trust when its sandbox can write the working directory (codex-rs app-server thread_processor.rs at rust-v0.163.0), so "never records trust" was wrong. A default run, with no trust decision, is read-only, records none and keeps the 32768 budget. - public/agents row: drop "read by every agent touching public/agents". Codex reads AGENTS.md only from the project root down to its working directory, and Claude Code loads a subdirectory's file on demand. The numbers and the five-column format are unchanged. * Correct which name a required check matches in CI_ARCHITECTURE.md GitHub names a workflow job's check by the job's name: alone, so a required check matches the gate job's name, not the workflow's. Ruleset 15121147 requires public-open-knowledge-mirror-runnability while that workflow is named Public Open Knowledge Mirror Runnability, and the merge queue admits it. The sharp-edge bullet now says a drift between the gate job's name and the ruleset entry is the silent miss, and that a drift in the workflow's name breaks what names the workflow instead, such as a workflow_run trigger. * Exclude docs/content from Open Knowledge's own OK project The root .okignore keeps public/open-knowledge/docs/content/ out of the root OK project (PRD-7491). public/open-knowledge is its own OK project (.ok/config.yml), and its content filter reads only its own ignore files. Run against the subtree, its createContentFilter admitted all 78 docs/content MDX files without this file and excluded the directory with it. Line 286 of the subtree's AGENTS.md now says the exclusion comes from their project's .gitignore or .okignore, which stays true whether the subtree remains its own project or later joins the root one. Neither file ships: the mirror's manifest does not include .okignore and forbids it as an output, and the mirror ships its own AGENTS.md. * Correct Step 2's claim about the required-check entry in CI_ARCHITECTURE.md Step 2 said the workflow's name: becomes the required-check entry. GitHub names a workflow job's check by the job's name: alone, as the gate-name bullet this PR corrects now says, so the entry comes from the gate job's name:, which Step 4 sets equal to the workflow's name:. Without this the file contradicted itself. The cold review and claude[bot] both raised it. GitOrigin-RevId: 5120a9976ce31a8a93fd7024ae40c99e626ca6c9
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Copybara-translated 1 Inkeep OSS change. Rebase-merge this PR so the prepared commit lands directly on public main.