Add an agent substrate: working rules, learnings, and a guard that enforces both by tylervick · Pull Request #419 · XCTestHTMLReport/XCTestHTMLReport · GitHub
Skip to content

Add an agent substrate: working rules, learnings, and a guard that enforces both - #419

Draft
tylervick wants to merge 16 commits into
mainfrom
tylervick/agent-substrate
Draft

tylervick wants to merge 16 commits into
mainfrom
tylervick/agent-substrate

Conversation

@tylervick

@tylervick tylervick commented Aug 8, 2026 •

Copy link
Copy Markdown
Member

Ports the agent substrate proven in a sibling repository, adapted. Four artifacts plus a CI job:

File Purpose
CLAUDE.md always-on working rules, capped at 50 lines (currently 42)
docs/learnings/ + INDEX.md six traps this project paid for during the 3.0 release
Scripts/check-substrate.sh enforces the cap and an exact file↔index bijection
Scripts/test-check-substrate.sh that guard's own hermetic test — 18 cases
lint.yml substrate job runs the guard's test, then the guard

Why a guard at all

CLAUDE.md and docs/learnings/ are conventions, and conventions decay silently. An unindexed learning is invisible to anyone reading only the index; a CLAUDE.md that grows without bound stops being read. Both failures are quiet and both are cheap to catch mechanically, so they are caught mechanically.

The guard's own test runs before the guard in CI. A guard nobody tests can silently stop checking anything — which is the same failure mode it exists to prevent.

The learnings are seeded, not empty

