diff --git a/.github/workflows/ci-pr.yml b/.github/workflows/ci-pr.yml index 2cd5bd7..3413882 100644 --- a/.github/workflows/ci-pr.yml +++ b/.github/workflows/ci-pr.yml @@ -20,6 +20,10 @@ jobs: - name: Checkout Repository uses: actions/checkout@v7 + - name: Validate latest Codex review policy + uses: thatfactory/agent-guidelines/.github/actions/codex-review-policy@1c957669005eea010131c53f0a4146db2ccf7859 + with: + profile: full - name: Validate Consumer Integration run: AgentGuidelines/Scripts/validate_consumer_setup.swift diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0efac31..7dabe4c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,6 +21,10 @@ jobs: - name: Checkout Repository uses: actions/checkout@v7 + - name: Validate latest Codex review policy + uses: thatfactory/agent-guidelines/.github/actions/codex-review-policy@1c957669005eea010131c53f0a4146db2ccf7859 + with: + profile: full - name: Validate Consumer Integration run: AgentGuidelines/Scripts/validate_consumer_setup.swift diff --git a/AGENTS.md b/AGENTS.md index 3bd2e3e..0f31d96 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -53,19 +53,21 @@ Treat privacy-safe runtime observability as part of implementing or changing sta Every ThatFactory package log starts with its canonical emoji and uses its own stable subsystem. Never log credentials, account or record identifiers, share URLs, captured content, images, or other user-generated values as public metadata. Keep pure values and utilities silent when they have no meaningful diagnostic event; record that deliberate decision in the implementation handoff instead of adding initializer or property-access noise. Follow [Logging](AgentGuidelines/Guidelines/Logging.md) for ownership, privacy, severity, message design, and tests. - + ## External Dependency Policy Do not introduce third-party source or binary dependencies into ThatFactory applications, games, or reusable packages during normal development. Prefer Apple platform APIs, the Swift standard library, code owned by the current repository, or focused ThatFactory-owned packages. If reusable capability is missing, implement it natively at the appropriate boundary and consider extracting it into a first-party package instead of selecting an external library. -A third-party dependency may be added, or expanded to a new target or runtime role, only with explicit repository-owner approval for that specific use before modifying the dependency graph. Convenience, reduced implementation effort, popularity, or an agent's preference for an existing library are not sufficient justification. Do not make an external library acceptable merely by hiding it behind a first-party wrapper. +A third-party dependency may be added, or expanded to a new target or runtime role, only under an explicit standing approval in these guidelines or with explicit repository-owner approval for that specific use before modifying the dependency graph. Convenience, reduced implementation effort, popularity, or an agent's preference for an existing library are not sufficient justification. Do not make an external library acceptable merely by hiding it behind a first-party wrapper. Document every approved exception in durable repository documentation in the same change. Record the dependency and source, purpose and target scope, why a native or first-party implementation is not appropriate, relevant license, security, and maintenance considerations, and the approval context. Merely mentioning or using the dependency in an execution plan, pull-request description, or transient chat is not approval. Regardless of where explicit approval occurs, reflect the exception in durable repository documentation. Apple system frameworks and the Swift standard library are not third-party dependencies. ThatFactory-owned packages are first-party dependencies. Tooling explicitly required by the shared guidelines is allowed only for its documented tooling role and must not be linked into or shipped with product runtime targets unless separately approved and documented. +Follow [Networking](AgentGuidelines/Guidelines/Networking.md) for Apple-native networking and typed HTTP. Its narrow standing approval for Apple's `swift-http-types` applies only to the documented HTTPTypes and HTTPTypesFoundation runtime role; it does not approve other Apple open-source packages or unrelated roles. Use the guide's normal dependency documentation for that role instead of requesting a separate consumer exception. + Follow [Development workflow](AgentGuidelines/Guidelines/Development.md) for the detailed policy. - + ## Documentation Maintenance @@ -77,26 +79,35 @@ Update documentation when a change alters durable or core feature behavior or an Do not create documentation churn for incidental implementation details that are not durable and do not affect an existing documented claim. Follow [Documentation](AgentGuidelines/Guidelines/Documentation.md) for detailed scope and the completion checklist. - + + ## Code Review Rules -Review for release-blocking defects introduced or materially exposed by the pull request. A clean review means no unresolved P0/P1 findings; it does not mean exhaustive or perfect software. +Report only substantiated P0/P1 defects introduced or materially exposed by this change. Require a concrete, reachable failure in a supported use case or documented threat model, with serious impact such as a security-boundary bypass, durable data loss or corruption, a crash or deadlock in an important workflow, loss of availability, or a serious compatibility regression. An acceptance-criterion violation alone does not establish P1 severity. + +For each finding, state severity, preconditions, execution path, impact, evidence, and actionable remediation. Group manifestations of one root cause. Report substantiated blockers together. Omit unsolicited P2/P3 observations, style preferences, speculative hardening, optional tests, and refactors. A clean review means no unresolved P0/P1 blockers, not perfect software. -A blocking finding must identify a concrete, reachable path in a supported use case or the documented threat model that can cause a credible security-boundary bypass, durable data loss or corruption, a crash or deadlock, loss of availability, violation of an explicit acceptance criterion, or a serious compatibility regression. +A follow-up review covers unresolved P0/P1 findings, changes since the previous reviewed commit, and directly affected code. Do not restart a full review of unchanged code. New follow-up findings must be P0/P1 defects introduced by remediation or genuinely hidden by the previous blocker. + -For every blocking finding, state the severity, preconditions, execution path, impact, evidence, and actionable remediation. Group manifestations that share the same root cause into one finding. + +## Pull Request Lifecycle -Treat P2/P3 observations as non-blocking, including defense-in-depth, theoretical completeness, unsupported use cases, malformed state that trusted code cannot produce, behavior by components outside the threat model, style preferences, and speculative refactoring. Record a useful lower-severity observation once as deferred, declined, duplicate, or follow-up work; do not keep the review loop open for it. +Finish implementation, inspect the diff, and run relevant local validation before opening a ready-for-review PR. Drafts are for incomplete preparation; mark ready before waiting for automatic review. Keep the review head stable while the initial review runs. One automatic Codex review is the initial review; never duplicate it with a manual request or repeatedly toggle draft/ready state. -In an initial review, report substantiated blockers together. A follow-up review is limited to unresolved P0/P1 findings, changes since the last reviewed commit, and code directly affected by those changes. Do not restart an unrestricted review of unchanged code. A new follow-up finding must be a P0/P1 defect introduced by the remediation or genuinely hidden by the previous blocker. +Establish review configuration from repository-specific evidence. Verify the bot, PR, review round, and reviewed commit using PR/request-comment reactions, review submissions, inline threads, and checks. Eyes means processing, not approval; a thumbs-up means no suggestions only for its verified review round. Historical reactions, missing comments, and elapsed time never certify a newer head. Monitor at most five minutes total with bounded, non-accelerating polls; timeout leaves a required gate unresolved and never authorizes merge. -The review-round budget below applies only to Codex GitHub reviews: the configured automatic Codex review and any manual `@codex review` request. It does not apply to ChatGPT review or reasoning delegated through Reasoning Relay. An otherwise-authorized Reasoning Relay workflow may request as many Relay review or follow-up delegations as its own governing workflow requires; those requests neither consume the Codex budget nor require repository-owner authorization under it. +Batch accepted P0/P1 fixes, verify them locally, rerun affected checks, and disposition and resolve every review thread. A completed initial review plus an explicitly recorded, locally verified remediation-only delta can satisfy the review gate; do not claim Codex reviewed the newer head. New unrelated scope belongs in another PR. Request at most one delta-scoped verification only when the owner explicitly authorizes it. Do not request reviews after every fix or casually mention the bot to start extra work. Separate Security Review is manual and requires explicit owner instruction. -When repository-specific evidence establishes that automatic Codex review is enabled and applies to the current pull request/head, it is the initial Codex review. Otherwise, an absent review does not establish a pending gate. Do not request a manual Codex review unless the repository owner explicitly asks. Never request another Codex review after each remediation commit. Within the normal Codex review budget, at most one owner-authorized, delta-scoped Codex verification review may be requested under [the pull-request review workflow](AgentGuidelines/Guidelines/GitHub/PullRequests.md). - +Before merge, rerun the latest published review-policy check, confirm no required review is pending, no unresolved P0/P1 blocker remains, every thread is dispositioned and resolved, required checks pass on the current head, and existing human approval or merge authorization is satisfied. A bot thumbs-up is not a substitute for required GitHub approval. Opening a PR does not authorize merging it. + ## Codex review scope For consumer pull requests, do not substantively review `AgentGuidelines/**` after exact tagged-tree provenance has been verified. Verify its `VERSION`, compare its tree with the matching central tag, and verify the required `.gitattributes` rule. If provenance does not match exactly, review the subtree contents and stop the merge. Report substantive guideline feedback against the central `agent-guidelines` pull request. The marked block is intentional controlled duplication of the shared review policy. The tracked, synchronized subtree is reviewed centrally in `thatfactory/agent-guidelines`; the root-level instructions ensure the review contract and subtree scope are loaded even when Codex starts from the repository root. + +## Codex review configuration + +Automatic Codex PR review is enabled. Use all PR authors and the opening/ready trigger, keep exhaustive review off, and keep separate Security Review manual. Apply the managed lifecycle; do not request a duplicate manual review. The policy-validation hosting check is required for merge. diff --git a/AgentGuidelines/.agents/skills/agent-guidelines-audit/SKILL.md b/AgentGuidelines/.agents/skills/agent-guidelines-audit/SKILL.md index c47f52d..f0ecbce 100644 --- a/AgentGuidelines/.agents/skills/agent-guidelines-audit/SKILL.md +++ b/AgentGuidelines/.agents/skills/agent-guidelines-audit/SKILL.md @@ -27,23 +27,70 @@ Review the actual change rather than only checking whether files exist: - Confirm physical folders, familiar domain grouping, filenames, declaration order, type ownership, namespacing, documentation, and `MARK` organization follow the applicable guides. Distinguish values that describe data from tools that primarily execute algorithms or accumulate behavior. - For Redux applications, trace actions, state, reducers, middleware, services, tools, presentation models, views, and side-effect results through the complete data flow. Confirm each Redux component folder contains only that component type. - Check that framework objects, persistence, logging, and asynchronous work remain in their allowed boundaries. +- Perform the mandatory [networking check](#audit-networking) for network implementation, dependency changes, and networking-policy adoption. - Check SwiftUI composition, narrow inputs, local versus durable state, localization, accessibility, and safe deterministic previews where applicable. - For each new or changed SwiftUI `ScrollView`, apply the shared [scrolling guidance](../../../Guidelines/Swift/SwiftUI.md#scrolling): verify `.scrollBounceBehavior(.basedOnSize)` covers its scrolling axes on supported deployment targets, including scroll views in modal sheets. Check the modifier's environment scope around nested scrollables and require a documented interaction reason for a different bounce behavior. - Perform the mandatory [native-component and presentation decision check](#audit-native-components-and-presentation) for changed SwiftUI controls and presentation behavior. - Perform the mandatory [system-control placement check](#audit-system-control-placement) for changed navigation, toolbars, dismissal controls, and window chrome, including affected UIKit/AppKit wrappers. - Perform the mandatory [animation and image-rendering check](#audit-animation-and-image-rendering) for changed visual transitions, reusable control styles, and dynamic image paths, including directly affected consumers. - Perform the [Reduce Motion check](#audit-reduce-motion) for affected motion and for Reduced Motion support declarations. +- Perform the [tvOS current-user identity check](#audit-tvos-current-user-identity) when changed behavior relies on selected-profile preferences, CloudKit accounts, or personal store binding. - Check tests for the required framework, mirrored paths, shared tags, Given/When/Then structure, deterministic seams, and coverage of changed behavior and failure paths. - Trace every new or changed stateful, asynchronous, fallible, or lifecycle-oriented behavior and verify that its owning artifact emits privacy-safe AppLogger events for the meaningful success, failure, cancellation, recovery, and state-transition outcomes needed to diagnose it. Dependency declaration and target linkage alone do not establish logging coverage. Accept silence for pure values or utilities only when there is no meaningful event boundary and the implementation handoff records that deliberate decision. - Check logging ownership, subsystem, categories, emoji, privacy, severity, metadata stability, noise controls, and focused formatter or sink tests when logging changed. - For every in-scope package that emits logs, resolve its canonical emoji from local instructions or documentation and the package list on the current `main` branch of the [ThatFactory Swift Package Collection](https://github.com/thatfactory/swift-package-collection). Enumerate the registered package entries and require each canonical emoji to identify exactly one package, treating visually identical presentation-selector variants as a collision. Confirm the package documentation, package-local logging gateway, and registry entry use the same emoji. When creating a package or assigning or changing its emoji, verify the intended emoji is unused before accepting it and require the registry update in the publication work. A collision, a missing registry entry for a published package, an unreachable current registry, or disagreement among these sources blocks audit completion and release readiness. - For every Apple-platform application or Swift package in scope, except the AppLogger provider repository itself, verify integration with the shared [Logging guide](../../../Guidelines/Logging.md): confirm the AppLogger dependency is declared, the `AppLogger` library product is linked to every target that emits diagnostics, and any new project has it available in its primary runtime target before its first log call. Search the actual package or Xcode dependency graph rather than relying on an `import` alone, and treat `print`, direct `Logger` instances, or duplicate logging backends as incomplete integration when they emit project diagnostics. When implementation is authorized, add or repair the dependency and target linkage and migrate affected calls while preserving the guide's ownership, subsystem, category, emoji, privacy, severity, and noise rules; report an exact blocker when target or platform constraints make safe integration ambiguous. -- Inspect dependency manifests, resolver or lock files, Xcode package references, vendored source or binary frameworks, and equivalent dependency declarations. Compare the change with the baseline and identify every new third-party dependency or expansion of an existing third-party dependency into a new target or runtime role. Apply the shared [external dependency policy](../../../Guidelines/Development.md#external-dependencies): require explicit repository-owner approval before the dependency is introduced and require the durable exception record in repository documentation. Do not infer approval merely from an execution plan, pull-request description, implementation convenience, package popularity, or the dependency already appearing in the diff. Treat an unapproved or undocumented third-party dependency as a blocker to completion. Do not flag Apple system frameworks, the Swift standard library, ThatFactory-owned packages, or guideline-mandated tooling used only for its documented tooling role. If a newly resolved transitive third-party package will be linked into or shipped with the product, verify that its owning direct dependency is covered by an approved exception rather than dismissing it solely because it is transitive. +- Inspect dependency manifests, resolver or lock files, Xcode package references, vendored source or binary frameworks, and equivalent dependency declarations. Compare the change with the baseline and identify every new third-party dependency or expansion of an existing third-party dependency into a new target or runtime role. Apply the shared [external dependency policy](../../../Guidelines/Development.md#external-dependencies): verify any explicit standing approval covers the exact dependency and role; otherwise require explicit repository-owner approval before the dependency is introduced and require the durable exception record in repository documentation. Do not infer approval merely from an execution plan, pull-request description, implementation convenience, package popularity, or the dependency already appearing in the diff. Treat an unapproved or undocumented third-party dependency as a blocker to completion. Do not flag Apple system frameworks, the Swift standard library, ThatFactory-owned packages, or guideline-mandated tooling used only for its documented tooling role. Apply the [networking standing approval](../../../Guidelines/Networking.md#standing-dependency-approval) only within its documented scope. If a newly resolved transitive third-party package will be linked into or shipped with the product, verify that its owning direct dependency is covered by an approved role and the transitive package fits that role rather than dismissing it solely because it is transitive. - Search dependency manifests, generated directories, project files, and documentation for CocoaPods or Carthage adoption. The shared [external dependency policy](../../../Guidelines/Development.md#external-dependencies) forbids both without an exception path and requires Swift Package Manager for package dependencies. When implementation is authorized, remove newly introduced adoption and its generated or configuration files; report pre-existing adoption as a completion blocker when safe migration is outside the task scope. - Check package configuration, CI/CD, Xcode project configuration, security-sensitive changes, and physical-device limitations when they are in scope. For every Xcode project, perform the project-settings audit below. Compare documented Swift and concurrency settings with the effective application and test-target settings; flag both redundant isolation annotations and missing annotations at compiler-verified boundaries. - Search for stale type names, superseded files, direct APIs forbidden by the new architecture, empty folders, and references to removed behavior. - Apply the shared [CI/CD guide](../../../Guidelines/CICD.md) to workflow and repository-automation changes. Search scripts, generated directories, workflow files, and documentation for fastlane adoption and treat it as forbidden without an exception path. Require focused ThatFactory tooling or repository-owned Swift scripts for CI/CD and delivery, and inspect new repository-owned executable scripts for compliance. For Swift-focused applications, games, and packages, accept Python or POSIX shell only when durable repository documentation identifies the missing Swift capability, exact script and task scope, runtime and dependency requirements, security and maintenance impact, validation method, and revisit or removal condition. Reject convenience, familiarity, shorter code, an existing interpreter, or another non-Swift script as justification. Treat every exception as narrow; the central `Scripts/swift_format.sh` wrapper is the retained documented exception for invoking Xcode's `swift-format` modes. -- For pull-request or merge readiness, apply the root `## Code Review Rules`: confirm the Codex review covers the current head, no allowed Codex review round is pending, every Codex review thread has a disposition, and no unresolved P0/P1 blocker remains. Treat P2/P3 observations as non-blocking and never request another Codex review unless the repository owner explicitly authorizes it. This Codex review-round budget does not apply to otherwise-authorized Reasoning Relay/ChatGPT review delegations; do not block them waiting for a Codex-budget exception. +- For pull-request or merge readiness, apply the root review and lifecycle contracts and [review-policy integration](../../../ReviewPolicy/README.md). Rerun the latest published policy check immediately before merge. Require a completed initial Codex review for its recorded head, or that review plus a recorded locally verified remediation-only delta; do not claim the newer head was bot-reviewed. Confirm no allowed review is pending, every thread is dispositioned and resolved, and no unresolved P0/P1 blocker remains. Treat P2/P3 observations as non-blocking and never request another review unless the owner explicitly authorizes it. Review-only adoption enforces only these contracts, not unrelated engineering audits. + +## Audit runner toolchains + +Apply the [runner toolchain policy](../../../Guidelines/CICD.md#runner-toolchain) when reviewing or changing Apple build automation, adopting that policy, or performing an Xcode upgrade: + +1. Inspect every applicable workflow and its invoked scripts, setup actions, environment overrides, and runner labels. Search for Xcode/Swift version comparisons, build-number checks, allowlists, upper bounds, and versioned installation paths; inspecting workflow YAML alone is insufficient when a script performs the check. +2. Verify ordinary CI uses and logs the runner's active toolchain. Distinguish version pins from minimum package tools versions, Swift language modes, SDK/deployment requirements, simulator destinations, and actual capability/readiness checks. A documented historical test version and a process-scoped selection for an explicit upgrade are not permanent CI constraints. +3. For each retained pin, inspect the canonical guide's required repository exception and evidence. Do not invent a compatibility reason or treat the previous upgrade target as one. When implementation is authorized, remove unjustified pins and reconcile current setup documentation; in review-only work, report their locations and missing justification. An unjustified pin in scope prevents audit completion. +4. Run affected toolchain reporting and the repository's build/test validation with the active installation. Record the selected Xcode/Swift versions, commands, outcomes, and any justified exception. Do not claim compatibility with untested future releases; removing a string check ensures those releases reach real validation. + +| Scenario | Audit outcome | +| --- | --- | +| New runner Xcode passes compilation and tests, but an old equality check fails | Remove the unjustified check and rerun affected validation | +| Workflow accepts the runner, but its invoked script requires one build number | Treat the script as the same pin and require removal or a documented exception | +| A supported package declares its minimum Swift tools version and language mode | Preserve the capability requirements | +| An exact toolchain has a documented incompatibility or external requirement with evidence and a revisit condition | Accept only the scoped exception and report it | + +## Audit Swift package badges + +When creating or changing a Swift package README, adopting the badge policy, or auditing package documentation, inspect the actual README badge URLs and labels against [README badges](../../../Guidelines/Packages.md#readme-badges). Omit Updated/last-commit-date and Revision/latest-release-version badges unless the repository documents the guide's specific exception. Distinguish release-version endpoints from Release/publishing workflow-status badges; keep the latter when they describe the package's own workflow. A Release label alone does not identify which kind it is. + +For authorized implementation, remove unjustified date/version badges and correct affected documentation; for review-only work, report the badge and missing rationale. An undocumented date/version badge in scope prevents audit completion. Record the inspected README, retained workflow-status badges, and any accepted exception. Do not create badges or release workflows solely to fill an inapplicable slot. + +## Audit networking + +Apply [Networking](../../../Guidelines/Networking.md) to Apple-platform applications, games, and Swift packages: + +1. Identify changed networking, dependency declarations, and directly affected existing request paths. On first adoption of this networking policy, inventory all existing network implementations and dependencies. For unrelated changes, do not expand into a whole-repository migration. Record evidence-backed `not applicable` when there is no networking in scope, or distinguish non-HTTP and framework-owned traffic from owned HTTP. +2. Inspect actual transport calls, request construction, response handling, package manifests, resolver files, Xcode product references, and target linkage. For owned HTTP, verify `HTTPTypes` and `HTTPTypesFoundation` from Apple's `swift-http-types` are available to the appropriate targets and that requests, responses, methods, header fields/names, and statuses use the guide's typed representations with native `URLSession`. Dependency presence or imports alone do not establish compliance. Verify the selected release and compatibility evidence against supported platforms and toolchains. +3. Inspect native Foundation bridges in context. Require the native API reason, conversions at that boundary, failure handling, and retained typed request/response ownership. Accept non-HTTP Apple APIs and framework-owned traffic without artificial HTTP wrappers. Search directly affected code for parallel string/integer HTTP models and hidden alternative transports; a first-party wrapper does not exempt its implementation. +4. For an alternative networking dependency, require the guide's demonstrated Apple capability gap, explicit repository-owner approval, durable scoped exception, and validation. An existing dependency is not approval for a new role. For adoption debt outside authorized migration scope, require an explicit owner-approved migration deferral identifying paths, rationale, and follow-up condition; report it as deferred compliance, never full networking compliance. A deferral does not authorize new noncompliant networking. +5. Require the guide's relevant deterministic request/response, status, failure, cancellation, and bridge tests for changed implementation. Record source paths, target/dependency evidence, exceptions or deferrals, commands, and outcomes. Documentation-only policy work uses scenario walkthroughs; do not invent live-network requirements for pure policy changes. +6. Missing typed implementation or native transport in scope, unapproved alternatives, undocumented bridges, unaccounted adoption debt, or missing applicable validation prevents audit completion. Fix authorized in-scope violations; report exact blockers instead of rewriting unrelated consumers. + +Use these scenarios to distinguish valid outcomes: + +| Scenario | Audit outcome | +| --- | --- | +| Owned HTTP uses typed requests/responses and both products at the appropriate targets with `URLSession` | Accept with implementation and validation evidence | +| Dependency/import is present but the service still owns untyped HTTP | Incomplete | +| Native background/delegate API uses a documented, tested Foundation conversion boundary | Accept with retained typed HTTP ownership | +| Only CloudKit, WebKit, or non-HTTP Apple networking is in scope | No artificial HTTP dependency; verify the relevant native boundary | +| Alternative library chosen for convenience, including behind a ThatFactory wrapper | Block until compliant or covered by the capability-gap exception | +| Adoption discovers untouched legacy HTTP | Migrate within scope or record an explicit owner-approved deferral; no full-compliance claim | +| No networking implementation or dependency change, and no policy adoption | Evidence-backed not applicable | ## Audit native components and presentation @@ -107,6 +154,21 @@ Apply the canonical [Reduce Motion policy](../../../Guidelines/Swift/SwiftUI.md# | No problematic motion across evaluated common tasks | Support declaration may pass with platform-specific evidence | | One compliant component or guideline adoption used to claim app-wide support | Declaration blocked pending common-task evaluation | +## Audit tvOS current-user identity + +Apply the [tvOS current-user and CloudKit guidance](../../../Guidelines/Xcode/DeviceInteraction.md#tvos-current-user-data-and-cloudkit) only when the change implements or materially affects selected-profile storage, CloudKit account routing, or personal-store transitions on tvOS. Documentation-only work and unrelated UI changes do not require a physical multi-user run. + +For applicable implementation, inspect the effective signed tvOS entitlement and provisioning privilege, then require privacy-safe real-device evidence from two distinct full Apple Accounts: native profile selection, fresh app process/build identity, the account verified by the CloudKit boundary, its bound store/checkpoint, and distinct A/B content across repeated A → B → A switches and a cold launch. A profile label, `shouldStorePreferencesForCurrentUser`, a new PID, unit tests, or an unsigned/simulator build does not establish account isolation. If CloudKit verifies one account while the store belongs to another, treat the host binding as a defect; if it verifies the default account under a selected secondary profile, preserve evidence and diagnose the device context before proposing app code. Do not retain raw account IDs, record IDs, credentials, or user-authored content in audit evidence. + +When the consumer's acceptance contract requires genuine two-account behavior and that evidence is absent or contradictory, report the gate as incomplete and withhold completion or merge readiness. Record an explicit consumer-approved deferral only when its product contract permits one; never silently substitute compilation for physical isolation. + +| Scenario | Audit outcome | +| --- | --- | +| A and B each verify distinct CloudKit accounts and stores through repeated native switches | Accept the isolation evidence for the tested build and path | +| B is selected but CloudKit verifies A despite a fresh, correctly entitled process | Block isolation acceptance and diagnose device profile context without app-side account guessing | +| CloudKit verifies B but the host opens A's store | Block as a host account-to-store binding defect | +| Only build, simulator, or profile-label evidence exists | Record the real-device isolation gate as unverified when applicable | + ## Audit Xcode project settings For every checked-in `.xcodeproj`, read and apply the shared [Xcode project-settings guide](../../../Guidelines/Xcode/ProjectSettings.md): @@ -211,6 +273,7 @@ Summarize: - the instruction and guideline areas audited; - consumer-integration validation and any drift found; +- networking applicability, typed/native implementation evidence, exceptions or migration deferrals, and relevant validation; - `.gitignore` reconciliation, including added shared patterns and every accepted or unresolved project-specific entry; - findings fixed during the audit; - documentation updated or removed, or why no documentation change was required; diff --git a/AgentGuidelines/.agents/skills/update-agent-guidelines/SKILL.md b/AgentGuidelines/.agents/skills/update-agent-guidelines/SKILL.md index c2abf3a..503e926 100644 --- a/AgentGuidelines/.agents/skills/update-agent-guidelines/SKILL.md +++ b/AgentGuidelines/.agents/skills/update-agent-guidelines/SKILL.md @@ -12,7 +12,7 @@ Turn the supplied instruction or decision into the narrowest durable shared rule - Work in the central `thatfactory/agent-guidelines` repository. Verify its Git remote and applicable [repository instructions](../../../AGENTS.md). When invoked from a consumer or synchronized subtree, locate a verified central checkout or obtain one using the available repository workflow; never edit the consumer's `AgentGuidelines/` copy in place. Ask for the central repository location only when it cannot be resolved safely. - Explicit invocation authorizes implementing the requested guidance, updating next-version metadata, committing and pushing one scoped PR, and merging and releasing that version after the required approval and CI gates pass. Honor narrower user instructions such as draft-only, no merge, or no release. Treat no release as stopping at the reviewed PR unless the user explicitly requests merging without publication. Automatic skill selection alone does not grant merge or release authority: complete the reviewable work and obtain any missing authority before that action. - Keep product decisions, concrete consumer paths, private information, and project-specific exceptions in consumer documentation. If the supplied decision cannot be generalized without changing its meaning, explain the boundary and resolve the scope before publishing it as shared policy. -- Use a requested reasoning/review workflow when available. If the user invokes Reasoning Relay, read and follow that skill for planning when needed and immutable PR review; this skill does not substitute for its transport, recovery, or approval rules. Do not silently replace a required review mechanism when it is unavailable. +- Follow the shared Codex PR lifecycle. Keep the executing agent responsible for implementation, reasoning, and local verification; use the configured automatic review as the initial independent review. Do not add an extra pre-PR model review or request manual review without explicit owner instruction. ## Resolve the decision and audit coverage @@ -32,7 +32,11 @@ Turn the supplied instruction or decision into the narrowest durable shared rule 1. Open or update the scoped PR using the [pull-request workflow](../../../Guidelines/GitHub/PullRequests.md). Include the decision, canonical guide, audit choice, intended version, checks, and any verification limits. 2. Obtain the required review for the actual PR and exact base/head. Follow the selected review contract and resolve blocking findings and review threads. Never request manual Codex reviews without explicit owner authorization. -3. Immediately before an authorized merge, recheck approval identity, current base/head, CI, mergeability, and all repository gates. Changed source invalidates the previous exact-head approval. Follow the governing workflow for a delta review or fresh review; do not bypass a pending required review or failed check. +3. Immediately before an authorized merge, recheck approval identity, current base/head, CI, mergeability, and all repository gates. For remediation after a completed initial review, record and locally verify the remediation-only delta without claiming bot coverage of the newer head. Follow the shared workflow for material scope or integration changes; do not bypass a pending required review or failed check. 4. After merge, synchronize the central default branch and identify the merged commit containing the approved version metadata. Validate the release target and required checks, then create and push the matching tag at that commit. Never move or overwrite a published tag. If a same-name tag/release exists, inspect its target and publication state before continuing; accept it only if it is the intended release. 5. Follow the repository's release automation. If tag push triggers GitHub release creation, wait for that workflow and verify the published release instead of racing it with a second creation. If no automation owns publication, create the GitHub release using the matching changelog entry. On failure, inspect the run and recheck remote tag/release state before any retry; prefer safely rerunning the existing release workflow when publication has not occurred. Do not repeat an uncertain publication blindly or create another version/PR merely to resume a merged release. 6. Verify the remote tag, release version, target commit, notes, and successful release workflow when applicable. Report the PR and release links, audit choice, and validation. Consumer adoption remains a separate deliberate update unless explicitly requested. + +## Review-policy publication + +When either canonical review contract changes, increment `ReviewPolicy/VERSION`, synchronize the central root, full template, and global reviewer section, and update policy tests. After the regular reviewed guidelines release is published, create the immutable `codex-review-vN` tag at the same release commit. Verify its remote target and include the authorized full/review-only consumer rollout; unrelated guidelines changes retain the policy revision. Use the central Swift synchronizer for managed blocks and never publish private consumer inventories here. diff --git a/AgentGuidelines/.agents/skills/xcode-upgrade/SKILL.md b/AgentGuidelines/.agents/skills/xcode-upgrade/SKILL.md new file mode 100644 index 0000000..af833a2 --- /dev/null +++ b/AgentGuidelines/.agents/skills/xcode-upgrade/SKILL.md @@ -0,0 +1,51 @@ +--- +name: xcode-upgrade +description: Upgrade a ThatFactory Apple-platform application or Swift package to a requested Xcode version, including dependencies, compiler settings, CI, validation, and optional project-format conversion. Use for /xcode-upgrade, moving to a new Xcode, or adopting a requested Xcode project format. +--- + +# Xcode Upgrade + +Deliver a coherent, verified toolchain upgrade in the current repository. In Codex, explicitly invoke `$xcode-upgrade to Xcode 27.2 and use the new JSON format` or select the skill. Also recognize `/xcode-upgrade ...` when supplied as request text by a client; this skill does not register a client slash command. Parse the requested version and stable, beta, or RC channel, and treat trailing instructions as scope and constraints. Ask only when the target or an incompatible requirement cannot be resolved from the request and repository evidence. + +Read linked guidance relative to this skill's real location in the central checkout or the consumer's `AgentGuidelines/` subtree. Keep concrete schemes, paths, runner identities, simulator choices, and exceptions in the consumer. + +## Establish the upgrade boundary + +1. Read the nearest applicable `AGENTS.md` and local build, testing, dependency, and delivery documentation. Follow the [guidelines-version workflow](../../../Guidelines/Development.md#guidelines-version) before implementation; verify the checked-in release and provenance when updating a subtree, and keep that update in its own commit. Never edit synchronized guidance in place. +2. Record the starting Git state and current Xcode/Swift versions. Inventory authored projects and workspaces, shared schemes and test plans, targets and configurations, checked-in `Package.swift` manifests and resolved files, configuration files, automation, CI toolchain selection, and documented supported platforms. Exclude generated checkouts and build output. For a package-only repository, skip project-document and simulator steps unless its actual tests require them. +3. Locate the requested Xcode and verify its version and build with `xcodebuild -version`, its Swift compiler with `xcrun swift --version`, and readiness with `xcodebuild -checkFirstLaunchStatus`, all under that installation's `DEVELOPER_DIR`. Use process-scoped `DEVELOPER_DIR` for subsequent commands; do not change machine-global `xcode-select` as part of the upgrade. If the toolchain or required component is missing, continue independent inspection and report the exact installation or first-launch prerequisite; do not claim target-toolchain validation. +4. Read the selected version's official [Xcode release notes](https://developer.apple.com/documentation/xcode-release-notes), relevant Swift release documentation, and installed command help. Distinguish compiler version, stable Swift language mode, SDK version, deployment target, and project-format compatibility. An Xcode upgrade does not itself request higher minimum OS versions or beta language features. +5. Present the concrete scope, including requested format changes, dependency candidates, settings migration, CI/runtime requirements, and validation. Continue changes already authorized by the user; obtain only genuinely missing decisions or authority. Skill invocation alone does not authorize remote runner administration, publication, distribution, or merging. + +## Align the toolchain and project document + +- Discover every current toolchain pin and version check in workflows, configuration, scripts, badges, and current-setup documentation. Apply the [runner toolchain policy](../../../Guidelines/CICD.md#runner-toolchain): validate the requested installation for this upgrade, then leave ordinary CI using the runner's active toolchain. Remove unjustified exact-version checks instead of updating them to the newly tested version, and retain a pin only with the guide's documented repository-specific exception. Preserve capability requirements and the repository's runner-label policy. Inspect Xcode Cloud configuration when used; a local workflow-file edit does not prove the remote workflow selected the new Xcode. +- Verify required runner capabilities and simulator runtimes through available authorized read-only tools. Keep infrastructure readiness distinct from repository correctness. Missing runner access or capacity is an explicit validation gap, not evidence that CI will pass. Never invent host addresses, require SSH universally, or change shared machines without authorization. +- Open each authored project in the selected Xcode and inspect **Editor > Validate Settings** with the project selected. Evaluate proposed changes against local exceptions and the canonical settings guides. Apply in-scope recommendations and inspect the resulting diff; report an unavailable editor check honestly. Use project-aware tooling and selected-Xcode output for document compatibility and upgrade markers, including shared schemes. Do not blindly stamp version markers to imply that validation occurred. +- If projects are generated, update the generator inputs and compatible tooling, then regenerate through the documented workflow instead of editing disposable output. + +### Requested JSON project conversion + +Convert only when the user requests it. Follow Apple's [project configuration format migration](https://developer.apple.com/documentation/xcode/updating-your-xcode-project-configuration-file-format) and verify support in the selected Xcode and the oldest Xcode the repository promises to support. + +1. Before converting, inventory tools that read or write the project: repository Swift scripts, version/release automation, project generators, linters, validators, CI paths, and external tooling. Search for `project.pbxproj`, `.pbxproj`, and assumptions about legacy keys. Verify support for the installed tool versions rather than copying a historical compatibility list. Adapt and validate repository-owned consumers within scope; legacy assumptions alone are not a blocker. Stop the conversion if a required consumer cannot be made compatible; explain the exact conflict and available options without silently abandoning the requested format. +2. Preserve a recoverable Git state and close the project in Xcode before a CLI conversion to avoid competing writes. Use Xcode's Project Format control or the conversion command supported by that installation's `xcodebuild -help`. Let Xcode generate `project.xcproj`; do not hand-translate the project or assume a strict JSON parser accepts its syntax. +3. Inspect the complete conversion diff for preserved targets, configurations, build phases, package references, resources, membership exceptions, signing, and entitlements. Update format-dependent automation and documentation in the same PR. Keep conversion separate from settings and dependency changes where practical so its effect can be reviewed independently. +4. Reopen the project, list its schemes/targets, resolve packages, and build before layering further changes. Exercise affected versioning, generation, and delivery tools through nonpublishing checks or disposable copies; restore only task-owned temporary changes. A successful Xcode build alone does not establish automation compatibility. If conversion fails, preserve the evidence and restore only conversion-owned edits before reporting the blocker. + +## Apply compiler settings and dependency updates + +- For every checked-in Xcode project, apply [Xcode project settings](../../../Guidelines/Xcode/ProjectSettings.md), including project-level ownership, effective values in all configurations and targets, selected-language-mode upcoming-feature discovery, and documented exceptions. Fix compiler diagnostics instead of weakening required warnings or concurrency settings to make the upgrade pass. +- For every in-scope Swift package, separately apply [the package compiler-settings baseline](../../../Guidelines/Packages.md#compiler-settings-baseline) using its evaluated manifest. Do not copy an application's default actor isolation or application-only settings into reusable packages. If a new toolchain requires a shared compiler-policy change, that central change needs its own authorization, package-parity, audit, validation, and reviewed release before consumer adoption. Continue only when that authority exists; otherwise report the dependency as incomplete rather than editing the subtree or silently expanding the task. +- Review available updates for the repository's actual package dependencies, build/documentation plugins, formatter/linter, and CI actions. Use release notes and declared compatibility to choose current compatible versions; update dependencies intentionally under [Packages](../../../Guidelines/Packages.md#consumer-updates) and the [external dependency policy](../../../Guidelines/Development.md#external-dependencies). Review major-version migrations and exact-pin history before changing requirements. Preserve deliberate pins with a reason, and avoid unrelated resolver churn. +- Check changed direct and transitive dependency roles, platform/toolchain requirements, licensing, security, and maintenance impact. Obtain the required approval and durable exception for new third-party runtime dependencies or expanded roles. Do not introduce fastlane, CocoaPods, or Carthage while adapting automation. New repository-owned executable automation follows the Swift policy in [CI/CD](../../../Guidelines/CICD.md#tooling-and-automation). +- Update requirement declarations and the appropriate checked-in resolved files together, and prove that the final remote dependency versions resolve without local-path overrides. Do not publish sibling packages merely to move a version pin; any necessary package change follows its own reviewed release workflow and authorization. + +## Verify and hand off + +1. Use isolated build output and explicit repository-supported destinations with the selected toolchain. Follow the declared formatter/lint workflow and resolve dependencies. Build a coverage matrix for every applicable declared project, scheme/test plan, package, supported platform, and Debug, Release, or custom configuration; run its builds/tests and document the reason for any exclusion. Require warning-clean results under the canonical baseline and report test execution counts so zero tests cannot pass as coverage. +2. Run applicable UI/snapshot suites and targeted runtime checks for changed behavior. Re-record only explained baseline differences after examining them; do not blanket-accept new snapshots or delete unaffected baselines. Follow [device interaction](../../../Guidelines/Xcode/DeviceInteraction.md) when runtime evidence is needed, and distinguish simulator, physical-device, signing, and distribution evidence. +3. Check format-dependent versioning/release tools without uploads, tags, distribution, or publication. Do not assume a lane or script is safe from its name; inspect its effects and use supported dry-run modes or disposable copies. Record unavailable checks and their impact. +4. Update current-setup documentation, README badges, toolchain prerequisites, supported developer/CI Xcode versions, simulator instructions, dependency integration notes, and format references that changed. Search for stale version strings and legacy file assumptions in context; preserve historical release and incident records. +5. Invoke [agent-guidelines-audit](../agent-guidelines-audit/SKILL.md), reuse fresh validation evidence, and run `git diff --check`. Report the requested and verified Xcode/build, Swift mode, format decision, dependency changes and retained pins, settings exceptions, CI/runner readiness, commands/results, and unresolved prerequisites. Keep unavailable required evidence visibly incomplete. +6. Commit and prepare the scoped PR when authorized, with logical separation of guideline adoption, project conversion, settings, dependencies, and automation where useful. Follow [the pull-request workflow](../../../Guidelines/GitHub/PullRequests.md) and any user-requested review mechanism. Merge, tag, release, or distribute only within the user's authorization and after the relevant gates pass. diff --git a/AgentGuidelines/.github/actions/codex-review-policy/action.yml b/AgentGuidelines/.github/actions/codex-review-policy/action.yml new file mode 100644 index 0000000..5a39c46 --- /dev/null +++ b/AgentGuidelines/.github/actions/codex-review-policy/action.yml @@ -0,0 +1,15 @@ +name: Validate Codex review policy +description: Verify managed AGENTS.md sections against the latest published ThatFactory review policy. +inputs: + profile: + description: Consumer adoption profile (full or review-only). + required: true +runs: + using: composite + steps: + - name: Check latest published review policy + shell: bash + env: + POLICY_ACTION_PATH: ${{ github.action_path }} + POLICY_PROFILE: ${{ inputs.profile }} + run: swift "$POLICY_ACTION_PATH/../../../Scripts/sync_codex_review_policy.swift" check --consumer-root "$GITHUB_WORKSPACE" --profile "$POLICY_PROFILE" diff --git a/AgentGuidelines/.github/workflows/ci.yml b/AgentGuidelines/.github/workflows/ci.yml index af5e61e..d52ab0d 100644 --- a/AgentGuidelines/.github/workflows/ci.yml +++ b/AgentGuidelines/.github/workflows/ci.yml @@ -20,4 +20,6 @@ jobs: - name: Validate run: | Tests/run_tests.swift + Tests/run_review_policy_tests.swift Scripts/validate_guidelines.swift + Scripts/sync_codex_review_policy.swift check --local-source . --profile central diff --git a/AgentGuidelines/.github/workflows/release.yml b/AgentGuidelines/.github/workflows/release.yml index 6e27cca..e35642c 100644 --- a/AgentGuidelines/.github/workflows/release.yml +++ b/AgentGuidelines/.github/workflows/release.yml @@ -19,7 +19,9 @@ jobs: - name: Validate guidelines run: | Tests/run_tests.swift + Tests/run_review_policy_tests.swift Scripts/validate_guidelines.swift + Scripts/sync_codex_review_policy.swift check --local-source . --profile central - name: Validate tag run: | diff --git a/AgentGuidelines/AGENTS.md b/AgentGuidelines/AGENTS.md index 8d4f74a..0f37c0a 100644 --- a/AgentGuidelines/AGENTS.md +++ b/AgentGuidelines/AGENTS.md @@ -43,23 +43,28 @@ This public repository is the versioned source of truth for reusable ThatFactory - `Scripts/swift_format.sh` is the sole retained shell-script exception because it is the existing command wrapper around Xcode's `swift-format`; do not use it as precedent for new shell automation. - Do not add Python, Ruby, JavaScript, or other scripting-language runtimes for repository automation. - + + ## Code Review Rules -Review for release-blocking defects introduced or materially exposed by the pull request. A clean review means no unresolved P0/P1 findings; it does not mean exhaustive or perfect software. +Report only substantiated P0/P1 defects introduced or materially exposed by this change. Require a concrete, reachable failure in a supported use case or documented threat model, with serious impact such as a security-boundary bypass, durable data loss or corruption, a crash or deadlock in an important workflow, loss of availability, or a serious compatibility regression. An acceptance-criterion violation alone does not establish P1 severity. -A blocking finding must identify a concrete, reachable path in a supported use case or the documented threat model that can cause a credible security-boundary bypass, durable data loss or corruption, a crash or deadlock, loss of availability, violation of an explicit acceptance criterion, or a serious compatibility regression. +For each finding, state severity, preconditions, execution path, impact, evidence, and actionable remediation. Group manifestations of one root cause. Report substantiated blockers together. Omit unsolicited P2/P3 observations, style preferences, speculative hardening, optional tests, and refactors. A clean review means no unresolved P0/P1 blockers, not perfect software. -For every blocking finding, state the severity, preconditions, execution path, impact, evidence, and actionable remediation. Group manifestations that share the same root cause into one finding. +A follow-up review covers unresolved P0/P1 findings, changes since the previous reviewed commit, and directly affected code. Do not restart a full review of unchanged code. New follow-up findings must be P0/P1 defects introduced by remediation or genuinely hidden by the previous blocker. + -Treat P2/P3 observations as non-blocking, including defense-in-depth, theoretical completeness, unsupported use cases, malformed state that trusted code cannot produce, behavior by components outside the threat model, style preferences, and speculative refactoring. Record a useful lower-severity observation once as deferred, declined, duplicate, or follow-up work; do not keep the review loop open for it. + +## Pull Request Lifecycle -In an initial review, report substantiated blockers together. A follow-up review is limited to unresolved P0/P1 findings, changes since the last reviewed commit, and code directly affected by those changes. Do not restart an unrestricted review of unchanged code. A new follow-up finding must be a P0/P1 defect introduced by the remediation or genuinely hidden by the previous blocker. +Finish implementation, inspect the diff, and run relevant local validation before opening a ready-for-review PR. Drafts are for incomplete preparation; mark ready before waiting for automatic review. Keep the review head stable while the initial review runs. One automatic Codex review is the initial review; never duplicate it with a manual request or repeatedly toggle draft/ready state. -The review-round budget below applies only to Codex GitHub reviews: the configured automatic Codex review and any manual `@codex review` request. It does not apply to ChatGPT review or reasoning delegated through Reasoning Relay. An otherwise-authorized Reasoning Relay workflow may request as many Relay review or follow-up delegations as its own governing workflow requires; those requests neither consume the Codex budget nor require repository-owner authorization under it. +Establish review configuration from repository-specific evidence. Verify the bot, PR, review round, and reviewed commit using PR/request-comment reactions, review submissions, inline threads, and checks. Eyes means processing, not approval; a thumbs-up means no suggestions only for its verified review round. Historical reactions, missing comments, and elapsed time never certify a newer head. Monitor at most five minutes total with bounded, non-accelerating polls; timeout leaves a required gate unresolved and never authorizes merge. -When repository-specific evidence establishes that automatic Codex review is enabled and applies to the current pull request/head, it is the initial Codex review. Otherwise, an absent review does not establish a pending gate. Do not request a manual Codex review unless the repository owner explicitly asks. Never request another Codex review after each remediation commit. Within the normal Codex review budget, at most one owner-authorized, delta-scoped Codex verification review may be requested under [the pull-request review workflow](Guidelines/GitHub/PullRequests.md). - +Batch accepted P0/P1 fixes, verify them locally, rerun affected checks, and disposition and resolve every review thread. A completed initial review plus an explicitly recorded, locally verified remediation-only delta can satisfy the review gate; do not claim Codex reviewed the newer head. New unrelated scope belongs in another PR. Request at most one delta-scoped verification only when the owner explicitly authorizes it. Do not request reviews after every fix or casually mention the bot to start extra work. Separate Security Review is manual and requires explicit owner instruction. + +Before merge, rerun the latest published review-policy check, confirm no required review is pending, no unresolved P0/P1 blocker remains, every thread is dispositioned and resolved, required checks pass on the current head, and existing human approval or merge authorization is satisfied. A bot thumbs-up is not a substitute for required GitHub approval. Opening a PR does not authorize merging it. + ## Validation @@ -67,13 +72,15 @@ Run: ```sh Scripts/validate_guidelines.swift +Tests/run_tests.swift +Scripts/sync_codex_review_policy.swift check --local-source . --profile central ``` Fix every validation failure before releasing a version. ## Consumer pull-request review scope -When reviewing a consumer pull request, do not review or comment on files under `AgentGuidelines/**` after exact tagged-tree provenance has been verified. That subtree is a tracked, synchronized copy marked `linguist-generated`; substantive guideline changes are reviewed in this repository. Verify the intended `AgentGuidelines/VERSION`, compare the subtree tree with the matching central tag (for example with `git subtree split --prefix=AgentGuidelines HEAD` and a tree comparison after fetching that tag), and verify the required `.gitattributes` rule. If provenance does not match exactly, review the subtree contents and stop the merge. Report substantive guideline feedback against the central `agent-guidelines` pull request instead. +When reviewing a consumer pull request, do not review or comment on files under `AgentGuidelines/**` after exact tagged-tree provenance has been verified. That subtree is a tracked, synchronized copy marked `linguist-generated`; substantive guideline changes are reviewed in this repository. Verify the intended `AgentGuidelines/VERSION`, compare the subtree tree with the matching central tag (for example by comparing `git rev-parse HEAD:AgentGuidelines` with the fetched tag's root tree), and verify the required `.gitattributes` rule. If provenance does not match exactly, review the subtree contents and stop the merge. Report substantive guideline feedback against the central `agent-guidelines` pull request instead. ## Releases @@ -81,3 +88,7 @@ When reviewing a consumer pull request, do not review or comment on files under - Create a Git tag and GitHub release matching `VERSION`. - Consumer repositories adopt releases deliberately through Git subtree updates. - Follow [the pull-request review workflow](Guidelines/GitHub/PullRequests.md) before merging any release change. + +## Repository review configuration + +Automatic Codex PR review is enabled. Use all PR authors and the opening/ready trigger, keep exhaustive review off, and keep separate Security Review manual. Apply the managed lifecycle and [review-policy integration](ReviewPolicy/README.md). diff --git a/AgentGuidelines/CHANGELOG.md b/AgentGuidelines/CHANGELOG.md index ac202c2..1ece3b4 100644 --- a/AgentGuidelines/CHANGELOG.md +++ b/AgentGuidelines/CHANGELOG.md @@ -2,6 +2,62 @@ All notable changes to this project are documented in this file. +## [0.0.48] - 2026-11-06 + +### Changed + +- Simplified the repository review declaration to state directly that automatic Codex PR review is enabled. The canonical review contracts and independent review-policy revision remain unchanged. + +## [0.0.47] - 2026-11-06 + +### Added + +- Added independently versioned Codex review policy with full and review-only adoption profiles, exact managed root contracts, a Swift synchronizer, and a pinned shared CI action. Latest-policy checks reject stale or modified content and uncertain provenance. +- Added regression coverage for synchronization, preservation of local instructions, profile boundaries, duplicate contracts, publication identities, and unavailable policy sources. + +### Changed + +- Restricted review output to substantiated P0/P1 defects, clarified one automatic opening/ready review with local verification of batched remediation, and retained bounded monitoring and explicit owner authorization for extra review. +- Made reviewer rules and PR lifecycle available directly in every root instruction file without adopting unrelated guidance. Removed retired delegated-review exceptions and aligned audit, release, and global instruction templates. + +## [0.0.46] - 2026-11-05 + +### Changed + +- Made the self-hosted runner's active Xcode/Swift toolchain the CI default, with environment logging and real build/test validation. Exact version or build pins require a documented, evidence-backed repository exception with a revisit condition. +- Updated the Xcode-upgrade skill to remove unjustified CI pins instead of moving them to the newly validated version. +- Clarified Swift package badges: omit Updated and Revision/latest-release-version badges unless a documented reader need justifies them; retain Release/publishing workflow-status badges for the package's own workflows. + +### Added + +- Added completion-audit checks for toolchain constraints in workflows and invoked scripts, documented exceptions, active-toolchain validation, and Swift package badge semantics. + +## [0.0.45] - 2026-11-04 + +### Added + +- Added the canonical Networking guide requiring Apple-native networking and typed HTTP through Apple's `swift-http-types` HTTPTypes and HTTPTypesFoundation products with URLSession transport, including native bridges and justified capability-gap exceptions. +- Added networking audit coverage for actual typed implementation, target linkage, native transport, validation, and explicit adoption deferrals without a false full-compliance claim. + +### Changed + +- Centralized network transport ownership and validation guidance with links from architecture, package, testing, and security guides. +- Updated the consumer external-dependency contract to v2 for the narrow standing approval of `swift-http-types`; consumers must synchronize the root contract on adoption. Other external runtime dependencies retain their approval requirements. + +## [0.0.44] - 2026-10-30 + +### Added + +- Added the reusable `xcode-upgrade` skill for intentional Xcode, dependency, compiler-settings, and CI upgrades in applications and Swift packages, including requested project-format conversion and evidence-based validation. +- Documented consumer discovery through a subtree skill symlink and explicit invocation with a target Xcode and additional instructions. + +## [0.0.43] - 2026-10-29 + +### Added + +- Documented a bounded physical Apple TV diagnostic for current-user CloudKit and local-store identity, including the observed secondary-profile account crossover, privacy-safe comparisons, profile re-addition, and limits on attributing a tvOS cause. +- Added audit coverage for real two-account evidence when a consumer changes tvOS per-user storage or CloudKit account binding; unrelated changes and documentation-only work remain outside that gate. + ## [0.0.42] - 2026-10-20 ### Changed @@ -184,7 +240,7 @@ All notable changes to this project are documented in this file. ### Changed -- Clarified that the bounded review-round policy applies to Codex GitHub reviews, while otherwise-authorized ChatGPT and Reasoning Relay delegations use their own workflow limits. +- Clarified that the bounded review-round policy applies to Codex GitHub reviews, while other explicitly selected advisory workflows used their own limits. - Namespaced Codex review tracking fields and advanced the consumer code-review contract to v2. ## [0.0.20] - 2026-08-21 diff --git a/AgentGuidelines/Guidelines/Architecture/Redux.md b/AgentGuidelines/Guidelines/Architecture/Redux.md index 4a28847..7c3920e 100644 --- a/AgentGuidelines/Guidelines/Architecture/Redux.md +++ b/AgentGuidelines/Guidelines/Architecture/Redux.md @@ -216,7 +216,7 @@ Do not put SwiftUI types, colors, images, localized display strings, or render-r Put focused external-boundary abstractions in `Services/`. Services wrap APIs, persistence, packages, frameworks, sensors, system features, and other impure operations. Keep this folder flat while a capability has only one file; introduce a familiar capability folder such as `Services/FaceService/` or `Services/CalibrationService/` when that capability genuinely requires several related files. Middleware calls services; views and reducers do not. -Prefer a protocol or otherwise injectable contract when a service must be replaced in tests. Keep transport-specific details behind the service boundary. +Prefer a protocol or otherwise injectable contract when a service must be replaced in tests. Follow [Networking](../Networking.md) for networking implementation, transport ownership, and validation. Keep a supporting delegate, adapter, or helper beside its service when only that capability uses it. Local ownership is clearer than promoting a service-private framework bridge to a global `Tools/` folder. diff --git a/AgentGuidelines/Guidelines/CICD.md b/AgentGuidelines/Guidelines/CICD.md index 5138aca..c96e469 100644 --- a/AgentGuidelines/Guidelines/CICD.md +++ b/AgentGuidelines/Guidelines/CICD.md @@ -2,7 +2,7 @@ ## Workflow principles -- Keep CI deterministic, reproducible, and aligned with the repository's supported Xcode, Swift, and platform versions. +- Keep CI deterministic and reproducible through declared inputs, compiler settings, and validation, using the [runner toolchain policy](#runner-toolchain). - Treat warnings introduced by a change as failures even when the compiler does not. - Prefer the smallest permissions required by each workflow and job. - For a new workflow, use the latest stable major version of every GitHub Action available at the time of creation. @@ -12,6 +12,26 @@ - Do not place secrets in workflow files, logs, fixtures, or command arguments that may be echoed. - Keep release workflows separate from pull-request validation when their permissions differ. +## Runner toolchain + +Use the active Xcode and Swift toolchain supplied by the self-hosted runner. Log the selected developer directory, Xcode version/build, and Swift version so failures identify the actual environment. Let dependency resolution, compilation, tests, and required validation expose real incompatibilities after an update; a different version string alone must not fail a job. + +Do not pin an exact Xcode release, build number, or bundled Swift compiler version by default. This applies to workflow conditions, equality or text checks, setup-action inputs, versioned installation paths, runner labels, and scripts invoked by CI. Do not replace an exact check with an allowlist or upper bound that still rejects a compatible newer toolchain. Preserve legitimate minimum tools versions, Swift language modes, SDK/deployment requirements, simulator destinations, and capability or first-launch checks. These express supported capabilities rather than the last environment an agent happened to validate. + +A version pin is permitted only for a concrete repository-specific reason documented in that repository's `AGENTS.md` or linked durable build/CI documentation. Record the affected workflows and scripts, exact constraint, demonstrated incompatibility or external requirement, evidence, and the condition for revisiting or removing it. A historical upgrade target or a generic claim of reproducibility is not sufficient justification. Keep the exception as narrow as the demonstrated requirement, and report it during the completion audit. + +Respect the runner's configured selection, including an inherited `DEVELOPER_DIR`. A process-scoped override for an explicitly requested toolchain investigation or upgrade is appropriate; it does not authorize adding a permanent CI pin or changing machine-global `xcode-select`. Record historical validation versions as evidence without turning them into future acceptance conditions. + +For example, a workflow can report its environment without comparing version strings: + +```yaml +- name: Log active toolchain + run: | + xcode-select -p + xcodebuild -version + xcrun swiftc --version +``` + ## Tooling and automation Fastlane is forbidden in every ThatFactory project and has no exception path. Build repository-owned CI/CD and delivery automation with focused ThatFactory tooling such as `xcode-cloud-mcp` and `app-store-connect-mcp`, plus Swift scripts where repository-specific orchestration is needed. diff --git a/AgentGuidelines/Guidelines/Development.md b/AgentGuidelines/Guidelines/Development.md index 557951d..38f650f 100644 --- a/AgentGuidelines/Guidelines/Development.md +++ b/AgentGuidelines/Guidelines/Development.md @@ -10,7 +10,7 @@ ThatFactory applications, games, and reusable packages are first-party by defaul Do not add a third-party dependency merely to save implementation time, reduce code volume, or avoid learning a platform API. Do not make an external dependency acceptable by hiding it behind a first-party wrapper. -An exception requires explicit approval from the repository owner for the specific dependency and use before changing the dependency graph. If approved, record the decision in durable repository documentation in the same change, such as the architecture document or an ADR. Record the dependency and source, purpose and target scope, why a native or first-party implementation is not appropriate, relevant license, security, and maintenance considerations, version or update policy when material, and the approval context. Do not infer approval from an execution plan, pull-request description, agent choice, or transient chat that merely mentions or uses the dependency. Regardless of where explicit approval occurs, reflect the exception in durable repository documentation. +Unless these guidelines explicitly pre-approve the dependency for that role, an exception requires explicit approval from the repository owner for the specific dependency and use before changing the dependency graph. If approved, record the decision in durable repository documentation in the same change, such as the architecture document or an ADR. Record the dependency and source, purpose and target scope, why a native or first-party implementation is not appropriate, relevant license, security, and maintenance considerations, version or update policy when material, and the approval context. Do not infer approval from an execution plan, pull-request description, agent choice, or transient chat that merely mentions or uses the dependency. Regardless of where explicit approval occurs, reflect the exception in durable repository documentation. An existing approved dependency does not authorize a different dependency. Expanding an existing third-party dependency to a new target or runtime role requires the same approval and documentation. Routine version updates that do not change the approved role remain governed by the intentional consumer-update workflow in [Packages](Packages.md). @@ -18,6 +18,8 @@ For this policy, a third-party dependency is externally maintained source or bin Tooling dependencies explicitly required by these shared guidelines, such as documentation or build plugins used only by tooling, are pre-approved for that documented role. They must not be linked into or shipped with product runtime targets unless the repository owner separately approves and documents that use. +For networking, follow [Networking](Networking.md), including its narrow [standing dependency approval](Networking.md#standing-dependency-approval) and capability-gap requirements. That approval does not make every Apple-maintained package a platform dependency. + CocoaPods and Carthage are forbidden in every ThatFactory project and are not eligible for the exception process above. Use Swift Package Manager for package dependencies. ## Guidelines version diff --git a/AgentGuidelines/Guidelines/GitHub/PullRequests.md b/AgentGuidelines/Guidelines/GitHub/PullRequests.md index e7456de..60f4309 100644 --- a/AgentGuidelines/Guidelines/GitHub/PullRequests.md +++ b/AgentGuidelines/Guidelines/GitHub/PullRequests.md @@ -1,226 +1,86 @@ # GitHub Pull Requests -Use this guide whenever creating, reviewing, updating, or merging a GitHub pull request. +Use this guide whenever creating, reviewing, updating, or merging a GitHub pull request. The self-contained root contracts come from [the canonical review policy](../../ReviewPolicy/Policy.md); [review-policy integration](../../ReviewPolicy/README.md) describes both adoption profiles and freshness checks. ## Before opening -- Review the complete diff and exclude unrelated changes. -- Keep each pull request to a coherent review unit with a bounded set of invariants. Split changes that combine independent architecture, persistence, security, transport, and CI concerns when they can be reviewed and delivered separately; do not split merely to minimize line count. -- State the supported use cases, explicit acceptance criteria, and relevant threat model for behavior whose review priority depends on those boundaries. -- For security guarantees based on enumerating formats or signatures, define the finite coverage contract and residual risk, or use a systemic boundary that enforces the guarantee without exhaustive enumeration. -- Follow the repository's pull-request template and local contribution instructions. -- Run the relevant local validation and document anything that could not be run. -- Open the pull request without auto-merge and keep it unmerged while automated or agent review is pending. Use draft state only when configured reviewers also run on drafts. -- When repository-specific evidence establishes that automatic Codex review is enabled and the current pull request/head meets its trigger, track that review round. Do not also post `@codex review` or make another manual request; duplicate reviews waste review capacity and tokens. Do not request a Codex review manually unless the user explicitly asks for one. +- Review the complete diff, keep one coherent review unit, and exclude unrelated changes. State supported use cases, acceptance criteria, threat-model boundaries, validation, and limitations in the PR description. +- Run relevant local validation and follow the repository's contribution instructions. Open a ready-for-review PR at a stable head without auto-merge. Drafts are appropriate for incomplete preparation, data approval, and source promotion; mark ready before waiting for automatic Codex review. +- Configure automatic review for all PR authors on opening a ready PR or marking ready, disable exhaustive review, and keep separate Security Review manual. Record actual repository configuration separately from the generic template; this template does not establish that automatic review is configured. +- One automatic full review is the initial review. Do not post a duplicate manual request, toggle draft/ready to force another round, or casually mention the bot. A mention can start extra cloud work. Manual review and Security Review require explicit owner instruction. ## Consumer subtree review scope -When reviewing a consumer pull request, do not review or comment on files under `AgentGuidelines/**` after exact tagged-tree provenance has been verified. The subtree is a tracked, synchronized copy marked `linguist-generated`; substantive guideline changes are reviewed in the central `thatfactory/agent-guidelines` pull request. Verify `AgentGuidelines/VERSION`, compare the subtree tree with the matching central tag (for example with `git subtree split --prefix=AgentGuidelines HEAD` and a tree comparison after fetching that tag), and verify the required `.gitattributes` rule. If provenance does not match exactly, review the subtree contents and stop the merge. Report substantive guideline feedback against the central pull request instead. +After exact tagged-tree provenance has been verified, do not substantively review `AgentGuidelines/**` in a consumer PR. Verify `AgentGuidelines/VERSION`, compare `git rev-parse HEAD:AgentGuidelines` with the matching central tag's root tree, and verify `.gitattributes` contains `AgentGuidelines/** linguist-generated`. A tree-object comparison avoids reconstructing subtree history. If provenance differs, review the discrepancy and stop merge. Substantive shared-policy feedback belongs in the central PR. Review-only consumers have no subtree to exclude. ## Review objective -Automated review identifies release-blocking regressions; it does not attempt to eliminate every possible improvement. +Report only substantiated P0/P1 regressions introduced or materially exposed by the change. P0 is a critical security failure, catastrophic durable data loss, or critical outage. P1 requires serious, concrete impact in a supported workflow or documented threat boundary. An acceptance-criterion violation alone does not establish P1. Omit unsolicited P2/P3 hardening, style, optional tests, speculative refactoring, and theoretical completeness. Group one root cause into one finding and report blockers together. -Classify findings by impact and reachable scope: +## Review evidence and gate -- **P0 — critical:** an actively exploitable critical security issue, catastrophic durable data loss, or critical production outage. -- **P1 — blocking:** a supported use case, explicit acceptance criterion, or documented threat-model boundary has a concrete reachable failure path that causes a security-boundary bypass, durable data loss or corruption, a crash or deadlock, loss of availability, or a serious compatibility regression. -- **P2 — non-blocking:** robustness, defense-in-depth, bounded edge cases, malformed state that trusted code cannot produce, unsupported scenarios, theoretical completeness, or useful hardening. -- **P3 — non-blocking:** style, naming, preferred refactoring, documentation polish, or optional test improvements. - -Only unresolved P0 and P1 findings block merge. A finding may be technically correct without being release-blocking. - -## Review gate - -Opening a pull request starts review; it does not authorize merging it. - -1. Establish the Codex review state from the evidence rules below. An absent review is pending only for a positively established current-head review round. Unknown or disabled configuration and no Codex activity do not block merge by themselves. -2. Record the reviewed head SHA and inspect all review summaries, inline threads, checks, and requested changes. -3. Assess each comment for technical correctness, severity, supported reachability, and root cause. -4. Give every thread one explicit disposition: `BLOCKER-P0`, `BLOCKER-P1`, `DEFER-P2`, `DEFER-P3`, `DECLINE`, or `DUPLICATE`. -5. Batch accepted P0/P1 corrections into one remediation pass and add regression coverage where reasonably possible. Lower-severity improvements may be included when they are small and clearly in scope, but they do not keep the review loop open. -6. Reply in the original thread with the disposition and either what changed or the concise technical reason for deferring, declining, or grouping it. -7. Resolve a thread only after its disposition is recorded. Reference a follow-up issue for deferred work when its value justifies one. -8. Rerun affected validation, then update the pull-request description so it matches the current implementation, validation, deferred work, and remaining limitations. -9. Recheck the pull request immediately before merge for late P0/P1 findings and check-state changes. - -When replying with a commit reference, write the commit hash as raw text without backticks (for example, the hash 185c04f should remain 185c04f). GitHub then auto-links the hash to the commit. - -A thumbs-up or clean Codex review satisfies the agent-review step, but it does not replace any human approval required by the repository. Do not enable auto-merge before all review gates are satisfied. - -### Codex review state and round budget - -#### Codex review evidence and state - -Track configuration and execution separately in the active task; these are not checked-in runtime files: +Track configuration and execution separately in the active task, without adding runtime journals to the repository: ```text codex_review_configuration = enabled | disabled | unknown -codex_review_configuration_evidence = | unset +codex_review_configuration_evidence = | unset codex_review_execution = not_started | scheduled | processing | completed -codex_review_execution_evidence = | unset -codex_review_expected_sha = -codex_review_completed_sha = | unset +codex_review_expected_sha = +codex_review_completed_sha = | unset +codex_initial_reviewed_base = | unset +codex_remediation_verified_sha = | unset ``` -Start at `unknown`, `not_started`, the current head SHA, and an unset completed SHA. The executing agent is not assumed to have access to OpenAI's automatic-review configuration. Only positive repository-specific evidence may change configuration to `enabled` or `disabled`: a durable declaration in the consumer's root `AGENTS.md` or other tracked repository policy, explicit repository-owner confirmation in the active task, an authoritative repository or organization setting observed through an available interface, or another source whose semantics explicitly establish this repository's configuration. A repository may declare that automatic Codex pull-request review is enabled or disabled; no declaration is required. Generic AgentGuidelines or template wording, an automation prompt, another repository's setting, and a previous pull request's behavior are insufficient. - -Execution evidence must identify the active pull request and review round: a current-PR Codex processing reaction or equivalent event, a current review request/event with unambiguous round semantics, a submitted review covering the expected head, or another authoritative GitHub/OpenAI signal identifying the current PR/head. Absence of a review or reaction and elapsed time are not evidence. In particular, `unknown` plus no signal remains `not_started`, never `scheduled` or pending. - -Discovering `disabled` leaves execution `not_started` when no current activity exists. Discovering `enabled` alone does not schedule a review. Set `scheduled` only when positive evidence also establishes that the current PR/head meets the automatic trigger; start the bounded signal monitor then. A current-PR/current-head processing signal advances to `processing`. A completed review may advance `scheduled` or `processing` to `completed`; record its commit SHA when available. The gate is satisfied only when the reviewed SHA equals the expected SHA or another authoritative signal proves coverage of that head. A review for an earlier head is stale. +Only positive repository-specific evidence establishes configuration: an observed setting, a durable local declaration, or explicit owner confirmation for this repository. Generic templates, another repository, old PR behavior, and an automation prompt are insufficient. With no signal, `unknown` plus no signal remains `not_started`. Discovering `enabled` alone does not schedule a review; the PR must meet its configured trigger. A declared required review with a conflicting disabled setting is a configuration problem to resolve, not a waived gate. -When the head changes, set `codex_review_expected_sha` to the new head and clear `codex_review_completed_sha`. Terminate monitoring of the old head. Re-establish execution evidence for the new head; do not transfer `scheduled`, `processing`, or `completed`, or infer a new automatic round from the old one. - -Apply these outcomes to the current head: +Verify bot identity, repository, PR, review round, and commit. Inspect PR-body reactions for automatic reviews and the exact request-comment reactions for authorized manual reviews, plus review submissions, inline threads, checks, and failure comments. Eyes is processing, not approval. A thumbs-up means no suggestions only when tied to the verified round and stable head; a historical reaction never certifies a newer head. A review with a commit ID must match the expected round SHA. Neither absent comments nor elapsed time establishes completion. | Evidence | Execution | Merge gate | Start monitor | | --- | --- | --- | --- | -| Enabled with a current-head processing signal | `processing` | Pending | No; processing already began | +| Enabled and positively eligible, no start signal | `scheduled` | Pending | Yes, at most five minutes | +| Enabled with a current-head processing signal | `processing` | Pending | Continue within original deadline | | Enabled with a completed review covering the expected head and dispositioned findings | `completed` | Satisfied | No | | Unknown with no signal or configuration evidence | `not_started` | Not blocked by absent Codex review | No | | Disabled with no review | `not_started` | Not blocked by absent Codex review | No | -| Review completed for an old head | Re-establish for new head | Old review does not satisfy the gate | No inherited monitor | -| Enabled and positively eligible, with no start signal | `scheduled` | Pending while required | Yes, at most five minutes | - -For repeated identical snapshots, retain the fingerprint and poll count without re-analysis or notification. A signal inside the budget advances to `processing` while preserving configuration evidence and expected SHA. If the head changes during monitoring, terminate the old monitor and re-establish the new head's review state. If no signal appears by the deadline, terminate monitoring and report the unresolved verified state once; timeout never satisfies the gate. - -This round budget applies only to Codex GitHub reviews: the configured automatic Codex review and any manual `@codex review` request. It does not apply to ChatGPT review or reasoning delegated through Reasoning Relay. An otherwise-authorized Reasoning Relay workflow may request as many Relay review or follow-up delegations as its own governing workflow requires; those requests neither consume this Codex budget nor require repository-owner authorization under it. Do not block an agentic goal waiting for a Codex-budget exception before issuing an otherwise-authorized Reasoning Relay request. - -Track enough Codex-review state to prevent duplicate requests and unbounded Codex review loops: - -```text -codex_initial_review_sha -codex_last_reviewed_sha -codex_review_requested_sha -codex_review_round -codex_pending_review -codex_unresolved_p0 -codex_unresolved_p1 -codex_deferred_findings -``` - -The automatic Codex review is the one initial full Codex review. Do not request another Codex review after each fix. A repository owner may explicitly authorize at most one delta-scoped Codex verification review after the known P0/P1 findings have been batch-remediated. - -Before sending that Codex request, verify that no Codex review is pending, no existing request targets the current head SHA, the current head differs from `codex_last_reviewed_sha`, and the Codex verification-round budget is unused. Persist `codex_review_requested_sha`, increment `codex_review_round`, and mark `codex_pending_review` before waiting for a result so a retry cannot submit a duplicate request. - -When authorized, scope the Codex verification request explicitly: +| Review completed for an old head | Re-establish for new head | Historical reaction alone is insufficient | No inherited monitor | +| Completed initial review plus a locally verified remediation-only delta | `completed` | Satisfied after all blockers and checks are cleared | No new review | -```text -@codex review only unresolved P0/P1 findings and changes since . -Do not search unchanged code for new P2/P3 issues. -``` - -Do not request a third Codex review or restart a full Codex review without separate, explicit repository-owner authorization and a named unresolved P0/P1 concern. This restriction does not cap Reasoning Relay/ChatGPT review delegations. A new finding in Codex verification must be a P0/P1 defect introduced by the remediation or genuinely hidden by the previous blocker. +For the disabled outcome, the repository must not independently require Codex review. None of these outcomes grants merge authority or replaces required checks or human approval. -Stop the review loop when no unresolved P0/P1 finding remains, every thread has an explicit disposition, required checks pass, and required human authorization is present. Zero comments, zero possible improvements, and zero technical debt are not completion criteria. +### Remediation and convergence -### Codex review monitoring +1. Record the initial reviewed base/head and inspect every review summary and inline thread. Classify each thread as `BLOCKER-P0`, `BLOCKER-P1`, `DEFER-P2`, `DEFER-P3`, `DECLINE`, or `DUPLICATE`. +2. Batch accepted P0/P1 fixes and add meaningful regression coverage where appropriate. Record useful unsolicited lower-priority feedback once; it does not keep the review loop open. +3. Reply in the original thread with its disposition and the change or technical rationale. Resolve only after recording the disposition. Write commit hashes as raw text, for example 185c04f, so GitHub links them. +4. Rerun affected validation, inspect the complete delta from the actual reviewed head, and record the final verified SHA and why the delta only remediates known findings. The completed initial review remains evidence for its original head; never relabel the newer head as bot-reviewed. +5. A completed initial review plus a locally verified remediation-only delta satisfies the agent-review step when no unresolved P0/P1 blocker remains. New unrelated functionality belongs in another PR. A material base/integration change requires renewed assessment and an owner-authorized targeted review when the original review no longer covers the risk. +6. Update PR metadata and immediately before merge rerun the latest review-policy check, inspect late findings and current-head checks, and verify existing merge authority. A thumbs-up is not a formal GitHub approving review. -Monitor an automatic review for at most five minutes total from entry into `scheduled`, including time spent in `processing`. Take an initial snapshot, then at most one each around 30, 90, 180, and 300 seconds; equivalent non-accelerating schedules are allowed if they stop by five minutes. Do not create an indefinite recurring automation. A processing signal changes the execution state but does not reset the deadline or poll budget. A completion signal ends monitoring. Review completion is not assumed to occur within five minutes. +Never request another review after each fix. The owner may explicitly authorize at most one delta-scoped verification after batching remediation. Before requesting it, confirm no review is pending, no request targets that head already, the head differs from the last reviewed SHA, and the verification budget is unused. Record the requested SHA and round before waiting. Scope the request to unresolved P0/P1 findings, changes since the actual reviewed SHA, and directly affected code. A new verification finding must be a P0/P1 defect introduced by remediation or genuinely hidden by the prior blocker. A third review requires separate explicit owner authorization and a named unresolved P0/P1 concern. -A temporary monitor owns `started_at`, `deadline`, `poll_count`, `last_state_fingerprint`, and `last_observed_state`; discard them when it ends. Fingerprint at least repository, PR number, base SHA, head SHA, configuration, execution, signal state, review commit SHA, review-thread state, required checks, and mergeability. An identical fingerprint causes no substantive re-analysis or user notification. Continue only within the deadline. +### Bounded monitoring -At the deadline, stop and terminate the monitor, report the observed facts once, and never infer approval. Keep `scheduled` only if positive current-head scheduling evidence remains; retain `processing` only while its current-head signal remains valid. Otherwise use the strongest evidence-supported state. Do not continue polling or block unrelated work. If a positively established review remains a required merge gate, surface that unresolved gate to the user. +Monitor at most five minutes total from entry into `scheduled`, including time spent in `processing`. If processing is first observed without a known scheduling time, start the single bounded observation window then. Take an initial snapshot and at most one each around 30, 90, 180, and 300 seconds. A processing signal changes the execution state but does not reset the deadline or poll budget. A completion signal ends monitoring. -Any generated monitor prompt must preserve repository, PR number, expected head SHA, configuration and its authoritative evidence, current execution state, monitor start, and deadline. Without configuration evidence, do not create a monitor for an absent review. A later heartbeat must not reconstruct `enabled` from its own prompt. +Keep the expected SHA, request identity, start, deadline, poll count, and a fingerprint covering base/head, signals, review commit, threads, checks, and mergeability. An identical fingerprint causes no substantive re-analysis or user notification. Without configuration evidence, do not create a monitor for an absent review. If the head changes during monitoring, terminate the old monitor; do not transfer `scheduled`, `processing`, or `completed` to a new round or infer that a push triggers another review. -Use GitHub review data, reactions, and checks together. An eyes reaction means Codex is processing the pull request; it is not an approval. A thumbs-up means the review completed without suggestions. A submitted review means its inline threads must be assessed individually. - -```text -PR opened at stable head - | - v -One automatic full review - | - +--> thumbs-up ----------------> No P0/P1 blockers - | - `--> Review comments ----------> Classify and group - | - batch P0/P1 fixes - | - owner-authorized delta review? - | | - no yes - | | - stop one verification pass - | - no unresolved P0/P1 - | - stop -``` +At the deadline, stop and terminate the monitor, report the observed facts once, and never infer approval. An unresolved required round remains a merge blocker. Do not submit another request, create a recurring automation, or start fresh observation windows to evade the budget. Continue independent work or report the unresolved gate. -When using the GitHub CLI, monitor all three surfaces: +Use all relevant GitHub surfaces with complete pagination: ```sh -gh api --paginate repos///issues//reactions -gh pr view --repo / --json reviews,headRefOid -gh pr checks --repo / +gh api --paginate repos///issues//reactions +gh api --paginate repos///issues/comments//reactions +gh pr view --repo / --json reviews,headRefOid,baseRefOid +gh pr checks --repo / ``` -Retrieve inline review threads and their resolution state through GraphQL; top-level pull-request comments do not include this information: - -```sh -gh api graphql --paginate \ - -f query='query($owner: String!, $repository: String!, $number: Int!, $endCursor: String) { - repository(owner: $owner, name: $repository) { - pullRequest(number: $number) { - reviewThreads(first: 100, after: $endCursor) { - nodes { id isResolved } - pageInfo { hasNextPage endCursor } - } - } - } - }' \ - -F owner= \ - -F repository= \ - -F number= -``` - -For every unresolved thread identifier returned above, retrieve its complete comment history with a second paginated query: - -```sh -gh api graphql --paginate \ - -f query='query($thread: ID!, $endCursor: String) { - node(id: $thread) { - ... on PullRequestReviewThread { - comments(first: 100, after: $endCursor) { - nodes { id author { login } body url } - pageInfo { hasNextPage endCursor } - } - } - } - }' \ - -F thread= -``` - -Poll for automatic start or completion only under the five-minute ceiling above. After a verified processing signal, inspect every returned page for reactions, review threads, and thread comments when checking completion. Do not treat missing comments, a pending reaction, truncated results, or elapsed time as review completion, and do not submit a duplicate request merely because polling has not completed. - -## Merge method - -ThatFactory repositories use squash merges by default. Do not attempt a merge commit; GitHub rejects that method in these repositories, and retrying with squash wastes execution time and tokens. Use the GitHub UI or `gh pr merge --squash` after all review, approval, and check requirements are satisfied. Use another merge method only when the repository explicitly allows it and the owner authorizes the exception. - -## Merge requirements - -Do not merge while any of the following is true: - -- a positively established current-head Codex review round is `scheduled` or `processing` and remains a required gate; -- an unresolved P0/P1 finding remains; -- a review thread lacks an explicit disposition or remains unresolved; -- a required check is pending or failing; -- the branch is out of date when the repository requires an up-to-date branch; -- required human approval or explicit owner authorization is missing. - -## Late findings - -If a review arrives after merge, assess and disposition its findings. A valid late P0/P1 finding requires prompt remediation through a corrective pull request and indicates that a review gate was missed. A late P2/P3 observation becomes backlog work when useful and is not by itself a process failure. +Use GraphQL `reviewThreads` with `isResolved` and paginated comment histories. Top-level PR comments omit inline thread state. Missing pages, unresolved threads, a processing reaction, and a timeout are never approval. -## Repository protection +## Merge and late findings -Prefer GitHub rulesets or branch protection for the default branch. At minimum: +Opening a PR does not authorize merging it. Squash merge only after current-head required checks, resolved/dispositioned threads, required review completion or verified remediation closure, and existing owner/human authorization are satisfied. Do not enable auto-merge while a gate remains pending. Prefer rulesets requiring PRs, conversation resolution, mandatory checks, and an intentional emergency bypass path. Require an up-to-date branch where repository policy calls for it. -- require changes to arrive through a pull request; -- require conversations to be resolved before merging; -- require the repository's mandatory status checks; -- prevent bypass except for an intentional emergency path. +GitHub does not count author self-approval. In a solo repository, do not impose an impossible one-approval gate; preserve explicit owner authorization and enforce the available technical gates. Review-policy validation is a required hosting check, not a substitute for code review. -A formal one-approval rule works only when someone other than the pull-request author can submit an approving review. In a solo repository where the owner account also authors pull requests, use a bot or service account for authored changes before requiring owner approval; GitHub does not count self-approval. Until that separation exists, require explicit owner authorization operationally and keep conversation resolution enforced technically. +Assess late findings individually. A valid late P0/P1 finding requires prompt corrective work and indicates a missed gate. A late P2/P3 observation can become backlog work and is not itself a process failure. diff --git a/AgentGuidelines/Guidelines/Networking.md b/AgentGuidelines/Guidelines/Networking.md new file mode 100644 index 0000000..ca965f7 --- /dev/null +++ b/AgentGuidelines/Guidelines/Networking.md @@ -0,0 +1,65 @@ +# Networking + +Use this guide for networking in ThatFactory Apple-platform applications, games, and Swift packages. These are repository requirements; official Apple documentation defines API behavior and availability. + +## Apple networking APIs + +- Use Apple's native networking APIs. Use Foundation's `URLSession` for HTTP and HTTPS transport, including data, upload, download, and streaming tasks. +- Use the appropriate Apple framework for other networking needs, such as Network for supported lower-level protocols or `URLSessionWebSocketTask` for WebSockets. Verify support on every deployment target; do not build an HTTP transport on sockets when `URLSession` covers the requirement. +- Preserve framework-owned networking, such as CloudKit or WebKit operations. Do not replace those APIs merely to expose their internal HTTP traffic. +- Do not add a third-party networking client, transport, wrapper, or protocol stack when Apple covers the required capability. A first-party wrapper must not conceal such a dependency. Follow the [capability-gap exception](#capability-gap-exceptions) before introducing an alternative. + +## Typed HTTP with native transport + +For HTTP requests and responses owned by the application or package, add Apple's `swift-http-types` package through Swift Package Manager and use its `HTTPTypes` and `HTTPTypesFoundation` products with native `URLSession` transport. `HTTPTypes` supplies HTTP values; `HTTPTypesFoundation` supplies Foundation conversion and `URLSession` conveniences. Neither product replaces the transport. + +Use the following types throughout the owned HTTP boundary: + +| HTTP concept | Required representation | +| --- | --- | +| Request | `HTTPRequest` | +| Response | `HTTPResponse` | +| Method | `HTTPRequest.Method`, such as `.get` or `.post` | +| Header fields and names | `HTTPFields` and `HTTPField.Name` | +| Response status | `HTTPResponse.Status`, including named values or status kinds as appropriate | + +Do not maintain parallel string-based method/header models or integer status enums where these types apply. Custom protocol methods, header names, and statuses still use the extensible HTTP types; header values and encoded bodies remain protocol data. Validate response status before treating a completed transfer as application success. + +Declare both products in the package or Xcode dependency graph and link them to the targets that use them. A pure HTTP-model target may depend only on `HTTPTypes`; the Foundation transport target also needs `HTTPTypesFoundation`. An import, unused dependency, or dependency declaration without actual typed request and response handling does not satisfy this policy. + +Use the typed `URLSession` overloads where they support the operation. When a native API requires `URLRequest` or `HTTPURLResponse`, such as a delegate, background-task, or Foundation-only request-configuration boundary, use `HTTPTypesFoundation` conversion there and retain typed HTTP in the code that owns request construction and response handling. Document the native API reason, handle failed conversions, and preserve bodies and required transport configuration. A Foundation bridge needs no third-party exception and is not permission to retain an untyped HTTP implementation throughout the service. + +The [1.8.0 release](https://github.com/apple/swift-http-types/tree/1.8.0) is the minimum baseline for this policy, for example `.package(url: "https://github.com/apple/swift-http-types.git", from: "1.8.0")`. Its manifest requires Swift tools 6.1. Select a compatible released version, verify its Swift and deployment requirements, and record the version requirement and resolution according to the [package update workflow](Packages.md). Do not silently lower consumer platform support or substitute another library to resolve an incompatibility. + +### Standing dependency approval + +Apple's `swift-http-types` products `HTTPTypes` and `HTTPTypesFoundation` are pre-approved for the typed HTTP and Foundation integration role above. Consumers do not need a separate owner exception for that role. Record the dependency, source, target linkage, and version policy in normal package or architecture documentation. This approval does not extend to other Apple open-source packages, third-party networking libraries, or unrelated runtime roles; the general [external dependency policy](Development.md#external-dependencies) still applies. + +Do not add these products to a target that has no owned HTTP behavior merely because it uses a network-backed Apple framework or a first-party service. Non-HTTP networking and framework-owned traffic follow the native-API rule without requiring artificial HTTP wrappers. These exclusions do not exempt external SDKs or a first-party service's own networking implementation from dependency and networking policy. + +## Ownership and validation + +Keep transport-specific details behind the service or package boundary that owns networking. Use an injectable boundary for deterministic tests. Follow [Redux service ownership](Architecture/Redux.md#services) where applicable and the shared [testing guide](Testing/UnitTesting.md) for test structure. + +- Verify typed request construction, headers and body encoding, expected success and failure statuses, transport errors, and cancellation for the changed behavior. Test conversion failures when a Foundation bridge is involved. +- Use controlled responses or local fixtures for ordinary tests; do not make normal CI depend on an external service. Run a live integration check only when needed to verify behavior fixtures cannot establish, and report its limits. +- Preserve native transport security and authentication behavior. For security-sensitive changes, follow [Xcode security audits](Xcode/Security.md) and record affected network configuration and relevant runtime evidence. +- Follow [Logging](Logging.md) for privacy-safe diagnostics owned by the networking implementation; do not expose credentials, headers containing secrets, or user content. + +## Capability-gap exceptions + +An alternative networking dependency is eligible only when a required capability is unavailable through Apple's APIs on the supported platforms. Convenience, familiarity, popularity, and reduced code volume are not capability gaps. Check current official Apple documentation and demonstrate the missing behavior before proposing an exception; do not assume Apple covers every possible protocol or deployment constraint. + +Before changing the dependency graph, obtain explicit repository-owner approval for the specific alternative and role under the [external dependency policy](Development.md#external-dependencies). Keep a durable consumer exception identifying the requirement, Apple APIs evaluated and their concrete limitation, affected platforms and targets, dependency/source, license/security/maintenance impact, version policy, validation, approval context, and condition for revisiting the exception. An undocumented or unapproved alternative prevents audit completion. + +## Adoption and audit + +New or changed networking must follow this guide, including directly affected existing request paths. On adoption of this networking policy, inventory existing networking and either migrate noncompliant paths within authorized scope or record an explicit owner-approved, scoped migration deferral with rationale, affected paths, and a follow-up condition. Existing code alone is not an exception; do not silently rewrite unrelated consumers or claim full compliance while a deferral remains. + +Use the [networking completion audit](../.agents/skills/agent-guidelines-audit/SKILL.md#audit-networking) to record applicability, dependency and implementation evidence, native bridges, exceptions or migration deferrals, and validation results. + +## Official references + +- [URLSession](https://developer.apple.com/documentation/foundation/urlsession). +- [Network](https://developer.apple.com/documentation/network). +- [Swift HTTP Types and Foundation integration](https://github.com/apple/swift-http-types/tree/1.8.0). diff --git a/AgentGuidelines/Guidelines/Packages.md b/AgentGuidelines/Guidelines/Packages.md index 2de2bec..5580a07 100644 --- a/AgentGuidelines/Guidelines/Packages.md +++ b/AgentGuidelines/Guidelines/Packages.md @@ -10,7 +10,7 @@ Start a new ThatFactory project or package README with a centered HTML badge blo