Each cost real time during the 3.0 work, and each is written as a trap with its evidence:

  • macos-latest became macos-26 and dropped Xcode 16, silently breaking two workflows
  • SwiftLint run unscoped lints .build/checkouts — tens of thousands of findings from dependencies, not this codebase
  • reformatting HTMLTemplates.swift moves the closing """ delimiters and changes the bytes of every generated report
  • the sample UI suite flakes on XCUIApplication().launch(), so exact per-status counts are not assertable
  • the release job is guarded on push, so the dry run structurally cannot reach it
  • read under set -e returns non-zero on empty input and dies before its own guard

Per CLAUDE.md's own rule, where a check already enforces a lesson the file points at the check rather than restating it.

The guard was hardened after a review demonstrated it could be defeated

The first version enforced an upper bound and a bijection, and nothing else. A whole-branch review showed five ways to leave it green while the substrate rotted:

Vector Why it passed
100 KB CLAUDE.md in 50 lines the cap counted newlines, not size
51 lines with no trailing newline wc -l reports 50
a learning at docs/learnings/ci/hidden.md find … -maxdepth 1 never recursed
every learning deleted a bijection over the empty set is satisfied
CLAUDE.md truncated to 0 bytes only existence and an upper bound were checked

All six are now closed (the sixth: deleting the substrate job itself). The suite went from 9 cases to 18, and every new check was mutation-tested — each one disabled in turn against the real tree to prove a test fails when it does. A check nobody can make fail is not protection.

The wiring check greps for the literal Scripts/check-substrate.sh rather than the bare filename, deliberately: a bare match would let a workflow that runs only the test count as wired, which is a false green.

Required status checks

main had no required status checks — branch protection required one approving review and nothing else, so every CI job including this one was advisory. substrate, shell, swift and test are now required. Without that, this PR would have shipped the appearance of enforcement rather than enforcement.

Two figures in the learnings were wrong and were corrected

Both caught by running the commands rather than reading the prose:

  • the scoped SwiftLint figure said "65 and 24"; the tool prints 26 violations, 0 serious. The 65/24 was an interim number from mid-PR-Add shellcheck, SwiftFormat and SwiftLint #410 that never described the shipped state.
  • the unscoped figure said "7,285 warnings and 2,570 errors" and did not reproduce either. It is now stated as a magnitude, since the exact count moves with dependency and SwiftLint versions.

A learnings file whose closing line tells the reader to run a command cannot afford to state a number that command does not print.

Not in this PR

  • The loop protocol (loop-prompt.md, loop-precheck.sh, loop-report.sh). Deliberately deferred: the real prerequisite for an unattended loop is rewriting the open 3.0-milestone issues to a standard it can work from, which is larger than porting the scripts.
  • An anti-vacuity check for the Swift test suite — scheduled and mechanical, so a test that passes for the wrong reason is caught by something other than whoever is reading carefully. Three such tests were found by hand during the 3.0 work.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added automated repository structure validation to local commit checks and CI.
    • Added comprehensive project guidance and troubleshooting documentation.
    • Added an indexed collection of learnings covering build, testing, release, and tooling considerations.
  • Documentation

    • Updated contribution guidance for structural checks and committed-file linting.
    • Documented common issues involving generated files, shell behavior, simulator tests, release validation, runner images, and lint configuration.
  • Tests

    • Added coverage for valid and invalid repository structures, including nested learning documents and CI validation wiring.

tylervick and others added 13 commits August 7, 2026 19:50
Ports the agent substrate proven in doom-ios-2026 rather than designing a
parallel one: CLAUDE.md, docs/learnings/, check-substrate.sh, and the loop
protocol, copied rather than shared because they will diverge.

Records the failure class this is built against: six cases in one day where
a signal was a cached representation rather than the state itself. The same
class appears independently in doom-ios-2026's own trial record, where a
rate-limited CodeRabbit review reported as "pass" with zero review bodies —
so "none" meant "never measured", not "nothing found".

Adds the one mechanism that substrate lacks: a check for tests that pass for
the wrong reason. Its rule exists ("never weaken a test") with nothing
mechanical behind it, and two such tests were written and caught by hand
today.

Notes that porting must come from PR #58's branch rather than main, since
main still carries the rate-limited-review defect, and that rewriting the
backlog to the agent:eligible bar gates the loop.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four tasks: the guard and its hermetic test first, then CLAUDE.md, then
docs/learnings/ seeded from the 3.0 work, then CI wiring. The guard comes
first so the artifacts it governs are validated as they are written.

Scoped to the substrate alone. It is useful for supervised agent work
immediately and does not depend on doom-ios-2026 PR #58 landing, unlike
the loop protocol.

Two steps deliberately prove a check can fail rather than only that it
passes: Task 3 Step 4 confirms the hermetic test still passes once a real
substrate exists, and Task 4 Step 3 removes an indexed learning to confirm
the CI job actually goes red.

NOTE: committed unsigned because the 1Password SSH agent was locked.
Re-sign before pushing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLAUDE.md and docs/learnings/ are conventions, and conventions decay
silently: an unindexed learning is invisible to anyone reading only the
index, and a CLAUDE.md that grows without bound stops being read.

Ported from doom-ios-2026 and copied rather than shared, since the two
repositories' rules and traps will diverge. Deliberately offline, so it
cannot turn an unrelated pull request red for repo-wide state its author
cannot change.

The guard has its own hermetic test, which builds a fake repo in a temp
dir and never touches the real substrate.
Case 5 (duplicate on separate lines) would pass with either grep -c or
grep -oF, so it doesn't test the implementation choice. Case 5b adds the
scenario the guard's grep -oF exists for: two links to the same file on
ONE line. grep -c would count that as one matching line and wrongly pass.

This test ensures the bijection check correctly counts each occurrence,
not each matching line.
The always-on rules any agent working in this repository is bound by:
how to build and test, the invariants that are easy to break silently,
and the guardrail files that must never be modified by the thing they
judge.

Capped at 50 lines by Scripts/check-substrate.sh so it keeps being read;
detail belongs in docs/learnings/
Six traps that each cost real time during the 3.0 release, written as
traps with their evidence rather than as narrative: runner images
dropping Xcode versions, SwiftLint reading .build when unscoped,
HTMLTemplates.swift being generated, the sample UI suite's launch flake,
the release job being unreachable by the dry run, and read under set -e
skipping its own guard.

Where a check already enforces the lesson, the file points at the check
rather than restating it.
swiftlint-lints-dependencies-unscoped.md claimed the scoped run reports
"65 and 24". It reports 26 warnings and 0 errors. The 65/24 figure was an
interim measurement taken before two violations were fixed and the config
was finalised, so it never described the shipped state — in a file whose
closing line tells the reader to run that very command.

Also adds SwiftFormat to the list of dependencies producing the unscoped
noise, and corrects release-job-is-unreachable-by-dry-run.md, which said
the artifact-version mismatch "bit once already". It was caught inside
PR #404 before merge — a near-miss, not a realised incident.
Runs the guard's own test before the guard itself — a guard nobody tests
is one that can silently stop checking anything, which is the same
failure mode it exists to prevent.

Verified the job can actually go red by removing an indexed learning and
confirming a non-zero exit, not just that it passes today.
CLAUDE.md forbade modifying anything under .github/workflows/ outright,
which the same plan's CI-wiring task does. The rule was lifted from a
sibling repository's loop protocol, where it binds unattended runs
specifically, not from its CLAUDE.md, which carries no such rule.

Rewritten to forbid weakening the checks that judge the work, while
allowing deliberate changes made in their own pull request. Also drops
CLAUDE.md from the list, since a file cannot coherently forbid its own
editing while asking agents to keep it current.
docs/superpowers/ held the spec and plan that produced this branch. They are
stale against the code in the same commit range in several places — including a
figure this branch corrected and a guardrail rule this branch narrowed — so an
agent replaying the plan would reintroduce both. Nothing in the tree references
them.

The working rules that survive are in CLAUDE.md and docs/learnings/, which are
guarded by Scripts/check-substrate.sh. The design documents are not, and an
unguarded document that contradicts the code is worse than no document.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The file claimed a bare `swiftlint lint` reports 7,285 warnings and 2,570
errors. Reconstructing the condition — the tree at this commit, the same
Package.resolved, `.swiftlint.yml` removed, `swift package resolve`, then
`swiftlint lint` — measures 12,577 violations, 3,192 serious across 1,031
files. Neither claimed number reproduces.

The exact count moves with dependency and SwiftLint versions, so pinning a new
one just schedules the next correction. The lesson is the magnitude: a count in
the thousands means the tool is reading `.build`, and 12,484 of those 12,577 are
from `.build/checkouts`. The file closes by telling the reader to run the
command, so a headline number they cannot reproduce teaches distrust of the
whole directory.

The scoped half — 26 warnings and 0 errors with the committed config — was
re-run and still holds.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard enforced an upper bound and a bijection, and neither is enough. Six
ways to leave it green were demonstrated against the real tree: a 100 KB
CLAUDE.md on 50 lines, 51 lines with the trailing newline deleted, a learning
filed in a subdirectory, every learning deleted, CLAUDE.md truncated to zero
bytes, and the CI job that runs the guard deleted.

Each is quiet, and each is the shape of failure this script exists to catch —
the substrate decaying without anyone noticing.

CLAUDE.md now has bounds on both ends. Bytes bound size (500..20000; the file
is 2061), lines keep the shape scannable, and lines are counted with awk's NR
rather than `wc -l` because `wc -l` counts newline characters and reports 50
for a 51-line file that lacks a trailing one.

The learnings walk recurses, so a subdirectory is no longer a hiding place. The
bijection key is therefore the path relative to docs/learnings/ rather than the
basename: INDEX.md links resolve relative to its own directory, so ci/hidden.md
is reachable only as `](ci/hidden.md)`. A basename key would accept a link that
renders as a dead end, and would be ambiguous once two subdirectories held the
same filename. Top-level learnings are unaffected — their relative path is their
basename. A floor of one learning file closes the emptied-directory case, which
a bijection cannot: it holds over the empty set.

The guard asserts its own wiring by grepping .github/workflows/ for an
invocation. A check nobody runs is indistinguishable from a check that always
passes, and deleting the job is quieter than editing the script. It matches the
path and not the bare filename, because `check-substrate.sh` is a substring of
`test-check-substrate.sh` — a workflow running only the test would otherwise
look wired. It stays offline like the rest of the script, and survives the job
moving to a different workflow file.

Every new check has a case that makes it fail, plus two positive cases pinning
the relative-path key and the move-the-job-anywhere behaviour. The suite counts
its own cases now, 9 to 18, so the closing line cannot go stale.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four statements that a reader can check and find wrong.

The pre-commit hook said it "runs the same checks CI runs" while CI had grown a
fourth. Adding Scripts/check-substrate.sh to the hook is the better repair: it
costs milliseconds, takes no staged-path argument, and catches an unindexed
learning at commit time instead of on the pull request. It runs unconditionally
because it is a structural check over the tree, not a per-file lint — which also
means a docs-only commit, the exact commit that adds a learning, is now checked
rather than skipped. CONTRIBUTING's neighbouring sentence about the hook only
looking at staged files is narrowed to the linters, which is what it describes.

CONTRIBUTING listed the CI checks as three.

Neither README nor CONTRIBUTING mentioned docs/learnings/, so the traps were
reachable only by agents that auto-load CLAUDE.md. CONTRIBUTING's build section
now points at the index, where a human looking at CI or fixtures will pass it.

CLAUDE.md called upload-artifact and download-artifact "a matched pair" that
"must move together". They are independently versioned actions and the tree
reads v7 and v8, so a reader checking for equal numbers concludes the invariant
is already violated and learns to discount the list. The requirement is that the
release job can read what the build job wrote; the wording now says that.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 8, 2026 •

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ca0b2f7b-af58-4d52-947d-f7e78fa650f0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The pull request adds an offline substrate guard, hermetic tests, pre-commit and CI integration, project contribution rules, and indexed learning documents.

Changes

Substrate validation

Layer / File(s) Summary
Substrate guard implementation
Scripts/check-substrate.sh
Validates CLAUDE.md, recursive learning-file/index consistency, and CI wiring. It reports all errors before returning a failure status.
Hermetic guard tests
Scripts/test-check-substrate.sh
Tests valid and invalid temporary repository fixtures, including malformed indexes, size limits, aggregate errors, and workflow wiring.
Pre-commit and CI wiring
.githooks/pre-commit, .github/workflows/lint.yml
Runs the substrate check during commits and adds an Ubuntu workflow job that runs the guard tests and the guard.
Project guidance and learning index
CLAUDE.md, CONTRIBUTING.md, docs/learnings/*
Adds project rules, contributor guidance, an exact learning index, and notes for recurring build, test, release, generated-file, runner, lint, and shell issues.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Developer
  participant PreCommit as pre-commit
  participant Guard as check-substrate.sh
  participant CI as lint.yml
  participant GuardTests as test-check-substrate.sh

  Developer->>PreCommit: commit changes
  PreCommit->>Guard: validate repository substrate
  Guard-->>PreCommit: return validation status
  CI->>GuardTests: run hermetic guard tests
  GuardTests-->>CI: return test status
  CI->>Guard: validate repository substrate
  Guard-->>CI: return validation status
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: working rules, learnings, and an enforcement guard for the agent substrate.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tylervick/agent-substrate

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 3

🧹 Nitpick comments (3)
Scripts/test-check-substrate.sh (1)

163-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case for the missing .github/workflows/ branch.

Case 9 deletes lint.yml but keeps the directory, so the guard takes the elif branch at line 115 of Scripts/check-substrate.sh. The [ ! -d "$WORKFLOWS" ] branch at line 113 and its message ".github/workflows/ is missing" are never exercised.

The header of this file states that every check in the guard has a case that makes it fail. One branch does not meet that claim yet.

💚 Proposed additional case
 pass "fails when no workflow invokes the guard"
+
+# 9a. No .github/workflows/ directory at all -> refuse. Deleting the directory
+#     is a broader version of deleting the job, and it takes a different branch
+#     in the guard than case 9 does.
+make_fixture "$TMP/i1"; rm -rf "$TMP/i1/.github"
+if check "$TMP/i1" >"$TMP/out" 2>&1; then fail "passed with no .github/workflows/ directory"; fi
+grep -q "workflows/ is missing" "$TMP/out" || fail "missing-workflows-dir error unclear"
+pass "fails when .github/workflows/ does not exist"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Scripts/test-check-substrate.sh` around lines 163 - 166, Add a test case in
Scripts/test-check-substrate.sh that removes the entire .github/workflows
directory, runs check, asserts failure, and verifies the output contains
".github/workflows/ is missing". Keep the existing case that removes only
lint.yml to continue covering the unwired-workflow branch.
Scripts/check-substrate.sh (2)

113-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The wiring check accepts any mention of the path, not only an invocation.

grep -rqF matches the string anywhere in any workflow file. A comment line, or a paths: trigger filter that lists Scripts/check-substrate.sh, satisfies the check while no job runs the guard. That is the exact failure mode the comment at lines 102-104 describes.

Requiring the reference to appear in a run context closes the common accidental case. It does not cover every YAML shape, so keep the substring form as the fallback and treat this as a tightening, not a complete parse.

♻️ Proposed tightening of the wiring check
 if [ ! -d "$WORKFLOWS" ]; then
     err ".github/workflows/ is missing — nothing can be invoking Scripts/check-substrate.sh."
-elif ! grep -rqF 'Scripts/check-substrate.sh' "$WORKFLOWS"; then
+elif ! grep -rqE '(^|[^-])run:.*Scripts/check-substrate\.sh|^[[:space:]]*[^#]*Scripts/check-substrate\.sh' "$WORKFLOWS"; then
     err "no file under .github/workflows/ invokes Scripts/check-substrate.sh — the guard is not wired into CI."
 fi

If you tighten this, add a case to Scripts/test-check-substrate.sh that puts the path in a # comment line only and asserts the guard refuses. I can draft that case.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Scripts/check-substrate.sh` around lines 113 - 117, Update the workflow
wiring check around the grep in Scripts/check-substrate.sh to require
Scripts/check-substrate.sh within a run context, while retaining the existing
substring check as a fallback rather than parsing YAML completely. Extend
Scripts/test-check-substrate.sh with a fixture containing only a # comment
reference and assert that the guard rejects it.

43-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

SonarCloud reports [ instead of [[ as a failure across both scripts. Both files use #!/bin/bash, so [[ is available. Every operand here is quoted, so the current tests are safe at runtime. The finding is style-level, but SonarCloud raises it at failure severity, so it can gate the pull request.

  • Scripts/check-substrate.sh#L43-L57: replace [ with [[ at lines 43, 48, 51, 54, 60, 65, 78, 90, 95, and 113.
  • Scripts/test-check-substrate.sh#L41-L85: replace [ with [[ at lines 43, 63, and 81.

If you prefer to keep POSIX-style tests, suppress the rule in the SonarCloud configuration instead. Do not change the assertions themselves.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Scripts/check-substrate.sh` around lines 43 - 57, Replace the POSIX
single-bracket test syntax with Bash double-bracket syntax without changing any
assertions: in Scripts/check-substrate.sh, update the tests at lines 43, 48, 51,
54, 60, 65, 78, 90, 95, and 113; in Scripts/test-check-substrate.sh, update the
tests at lines 43, 63, and 81. Keep all operands and conditions unchanged.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CLAUDE.md`:
- Around line 9-11: Update the “exactly two commands” statement in the
fixture-testing guidance to scope it to the test job rather than all CI.
Preserve the listed prepareTestResults.sh and swift test commands and clarify
that other CI checks run separately.

In `@docs/learnings/html-templates-is-generated.md`:
- Line 12: Make both learning-file tool-output figures reproducible: in
docs/learnings/html-templates-is-generated.md:12-12, record the SwiftFormat
version and repository revision or replace “1,200” with a qualitative statement;
in docs/learnings/swiftlint-lints-dependencies-unscoped.md:6-10, record the
SwiftLint and dependency versions or remove the fixed count of 26 warnings.

In `@docs/learnings/read-under-set-e-skips-its-guard.md`:
- Around line 16-19: Update the learning around the documented set -e/read
reproduction to reference a maintained, executable shell test. Add a hermetic
check covering both empty input behavior and the read command guarded with ||
true, then link that test from the learning instead of relying solely on the
manual reproduction.

---

Nitpick comments:
In `@Scripts/check-substrate.sh`:
- Around line 113-117: Update the workflow wiring check around the grep in
Scripts/check-substrate.sh to require Scripts/check-substrate.sh within a run
context, while retaining the existing substring check as a fallback rather than
parsing YAML completely. Extend Scripts/test-check-substrate.sh with a fixture
containing only a # comment reference and assert that the guard rejects it.
- Around line 43-57: Replace the POSIX single-bracket test syntax with Bash
double-bracket syntax without changing any assertions: in
Scripts/check-substrate.sh, update the tests at lines 43, 48, 51, 54, 60, 65,
78, 90, 95, and 113; in Scripts/test-check-substrate.sh, update the tests at
lines 43, 63, and 81. Keep all operands and conditions unchanged.

In `@Scripts/test-check-substrate.sh`:
- Around line 163-166: Add a test case in Scripts/test-check-substrate.sh that
removes the entire .github/workflows directory, runs check, asserts failure, and
verifies the output contains ".github/workflows/ is missing". Keep the existing
case that removes only lint.yml to continue covering the unwired-workflow
branch.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: be948a54-a12e-4ef8-963a-2d067a9532d8

📥 Commits

Reviewing files that changed from the base of the PR and between ee40463 and 8fbba31.

📒 Files selected for processing (13)
  • .githooks/pre-commit
  • .github/workflows/lint.yml
  • CLAUDE.md
  • CONTRIBUTING.md
  • Scripts/check-substrate.sh
  • Scripts/test-check-substrate.sh
  • docs/learnings/INDEX.md
  • docs/learnings/html-templates-is-generated.md
  • docs/learnings/read-under-set-e-skips-its-guard.md
  • docs/learnings/release-job-is-unreachable-by-dry-run.md
  • docs/learnings/runner-image-drops-xcode-versions.md
  • docs/learnings/sample-ui-suite-flakes-on-launch.md
  • docs/learnings/swiftlint-lints-dependencies-unscoped.md

Comment thread CLAUDE.md Outdated
Comment on lines +9 to +11
- Fixtures first: `./prepareTestResults.sh`, then `swift test`. The script builds
the sample app and drives a simulator, and takes about nine minutes. It needs no
credentials — CI runs exactly these two commands.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Scope the “exactly two commands” statement to the test job.

CONTRIBUTING.md Lines [63]-[64] documents additional CI checks, including shellcheck, SwiftFormat, SwiftLint, and the structural substrate check. The PR also requires separate substrate, shell, swift, and test checks. As written, this rule says that CI runs only the fixture and Swift test commands.

Change CI to the test job or name the complete CI workflow.

Proposed wording
-  credentials — CI runs exactly these two commands.
+  credentials — the test job runs exactly these two commands.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- Fixtures first: `./prepareTestResults.sh`, then `swift test`. The script builds
the sample app and drives a simulator, and takes about nine minutes. It needs no
credentials — CI runs exactly these two commands.
- Fixtures first: `./prepareTestResults.sh`, then `swift test`. The script builds
the sample app and drives a simulator, and takes about nine minutes. It needs no
credentials — the test job runs exactly these two commands.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CLAUDE.md` around lines 9 - 11, Update the “exactly two commands” statement
in the fixture-testing guidance to scope it to the test job rather than all CI.
Preserve the listed prepareTestResults.sh and swift test commands and clarify
that other CI checks run separately.

is stripped from every line of the literal, so reformatting **changes the bytes of
every generated report**.

`swiftformat --lint` wanted 1,200 indent changes in this one file.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Make tool-output figures reproducible or remove them.

These learning files record version-sensitive counts without tool-version or repository-revision metadata. The SwiftLint learning also states that counts move with dependency and SwiftLint versions, then gives a fixed count of 26 warnings.

  • docs/learnings/html-templates-is-generated.md#L12-L12: record the SwiftFormat version and repository revision, or replace 1,200 with a qualitative statement.
  • docs/learnings/swiftlint-lints-dependencies-unscoped.md#L6-L10: record the SwiftLint and dependency versions, or remove the fixed count of 26 warnings.
📍 Affects 2 files
  • docs/learnings/html-templates-is-generated.md#L12-L12 (this comment)
  • docs/learnings/swiftlint-lints-dependencies-unscoped.md#L6-L10
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/learnings/html-templates-is-generated.md` at line 12, Make both
learning-file tool-output figures reproducible: in
docs/learnings/html-templates-is-generated.md:12-12, record the SwiftFormat
version and repository revision or replace “1,200” with a qualitative statement;
in docs/learnings/swiftlint-lints-dependencies-unscoped.md:6-10, record the
SwiftLint and dependency versions or remove the fixed count of 26 warnings.

Comment on lines +16 to +19
Reproduce: `set -ex; read -r A B < <(true); echo after` — `after` never prints.

`prepareTestResults.sh` hit exactly this. It now ends that read with `|| true` so
the guard is reachable.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Link this reproduction to a maintained executable check.

The set -e and read behavior is deterministic. The command at Line [16] only reproduces it manually. Add a hermetic shell test for empty input and the || true fix, then link that test from this learning.

As per coding guidelines, a checkable learning must use an executable check and link to it instead of only restating the rule.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/learnings/read-under-set-e-skips-its-guard.md` around lines 16 - 19,
Update the learning around the documented set -e/read reproduction to reference
a maintained, executable shell test. Add a hermetic check covering both empty
input behavior and the read command guarded with || true, then link that test
from the learning instead of relying solely on the manual reproduction.

Source: Coding guidelines

check-substrate.sh's `[ ! -d "$WORKFLOWS" ]` branch had no case exercising
it — test 9 deletes lint.yml but keeps the directory, so it only ever hit
the `elif`. Add a case that removes .github/workflows/ entirely and asserts
the guard's specific "is missing" message.

Proved non-vacuous: with the branch deleted from a scratch copy of the
guard, every other case still passes but this one correctly fails.
CLAUDE.md and CONTRIBUTING.md both said CI runs "exactly these two
commands" (prepareTestResults.sh, swift test). That stopped being true
once the substrate and lint jobs landed — CI now runs four required
checks plus shellcheck/SwiftFormat/SwiftLint. Reword both to say what is
actually true: the two commands reproduce CI's `test` job locally.
@tylervick
tylervick marked this pull request as draft August 10, 2026 20:11
CodeRabbit flagged that docs/learnings/read-under-set-e-skips-its-guard.md
reproduced the failure only by hand, while the repo's own rule is that a
checkable learning must be backed by an executable check.

Adds Scripts/test-prepare-test-results.sh: stubs `xcrun` on PATH with a device
list containing runtimes but no iPhones, runs prepareTestResults.sh in a temp
dir, and asserts it fails with the guard's own "No iPhone simulator available"
message rather than a bare `read`-failure trace. Remove the `|| true` and the
assertion fails. Fully hermetic -- never touches real simulators or xcodebuild.

Wired into the `shell` job and linked from the learning.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tylervick

Copy link
Copy Markdown
Member Author

This branch has not been deployed

No deployments
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.

1 participant