``` -For example, a repository using all supported badge configurations could use: +For example, a repository using the baseline and relevant optional badge configurations could use: ```html

@@ -25,8 +25,6 @@ For example, a repository using all supported badge configurations could use: Claude MCP DocC License - Updated - Revision CI Publish Nightly @@ -42,16 +40,15 @@ Use only badges that describe the repository, in this order: 5. Relevant agent or tooling badges, such as Xcode MCP, Codex, or Claude. 6. DocC, documentation. 7. License. -8. Updated date. -9. Revision or latest release. -10. CI badges. -11. Release/publishing status when applicable. +8. CI badges. +9. Release/publishing workflow status when applicable. -The common package baseline is Swift, Xcode, Platforms, License, and CI. Add optional badges only when they convey useful repository-specific information. Keep the order stable even when some positions are omitted. +The common package baseline is Swift, Xcode, Platforms, License, and CI, plus a Release/publishing workflow-status badge when the package has that workflow. Add optional badges only when they convey useful repository-specific information. Keep the order stable even when some positions are omitted. + +Swift package READMEs omit Updated/last-commit-date and Revision/latest-release-version badges by default. Add or retain one only when the repository's `AGENTS.md` or linked durable documentation records the specific badge, its source, and the reader need that justifies the exception. A publishing workflow's success/failure badge remains part of the normal convention; it reports operational status rather than a revision or release version. An approved date/version badge belongs after License and before CI, uses a dynamic source, and must not require manual version/date edits. - Point CI, publishing, and documentation badges at workflows in the current repository; never copy another repository's badge URL unchanged. - Use descriptive `alt` text. Preserve a repository's established Xcode badge convention when the label intentionally records the last verified Xcode version. -- Prefer dynamic Updated and Revision badges backed by repository history or releases so maintainers do not edit dates and versions by hand. - Do not advertise a platform, integration, package manager, or agent that the repository does not support. - Keep the repository's license in a root `LICENSE` file when reuse or redistribution is permitted. - Retain the README license badge when the standard badge set applies, but do not add a dedicated License heading or license-description section to a package README. GitHub already presents the repository license beside the README, and the badge provides the summary without duplicating license prose. @@ -62,7 +59,7 @@ The common package baseline is Swift, Xcode, Platforms, License, and CI. Add opt - Keep a reusable package focused on one coherent capability. - Prefer UI-agnostic domain APIs unless UI is the package's explicit purpose. - Do not add application Redux, navigation, persistence, or product policy to a generic package. -- A first-party package must not introduce or conceal a third-party runtime dependency. Follow the [external dependency policy](Development.md#external-dependencies) before changing the dependency graph. +- A first-party package must not introduce or conceal a third-party runtime dependency outside an explicitly approved role. Follow the [external dependency policy](Development.md#external-dependencies) before changing the dependency graph and [Networking](Networking.md) for network implementation and its standing approval. - Keep public APIs minimal and stable. Prefer composing focused types over introducing umbrella abstractions before multiple consumers need them. - Declare platform and Swift toolchain requirements explicitly in `Package.swift`. - New Swift packages must start on the latest supported Swift language and toolchain version. Before adding a major package capability to an older package, plan and complete the required Swift/toolchain modernization first. diff --git a/AgentGuidelines/Guidelines/Testing/UnitTesting.md b/AgentGuidelines/Guidelines/Testing/UnitTesting.md index a694331..cc14fb3 100644 --- a/AgentGuidelines/Guidelines/Testing/UnitTesting.md +++ b/AgentGuidelines/Guidelines/Testing/UnitTesting.md @@ -50,3 +50,5 @@ - Report which tests ran and whether any relevant tests could not be executed. Follow [Xcode MCP](../Xcode/MCP.md) for build and runtime verification. + +Follow [Networking](../Networking.md#ownership-and-validation) for networking test boundaries and required evidence. diff --git a/AgentGuidelines/Guidelines/Xcode/DeviceInteraction.md b/AgentGuidelines/Guidelines/Xcode/DeviceInteraction.md index 50a0794..da9267c 100644 --- a/AgentGuidelines/Guidelines/Xcode/DeviceInteraction.md +++ b/AgentGuidelines/Guidelines/Xcode/DeviceInteraction.md @@ -66,6 +66,16 @@ Before testing a software-keyboard issue, inspect **Device → Keyboard** and re Focus the device canvas before sending Mac keyboard input, and check where it lands. For tvOS, move focus with arrow keys, activate with Return, and go back with Escape; use the current Device Hub controls or simulated remote for actions such as Home and Play/Pause. tvOS uses focus navigation, not touchscreen coordinate taps. Product-specific navigation and fixture instructions remain in the consumer repository. +### tvOS current-user data and CloudKit + +Apple's [current-user guidance](https://developer.apple.com/documentation/tvservices/personalizing-your-app-for-each-user-on-apple-tv) and [multi-user sample](https://developer.apple.com/documentation/tvservices/mapping-apple-tv-users-to-app-profiles) say that the User Management capability gives each selected tvOS user separate preferences and iCloud data. For tvOS 16 and later, the sample uses `runs-as-current-user-with-user-independent-keychain`. That entitlement value does not itself opt data into the shared Keychain; `kSecUseUserIndependentKeychain` is a separate API for apps that deliberately share a service login. A personal-data app should not add manual `TVUserManager` profile mapping to compensate for an unverified system account context. + +When testing an app that depends on this separation, use two distinct, fully signed-in Apple TV accounts on an authorized physical device. Confirm the current profile in native Settings, then launch the same signed build through the app grid under each profile. Record the effective app entitlement and embedded provisioning privilege, a fresh process and loaded-build identity, and the account identity returned by the app's CloudKit boundary. Compare that verified account, the bound local store/checkpoint, and a synthetic entry set across A → B → A and cold launches. Retain only privacy-safe equality results; do not publish account or record identifiers or learner content. A changed Settings selection, new process, successful build, `shouldStorePreferencesForCurrentUser == true`, or distinct-looking UI alone does not prove that CloudKit supplied a different account. + +If B is selected but CloudKit verifies A, separate that observation from an app bug where CloudKit verifies B but the host opens A's store. A temporary debug-only `UserDefaults` marker written under A and checked without a B write can independently test preference isolation; remove it afterward. Preserve the signed build, process timeline, account/store comparisons, and any existing containers before trying a repair. On one observed tvOS device, both supported current-user entitlement values and a full restart still routed the secondary profile to the default account; removing and re-adding only that secondary profile restored distinct CloudKit accounts and stores across repeated switches. That result does not establish the internal tvOS cause or guarantee the same repair elsewhere. + +After one normal restart fails, [removing a secondary Apple Account profile](https://support.apple.com/guide/tv/atvb0753b37d/tvos) is a later owner-coordinated diagnostic: verify the secondary user's local data is replaceable, obtain action-time confirmation, let the owner handle Apple Account sign-in, and leave the default and unrelated profiles untouched. Repeat the same native account/store comparisons after re-addition. Do not uninstall the app, clear checkpoints, change entitlements, or factory-reset the device as a speculative account-routing fix. If separation still fails, preserve a bounded reproduction for Apple support; do not conceal the mismatch with an unverified app-side account mapping. + ### Sensor limitations Apple documents camera and microphone conflicts during physical-device remote interaction. An active Device Hub screen session can prevent sensor access or be interrupted by higher-priority apps. Stop remote interaction and arrange an owner-operated check when real camera or microphone behavior is the test target. Do not label an empty capture caused by the test environment as an application defect, and do not terminate an unrelated call or recording to continue testing. diff --git a/AgentGuidelines/Guidelines/Xcode/Security.md b/AgentGuidelines/Guidelines/Xcode/Security.md index 5817b01..948a26f 100644 --- a/AgentGuidelines/Guidelines/Xcode/Security.md +++ b/AgentGuidelines/Guidelines/Xcode/Security.md @@ -16,7 +16,7 @@ Confirm the requested targets and configurations, then inspect only relevant are - code-signing and entitlements; - application capabilities; - privacy manifests and required-reason APIs; -- network security configuration; +- network security configuration under [Networking](../Networking.md#ownership-and-validation); - debug-only behavior and diagnostics; - sensitive data storage and logging; - unsafe interoperability boundaries. diff --git a/AgentGuidelines/README.md b/AgentGuidelines/README.md index ecc0aed..bea794f 100644 --- a/AgentGuidelines/README.md +++ b/AgentGuidelines/README.md @@ -63,6 +63,7 @@ The subtree does not automatically import every guide into an agent's context. A - [Agent workflow and tool execution](Guidelines/AgentWorkflow.md) - [App Store metadata](Guidelines/AppStore.md) - [CI/CD](Guidelines/CICD.md) +- [Codex review policy](ReviewPolicy/README.md) - [Development and reusability](Guidelines/Development.md) - [Device Hub, physical devices, and simulators](Guidelines/Xcode/DeviceInteraction.md) - [Documentation](Guidelines/Documentation.md) @@ -71,6 +72,7 @@ The subtree does not automatically import every guide into an agent's context. A - [GitHub pull requests](Guidelines/GitHub/PullRequests.md) - [Localization](Guidelines/Localization.md) - [Logging](Guidelines/Logging.md) +- [Networking](Guidelines/Networking.md) - [Redux architecture and physical folder organization](Guidelines/Architecture/Redux.md) - [Swift](Guidelines/Swift/Swift.md) - [Swift format](Guidelines/Swift/SwiftFormat.md) @@ -92,7 +94,7 @@ From the consumer repository root, install a tagged release: git subtree add \ --prefix=AgentGuidelines \ https://github.com/thatfactory/agent-guidelines.git \ - 0.0.42 \ + 0.0.48 \ --squash ``` @@ -110,14 +112,20 @@ Keep the subtree tracked, but add this to the consumer's tracked `.gitattributes AgentGuidelines/** linguist-generated ``` -Copy and adapt [the consumer template](Templates/AGENTS.md). Keep the consumer file small: describe the product or package, map its concrete physical folders, point to the applicable shared guides, and state only genuine exceptions. Keep the version-marked code-review contract, documentation-maintenance contract, external-dependency contract, and runtime-observability contract directly in the repository-root `AGENTS.md`; Markdown links to shared guides are navigation, not automatic instruction includes. +Copy and adapt [the consumer template](Templates/AGENTS.md). Keep the consumer file small: describe the product or package, map its concrete physical folders, point to the applicable shared guides, and state only genuine exceptions. Keep the version-marked code-review contract, PR-lifecycle contract, documentation-maintenance contract, external-dependency contract, and runtime-observability contract directly in the repository-root `AGENTS.md`; Markdown links to shared guides are navigation, not automatic instruction includes. Copy the shared [`.gitignore` template](Templates/.gitignore) into a new Xcode project or Swift package. Keep authored project files, workspaces, and package lockfiles eligible for version control, and follow the [Git ignore guidance](Guidelines/Git/IgnoreFiles.md) when an established consumer needs an additional project-specific rule. +### Adopt review instructions only + +For a repository that needs only Codex reviewer rules and the PR flow, use the [review-only profile](ReviewPolicy/README.md). It installs managed root instructions and CI validation without the shared subtree or unrelated engineering contracts. Both profiles require the latest independently published review-policy revision. Use the central synchronizer rather than hand-copying policy text. + ### Configure global Codex instructions Copy the contents of [`Templates/GlobalCodexInstructions.md`](Templates/GlobalCodexInstructions.md) into the user's global Codex instructions. +Copy only its code-review section into Settings → Code Review → Review Instructions for manually started reviews. Automatic GitHub reviews use the checked-in root contracts. + These instructions bootstrap discovery of repository-local `AGENTS.md` files and shared guides and provide generic high-signal code-review defaults. Repository engineering policy and specialized threat models remain versioned in this repository or the consumer rather than duplicated in each user's global configuration. Review this template when upgrading `agent-guidelines`, because the recommended global bootstrap instructions may change between releases. Installing or updating the Git subtree does not update a user's global Codex configuration. @@ -138,7 +146,7 @@ Validate the checked-in consumer integration directly or through the completion- AgentGuidelines/Scripts/validate_consumer_setup.swift ``` -The native Swift validator checks the version-marked root Code Review, Documentation Maintenance, External Dependency, and Runtime Observability contracts, Codex subtree-review scope, `.gitattributes`, local guide links, and the audit-skill symlink. When the root `AGENTS.md` links the shared Swift-format guide, it also requires both configuration symlinks and a non-mutating `lint-strict` CI invocation. Pass `--require-swift-format` only when auditing formatter adoption before adding that guide link. +The native Swift validator checks the version-marked root Code Review, PR Lifecycle, Documentation Maintenance, External Dependency, and Runtime Observability contracts, Codex subtree-review scope, `.gitattributes`, local guide links, and the audit-skill symlink. When the root `AGENTS.md` links the shared Swift-format guide, it also requires both configuration symlinks and a non-mutating `lint-strict` CI invocation. Pass `--require-swift-format` only when auditing formatter adoption before adding that guide link. ## Update a consumer @@ -148,11 +156,27 @@ Review the target release's changelog, then pull it deliberately: git subtree pull \ --prefix=AgentGuidelines \ https://github.com/thatfactory/agent-guidelines.git \ - 0.0.42 \ + 0.0.48 \ --squash ``` -Confirm `AgentGuidelines/VERSION`, review the subtree diff, synchronize the marked code-review contract, documentation-maintenance contract, external-dependency contract, and runtime-observability contract when their versions change, run `AgentGuidelines/Scripts/validate_consumer_setup.swift`, and run the consumer's relevant tests. Keep the subtree update in its own commit, and identify the old and new versions plus the central release or pull request in the consumer pull-request description. Updates are intentionally not automatic: one guideline release cannot silently change every project. +Confirm `AgentGuidelines/VERSION`, review the subtree diff, synchronize the marked code-review contract, PR-lifecycle contract, documentation-maintenance contract, external-dependency contract, and runtime-observability contract when their versions change, run `AgentGuidelines/Scripts/validate_consumer_setup.swift`, and run the consumer's relevant tests. Keep the subtree update in its own commit, and identify the old and new versions plus the central release or pull request in the consumer pull-request description. Updates are intentionally not automatic: one guideline release cannot silently change every project. + +When first adopting [Networking](Guidelines/Networking.md), synchronize external-dependency contract v2 and perform the [networking adoption audit](.agents/skills/agent-guidelines-audit/SKILL.md#audit-networking), including existing paths and any explicitly approved migration deferrals. + +## Upgrade a consumer's Xcode + +Use the [`xcode-upgrade` skill](.agents/skills/xcode-upgrade/SKILL.md) to move an Apple-platform application or Swift package to a requested Xcode, update dependencies and compiler settings, align CI, and validate the result. In Codex, invoke `$xcode-upgrade to Xcode 27.2 and use the new JSON format` or select the skill. The skill also recognizes `/xcode-upgrade ...` as request text when the client passes it through; it does not register a client slash command. The skill follows the shared [project-settings guide](Guidelines/Xcode/ProjectSettings.md) and [package compiler-settings baseline](Guidelines/Packages.md#compiler-settings-baseline), preserving consumer-specific exceptions and the user's authorization boundaries. + +The skill is discoverable in this central checkout. To use it in a consumer that has adopted this release, expose it from the subtree: + +```sh +mkdir -p .agents/skills +ln -s ../../AgentGuidelines/.agents/skills/xcode-upgrade \ + .agents/skills/xcode-upgrade +``` + +Alternatively, explicitly provide `AgentGuidelines/.agents/skills/xcode-upgrade/SKILL.md` with the request. Keep the skill beside its linked guides in the subtree; copying only `SKILL.md` loses those references. Subtree adoption and skill exposure do not upgrade a consumer's toolchain until requested. ## Maintain the source of truth diff --git a/AgentGuidelines/ReviewPolicy/Policy.md b/AgentGuidelines/ReviewPolicy/Policy.md new file mode 100644 index 0000000..cc525f9 --- /dev/null +++ b/AgentGuidelines/ReviewPolicy/Policy.md @@ -0,0 +1,21 @@ + +## Code Review Rules + +Report only substantiated P0/P1 defects introduced or materially exposed by this change. Require a concrete, reachable failure in a supported use case or documented threat model, with serious impact such as a security-boundary bypass, durable data loss or corruption, a crash or deadlock in an important workflow, loss of availability, or a serious compatibility regression. An acceptance-criterion violation alone does not establish P1 severity. + +For each finding, state severity, preconditions, execution path, impact, evidence, and actionable remediation. Group manifestations of one root cause. Report substantiated blockers together. Omit unsolicited P2/P3 observations, style preferences, speculative hardening, optional tests, and refactors. A clean review means no unresolved P0/P1 blockers, not perfect software. + +A follow-up review covers unresolved P0/P1 findings, changes since the previous reviewed commit, and directly affected code. Do not restart a full review of unchanged code. New follow-up findings must be P0/P1 defects introduced by remediation or genuinely hidden by the previous blocker. + + + +## Pull Request Lifecycle + +Finish implementation, inspect the diff, and run relevant local validation before opening a ready-for-review PR. Drafts are for incomplete preparation; mark ready before waiting for automatic review. Keep the review head stable while the initial review runs. One automatic Codex review is the initial review; never duplicate it with a manual request or repeatedly toggle draft/ready state. + +Establish review configuration from repository-specific evidence. Verify the bot, PR, review round, and reviewed commit using PR/request-comment reactions, review submissions, inline threads, and checks. Eyes means processing, not approval; a thumbs-up means no suggestions only for its verified review round. Historical reactions, missing comments, and elapsed time never certify a newer head. Monitor at most five minutes total with bounded, non-accelerating polls; timeout leaves a required gate unresolved and never authorizes merge. + +Batch accepted P0/P1 fixes, verify them locally, rerun affected checks, and disposition and resolve every review thread. A completed initial review plus an explicitly recorded, locally verified remediation-only delta can satisfy the review gate; do not claim Codex reviewed the newer head. New unrelated scope belongs in another PR. Request at most one delta-scoped verification only when the owner explicitly authorizes it. Do not request reviews after every fix or casually mention the bot to start extra work. Separate Security Review is manual and requires explicit owner instruction. + +Before merge, rerun the latest published review-policy check, confirm no required review is pending, no unresolved P0/P1 blocker remains, every thread is dispositioned and resolved, required checks pass on the current head, and existing human approval or merge authorization is satisfied. A bot thumbs-up is not a substitute for required GitHub approval. Opening a PR does not authorize merging it. + diff --git a/AgentGuidelines/ReviewPolicy/README.md b/AgentGuidelines/ReviewPolicy/README.md new file mode 100644 index 0000000..54108c4 --- /dev/null +++ b/AgentGuidelines/ReviewPolicy/README.md @@ -0,0 +1,38 @@ +# Codex Review Policy + +The [canonical policy](Policy.md) contains the only authored reviewer and PR-lifecycle contracts. [VERSION](VERSION) is its independent positive integer revision. Full consumers adopt engineering guides through `AgentGuidelines/`; review-only consumers install the two managed sections directly in root `AGENTS.md` and a small CI integration. The central root, global review instructions, and full consumer template mirror this canonical text and are validated together. A Markdown link is navigation, not an instruction include. + +## Profiles and synchronization + +Start a review-only repository with a root `AGENTS.md` describing its actual purpose and local constraints. Do not install unrelated Apple development guidance. Run the central synchronizer against the consumer: + +```sh +Scripts/sync_codex_review_policy.swift update --consumer-root --profile review-only +Scripts/sync_codex_review_policy.swift check --consumer-root --profile review-only +``` + +Use `--profile full` for a repository with an `AgentGuidelines/` subtree. Repeated `--consumer-root` options synchronize several repositories against one source resolution. The synchronizer updates only the provenance comment and complete managed contracts; it preserves local instructions, removes obsolete/duplicate complete contracts, and refuses malformed marker pairs. `check` never writes files. Review-only consumers need no vendored directory, formatter setup, or completion-audit symlink. + +The provenance comment records profile, policy revision, and the exact published source commit. Latest-policy verification always uses `thatfactory/agent-guidelines`. `--policy-repository` exists for isolated test fixtures; production CI never accepts a consumer-controlled repository override. Source documents are data and are never executed. `--local-source` is restricted to central/template development, whose source is `local` because a commit cannot contain its own hash; it cannot satisfy a consumer's published-provenance check. + +## CI and freshness + +Run the shared `.github/actions/codex-review-policy` composite action from an immutable full commit SHA, with `profile: full` or `profile: review-only`. Reuse an existing Swift-capable job after checkout, or add a small hosted `macos-latest` job when the repository has none. Make the hosting job a required check. The action invokes the pinned Swift validator; it downloads only canonical policy text and never runs executable code from a newly discovered tag. Do not run consumer scripts to validate policy text. + +The validator resolves the highest published `codex-review-vN` tag once per invocation, verifies the fetched tag object, peels its commit, checks that `ReviewPolicy/VERSION` agrees, and compares the exact managed contracts and provenance. Missing/modified/obsolete/duplicated sections, incorrect profile/source, older policy revisions, unavailable sources, and uncertain tag identity fail closed. An unrelated guideline release leaves the review-policy tag unchanged and does not force review-only updates. + +A green CI run cannot invalidate itself when a later policy tag appears. The executor must rerun the latest-policy check immediately before merge. Publishing a policy revision includes a coordinated consumer rollout; CI never edits files, opens PRs, or merges updates itself. Keep consumer inventories and private repository names in the authorized local task, not in this public repository. + +## Publishing and bootstrapping + +Change the canonical contracts, increment the independent policy revision, synchronize mirrors, and validate through the central PR. Prepare the regular guidelines release metadata as usual. After the reviewed release commit is merged and validated, publish its normal guidelines tag/release, then create the immutable `codex-review-vN` tag at that same commit. Never move a published policy tag. Only policy changes need a new policy tag; all other guidelines releases retain the existing policy revision. + +The central PR and release validation use owned local policy data, allowing a new revision to be tested before publication. The first consumer rollout starts only after the policy tag is public. Central development commands are: + +```sh +Scripts/sync_codex_review_policy.swift update --local-source . --profile central +Scripts/sync_codex_review_policy.swift update --local-source . --profile template --agents-file Templates/AGENTS.md +Scripts/sync_codex_review_policy.swift check --local-source . --profile central +``` + +Synchronize the global template's review-only section from the same canonical reviewer block. Copy the global template into user-level Codex instructions; copy only its reviewer section into Settings → Code Review → Review Instructions for reviews started with Review with Codex. These settings are a matching user-level layer, not the source of automatic GitHub review policy. Configure all PR authors, opening/ready triggers, exhaustive review off, and separate automatic Security Review off. Record repository-specific configuration outside managed contracts. diff --git a/AgentGuidelines/ReviewPolicy/VERSION b/AgentGuidelines/ReviewPolicy/VERSION new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/AgentGuidelines/ReviewPolicy/VERSION @@ -0,0 +1 @@ +1 diff --git a/AgentGuidelines/Scripts/sync_codex_review_policy.swift b/AgentGuidelines/Scripts/sync_codex_review_policy.swift new file mode 100755 index 0000000..b8cdca0 --- /dev/null +++ b/AgentGuidelines/Scripts/sync_codex_review_policy.swift @@ -0,0 +1,283 @@ +#!/usr/bin/env swift +import Foundation + +#if canImport(Darwin) + import Darwin +#else + import Glibc +#endif + +// MARK: - Policy identity + +/// The only production source of published policy text. +let policyRepository = "https://github.com/thatfactory/agent-guidelines.git" +let scriptRoot = URL(fileURLWithPath: #filePath).deletingLastPathComponent().deletingLastPathComponent() +let families = ["CODE REVIEW", "PR LIFECYCLE"] +let provenancePrefix = "", in: text) + let ends = try regions("", in: text) + let blocks = try regions("[\\s\\S]*?", in: text) + guard begins.count == ends.count, begins.count == blocks.count, + text.components(separatedBy: "" +} + +/// Validates exact content, policy freshness, profile, and immutable source identity. +func check(_ text: String, snapshot: Snapshot, profile: String) throws { + let metadata = try regions("", in: text) + guard metadata.count == 1, text.components(separatedBy: provenancePrefix).count - 1 == 1, + metadata[0].text == provenance(snapshot, profile: profile) + else { + throw PolicyError( + "missing, outdated, duplicated, or incorrect review-policy provenance; expected \(provenance(snapshot, profile: profile))" + ) + } + for (index, family) in families.enumerated() { + let actual = try contracts(text, family: family) + guard actual.count == 1, actual[0].text == snapshot.blocks[index] else { + throw PolicyError("missing, obsolete, duplicated, or modified \(family.lowercased()) contract") + } + } +} + +/// Replaces only complete managed regions, preserving every byte of local instructions. +func updated(_ text: String, snapshot: Snapshot, profile: String) throws -> String { + if (try? check(text, snapshot: snapshot, profile: profile)) != nil { return text } + var managed = try families.flatMap { try contracts(text, family: $0) } + managed += try regions("", in: text) + let payload = provenance(snapshot, profile: profile) + "\n" + snapshot.blocks.joined(separator: "\n\n") + guard !managed.isEmpty else { return text + (text.hasSuffix("\n\n") ? "" : "\n\n") + payload + "\n" } + managed.sort { $0.range.lowerBound < $1.range.lowerBound } + var result = "" + var cursor = text.startIndex + for (index, region) in managed.enumerated() { + guard region.range.lowerBound >= cursor else { throw PolicyError("overlapping managed regions") } + result += text[cursor.. Options { + var result = Options() + guard let mode = values.first, ["check", "update"].contains(mode) else { + throw PolicyError( + "Usage: sync_codex_review_policy.swift check|update [--consumer-root ] [--profile full|review-only] [--local-source --profile central|template] [--agents-file ] [--policy-repository ]" + ) + } + result.mode = mode + var index = 1 + while index < values.count { + guard index + 1 < values.count else { throw PolicyError("missing value for \(values[index])") } + let value = values[index + 1] + switch values[index] { + case "--consumer-root": result.roots.append(URL(fileURLWithPath: value).standardizedFileURL) + case "--profile": result.profile = value + case "--local-source": result.localSource = URL(fileURLWithPath: value).standardizedFileURL + case "--policy-repository": result.repository = value + case "--agents-file": + guard !value.hasPrefix("/"), !value.split(separator: "/").contains("..") else { + throw PolicyError("--agents-file must stay within the consumer root") + } + result.agentsFile = value + default: throw PolicyError("unknown argument: \(values[index])") + } + index += 2 + } + if result.roots.isEmpty { result.roots = [URL(fileURLWithPath: FileManager.default.currentDirectoryPath)] } + return result +} + +/// Synchronizes all requested roots against one verified source and reports every failed root. +func main() -> Int32 { + do { + let options = try parse(Array(CommandLine.arguments.dropFirst())) + let snapshot = try loadSnapshot(options) + var failures = 0 + for root in options.roots { + do { + let profile = try profile(for: root, options: options) + let file = root.appendingPathComponent(options.agentsFile) + let original = try String(contentsOf: file, encoding: .utf8) + if options.mode == "update" { + let replacement = try updated(original, snapshot: snapshot, profile: profile) + if replacement != original { try replacement.write(to: file, atomically: true, encoding: .utf8) } + } + let final = try String(contentsOf: file, encoding: .utf8) + try check(final, snapshot: snapshot, profile: profile) + print( + "Validated \(root.lastPathComponent): \(profile), review policy \(snapshot.revision), source \(snapshot.commit)." + ) + } catch { + failures += 1 + FileHandle.standardError.write(Data("\(root.lastPathComponent): \(error)\n".utf8)) + } + } + return failures == 0 ? 0 : 1 + } catch { + FileHandle.standardError.write(Data("Review policy verification failed: \(error)\n".utf8)) + return 1 + } +} + +exit(main()) diff --git a/AgentGuidelines/Scripts/validate_consumer_setup.swift b/AgentGuidelines/Scripts/validate_consumer_setup.swift index e5b5c37..1e17b99 100755 --- a/AgentGuidelines/Scripts/validate_consumer_setup.swift +++ b/AgentGuidelines/Scripts/validate_consumer_setup.swift @@ -8,12 +8,14 @@ import Foundation #endif let guidelinesRoot = URL(fileURLWithPath: #filePath).deletingLastPathComponent().deletingLastPathComponent() -let contractBegin = "" -let contractEnd = "" +let contractBegin = "" +let contractEnd = "" +let lifecycleContractBegin = "" +let lifecycleContractEnd = "" let documentationContractBegin = "" let documentationContractEnd = "" -let externalDependencyContractBegin = "" -let externalDependencyContractEnd = "" +let externalDependencyContractBegin = "" +let externalDependencyContractEnd = "" let observabilityContractBegin = "" let observabilityContractEnd = "" let markdownLinkPattern = #"\[[^\]]+\]\(([^)]+)\)"# @@ -335,6 +337,7 @@ func validateConsumerSetup( if let template, let agents { let contracts = [ (contractBegin, contractEnd, "code-review"), + (lifecycleContractBegin, lifecycleContractEnd, "PR-lifecycle"), (documentationContractBegin, documentationContractEnd, "documentation-maintenance"), (externalDependencyContractBegin, externalDependencyContractEnd, "external-dependency"), (observabilityContractBegin, observabilityContractEnd, "runtime-observability"), @@ -348,6 +351,12 @@ func validateConsumerSetup( errors.append("consumer AGENTS.md: \(name) contract does not match AgentGuidelines/Templates/AGENTS.md") } } + for family in ["CODE REVIEW", "PR LIFECYCLE"] { + let prefix = "[\\s\\S]*?" + let source = matches(pattern, in: canonical) + guard source.count == 1, let expected = capture(0, from: source[0], in: canonical) else { + errors.append("ReviewPolicy/Policy.md: expected one canonical \(family) contract") + continue + } + for relative in mirrors where family == "CODE REVIEW" || relative != "Templates/GlobalCodexInstructions.md" { + guard let value = readText(root.appendingPathComponent(relative), errors: &errors) else { continue } + let found = matches(pattern, in: value) + if found.count != 1 || capture(0, from: found[0], in: value) != expected + || value.components(separatedBy: "" + if !value.contains(expected) + || value.components(separatedBy: " + ## External Dependency Policy Do not introduce third-party source or binary dependencies into ThatFactory applications, games, or reusable packages during normal development. Prefer Apple platform APIs, the Swift standard library, code owned by the current repository, or focused ThatFactory-owned packages. If reusable capability is missing, implement it natively at the appropriate boundary and consider extracting it into a first-party package instead of selecting an external library. -A third-party dependency may be added, or expanded to a new target or runtime role, only with explicit repository-owner approval for that specific use before modifying the dependency graph. Convenience, reduced implementation effort, popularity, or an agent's preference for an existing library are not sufficient justification. Do not make an external library acceptable merely by hiding it behind a first-party wrapper. +A third-party dependency may be added, or expanded to a new target or runtime role, only under an explicit standing approval in these guidelines or with explicit repository-owner approval for that specific use before modifying the dependency graph. Convenience, reduced implementation effort, popularity, or an agent's preference for an existing library are not sufficient justification. Do not make an external library acceptable merely by hiding it behind a first-party wrapper. Document every approved exception in durable repository documentation in the same change. Record the dependency and source, purpose and target scope, why a native or first-party implementation is not appropriate, relevant license, security, and maintenance considerations, and the approval context. Merely mentioning or using the dependency in an execution plan, pull-request description, or transient chat is not approval. Regardless of where explicit approval occurs, reflect the exception in durable repository documentation. Apple system frameworks and the Swift standard library are not third-party dependencies. ThatFactory-owned packages are first-party dependencies. Tooling explicitly required by the shared guidelines is allowed only for its documented tooling role and must not be linked into or shipped with product runtime targets unless separately approved and documented. +Follow [Networking](AgentGuidelines/Guidelines/Networking.md) for Apple-native networking and typed HTTP. Its narrow standing approval for Apple's `swift-http-types` applies only to the documented HTTPTypes and HTTPTypesFoundation runtime role; it does not approve other Apple open-source packages or unrelated roles. Use the guide's normal dependency documentation for that role instead of requesting a separate consumer exception. + Follow [Development workflow](AgentGuidelines/Guidelines/Development.md) for the detailed policy. - + ``` Keep the following documentation-maintenance contract in the consumer repository's root `AGENTS.md` so implementation agents receive it directly rather than only through a linked guide. Copy it unchanged and update it when the marker version changes in this template. @@ -75,26 +78,31 @@ Do not create documentation churn for incidental implementation details that are ``` -Keep the following marked code-review contract in the consumer repository's root `AGENTS.md` so it is loaded directly for root-level Codex and pull-request work. Copy it unchanged and update it when the marker version changes in this template; a Markdown link to the detailed workflow is not an instruction include. +Keep the following code-review and PR-lifecycle contracts directly in the consumer repository's root `AGENTS.md`. Synchronize them with `Scripts/sync_codex_review_policy.swift`; both full and review-only profiles use identical canonical text. The template provenance below is development-only: consumer synchronization replaces it with the profile, latest policy revision, and immutable published commit. A Markdown link is not an instruction include. ```md - + + ## Code Review Rules -Review for release-blocking defects introduced or materially exposed by the pull request. A clean review means no unresolved P0/P1 findings; it does not mean exhaustive or perfect software. +Report only substantiated P0/P1 defects introduced or materially exposed by this change. Require a concrete, reachable failure in a supported use case or documented threat model, with serious impact such as a security-boundary bypass, durable data loss or corruption, a crash or deadlock in an important workflow, loss of availability, or a serious compatibility regression. An acceptance-criterion violation alone does not establish P1 severity. + +For each finding, state severity, preconditions, execution path, impact, evidence, and actionable remediation. Group manifestations of one root cause. Report substantiated blockers together. Omit unsolicited P2/P3 observations, style preferences, speculative hardening, optional tests, and refactors. A clean review means no unresolved P0/P1 blockers, not perfect software. -A blocking finding must identify a concrete, reachable path in a supported use case or the documented threat model that can cause a credible security-boundary bypass, durable data loss or corruption, a crash or deadlock, loss of availability, violation of an explicit acceptance criterion, or a serious compatibility regression. +A follow-up review covers unresolved P0/P1 findings, changes since the previous reviewed commit, and directly affected code. Do not restart a full review of unchanged code. New follow-up findings must be P0/P1 defects introduced by remediation or genuinely hidden by the previous blocker. + -For every blocking finding, state the severity, preconditions, execution path, impact, evidence, and actionable remediation. Group manifestations that share the same root cause into one finding. + +## Pull Request Lifecycle -Treat P2/P3 observations as non-blocking, including defense-in-depth, theoretical completeness, unsupported use cases, malformed state that trusted code cannot produce, behavior by components outside the threat model, style preferences, and speculative refactoring. Record a useful lower-severity observation once as deferred, declined, duplicate, or follow-up work; do not keep the review loop open for it. +Finish implementation, inspect the diff, and run relevant local validation before opening a ready-for-review PR. Drafts are for incomplete preparation; mark ready before waiting for automatic review. Keep the review head stable while the initial review runs. One automatic Codex review is the initial review; never duplicate it with a manual request or repeatedly toggle draft/ready state. -In an initial review, report substantiated blockers together. A follow-up review is limited to unresolved P0/P1 findings, changes since the last reviewed commit, and code directly affected by those changes. Do not restart an unrestricted review of unchanged code. A new follow-up finding must be a P0/P1 defect introduced by the remediation or genuinely hidden by the previous blocker. +Establish review configuration from repository-specific evidence. Verify the bot, PR, review round, and reviewed commit using PR/request-comment reactions, review submissions, inline threads, and checks. Eyes means processing, not approval; a thumbs-up means no suggestions only for its verified review round. Historical reactions, missing comments, and elapsed time never certify a newer head. Monitor at most five minutes total with bounded, non-accelerating polls; timeout leaves a required gate unresolved and never authorizes merge. -The review-round budget below applies only to Codex GitHub reviews: the configured automatic Codex review and any manual `@codex review` request. It does not apply to ChatGPT review or reasoning delegated through Reasoning Relay. An otherwise-authorized Reasoning Relay workflow may request as many Relay review or follow-up delegations as its own governing workflow requires; those requests neither consume the Codex budget nor require repository-owner authorization under it. +Batch accepted P0/P1 fixes, verify them locally, rerun affected checks, and disposition and resolve every review thread. A completed initial review plus an explicitly recorded, locally verified remediation-only delta can satisfy the review gate; do not claim Codex reviewed the newer head. New unrelated scope belongs in another PR. Request at most one delta-scoped verification only when the owner explicitly authorizes it. Do not request reviews after every fix or casually mention the bot to start extra work. Separate Security Review is manual and requires explicit owner instruction. -When repository-specific evidence establishes that automatic Codex review is enabled and applies to the current pull request/head, it is the initial Codex review. Otherwise, an absent review does not establish a pending gate. Do not request a manual Codex review unless the repository owner explicitly asks. Never request another Codex review after each remediation commit. Within the normal Codex review budget, at most one owner-authorized, delta-scoped Codex verification review may be requested under [the pull-request review workflow](AgentGuidelines/Guidelines/GitHub/PullRequests.md). - +Before merge, rerun the latest published review-policy check, confirm no required review is pending, no unresolved P0/P1 blocker remains, every thread is dispositioned and resolved, required checks pass on the current head, and existing human approval or merge authorization is satisfied. A bot thumbs-up is not a substitute for required GitHub approval. Opening a PR does not authorize merging it. + ## Codex review scope diff --git a/AgentGuidelines/Templates/GlobalCodexInstructions.md b/AgentGuidelines/Templates/GlobalCodexInstructions.md index abe4578..4906621 100644 --- a/AgentGuidelines/Templates/GlobalCodexInstructions.md +++ b/AgentGuidelines/Templates/GlobalCodexInstructions.md @@ -2,29 +2,18 @@ For repositories containing an `AGENTS.md`, read and follow the applicable repository instructions before starting substantive work. -When a repository includes shared agent guidelines, read only the guides referenced by the applicable `AGENTS.md`. Treat those guides as the source of truth for language conventions, architecture, development workflow, testing, and agent execution. +When a repository includes shared agent guidelines, read the relevant guides referenced by the applicable `AGENTS.md` on demand. Treat those guides as the source of truth for language conventions, architecture, development workflow, testing, and agent execution. Repository and folder-level instructions may specialize the shared baseline within their scope. Do not replace deliberate repository conventions with generic global preferences. Do not duplicate repository-specific guidance in global instructions. Global instructions should bootstrap discovery of the repository's own sources of truth. -## Code review behavior + +## Code Review Rules -When acting as a code reviewer, optimize for high-signal release risk and convergence rather than exhaustive perfection. +Report only substantiated P0/P1 defects introduced or materially exposed by this change. Require a concrete, reachable failure in a supported use case or documented threat model, with serious impact such as a security-boundary bypass, durable data loss or corruption, a crash or deadlock in an important workflow, loss of availability, or a serious compatibility regression. An acceptance-criterion violation alone does not establish P1 severity. -Create an inline finding only when all of the following are true: +For each finding, state severity, preconditions, execution path, impact, evidence, and actionable remediation. Group manifestations of one root cause. Report substantiated blockers together. Omit unsolicited P2/P3 observations, style preferences, speculative hardening, optional tests, and refactors. A clean review means no unresolved P0/P1 blockers, not perfect software. -1. The issue is introduced or materially exposed by the proposed change. -2. There is a concrete, reachable failure path in a supported use case or the documented threat model. -3. The impact is P0 or P1: a credible security-boundary bypass, durable data loss or corruption, a crash or deadlock, loss of availability, violation of an explicit acceptance criterion, or a serious compatibility regression. -4. The evidence and remediation are specific enough to be actionable. - -State the finding's severity, preconditions, execution path, impact, and evidence. Group findings that share the same root cause. Do not create separate serial comments for additional manifestations of an already reported root cause. - -Treat P2 and P3 observations as non-blocking. This includes defense-in-depth, theoretical completeness, unsupported use cases, malformed state that trusted code cannot produce, adversarial behavior by components outside the threat model, style preferences, speculative refactoring, and exhaustive enumeration of equivalent input formats. Summarize valuable lower-severity observations once or recommend a follow-up issue. - -In the initial review, report substantiated blockers together rather than drip-feeding them across repeated reviews. - -In a follow-up review, verify previously reported P0/P1 findings and review only changes since the previously reviewed commit plus code directly affected by those changes. Do not restart an unrestricted search of unchanged code. A newly introduced follow-up finding must be a P0/P1 issue introduced by the remediation or genuinely hidden by the previous defect. - -A clean review means that there are no unresolved P0/P1 blockers. It does not mean perfect software, zero possible improvements, or zero technical debt. +A follow-up review covers unresolved P0/P1 findings, changes since the previous reviewed commit, and directly affected code. Do not restart a full review of unchanged code. New follow-up findings must be P0/P1 defects introduced by remediation or genuinely hidden by the previous blocker. + diff --git a/AgentGuidelines/Tests/run_review_policy_tests.swift b/AgentGuidelines/Tests/run_review_policy_tests.swift new file mode 100755 index 0000000..7b028c7 --- /dev/null +++ b/AgentGuidelines/Tests/run_review_policy_tests.swift @@ -0,0 +1,250 @@ +#!/usr/bin/env swift +import Foundation + +#if canImport(Darwin) + import Darwin +#else + import Glibc +#endif + +let sourceRoot = URL(fileURLWithPath: #filePath).deletingLastPathComponent().deletingLastPathComponent() +let manager = FileManager.default + +/// One executed process with complete combined diagnostics. +struct Result { + let status: Int32 + let output: String +} + +/// A focused regression-test failure. +struct Failure: Error, CustomStringConvertible { + let description: String +} + +/// Executes an argument vector without interpreting repository text as shell syntax. +func run(_ arguments: [String], at root: URL? = nil) throws -> Result { + let process = Process() + let output = Pipe() + process.executableURL = URL(fileURLWithPath: "/usr/bin/env") + process.arguments = arguments + process.currentDirectoryURL = root + process.standardOutput = output + process.standardError = output + try process.run() + let bytes = output.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() + return Result(status: process.terminationStatus, output: String(decoding: bytes, as: UTF8.self)) +} + +/// Asserts a condition with actionable diagnostics. +func require(_ condition: @autoclosure () throws -> Bool, _ message: String) throws { + if try !condition() { throw Failure(description: message) } +} + +/// Writes fixture data while preserving its literal content. +func write(_ text: String, _ url: URL) throws { + try manager.createDirectory(at: url.deletingLastPathComponent(), withIntermediateDirectories: true) + try text.write(to: url, atomically: true, encoding: .utf8) +} + +/// Reads a UTF-8 fixture. +func read(_ url: URL) throws -> String { + try String(contentsOf: url, encoding: .utf8) +} + +/// Runs a Git fixture mutation and verifies success. +func git(_ arguments: [String], at root: URL) throws -> String { + let result = try run(["git"] + arguments, at: root) + try require(result.status == 0, result.output) + return result.output.trimmingCharacters(in: .whitespacesAndNewlines) +} + +/// Owns isolated, locally published policy tags and consumer repositories. +struct Fixture { + let root: URL + let repository: URL + let canonical: String + + /// Creates a minimal source repository, including an annotated policy tag. + init() throws { + root = manager.temporaryDirectory.appendingPathComponent("policy-tests-\(UUID().uuidString)") + repository = root.appendingPathComponent("policy") + canonical = try read(sourceRoot.appendingPathComponent("ReviewPolicy/Policy.md")) + try write(canonical, repository.appendingPathComponent("ReviewPolicy/Policy.md")) + try write("1\n", repository.appendingPathComponent("ReviewPolicy/VERSION")) + _ = try git(["init", "--quiet"], at: repository) + _ = try git(["config", "user.name", "Policy test"], at: repository) + _ = try git(["config", "user.email", "policy@example.com"], at: repository) + _ = try git(["add", "."], at: repository) + _ = try git(["commit", "--quiet", "-m", "Policy one"], at: repository) + _ = try git(["tag", "-a", "codex-review-v1", "-m", "Immutable policy one"], at: repository) + } + + /// Creates a consumer with identifiable local instructions and optional obsolete copies. + func consumer(_ name: String, full: Bool = false, obsolete: Bool = false) throws -> URL { + let consumer = root.appendingPathComponent(name) + var value = "# Local project\n\nPreserve local BEFORE instructions.\n" + if obsolete { + let old = canonical.components(separatedBy: "\n\n\n## Code Review Rules"), "obsolete review contract survived") + let second = try fixture.sync("update", root, profile: profile) + try require(second.status == 0, second.output) + try require(try read(root.appendingPathComponent("AGENTS.md")) == first, "update is not idempotent") + let check = try fixture.sync("check", root, profile: profile) + try require(check.status == 0, check.output) + } + } + ), + ( + "modified, duplicate, stale, and incorrect-source policies fail without writing", + { + let fixture = try Fixture() + defer { fixture.cleanup() } + let root = try fixture.consumer("consumer") + let update = try fixture.sync("update", root) + try require(update.status == 0, update.output) + let file = root.appendingPathComponent("AGENTS.md") + let good = try read(file) + let mutations = [ + good.replacingOccurrences(of: "P0/P1", with: "P0"), + good + "\n" + fixture.canonical, + good.replacingOccurrences(of: "revision=1", with: "revision=0"), + good.replacingOccurrences(of: "source=", with: "incorrect-source="), + good + "\n\n", + ] + for value in mutations { + try write(value, file) + let check = try fixture.sync("check", root) + try require(check.status != 0, "invalid policy passed") + try require(try read(file) == value, "check mutated instructions") + } + try write(mutations.last!, file) + let invalidUpdate = try fixture.sync("update", root) + try require(invalidUpdate.status != 0, "malformed update unexpectedly succeeded") + try require(try read(file) == mutations.last!, "failed update changed instructions") + } + ), + ( + "a new policy revision blocks old consumers while unrelated releases do not", + { + let fixture = try Fixture() + defer { fixture.cleanup() } + let root = try fixture.consumer("consumer") + let first = try fixture.sync("update", root) + try require(first.status == 0, first.output) + try write("Unrelated guide change.\n", fixture.repository.appendingPathComponent("README.md")) + _ = try git(["add", "."], at: fixture.repository) + _ = try git(["commit", "--quiet", "-m", "Unrelated guide release"], at: fixture.repository) + let unchanged = try fixture.sync("check", root) + try require(unchanged.status == 0, unchanged.output) + try write("2\n", fixture.repository.appendingPathComponent("ReviewPolicy/VERSION")) + _ = try git(["add", "."], at: fixture.repository) + _ = try git(["commit", "--quiet", "-m", "Policy two"], at: fixture.repository) + _ = try git(["tag", "codex-review-v2"], at: fixture.repository) + let stale = try fixture.sync("check", root) + try require(stale.status != 0, "old revision accepted") + let updated = try fixture.sync("update", root) + try require(updated.status == 0, updated.output) + let checked = try fixture.sync("check", root) + try require(checked.status == 0, checked.output) + } + ), + ( + "unavailable sources, tag/version disagreement, and profile bypass fail closed", + { + let fixture = try Fixture() + defer { fixture.cleanup() } + let root = try fixture.consumer("consumer") + let unavailable = try fixture.sync( + "check", root, extra: ["--policy-repository", fixture.root.appendingPathComponent("absent").path]) + try require(unavailable.status != 0, "unavailable source passed") + let wrongProfile = try fixture.sync("update", root, profile: "full") + try require(wrongProfile.status != 0, "wrong profile passed") + let localBypass = try fixture.sync("update", root, extra: ["--local-source", sourceRoot.path]) + try require(localBypass.status != 0, "consumer bypassed immutable source verification") + _ = try git(["tag", "codex-review-v2"], at: fixture.repository) + let mismatch = try fixture.sync("update", root) + try require(mismatch.status != 0 && mismatch.output.contains("disagree"), mismatch.output) + } + ), + ( + "current review, stale signals, remediation closure, and timeout remain distinct", + { + let guide = try read(sourceRoot.appendingPathComponent("Guidelines/GitHub/PullRequests.md")) + let source = try read(sourceRoot.appendingPathComponent("Scripts/validate_guidelines.swift")) + let invariants = [ + "the exact request-comment reactions", + "never relabel the newer head as bot-reviewed", + "Completed initial review plus a locally verified remediation-only delta | `completed` | Satisfied", + "At the deadline, stop and terminate the monitor, report the observed facts once, and never infer approval", + ] + for invariant in invariants { + try require( + guide.contains(invariant) && source.contains(invariant), + "missing protected review scenario: \(invariant)") + } + try require( + try fixturelessPolicy().contains("New unrelated scope belongs in another PR"), "scope boundary missing") + } + ), +] + +/// Reads canonical policy for assertions without constructing a remote fixture. +func fixturelessPolicy() throws -> String { + try read(sourceRoot.appendingPathComponent("ReviewPolicy/Policy.md")) +} + +var failures = 0 +for (name, test) in tests { + do { + try test() + print("PASS \(name)") + } catch { + failures += 1 + FileHandle.standardError.write(Data("FAIL \(name): \(error)\n".utf8)) + } +} +if failures > 0 { exit(1) } +print("All \(tests.count) review-policy test groups passed.") diff --git a/AgentGuidelines/Tests/run_tests.swift b/AgentGuidelines/Tests/run_tests.swift index 52ad3d9..5255035 100755 --- a/AgentGuidelines/Tests/run_tests.swift +++ b/AgentGuidelines/Tests/run_tests.swift @@ -120,6 +120,65 @@ func localization(_ value: String, state: String = "translated") -> String { } let tests: [(String, () throws -> Void)] = [ + ( + "networking validator rejects missing policy and audit gates", + { + let mutations: [(String, String)] = [ + ("Guidelines/Networking.md", "`HTTPTypes` and `HTTPTypesFoundation`"), + ("Guidelines/Networking.md", "## Capability-gap exceptions"), + ("Guidelines/Development.md", "Networking.md#standing-dependency-approval"), + ( + ".agents/skills/agent-guidelines-audit/SKILL.md", + "Dependency presence or imports alone do not establish compliance" + ), + ( + ".agents/skills/agent-guidelines-audit/SKILL.md", + "report it as deferred compliance, never full networking compliance" + ), + ] + try withTemporaryDirectory { temporary in + let fixture = temporary.appendingPathComponent("repository") + try copyRepositoryFixture(to: fixture) + let validator = fixture.appendingPathComponent("Scripts/validate_guidelines.swift").path + let baseline = try run([validator]) + try require(baseline.succeeded, baseline.output) + for (path, fragment) in mutations { + let file = fixture.appendingPathComponent(path) + let original = try String(contentsOf: file, encoding: .utf8) + try require(original.contains(fragment), "missing test fixture text: \(fragment)") + try write(original.replacingOccurrences(of: fragment, with: "REMOVED"), to: file) + let result = try run([validator]) + try require(!result.succeeded, "networking drift unexpectedly passed: \(fragment)") + try require(result.output.contains("missing networking requirement"), result.output) + try write(original, to: file) + } + } + } + ), + ( + "consumer validator requires dependency contract v2 and its standing approval", + { + try withTemporaryDirectory { root in + try createConsumerFixture(at: root, isPackage: true) + let agents = root.appendingPathComponent("AGENTS.md") + let original = try String(contentsOf: agents, encoding: .utf8) + let command = [script("Scripts/validate_consumer_setup.swift"), "--consumer-root", root.path] + let baseline = try run(command) + try require(baseline.succeeded, baseline.output) + try write( + original.replacingOccurrences( + of: "EXTERNAL DEPENDENCY CONTRACT v2", with: "EXTERNAL DEPENDENCY CONTRACT v1"), + to: agents) + let stale = try run(command) + try require(!stale.succeeded, "stale dependency contract unexpectedly passed") + try require(stale.output.contains("EXTERNAL DEPENDENCY CONTRACT v2"), stale.output) + try write(original.replacingOccurrences(of: "narrow standing approval", with: "approval"), to: agents) + let drift = try run(command) + try require(!drift.succeeded, "changed standing approval unexpectedly passed") + try require(drift.output.contains("external-dependency contract does not match"), drift.output) + } + } + ), ( "interaction/rendering validator rejects guide and audit drift", { diff --git a/AgentGuidelines/VERSION b/AgentGuidelines/VERSION index 54a0022..a24809a 100644 --- a/AgentGuidelines/VERSION +++ b/AgentGuidelines/VERSION @@ -1 +1 @@ -0.0.42 +0.0.48