diff --git a/.agents/hooks/skill_activation.ts b/.agents/hooks/skill_activation.ts index 8598d60..26e75e1 100644 --- a/.agents/hooks/skill_activation.ts +++ b/.agents/hooks/skill_activation.ts @@ -74,8 +74,12 @@ const main = (): void => { } const skills = rules.skills ?? {}; + const isReviewChild = process.env.TOKI_REVIEW_CHILD === "1"; const matched = Object.entries(skills).flatMap(([name, rule]) => { + if (isReviewChild && name === "codex-review-loop") { + return []; + } const triggers = rule.promptTriggers; const hasTriggers = triggers !== undefined; diff --git a/.agents/skills/codex-review-loop/SKILL.md b/.agents/skills/codex-review-loop/SKILL.md new file mode 100644 index 0000000..69f6760 --- /dev/null +++ b/.agents/skills/codex-review-loop/SKILL.md @@ -0,0 +1,225 @@ +--- +name: codex-review-loop +description: Review Toki uncommitted changes, branches, or commits with path- and semantic-routed specialist lanes, normalize P0-P3 findings, and optionally fix only findings the user explicitly approves. Use when the user asks for a local code review, review loop, review-and-fix pass, re-review, or approved finding remediation in the Toki repository. Keep GitHub review submission, PR comments, push, merge, and other remote writes outside this skill. +--- + +# Toki Codex Review Loop + +Run an rvw-compatible lane review, then apply the codex-lb-style approval, +atomic-fix, verification, and bounded re-review loop. + +## Core Contract + +- Treat every review request as read-only until the user explicitly approves a + write mode. +- Never infer permission to edit or commit from phrases such as "review this" + or from a GitHub `@codex review` request. +- Never push, submit a GitHub review, comment, resolve threads, label, merge, or + otherwise mutate remote state from this skill. +- Preserve unrelated staged, unstaged, and untracked changes. Never use + `git reset`, `git checkout`, or broad restoration to undo a finding fix. +- Treat local usage logs, databases, audit findings, prompts, transcripts, and + credentials as sensitive. Do not print raw matched diff content. +- Apply `project-conventions` and read its task-specific references before + fixing Toki source, tests, resources, or project configuration. +- Apply `.agents/conventions/git-workflow.md` before any commit. A commit + requires explicit user authorization separate from review authorization. + +## Trust Boundary + +Scope resolution runs before any review and must survive a hostile repository. +Treat this section as the contract a finding is measured against: report a +finding when it breaks an invariant below, and classify it `wont_fix` with this +section as the reason when it only restates a non-goal. + +### Untrusted Inputs + +- Repository file contents and tree layout, including untracked and ignored + paths. +- Repository Git configuration: `.git/config`, `.gitattributes`, `.gitmodules`, + and any configured hook or helper program. +- The inherited `PATH` and every `GIT_*` environment variable. +- Lane result JSON, including every path and line number it reports. + +### Enforced Invariants + +1. Only trusted executables run. `git` and the review binary resolve from the + platform default path, never from an inherited `PATH`, and the runner narrows + `PATH` before invoking any external command. +2. No repository-configured program executes. Content filters, `textconv`, + external diff, `core.fsmonitor`, `core.hooksPath`, and lazy fetch are + overridden to inert values or the scope is refused. +3. Workspace confinement is derived from the filesystem, not from Git. The + inventory walks the real tree from the repository root and validates every + symbolic link target, so ignore rules and index modes cannot hide a path. +4. Every scope fails closed. A submodule, embedded repository, escaping symlink, + non-UTF-8 path, or unbounded inventory marks the scope unsafe and stops before + Codex is invoked. +5. Paths keep byte fidelity across process boundaries. Roots and path lists cross + as NUL-terminated data and are never trimmed. +6. Reported findings are untrusted data. A finding path must be + repository-relative, free of NUL, and free of empty, current, or parent + segments before it is used. +7. The baseline lane is always on. A registry that marks it otherwise is + rejected. + +Add a regression test with every change that touches an invariant, and name the +invariant in the test. + +### Non-Goals + +- Sandboxing the host. This skill reduces what executes during review; it does + not isolate a repository the user already chose to open. +- Restricting native review to the reviewed diff. `codex exec review` may read + other in-repository files by design, so confinement is to the repository root, + not to `changedPaths`. +- Content-level secret detection inside reviewed changes. Exclusion is by path + pattern only. +- Supporting attacker-chosen repository root paths, such as sibling names that + differ only by trailing whitespace. Exact roots are preserved, but the user is + assumed to name their own repository. +- Byte-exact handling of non-UTF-8 paths. Such scopes are refused, not + supported. + +## Workflow + +### 1. Resolve One Review Scope + +Choose exactly one scope: + +- Use `--uncommitted` for staged, unstaged, and untracked changes. +- Use `--base ` for the current branch relative to a base branch. +- Use `--commit ` for one commit. + +Prefer an exact scope named by the user. Do not fetch or change branches merely +to infer a scope. + +Run the resolver: + +~~~bash +python3 .agents/skills/codex-review-loop/scripts/resolve_review_scope.py \ + --repo . \ + --uncommitted \ + --pretty +~~~ + +Stop before invoking Codex when `safeToReview` is false or `hasChanges` is +false. Excluded sensitive paths make the scope unsafe because +`codex exec review --uncommitted` cannot exclude individual paths. + +### 2. Load Only Activated Review Rules + +Always read the common reviewer prompt and baseline lane. Read only the +specialist lane files named by `activatedLanes`. Also read the finding schema +and verification reference before reporting or fixing findings. + +Treat lane activation as additive. Never disable baseline correctness or +security reasoning merely because a path pattern did not match. + +### 3. Run One Pass Per Activated Lane + +Run the baseline lane and every activated specialist lane: + +~~~bash +.agents/skills/codex-review-loop/scripts/run_review_lane.sh \ + --repo . \ + --lane baseline \ + --base main +~~~ + +The runner passes bounded lane instructions through Codex +`developer_instructions`, uses the structured-output schema, requests an +ephemeral Codex session, and refuses inactive or unsafe lanes. Run lanes +sequentially by default. Do not add replicated reviewers or an adjudicator +unless the user explicitly expands the workflow. + +### 4. Validate And Merge Findings + +Capture each lane's JSON result in a temporary directory, then merge: + +~~~bash +python3 .agents/skills/codex-review-loop/scripts/merge_findings.py merge \ + /tmp/toki-review/baseline.json \ + /tmp/toki-review/remote-sync.json +~~~ + +Reject malformed results. Conservatively merge overlapping findings with a +similar root cause, retain every contributing lane, use the highest priority, +and surface materially conflicting priority opinions. + +### 5. Report Before Writing + +Present the normalized findings with ID, priority, confidence, location, impact, +suggested fix, and verification plan. Then request one explicit mode: + +1. Report only. +2. Fix selected findings without commits. +3. Fix all actionable findings without commits. +4. Fix selected findings and create one verified commit per finding. +5. Fix all actionable findings and create one verified commit per finding. + +Treat "Fix P0/P1 only" as a valid selection. For a P0/P1 fix that changes +existing product behavior rather than restoring clearly intended behavior, +describe that behavior change and obtain a second confirmation. + +### 6. Fix One Finding At A Time + +Before each fix: + +1. Re-read the affected code and relevant `project-conventions` references. +2. Inspect `git status` and the affected diff. +3. Record the pre-fix state of only the files or hunks that will change. +4. Use `apply_patch` for the narrow fix. +5. Run the smallest verification profile that can prove the finding fixed. + +If verification fails, reverse only the patch introduced for that finding. If +that boundary cannot be proven, stop and report the failure instead of risking +the user's work. + +For `--uncommitted` reviews, default to no commits. Create atomic commits only +when the user explicitly selects a commit mode and the new fix can be separated +from pre-existing changes without staging unrelated hunks. + +### 7. Re-review With A Bound + +After approved fixes, rerun baseline plus only lanes affected by the fix. Stop +after three total review rounds. If the same root cause returns in two +consecutive rounds, mark it `wont_fix` with the reason instead of looping. + +Count externally triggered rounds against the same bound. A pushed branch can +re-request review automatically, so a GitHub `@codex review` round is a round +here too. + +Fix at the invariant, not at the reported case. When a finding names one variant +of an invariant in the trust boundary, audit every other variant of that same +invariant locally and fix them together in one change. Repairing only the +reported case is what turns one finding into a new round, because the next round +reports the next variant. Before pushing, state which invariant each fix +restores and which adjacent variants were checked. + +### 8. Finish With A Local Report + +Report: + +- Review scope and activated lanes. +- Findings fixed, skipped, `wont_fix`, or left for reporting only. +- Verification commands and outcomes. +- Commits created, if explicitly authorized. +- Remaining risks or unverified checks. + +State explicitly that no push or GitHub write occurred. + +## Reference Map + +- Common contract: [reviewer.md](references/prompts/reviewer.md) +- Lane registry: [lane-registry.json](references/lane-registry.json) +- Finding schema: [review-findings.schema.json](references/schemas/review-findings.schema.json) +- Verification profiles: [verification.md](references/verification.md) +- Baseline: [baseline.md](references/lanes/baseline.md) +- Usage and pricing: [usage-pricing.md](references/lanes/usage-pricing.md) +- Privacy and security: [privacy-security.md](references/lanes/privacy-security.md) +- Remote sync: [remote-sync.md](references/lanes/remote-sync.md) +- Concurrency and lifecycle: [concurrency-lifecycle.md](references/lanes/concurrency-lifecycle.md) +- SwiftUI architecture: [swiftui-architecture.md](references/lanes/swiftui-architecture.md) +- Build and portability: [build-portability.md](references/lanes/build-portability.md) +- Testing: [testing.md](references/lanes/testing.md) diff --git a/.agents/skills/codex-review-loop/agents/openai.yaml b/.agents/skills/codex-review-loop/agents/openai.yaml new file mode 100644 index 0000000..083f36a --- /dev/null +++ b/.agents/skills/codex-review-loop/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Toki Codex Review Loop" + short_description: "Route Toki review lanes and fix approved findings" + default_prompt: "Use $codex-review-loop to review the current Toki changes and report findings before any fixes." diff --git a/.agents/skills/codex-review-loop/references/lane-registry.json b/.agents/skills/codex-review-loop/references/lane-registry.json new file mode 100644 index 0000000..cc34c64 --- /dev/null +++ b/.agents/skills/codex-review-loop/references/lane-registry.json @@ -0,0 +1,213 @@ +{ + "version": "1.0", + "pathExclusions": [ + ".git", + ".git/**", + ".claude/projects", + ".claude/projects/**", + ".codex/state_5.sqlite*", + ".codex/sessions", + ".codex/sessions/**", + ".codex/archived_sessions", + ".codex/archived_sessions/**", + ".config/Cursor/User/globalStorage", + ".config/Cursor/User/globalStorage/**", + "Library/Application Support/Cursor/User/globalStorage", + "Library/Application Support/Cursor/User/globalStorage/**", + ".gemini/tmp", + ".gemini/tmp/**", + ".gjc/agent/sessions", + ".gjc/agent/sessions/**", + ".hermes", + ".hermes/**", + ".local/share/opencode", + ".local/share/opencode/**", + ".openclaw/agents", + ".openclaw/agents/**", + ".local/state/toki-agent", + ".local/state/toki-agent/**", + ".local/state/toki", + ".local/state/toki/**", + "Library/Application Support/Toki", + "Library/Application Support/Toki/**", + ".omo", + ".omo/**", + ".senpi", + ".senpi/**", + ".codegraph", + ".codegraph/**", + ".env", + ".env.*", + "**/.env", + "**/.env.*", + "**/*.pem", + "*.pem", + "**/*.key", + "*.key", + "**/*.p12", + "*.p12", + "**/*.pfx", + "*.pfx", + "**/id_rsa*", + "id_rsa*", + "**/id_ed25519*", + "id_ed25519*", + "**/credentials.json", + "credentials.json", + "**/secrets.json", + "secrets.json", + "build", + "build/**", + "DerivedData", + "DerivedData/**" + ], + "lanes": [ + { + "id": "baseline", + "always": true, + "prompt": "references/lanes/baseline.md", + "pathPatterns": [], + "semanticPatterns": [], + "verificationProfiles": ["common"], + "execution": {"replicas": 1, "adjudication": false} + }, + { + "id": "usage-pricing", + "always": false, + "prompt": "references/lanes/usage-pricing.md", + "pathPatterns": [ + "Toki/Domain/Usage/**", + "Toki/Infrastructure/UsageReaders/**", + "Sources/TokiUsageCore/**", + "Sources/TokiUsageReaders/**", + "TokiTests/*Usage*", + "TokiTests/*Pricing*", + "TokiTests/*Reader*", + "Tests/TokiAgentTests/*Usage*", + "Tests/TokiAgentTests/*Ledger*" + ], + "semanticPatterns": [ + "(?i)\\b(token|usage|cost|price|pricing|model|attribution|timezone|date\\s*range|active\\s*time)\\b" + ], + "verificationProfiles": ["swift-package", "app-tests"], + "execution": {"replicas": 1, "adjudication": false} + }, + { + "id": "privacy-security", + "always": false, + "prompt": "references/lanes/privacy-security.md", + "pathPatterns": [ + "Toki/Domain/SecurityAudit/**", + "Toki/Infrastructure/SecurityAudit/**", + "Sources/TokiDurableStorage/**", + "Sources/TokiUsageReaders/**", + "TokiTests/*Security*", + "Tests/**/*Security*", + "Tests/**/*Privacy*" + ], + "semanticPatterns": [ + "(?i)\\b(secret|credential|api[_ -]?key|jwt|private[_ -]?key|mask|redact|sqlite|database|transcript|prompt|telemetry)\\b" + ], + "verificationProfiles": ["swift-package", "app-tests"], + "execution": {"replicas": 1, "adjudication": false} + }, + { + "id": "remote-sync", + "always": false, + "prompt": "references/lanes/remote-sync.md", + "pathPatterns": [ + "Toki/Infrastructure/RemoteSync/**", + "Sources/TokiSyncProtocol/**", + "Sources/TokiAgentCore/**", + "Sources/TokiAgent/**", + "Tests/TokiSyncProtocolTests/**", + "Tests/TokiAgentTests/**", + "TokiHub/Sources/**", + "TokiHub/Tests/**" + ], + "semanticPatterns": [ + "(?i)\\b(nonce|cipher|encrypt|decrypt|signature|replay|rollback|snapshot|anchor|pairing|durable|hub\\s*route)\\b" + ], + "verificationProfiles": ["swift-package", "hub"], + "execution": {"replicas": 1, "adjudication": false} + }, + { + "id": "concurrency-lifecycle", + "always": false, + "prompt": "references/lanes/concurrency-lifecycle.md", + "pathPatterns": [ + "Toki/App/**", + "Toki/Features/**", + "Toki/Infrastructure/Activity/**", + "Toki/Infrastructure/RemoteSync/**", + "Sources/TokiAgentCore/**", + "TokiHub/Sources/**" + ], + "semanticPatterns": [ + "(?i)\\b(Task|actor|MainActor|Sendable|async|await|Timer|cancell?ation|lock|mutex)\\b" + ], + "verificationProfiles": ["swift-package", "hub", "app-tests"], + "execution": {"replicas": 1, "adjudication": false} + }, + { + "id": "swiftui-architecture", + "always": false, + "prompt": "references/lanes/swiftui-architecture.md", + "pathPatterns": [ + "Toki/App/**", + "Toki/Features/**", + "TokiTests/*Panel*", + "TokiTests/*ViewModel*", + "TokiTests/*MenuBar*" + ], + "semanticPatterns": [ + "(?i)(SwiftUI|@State\\b|@StateObject\\b|@ObservedObject\\b|@Environment\\b|ViewModel\\b|NSStatusItem\\b|NSPanel\\b)" + ], + "verificationProfiles": ["app-format", "app-lint", "app-tests"], + "execution": {"replicas": 1, "adjudication": false} + }, + { + "id": "build-portability", + "always": false, + "prompt": "references/lanes/build-portability.md", + "pathPatterns": [ + "Package.swift", + "Package.resolved", + "TokiHub/Package.swift", + "TokiHub/Package.resolved", + "project.yml", + ".swiftformat", + ".swiftlint.yml", + ".github/workflows/**", + "Toki.xcodeproj/**", + "Sources/CSQLite/**", + "Toki/Resources/**", + "Toki/Assets.xcassets/**" + ], + "semanticPatterns": [ + "(?i)\\b(PackageDescription|swift-tools-version|dependency|xcodegen|resource|Vapor|Linux|CSQLite|Package\\.resolved)\\b" + ], + "verificationProfiles": ["swift-package", "hub", "app-format", "app-lint", "project"], + "execution": {"replicas": 1, "adjudication": false} + }, + { + "id": "testing", + "always": false, + "prompt": "references/lanes/testing.md", + "pathPatterns": [ + "Toki/**/*.swift", + "TokiTests/*.swift", + "TokiTests/**/*.swift", + "Sources/**/*.swift", + "Tests/**/*.swift", + "TokiHub/Sources/**/*.swift", + "TokiHub/Tests/**/*.swift" + ], + "semanticPatterns": [ + "(?i)\\b(XCTest|Testing|test[A-Z_]|assert|fixture|mock|stub)\\b" + ], + "verificationProfiles": ["swift-package", "hub", "app-tests"], + "execution": {"replicas": 1, "adjudication": false} + } + ] +} diff --git a/.agents/skills/codex-review-loop/references/lanes/baseline.md b/.agents/skills/codex-review-loop/references/lanes/baseline.md new file mode 100644 index 0000000..7f41d6a --- /dev/null +++ b/.agents/skills/codex-review-loop/references/lanes/baseline.md @@ -0,0 +1,15 @@ +# Baseline Lane + +Review every change for: + +- Incorrect behavior, broken invariants, off-by-one and boundary mistakes. +- Missing error handling, failure propagation, or invalid fallback behavior. +- Crashes, data loss, corruption, stale state, and silent partial success. +- Compatibility regressions in public or persisted data contracts. +- Unsafe assumptions about optional values, file presence, ordering, or input + shape. +- Material performance regressions on frequently executed paths. + +Trace important callers and consumers when the changed code alters a contract. +Do not duplicate a specialist finding unless baseline reasoning independently +identifies the same root cause. diff --git a/.agents/skills/codex-review-loop/references/lanes/build-portability.md b/.agents/skills/codex-review-loop/references/lanes/build-portability.md new file mode 100644 index 0000000..5c7f132 --- /dev/null +++ b/.agents/skills/codex-review-loop/references/lanes/build-portability.md @@ -0,0 +1,16 @@ +# Build And Portability Lane + +Review build, package, resource, and CI changes for: + +- Swift 5.9.2 compatibility for the Linux Agent and Hub. +- macOS-only imports or APIs leaking into cross-platform package targets. +- CSQLite availability, conditional dependencies, linker assumptions, and + platform guards. +- Vapor dependencies remaining isolated to the Hub package. +- XcodeGen source/resource membership, build settings, generated project drift, + and case-sensitive paths. +- Package resolution or lockfile changes that are missing, unintended, or + inconsistent across root, Hub, and Xcode workspaces. +- CI jobs no longer exercising required builds or tests. + +Prefer `project.yml` plus regeneration over direct project-file edits. diff --git a/.agents/skills/codex-review-loop/references/lanes/concurrency-lifecycle.md b/.agents/skills/codex-review-loop/references/lanes/concurrency-lifecycle.md new file mode 100644 index 0000000..343a28a --- /dev/null +++ b/.agents/skills/codex-review-loop/references/lanes/concurrency-lifecycle.md @@ -0,0 +1,19 @@ +# Concurrency And Lifecycle Lane + +Review asynchronous and lifecycle behavior for: + +- Actor isolation, `Sendable` assumptions, shared mutable state, and race + conditions. +- Main-actor blocking by file IO, SQLite, network calls, parsing, scanning, or + aggregation. +- Unstructured tasks that outlive their owner, duplicate work, or ignore + cancellation. +- Timer and notification lifecycles, retain cycles, repeated registration, and + missed teardown. +- Lock ordering, reentrancy, deadlocks, and state read outside protection. +- Stale async results overwriting newer refresh or configuration state. +- Errors or cancellation converted into successful or permanently loading UI + state. + +Verify ownership from creation through cancellation and deinitialization, not +only the task body. diff --git a/.agents/skills/codex-review-loop/references/lanes/privacy-security.md b/.agents/skills/codex-review-loop/references/lanes/privacy-security.md new file mode 100644 index 0000000..7351233 --- /dev/null +++ b/.agents/skills/codex-review-loop/references/lanes/privacy-security.md @@ -0,0 +1,17 @@ +# Privacy And Security Lane + +Review local data and security-audit behavior for: + +- Raw prompts, transcripts, usage rows, credentials, tokens, private keys, or + audit matches reaching logs, diagnostics, caches, exports, or the network. +- Missing masking, unsafe previews, secret reconstruction, or overly broad + persisted cache fields. +- Unsafe filesystem traversal, symlink handling, permissions, or SQLite query + construction. +- Scanner false negatives caused by decoding, truncation, cancellation, or + cache invalidation errors. +- Telemetry or network transmission added without explicit product intent. +- Error messages that disclose sensitive paths or values. + +Never quote a discovered secret or sensitive record in a finding. Identify only +the data category and affected code path. diff --git a/.agents/skills/codex-review-loop/references/lanes/remote-sync.md b/.agents/skills/codex-review-loop/references/lanes/remote-sync.md new file mode 100644 index 0000000..f89ad8c --- /dev/null +++ b/.agents/skills/codex-review-loop/references/lanes/remote-sync.md @@ -0,0 +1,19 @@ +# Remote Sync Lane + +Review protocol, Agent, Hub, and app sync behavior for: + +- Encryption and authentication ordering, nonce uniqueness, key/identity + binding, signature validation, and downgrade resistance. +- Replay, rollback, stale snapshot, anchor, generation, and replacement checks. +- Validation symmetry across producer, transport, Hub storage, cache, and app + consumption. +- Atomic durable writes, crash recovery, lock behavior, and corrupt-state + fallback. +- Cache reuse and offline fallback that could accept incompatible or older + data. +- Vapor route authentication, payload limits, status mapping, and error + disclosure. +- Linux Agent/Hub behavior and wire-format compatibility. + +Treat weakened validation or recovery that can silently accept stale or +unauthenticated data as a high-priority correctness or security defect. diff --git a/.agents/skills/codex-review-loop/references/lanes/swiftui-architecture.md b/.agents/skills/codex-review-loop/references/lanes/swiftui-architecture.md new file mode 100644 index 0000000..d66e814 --- /dev/null +++ b/.agents/skills/codex-review-loop/references/lanes/swiftui-architecture.md @@ -0,0 +1,17 @@ +# SwiftUI Architecture Lane + +Review app, menu-bar, view, and view-model changes for: + +- Correct ownership of `@State`, `@StateObject`, `@ObservedObject`, and + environment values. +- Views performing parsing, aggregation, scanning, persistence, file IO, or + refresh orchestration. +- Feedback loops, repeated `onAppear` work, stale bindings, and state reset + caused by view identity changes. +- Main-actor correctness and observable updates arriving from background work. +- Menu-bar, panel, status-item, AppKit bridge, and application lifecycle + regressions. +- Loading, failure, and empty states that make operational status misleading. +- Compact layout instability that makes controls or counters jump or disappear. + +Respect the existing Toki layer boundaries and compact operational design. diff --git a/.agents/skills/codex-review-loop/references/lanes/testing.md b/.agents/skills/codex-review-loop/references/lanes/testing.md new file mode 100644 index 0000000..2a4bdb0 --- /dev/null +++ b/.agents/skills/codex-review-loop/references/lanes/testing.md @@ -0,0 +1,18 @@ +# Testing Lane + +Review tests and test coverage for: + +- Changed behavior without a focused regression test that would fail for the + identified defect. +- Missing boundary cases for date ranges, time zones, pricing fallbacks, + malformed reader input, security masking, and sync validation. +- Nondeterministic clocks, filesystem order, networking, timers, concurrency, + or global state. +- Fixtures that accidentally contain real local usage data, secrets, prompts, + or credentials. +- Assertions that cannot distinguish success from a silent fallback or empty + result. +- Platform gaps between macOS app tests and Swift 5.9.2 Linux package tests. + +Report a test-only finding only when the missing or incorrect test creates a +concrete regression risk. Avoid generic requests for more coverage. diff --git a/.agents/skills/codex-review-loop/references/lanes/usage-pricing.md b/.agents/skills/codex-review-loop/references/lanes/usage-pricing.md new file mode 100644 index 0000000..c4e7afd --- /dev/null +++ b/.agents/skills/codex-review-loop/references/lanes/usage-pricing.md @@ -0,0 +1,18 @@ +# Usage And Pricing Lane + +Review usage readers, pricing, aggregation, and reporting for: + +- JSONL, SQLite, filesystem, and cache parsing across malformed, partial, or + version-skewed records. +- Token totals, model/source/project attribution, deduplication, and migration + behavior. +- Cost calculation, pricing fallback precedence, unpriced rows, and model-name + normalization. +- Date boundaries, time zones, daylight-saving transitions, active time, and + wall-clock aggregation. +- Reader diagnostics that distinguish missing data from parse or permission + failures without exposing raw local content. +- Deterministic aggregation and focused tests for edge cases and fallbacks. + +Treat a plausible overcount, undercount, or incorrect charge as a correctness +finding, not a cosmetic issue. diff --git a/.agents/skills/codex-review-loop/references/prompts/reviewer.md b/.agents/skills/codex-review-loop/references/prompts/reviewer.md new file mode 100644 index 0000000..e25aa36 --- /dev/null +++ b/.agents/skills/codex-review-loop/references/prompts/reviewer.md @@ -0,0 +1,39 @@ +# Common Reviewer Contract + +Perform a read-only code review of only the native `codex exec review` scope. +Use the `review-scope-json` payload to preserve the resolver's comparison mode. +Do not edit files, create commits, or write remote state. +Act as the leaf reviewer for the already-running loop. Do not invoke +`codex-review-loop`, start another Codex process, or delegate another review. + +Inspect the changed code, the minimum surrounding implementation needed to +understand it, and relevant tests. Report only actionable defects introduced or +exposed by the reviewed change. Do not report optional refactors, naming +preferences, general hardening ideas, or pre-existing issues that the change +does not worsen. + +Use these priorities: + +- P0: Release-blocking widespread data loss, secret exposure, authentication + bypass, or a change that makes the primary product unusable. +- P1: High-impact incorrect core behavior, security failure, crash, corruption, + or a likely production outage. +- P2: Concrete functional, reliability, portability, or performance bug with + limited impact and a clear fix. +- P3: Minor but real correctness or maintainability defect. Do not use P3 for + style nits or speculative improvements. + +For each finding: + +- Point to the smallest relevant line range in the reviewed diff whenever + possible. +- Explain the failing scenario and observable impact. +- State a root cause independently of the symptom. +- Propose a narrow fix and a verification method. +- Mask credentials, prompts, transcripts, database values, and other sensitive + content. Describe the data category instead of quoting it. +- Use repository-relative paths. Never emit absolute paths or `..` segments. + +Return only JSON matching the supplied schema. Set `lane` to the selected lane +ID. Use `verdict: "clean"` with an empty findings array when no actionable +defects are found; otherwise use `verdict: "findings"`. diff --git a/.agents/skills/codex-review-loop/references/schemas/review-findings.schema.json b/.agents/skills/codex-review-loop/references/schemas/review-findings.schema.json new file mode 100644 index 0000000..917d94a --- /dev/null +++ b/.agents/skills/codex-review-loop/references/schemas/review-findings.schema.json @@ -0,0 +1,87 @@ +{ + "title": "Toki Codex Review Lane Result", + "type": "object", + "additionalProperties": false, + "required": [ + "schemaVersion", + "lane", + "verdict", + "summary", + "findings" + ], + "properties": { + "schemaVersion": { + "type": "string", + "enum": ["1.0"] + }, + "lane": { + "type": "string" + }, + "verdict": { + "type": "string", + "enum": ["clean", "findings"] + }, + "summary": { + "type": "string" + }, + "findings": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "priority", + "confidence", + "title", + "file", + "startLine", + "endLine", + "rootCause", + "evidence", + "impact", + "suggestedFix", + "verification" + ], + "properties": { + "priority": { + "type": "string", + "enum": ["P0", "P1", "P2", "P3"] + }, + "confidence": { + "type": "number" + }, + "title": { + "type": "string" + }, + "file": { + "type": "string" + }, + "startLine": { + "type": "integer" + }, + "endLine": { + "type": "integer" + }, + "rootCause": { + "type": "string" + }, + "evidence": { + "type": "string" + }, + "impact": { + "type": "string" + }, + "suggestedFix": { + "type": "string" + }, + "verification": { + "type": "array", + "items": { + "type": "string" + } + } + } + } + } + } +} diff --git a/.agents/skills/codex-review-loop/references/verification.md b/.agents/skills/codex-review-loop/references/verification.md new file mode 100644 index 0000000..d8dfd50 --- /dev/null +++ b/.agents/skills/codex-review-loop/references/verification.md @@ -0,0 +1,86 @@ +# Verification Profiles + +Run verification only after the user authorizes fixes. During review-only mode, +use read-only inspection commands and do not run `xcodegen generate` or other +commands that rewrite files. + +## Profiles + +### common + +~~~bash +git diff --check +~~~ + +### swift-package + +Use for root package, protocol, reader, durable-storage, and Agent changes: + +~~~bash +swift test +~~~ + +For broad Agent changes, also build the release product: + +~~~bash +swift build -c release --product toki-agent +~~~ + +### hub + +~~~bash +swift test --package-path TokiHub +swift build --package-path TokiHub -c release --product toki-hub +~~~ + +### app-format + +~~~bash +swiftformat . --lint +~~~ + +### app-lint + +~~~bash +swiftlint lint --strict --quiet +~~~ + +### app-tests + +~~~bash +xcodebuild test \ + -project Toki.xcodeproj \ + -scheme Toki \ + -destination "platform=macOS" \ + CODE_SIGN_IDENTITY="" \ + CODE_SIGNING_REQUIRED=NO \ + CODE_SIGNING_ALLOWED=NO +~~~ + +### project + +After an explicitly approved `project.yml` or resource change: + +~~~bash +xcodegen generate +git diff -- Toki.xcodeproj +~~~ + +Include the generated project diff with the source configuration change. + +## Selection + +- Start with the narrowest profile named by the affected lane and path. +- Run every affected package's tests for cross-package contract changes. +- Run the full format, lint, package, Hub, and app test set before a PR or broad + source change. +- Never print raw local logs, database rows, prompts, transcripts, audit + findings, or secret-bearing fixtures in test output. + +## Failed Verification + +1. Preserve the pre-fix user state. +2. Identify the exact patch introduced for the current finding. +3. Reverse only that patch with `apply_patch`. +4. Mark the finding `skipped` with the failed command and concise reason. +5. Stop instead of reverting when the fix boundary overlaps unrelated work. diff --git a/.agents/skills/codex-review-loop/scripts/finding_grouping.py b/.agents/skills/codex-review-loop/scripts/finding_grouping.py new file mode 100644 index 0000000..027d571 --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/finding_grouping.py @@ -0,0 +1,38 @@ +"""Order-independent connected components for review findings.""" + +from __future__ import annotations + +from collections.abc import Callable, Sequence +from typing import TypeVar + + +Item = TypeVar("Item") + + +def connected_components( + items: Sequence[Item], + related: Callable[[Item, Item], bool], +) -> list[list[Item]]: + parents = list(range(len(items))) + + def find(index: int) -> int: + while parents[index] != index: + parents[index] = parents[parents[index]] + index = parents[index] + return index + + def union(left: int, right: int) -> None: + left_root = find(left) + right_root = find(right) + if left_root != right_root: + parents[right_root] = left_root + + for left in range(len(items)): + for right in range(left + 1, len(items)): + if related(items[left], items[right]): + union(left, right) + + grouped: dict[int, list[Item]] = {} + for index, item in enumerate(items): + grouped.setdefault(find(index), []).append(item) + return list(grouped.values()) diff --git a/.agents/skills/codex-review-loop/scripts/finding_validation.py b/.agents/skills/codex-review-loop/scripts/finding_validation.py new file mode 100644 index 0000000..baa02ed --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/finding_validation.py @@ -0,0 +1,137 @@ +"""Validation for structured Toki review findings.""" + +from __future__ import annotations + +import json +import re +from pathlib import Path +from typing import Any + + +TOP_LEVEL_FIELDS = {"schemaVersion", "lane", "verdict", "summary", "findings"} +FINDING_FIELDS = { + "priority", + "confidence", + "title", + "file", + "startLine", + "endLine", + "rootCause", + "evidence", + "impact", + "suggestedFix", + "verification", +} +PRIORITY_RANK = {"P0": 0, "P1": 1, "P2": 2, "P3": 3} +LANE_RE = re.compile(r"^[a-z0-9]+(?:-[a-z0-9]+)*$") + + +class FindingError(ValueError): + """Raised for malformed lane output.""" + + +def require_non_empty_string(value: Any, field: str) -> str: + if not isinstance(value, str) or not value.strip(): + raise FindingError(f"{field} must be a non-empty string") + normalized = value.strip() + try: + normalized.encode("utf-8", errors="strict") + except UnicodeEncodeError as error: + raise FindingError(f"{field} must be valid UTF-8") from error + return normalized + + +def validate_repo_path(value: Any) -> str: + if not isinstance(value, str) or value == "": + raise FindingError("file must be a non-empty string") + try: + value.encode("utf-8", errors="strict") + except UnicodeEncodeError as error: + raise FindingError("file must be valid UTF-8") from error + path = value + if "\0" in path: + raise FindingError("file must not contain NUL") + if path.startswith("/"): + raise FindingError("file must be a repository-relative POSIX path") + parts = path.split("/") + if any(part in {"", ".", ".."} for part in parts): + raise FindingError( + "file must be repository-relative without empty, current, or parent path segments" + ) + return path + + +def validate_lane_result(data: Any, expected_lane: str | None = None) -> dict[str, Any]: + if not isinstance(data, dict): + raise FindingError("lane result must be an object") + if set(data) != TOP_LEVEL_FIELDS: + missing = sorted(TOP_LEVEL_FIELDS - set(data)) + extra = sorted(set(data) - TOP_LEVEL_FIELDS) + raise FindingError(f"lane result fields mismatch; missing={missing} extra={extra}") + if data["schemaVersion"] != "1.0": + raise FindingError("schemaVersion must be 1.0") + + lane = data["lane"] + if ( + not isinstance(lane, str) + or lane == "" + or lane != lane.strip() + or LANE_RE.fullmatch(lane) is None + ): + raise FindingError("lane must be a valid lane id") + if expected_lane is not None and lane != expected_lane: + raise FindingError(f"expected lane {expected_lane}, got {lane}") + + verdict = data["verdict"] + if verdict not in {"clean", "findings"}: + raise FindingError("verdict must be clean or findings") + require_non_empty_string(data["summary"], "summary") + findings = data["findings"] + if not isinstance(findings, list): + raise FindingError("findings must be an array") + if (verdict == "clean") != (len(findings) == 0): + raise FindingError("clean requires no findings and findings requires at least one") + + for index, finding in enumerate(findings): + prefix = f"findings[{index}]" + if not isinstance(finding, dict): + raise FindingError(f"{prefix} must be an object") + if set(finding) != FINDING_FIELDS: + missing = sorted(FINDING_FIELDS - set(finding)) + extra = sorted(set(finding) - FINDING_FIELDS) + raise FindingError(f"{prefix} fields mismatch; missing={missing} extra={extra}") + if finding["priority"] not in PRIORITY_RANK: + raise FindingError(f"{prefix}.priority is invalid") + confidence = finding["confidence"] + if isinstance(confidence, bool) or not isinstance(confidence, (int, float)): + raise FindingError(f"{prefix}.confidence must be numeric") + if not 0 <= confidence <= 1: + raise FindingError(f"{prefix}.confidence must be between 0 and 1") + for field in ("title", "rootCause", "evidence", "impact", "suggestedFix"): + require_non_empty_string(finding[field], f"{prefix}.{field}") + finding["file"] = validate_repo_path(finding["file"]) + start = finding["startLine"] + end = finding["endLine"] + if type(start) is not int or start < 1: + raise FindingError(f"{prefix}.startLine must be a positive integer") + if type(end) is not int or end < start: + raise FindingError(f"{prefix}.endLine must be an integer at least startLine") + verification = finding["verification"] + if not isinstance(verification, list) or not verification: + raise FindingError(f"{prefix}.verification must contain non-empty strings") + for verification_index, item in enumerate(verification): + require_non_empty_string( + item, + f"{prefix}.verification[{verification_index}]", + ) + return data + + +def load_result(path: Path, expected_lane: str | None = None) -> dict[str, Any]: + try: + data = json.loads(path.read_text(encoding="utf-8")) + except UnicodeError as error: + raise FindingError(f"result is not valid UTF-8: {error}") from error + except (OSError, json.JSONDecodeError) as error: + raise FindingError(f"cannot load {path}: {error}") from error + return validate_lane_result(data, expected_lane) diff --git a/.agents/skills/codex-review-loop/scripts/lane_registry_validation.py b/.agents/skills/codex-review-loop/scripts/lane_registry_validation.py new file mode 100644 index 0000000..cf8d109 --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/lane_registry_validation.py @@ -0,0 +1,70 @@ +"""Invariant validation for Codex review lane registries.""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + + +VERIFICATION_PROFILES = frozenset( + { + "common", + "swift-package", + "hub", + "app-format", + "app-lint", + "app-tests", + "project", + } +) + + +class RegistryError(ValueError): + """Raised when a lane registry violates the review contract.""" + + +def validate_registry_contract( + registry: dict[str, Any], + *, + skill_dir: Path, +) -> None: + lanes_dir = (skill_dir / "references" / "lanes").resolve() + baseline: dict[str, Any] | None = None + for lane in registry["lanes"]: + lane_id = lane["id"] + if lane_id == "baseline" and lane.get("always") is not True: + raise RegistryError("baseline lane must be always-on") + if type(lane.get("always")) is not bool: + raise RegistryError(f"{lane_id}.always must be a boolean") + if lane_id == "baseline": + baseline = lane + + profiles = lane["verificationProfiles"] + if ( + not profiles + or len(profiles) != len(set(profiles)) + or not set(profiles).issubset(VERIFICATION_PROFILES) + ): + raise RegistryError(f"{lane_id}.verificationProfiles is invalid") + + execution = lane["execution"] + if ( + set(execution) != {"replicas", "adjudication"} + or type(execution["replicas"]) is not int + or execution["replicas"] != 1 + or execution["adjudication"] is not False + ): + raise RegistryError(f"{lane_id}.execution is unsupported") + + prompt = lane["prompt"] + if Path(prompt).is_absolute() or not prompt.endswith(".md"): + raise RegistryError(f"{lane_id}.prompt must be a relative lane Markdown path") + prompt_path = (skill_dir / prompt).resolve() + try: + prompt_path.relative_to(lanes_dir) + prompt_path.read_text(encoding="utf-8") + except (OSError, UnicodeError, ValueError) as error: + raise RegistryError(f"{lane_id}.prompt is not a readable lane prompt") from error + + if baseline is None or baseline["always"] is not True: + raise RegistryError("baseline lane must be always-on") diff --git a/.agents/skills/codex-review-loop/scripts/merge_findings.py b/.agents/skills/codex-review-loop/scripts/merge_findings.py new file mode 100755 index 0000000..7434134 --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/merge_findings.py @@ -0,0 +1,258 @@ +#!/usr/bin/env python3 +"""Validate and conservatively merge structured Toki review findings.""" + +from __future__ import annotations + +import argparse +import difflib +import hashlib +import json +import re +import sys +from pathlib import Path +from typing import Any, Iterable + +from finding_grouping import connected_components +from finding_validation import FindingError, PRIORITY_RANK, load_result + + +WORD_RE = re.compile(r"[^\W_]+", re.UNICODE) + + +def normalized_words(value: str) -> set[str]: + return set(WORD_RE.findall(value.casefold())) + + +def similarity(left: str, right: str) -> float: + left_words = normalized_words(left) + right_words = normalized_words(right) + phrase_similarity = difflib.SequenceMatcher( + None, + left.casefold(), + right.casefold(), + ).ratio() + if not left_words or not right_words: + return phrase_similarity + token_similarity = len(left_words & right_words) / len(left_words | right_words) + return max(token_similarity, phrase_similarity) + + +def ranges_overlap(left: dict[str, Any], right: dict[str, Any]) -> bool: + return left["startLine"] <= right["endLine"] and right["startLine"] <= left["endLine"] + + +def is_duplicate(left: dict[str, Any], right: dict[str, Any]) -> bool: + if left["file"] != right["file"] or not ranges_overlap(left, right): + return False + return similarity(left["rootCause"], right["rootCause"]) >= 0.65 + + +def ordered_unique(values: Iterable[str]) -> list[str]: + seen: set[str] = set() + result: list[str] = [] + for value in values: + if value not in seen: + seen.add(value) + result.append(value) + return result + + +def finding_id(group: dict[str, Any]) -> str: + root = " ".join(sorted(normalized_words(group["rootCause"]))) + if not root: + root = group["rootCause"].casefold().strip() + identity = f"{group['file']}:{group['startLine']}:{group['endLine']}:{root}" + digest = hashlib.sha256(identity.encode("utf-8")).hexdigest()[:12] + return f"RF-{digest}" + + +def new_group(lane: str, finding: dict[str, Any]) -> dict[str, Any]: + return { + "id": "", + "lanes": [lane], + "priority": finding["priority"], + "priorityOpinions": {lane: finding["priority"]}, + "confidence": finding["confidence"], + "title": finding["title"].strip(), + "file": finding["file"], + "startLine": finding["startLine"], + "endLine": finding["endLine"], + "rootCause": finding["rootCause"].strip(), + "evidence": [{"lane": lane, "text": finding["evidence"].strip()}], + "impact": finding["impact"].strip(), + "suggestedFix": finding["suggestedFix"].strip(), + "verification": ordered_unique(item.strip() for item in finding["verification"]), + "status": "proposed", + "occurrences": 1, + } + + +def merge_into(group: dict[str, Any], lane: str, finding: dict[str, Any]) -> None: + group["occurrences"] += 1 + current_rank = PRIORITY_RANK[group["priority"]] + incoming_rank = PRIORITY_RANK[finding["priority"]] + use_incoming = incoming_rank < current_rank or ( + incoming_rank == current_rank and finding["confidence"] > group["confidence"] + ) + if use_incoming: + for field in ("title", "rootCause", "impact", "suggestedFix"): + group[field] = finding[field].strip() + + group["priority"] = min( + (group["priority"], finding["priority"]), + key=lambda priority: PRIORITY_RANK[priority], + ) + group["confidence"] = max(group["confidence"], finding["confidence"]) + group["startLine"] = min(group["startLine"], finding["startLine"]) + group["endLine"] = max(group["endLine"], finding["endLine"]) + group["lanes"] = ordered_unique([*group["lanes"], lane]) + previous_opinion = group["priorityOpinions"].get(lane) + if ( + previous_opinion is None + or PRIORITY_RANK[finding["priority"]] < PRIORITY_RANK[previous_opinion] + ): + group["priorityOpinions"][lane] = finding["priority"] + evidence = {"lane": lane, "text": finding["evidence"].strip()} + if evidence not in group["evidence"]: + group["evidence"].append(evidence) + group["verification"] = ordered_unique( + [*group["verification"], *(item.strip() for item in finding["verification"])] + ) + + +def member_sort_key(member: tuple[str, dict[str, Any]]) -> tuple[Any, ...]: + lane, finding = member + return ( + PRIORITY_RANK[finding["priority"]], + -finding["confidence"], + finding["rootCause"].strip().casefold(), + finding["title"].strip().casefold(), + finding["file"], + finding["startLine"], + finding["endLine"], + lane, + finding["evidence"].strip(), + finding["impact"].strip(), + finding["suggestedFix"].strip(), + ) + + +def finish_group(group: dict[str, Any]) -> None: + group["lanes"] = sorted(group["lanes"]) + group["priorityOpinions"] = { + lane: group["priorityOpinions"][lane] + for lane in sorted(group["priorityOpinions"]) + } + group["evidence"].sort(key=lambda evidence: (evidence["lane"], evidence["text"])) + group["verification"] = sorted(group["verification"]) + group["id"] = finding_id(group) + + +def merge_results(results: list[dict[str, Any]]) -> dict[str, Any]: + groups: list[dict[str, Any]] = [] + lane_results: list[dict[str, Any]] = [] + members: list[tuple[str, dict[str, Any]]] = [] + for result in results: + lane = result["lane"] + lane_results.append( + { + "lane": lane, + "verdict": result["verdict"], + "summary": result["summary"].strip(), + } + ) + members.extend((lane, finding) for finding in result["findings"]) + + for component in connected_components( + members, + lambda left, right: is_duplicate(left[1], right[1]), + ): + ordered_members = sorted(component, key=member_sort_key) + first_lane, first_finding = ordered_members[0] + group = new_group(first_lane, first_finding) + for lane, finding in ordered_members[1:]: + merge_into(group, lane, finding) + finish_group(group) + groups.append(group) + + lane_results.sort(key=lambda result: result["lane"]) + groups.sort( + key=lambda group: ( + PRIORITY_RANK[group["priority"]], + group["file"], + group["startLine"], + group["id"], + ) + ) + + conflicts: list[dict[str, Any]] = [] + for group in groups: + ranks = [PRIORITY_RANK[value] for value in group["priorityOpinions"].values()] + if ranks and max(ranks) - min(ranks) >= 2: + conflicts.append( + { + "id": group["id"], + "priorityOpinions": group["priorityOpinions"], + "reason": "Contributing lanes differ by at least two priority levels.", + } + ) + + return { + "schemaVersion": "1.0", + "verdict": "findings" if groups else "clean", + "laneResults": lane_results, + "findings": groups, + "conflicts": conflicts, + } + + +def build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser(description=__doc__) + subparsers = parser.add_subparsers(dest="command", required=True) + + validate_parser = subparsers.add_parser("validate", help="Validate one lane result.") + validate_parser.add_argument("--lane", help="Require this lane ID.") + validate_parser.add_argument("result", type=Path) + + merge_parser = subparsers.add_parser("merge", help="Merge validated lane results.") + merge_parser.add_argument("results", nargs="+", type=Path) + merge_parser.add_argument("--compact", action="store_true") + return parser + + +def main() -> int: + arguments = build_parser().parse_args() + try: + if arguments.command == "validate": + result = load_result(arguments.result, arguments.lane) + output: dict[str, Any] = { + "valid": True, + "lane": result["lane"], + "findingCount": len(result["findings"]), + } + compact = True + else: + results = [load_result(path) for path in arguments.results] + lanes = [result["lane"] for result in results] + if len(lanes) != len(set(lanes)): + raise FindingError("each lane may appear only once in a merge") + output = merge_results(results) + compact = arguments.compact + except FindingError as error: + print(f"merge_findings.py: {error}", file=sys.stderr) + return 2 + + json.dump( + output, + sys.stdout, + ensure_ascii=True, + indent=None if compact else 2, + separators=(",", ":") if compact else None, + sort_keys=False, + ) + sys.stdout.write("\n") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.agents/skills/codex-review-loop/scripts/resolve_repo_root.py b/.agents/skills/codex-review-loop/scripts/resolve_repo_root.py new file mode 100644 index 0000000..89f0f19 --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/resolve_repo_root.py @@ -0,0 +1,25 @@ +#!/usr/bin/env python3 +"""Emit an exact NUL-terminated repository root for the shell runner.""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path + +from review_git_process import ScopeError, git_root + + +def main() -> int: + try: + root = git_root(Path(sys.argv[1])) + except (OSError, ScopeError) as error: + print(f"resolve_repo_root.py: {error}", file=sys.stderr) + return 2 + sys.stdout.buffer.write(os.fsencode(root)) + sys.stdout.buffer.write(b"\0") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.agents/skills/codex-review-loop/scripts/resolve_review_scope.py b/.agents/skills/codex-review-loop/scripts/resolve_review_scope.py new file mode 100755 index 0000000..df76956 --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/resolve_review_scope.py @@ -0,0 +1,271 @@ +#!/usr/bin/env python3 +"""Resolve a Toki review scope and activate additive specialist lanes.""" + +from __future__ import annotations + +import argparse +import json +import re +import sys +from collections import Counter +from pathlib import Path +from typing import Any + +from lane_registry_validation import RegistryError, validate_registry_contract +from review_scope_git import ( + ScopeError, + base_scope, + commit_scope, + git_root, + matches_pattern, + matching_pattern, + ordered_unique, + semantic_text, + uncommitted_scope, +) + + +SCRIPT_DIR = Path(__file__).resolve().parent +SKILL_DIR = SCRIPT_DIR.parent +DEFAULT_REGISTRY = SKILL_DIR / "references" / "lane-registry.json" +LANE_ID_RE = re.compile(r"^[a-z0-9]+(?:-[a-z0-9]+)*$") + + +def load_registry(path: Path) -> dict[str, Any]: + try: + registry = json.loads(path.read_text(encoding="utf-8")) + except (OSError, UnicodeError, json.JSONDecodeError) as error: + raise ScopeError(f"cannot load lane registry: {error}") from error + + if not isinstance(registry, dict) or registry.get("version") != "1.0": + raise ScopeError("lane registry must be a version 1.0 object") + exclusions = registry.get("pathExclusions") + lanes = registry.get("lanes") + if not isinstance(exclusions, list) or not all(isinstance(item, str) for item in exclusions): + raise ScopeError("lane registry pathExclusions must be a string array") + if not isinstance(lanes, list) or not lanes: + raise ScopeError("lane registry lanes must be a non-empty array") + + seen_ids: set[str] = set() + for lane in lanes: + if not isinstance(lane, dict): + raise ScopeError("every lane must be an object") + lane_id = lane.get("id") + if not isinstance(lane_id, str) or LANE_ID_RE.fullmatch(lane_id) is None: + raise ScopeError(f"invalid lane id: {lane_id!r}") + if lane_id in seen_ids: + raise ScopeError(f"duplicate lane id: {lane_id}") + seen_ids.add(lane_id) + + for field in ("pathPatterns", "semanticPatterns", "verificationProfiles"): + value = lane.get(field) + if not isinstance(value, list) or not all(isinstance(item, str) for item in value): + raise ScopeError(f"{lane_id}.{field} must be a string array") + for pattern in lane["semanticPatterns"]: + try: + re.compile(pattern) + except re.error as error: + raise ScopeError(f"invalid semantic pattern for {lane_id}: {error}") from error + + prompt = lane.get("prompt") + if not isinstance(prompt, str) or not prompt: + raise ScopeError(f"{lane_id}.prompt must be a non-empty string") + prompt_path = (SKILL_DIR / prompt).resolve() + try: + prompt_path.relative_to(SKILL_DIR) + except ValueError as error: + raise ScopeError(f"{lane_id}.prompt escapes the skill directory") from error + if not prompt_path.is_file(): + raise ScopeError(f"lane prompt does not exist: {prompt}") + + execution = lane.get("execution") + if ( + not isinstance(execution, dict) + or type(execution.get("replicas")) is not int + or execution["replicas"] < 1 + or type(execution.get("adjudication")) is not bool + ): + raise ScopeError(f"{lane_id}.execution is invalid") + + if "baseline" not in seen_ids: + raise ScopeError("lane registry must include baseline") + try: + validate_registry_contract(registry, skill_dir=SKILL_DIR) + except RegistryError as error: + raise ScopeError(str(error)) from error + return registry + + +def activate_lanes( + registry: dict[str, Any], + changed_paths: list[str], + changed_semantics: str, + semantic_inspection_complete: bool, +) -> list[dict[str, Any]]: + activated: list[dict[str, Any]] = [] + for lane in registry["lanes"]: + reasons: list[dict[str, Any]] = [] + if lane.get("always") is True: + reasons.append({"type": "always"}) + elif not semantic_inspection_complete: + reasons.append({"type": "semantic-fallback"}) + + path_hits = [ + path + for path in changed_paths + if any(matches_pattern(path, pattern) for pattern in lane["pathPatterns"]) + ] + if path_hits: + reasons.append({"type": "path", "matches": path_hits}) + + semantic_hits = [ + pattern + for pattern in lane["semanticPatterns"] + if re.search(pattern, changed_semantics) is not None + ] + if semantic_hits: + reasons.append( + { + "type": "semantic", + "matchedPatternCount": len(semantic_hits), + } + ) + + if reasons: + activated.append( + { + "id": lane["id"], + "prompt": lane["prompt"], + "verificationProfiles": lane["verificationProfiles"], + "execution": lane["execution"], + "reasons": reasons, + } + ) + return activated + + +def parse_arguments() -> argparse.Namespace: + parser = argparse.ArgumentParser( + description="Resolve a git review scope and select Toki review lanes." + ) + parser.add_argument("--repo", default=".", help="Path inside the target git repository.") + parser.add_argument( + "--registry", + type=Path, + default=DEFAULT_REGISTRY, + help="Lane registry path.", + ) + parser.add_argument("--pretty", action="store_true", help="Pretty-print JSON output.") + scope = parser.add_mutually_exclusive_group(required=True) + scope.add_argument("--uncommitted", action="store_true") + scope.add_argument("--base") + scope.add_argument("--commit") + return parser.parse_args() + + +def resolve(arguments: argparse.Namespace) -> dict[str, Any]: + root = git_root(Path(arguments.repo).resolve()) + registry = load_registry(arguments.registry.resolve()) + exclusions: list[str] = registry["pathExclusions"] + if arguments.uncommitted: + changed_paths, untracked_paths, diff, semantic_inspection_complete = ( + uncommitted_scope(root, exclusions) + ) + scope = { + "kind": "uncommitted", + "argument": None, + "codexArgs": ["--uncommitted"], + } + elif arguments.base is not None: + changed_paths, diff, semantic_inspection_complete = base_scope( + root, + arguments.base, + exclusions, + ) + untracked_paths = [] + scope = { + "kind": "base", + "argument": arguments.base, + "codexArgs": ["--base", arguments.base], + } + else: + changed_paths, diff, semantic_inspection_complete = commit_scope( + root, + arguments.commit, + exclusions, + ) + untracked_paths = [] + scope = { + "kind": "commit", + "argument": arguments.commit, + "codexArgs": ["--commit", arguments.commit], + } + + excluded_by_pattern: Counter[str] = Counter() + reviewed_paths: list[str] = [] + for path in changed_paths: + pattern = matching_pattern(path, exclusions) + if pattern is None: + reviewed_paths.append(path) + else: + excluded_by_pattern[pattern] += 1 + + activated = activate_lanes( + registry, + reviewed_paths, + semantic_text(diff), + semantic_inspection_complete, + ) + verification_profiles = ordered_unique( + profile + for lane in activated + for profile in lane["verificationProfiles"] + ) + blocking_reasons: list[str] = [] + if excluded_by_pattern: + blocking_reasons.append( + "The review scope contains excluded sensitive or generated paths that Codex cannot " + "safely omit." + ) + + return { + "schemaVersion": "1.0", + "scope": scope, + "hasChanges": bool(changed_paths), + "safeToReview": not excluded_by_pattern, + "semanticInspectionComplete": semantic_inspection_complete, + "changedPaths": reviewed_paths, + "untrackedReviewedPaths": [ + path for path in untracked_paths if matching_pattern(path, exclusions) is None + ], + "excludedChanges": [ + {"pattern": pattern, "count": count} + for pattern, count in sorted(excluded_by_pattern.items()) + ], + "blockingReasons": blocking_reasons, + "activatedLanes": activated, + "verificationProfiles": verification_profiles, + } + + +def main() -> int: + arguments = parse_arguments() + try: + result = resolve(arguments) + except (OSError, ScopeError) as error: + print(f"resolve_review_scope.py: {error}", file=sys.stderr) + return 2 + + json.dump( + result, + sys.stdout, + ensure_ascii=False, + indent=2 if arguments.pretty else None, + separators=None if arguments.pretty else (",", ":"), + ) + sys.stdout.write("\n") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.agents/skills/codex-review-loop/scripts/review_git_process.py b/.agents/skills/codex-review-loop/scripts/review_git_process.py new file mode 100644 index 0000000..dc1e7b1 --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/review_git_process.py @@ -0,0 +1,236 @@ +"""Bounded Git command execution for review scope resolution.""" + +from __future__ import annotations + +import os +import re +import shutil +import subprocess +import tempfile +from pathlib import Path + + +FILTER_KEY_RE = re.compile( + r"^filter\.(.+)\.(?:clean|smudge|process|required)$", + re.IGNORECASE, +) +DIFF_KEY_RE = re.compile( + r"^diff\.(.+)\.(command|textconv)$", + re.IGNORECASE, +) +SAFE_CAT = shutil.which("cat", path=os.defpath) or "/bin/cat" +TRUSTED_GIT = shutil.which("git", path=os.defpath) or "/usr/bin/git" +FILTER_OVERRIDES = ( + ("clean", SAFE_CAT), + ("smudge", SAFE_CAT), + ("process", ""), + ("required", "false"), +) +DIFF_OVERRIDES = ( + ("textconv", SAFE_CAT), +) +STATIC_OVERRIDES = ( + ("core.fsmonitor", "false"), + ("core.hooksPath", os.devnull), +) +UNSAFE_GIT_ENVIRONMENT_KEYS = ( + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_COMMON_DIR", + "GIT_CONFIG_PARAMETERS", + "GIT_DIR", + "GIT_EXEC_PATH", + "GIT_INDEX_FILE", + "GIT_NAMESPACE", + "GIT_OBJECT_DIRECTORY", + "GIT_WORK_TREE", +) + + +class ScopeError(RuntimeError): + """Raised when a review scope cannot be resolved safely.""" + + +def display_git_path(path: str) -> str: + return ascii(path) + + +def decode_git_path(encoded_path: bytes) -> str: + try: + return encoded_path.decode("utf-8", errors="strict") + except UnicodeDecodeError as error: + raise ScopeError("Git path is not valid UTF-8") from error + + +def decode_z(output: bytes) -> list[str]: + return [ + decode_git_path(item) + for item in output.split(b"\0") + if item + ] + + +def git_environment_without_filters(repo: Path) -> dict[str, str]: + environment = dict(os.environ) + has_external_diff = "GIT_EXTERNAL_DIFF" in environment + environment.pop("GIT_EXTERNAL_DIFF", None) + for key in UNSAFE_GIT_ENVIRONMENT_KEYS: + environment.pop(key, None) + result = subprocess.run( + [ + TRUSTED_GIT, + "config", + "--null", + "--name-only", + "--get-regexp", + ( + r"^(filter\..*\.(clean|smudge|process|required)" + r"|diff\..*\.(command|textconv)|diff\.external)$" + ), + ], + cwd=repo, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=environment, + check=False, + ) + if result.returncode not in {0, 1}: + detail = result.stderr.decode("utf-8", errors="replace").strip() + raise ScopeError(f"git config failed: {detail or 'unknown git error'}") + filter_drivers: list[str] = [] + diff_drivers: list[str] = [] + for encoded_key in result.stdout.split(b"\0"): + if not encoded_key: + continue + key = decode_git_path(encoded_key) + if key.casefold() == "diff.external": + has_external_diff = True + continue + filter_match = FILTER_KEY_RE.fullmatch(key) + if filter_match is not None and filter_match.group(1) not in filter_drivers: + filter_drivers.append(filter_match.group(1)) + diff_match = DIFF_KEY_RE.fullmatch(key) + if diff_match is not None: + if diff_match.group(2).casefold() == "command": + has_external_diff = True + elif diff_match.group(1) not in diff_drivers: + diff_drivers.append(diff_match.group(1)) + if has_external_diff: + raise ScopeError("external diff configuration cannot be reviewed safely") + try: + count = int(environment.get("GIT_CONFIG_COUNT", "0")) + except ValueError as error: + raise ScopeError("GIT_CONFIG_COUNT must be an integer") from error + + def add_override(key: str, value: str) -> None: + nonlocal count + environment[f"GIT_CONFIG_KEY_{count}"] = key + environment[f"GIT_CONFIG_VALUE_{count}"] = value + count += 1 + + for key, value in STATIC_OVERRIDES: + add_override(key, value) + for driver in filter_drivers: + for suffix, value in FILTER_OVERRIDES: + add_override(f"filter.{driver}.{suffix}", value) + for driver in diff_drivers: + for suffix, value in DIFF_OVERRIDES: + add_override(f"diff.{driver}.{suffix}", value) + + environment["GIT_CONFIG_COUNT"] = str(count) + environment["GIT_OPTIONAL_LOCKS"] = "0" + environment["GIT_PAGER"] = "cat" + environment["PAGER"] = "cat" + environment["GIT_TERMINAL_PROMPT"] = "0" + safe_path_entries = [str(Path(TRUSTED_GIT).parent)] + candidate_path = environment.pop( + "TOKI_REVIEW_ORIGINAL_PATH", + environment.get("PATH", ""), + ) + for entry in candidate_path.split(os.pathsep): + if not entry or not Path(entry).is_absolute(): + continue + try: + resolved = Path(entry).resolve() + resolved.relative_to(repo.resolve()) + except ValueError: + if str(resolved) not in safe_path_entries: + safe_path_entries.append(str(resolved)) + except OSError: + continue + environment["PATH"] = os.pathsep.join(safe_path_entries) + environment["GIT_NO_LAZY_FETCH"] = "1" + return environment + + +def run_git(repo: Path, arguments: list[str]) -> bytes: + result = subprocess.run( + [TRUSTED_GIT, *arguments], + cwd=repo, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=git_environment_without_filters(repo), + check=False, + ) + if result.returncode != 0: + detail = result.stderr.decode("utf-8", errors="replace").strip() + command = arguments[0] if arguments else "command" + raise ScopeError(f"git {command} failed: {detail or 'unknown git error'}") + return result.stdout + + +def git_root(repo: Path) -> Path: + output = run_git(repo, ["rev-parse", "--show-toplevel"]) + root = output.decode("utf-8", errors="strict").removesuffix("\n").removesuffix("\r") + return Path(root).resolve() + + +def changed_paths_without_symlinks(repo: Path, arguments: list[str]) -> list[str]: + records = [record for record in run_git(repo, arguments).split(b"\0") if record] + if len(records) % 2 != 0: + raise ScopeError("Git raw diff has an invalid record count") + paths: list[str] = [] + for metadata, encoded_path in zip(records[::2], records[1::2]): + fields = metadata.split() + if len(fields) != 5 or not fields[0].startswith(b":"): + raise ScopeError("Git raw diff has invalid metadata") + path = decode_git_path(encoded_path) + modes = {fields[0].removeprefix(b":"), fields[1]} + if b"120000" in modes: + raise ScopeError( + f"changed symbolic link cannot be reviewed safely: {display_git_path(path)}" + ) + if b"160000" in modes: + raise ScopeError( + f"changed submodule cannot be reviewed safely: {display_git_path(path)}" + ) + paths.append(path) + return paths + + +def run_git_bounded( + repo: Path, + arguments: list[str], + max_bytes: int, +) -> tuple[bytes, bool]: + with tempfile.TemporaryFile() as stderr: + process = subprocess.Popen( + [TRUSTED_GIT, *arguments], + cwd=repo, + stdout=subprocess.PIPE, + stderr=stderr, + env=git_environment_without_filters(repo), + ) + if process.stdout is None: + raise ScopeError("git command did not expose stdout") + output = process.stdout.read(max_bytes + 1) + complete = len(output) <= max_bytes + if not complete and process.poll() is None: + process.terminate() + return_code = process.wait() + process.stdout.close() + if complete and return_code != 0: + stderr.seek(0) + detail = stderr.read().decode("utf-8", errors="replace").strip() + command = arguments[0] if arguments else "command" + raise ScopeError(f"git {command} failed: {detail or 'unknown git error'}") + return output[:max_bytes], complete diff --git a/.agents/skills/codex-review-loop/scripts/review_path_matching.py b/.agents/skills/codex-review-loop/scripts/review_path_matching.py new file mode 100644 index 0000000..fcd3619 --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/review_path_matching.py @@ -0,0 +1,40 @@ +"""Repository path normalization and exclusion matching.""" + +from __future__ import annotations + +import fnmatch +from collections.abc import Iterable + + +def normalize_path(path: str) -> str: + normalized = path + while normalized.startswith("./"): + normalized = normalized[2:] + return normalized.rstrip("/") + + +def ordered_unique(values: Iterable[str]) -> list[str]: + seen: set[str] = set() + result: list[str] = [] + for value in values: + normalized = normalize_path(value) + if normalized and normalized not in seen: + seen.add(normalized) + result.append(normalized) + return result + + +def matches_pattern(path: str, pattern: str) -> bool: + return fnmatch.fnmatchcase(path, pattern) + + +def matching_pattern(path: str, patterns: list[str]) -> str | None: + folded_path = path.casefold() + return next( + ( + pattern + for pattern in patterns + if fnmatch.fnmatchcase(folded_path, pattern.casefold()) + ), + None, + ) diff --git a/.agents/skills/codex-review-loop/scripts/review_scope_git.py b/.agents/skills/codex-review-loop/scripts/review_scope_git.py new file mode 100644 index 0000000..a564662 --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/review_scope_git.py @@ -0,0 +1,266 @@ +"""Git scope resolution for the Toki Codex review loop.""" + +from __future__ import annotations + +import os +import re +from pathlib import Path + +from review_git_process import ( + ScopeError, + changed_paths_without_symlinks, + decode_z, + display_git_path, + git_root, + run_git, + run_git_bounded, +) +from review_path_matching import ( + matches_pattern, + matching_pattern, + ordered_unique, +) +from review_workspace import ( + excluded_workspace_paths, + workspace_paths_without_symlinks, +) + + +COMMIT_RE = re.compile(r"^[0-9a-fA-F]{4,64}$") +MAX_SEMANTIC_FILE_BYTES = 1_048_576 +MAX_SEMANTIC_TOTAL_BYTES = 4 * MAX_SEMANTIC_FILE_BYTES +SEMANTIC_DIFF_FLAGS = [ + "--find-renames", + "--no-ext-diff", + "--no-textconv", + "--unified=0", +] +BINARY_DIFF_MARKERS = (b"Binary files ", b"GIT binary patch") + + +def semantic_output_complete(diff: bytes, bounded_complete: bool) -> bool: + return bounded_complete and not any(marker in diff for marker in BINARY_DIFF_MARKERS) + + +def validate_base(repo: Path, base: str) -> None: + if not base or base.startswith("-") or "\0" in base: + raise ScopeError("base branch is invalid") + try: + run_git(repo, ["check-ref-format", "--branch", base]) + except ScopeError as error: + raise ScopeError( + f"base branch is not a valid branch name: {display_git_path(base)}" + ) from error + run_git(repo, ["rev-parse", "--verify", f"{base}^{{commit}}"]) + + +def validate_commit(repo: Path, commit: str) -> None: + if COMMIT_RE.fullmatch(commit) is None: + raise ScopeError("commit must be a hexadecimal SHA") + run_git(repo, ["rev-parse", "--verify", f"{commit}^{{commit}}"]) + + +def untracked_semantic_diff( + repo: Path, + paths: list[str], + exclusions: list[str], + max_total_bytes: int = MAX_SEMANTIC_TOTAL_BYTES, +) -> tuple[bytes, bool]: + changed_content = bytearray() + inspected_bytes = 0 + repo_root = repo.resolve() + for path in paths: + if matching_pattern(path, exclusions) is not None: + continue + candidate = repo / path + if candidate.is_symlink(): + continue + candidate = candidate.resolve() + try: + candidate.relative_to(repo_root) + except ValueError: + return bytes(changed_content), False + if not candidate.is_file(): + continue + remaining_bytes = max_total_bytes - inspected_bytes + with candidate.open("rb") as handle: + file_size = os.fstat(handle.fileno()).st_size + if ( + file_size > MAX_SEMANTIC_FILE_BYTES + or file_size > remaining_bytes + ): + return bytes(changed_content), False + content = handle.read(min(MAX_SEMANTIC_FILE_BYTES, remaining_bytes) + 1) + if ( + len(content) > MAX_SEMANTIC_FILE_BYTES + or len(content) > remaining_bytes + ): + return bytes(changed_content), False + inspected_bytes += len(content) + if b"\0" in content: + continue + for line in content.splitlines(): + required_bytes = len(line) + 1 + bool(changed_content) + if len(changed_content) + required_bytes > max_total_bytes: + return bytes(changed_content), False + if changed_content: + changed_content.extend(b"\n") + changed_content.extend(b"+") + changed_content.extend(line) + return bytes(changed_content), True + + +def uncommitted_scope( + repo: Path, + exclusions: list[str], +) -> tuple[list[str], list[str], bytes, bool]: + unstaged = changed_paths_without_symlinks( + repo, + ["diff", "--raw", "--no-renames", "-z", "--"], + ) + staged = changed_paths_without_symlinks( + repo, + ["diff", "--cached", "--raw", "--no-renames", "-z", "--"], + ) + untracked_roots = decode_z( + run_git(repo, ["ls-files", "--others", "--exclude-standard", "--directory", "-z"]) + ) + tracked_paths = ordered_unique([*unstaged, *staged]) + collapsed_untracked = ordered_unique(untracked_roots) + has_excluded_candidate = any( + matching_pattern(path, exclusions) is not None + for path in [*tracked_paths, *collapsed_untracked] + ) + if has_excluded_candidate: + untracked = collapsed_untracked + else: + untracked = ordered_unique( + decode_z(run_git(repo, ["ls-files", "--others", "--exclude-standard", "-z"])) + ) + workspace_excluded = [ + path + for path in excluded_workspace_paths(repo, exclusions) + if not any( + matching_pattern(root, exclusions) is not None + and path != root + and path.startswith(f"{root.rstrip('/')}/") + for root in untracked + ) + ] + paths = ordered_unique([*unstaged, *staged, *untracked, *workspace_excluded]) + semantic_content = bytearray() + semantic_inspection_complete = True + diff_arguments = [ + ["diff", *SEMANTIC_DIFF_FLAGS, "--"], + ["diff", "--cached", *SEMANTIC_DIFF_FLAGS, "--"], + ] + for arguments in diff_arguments: + separator_bytes = 1 if semantic_content else 0 + remaining_bytes = MAX_SEMANTIC_TOTAL_BYTES - len(semantic_content) + part, complete = run_git_bounded( + repo, + arguments, + max(remaining_bytes - separator_bytes, 0), + ) + if part: + if semantic_content: + semantic_content.extend(b"\n") + semantic_content.extend(part) + if not semantic_output_complete(part, complete): + semantic_inspection_complete = False + break + if semantic_inspection_complete: + separator_bytes = 1 if semantic_content else 0 + remaining_bytes = MAX_SEMANTIC_TOTAL_BYTES - len(semantic_content) + untracked_diff, semantic_inspection_complete = untracked_semantic_diff( + repo, + untracked, + exclusions, + max(remaining_bytes - separator_bytes, 0), + ) + if untracked_diff: + if semantic_content: + semantic_content.extend(b"\n") + semantic_content.extend(untracked_diff) + return ( + paths, + ordered_unique(untracked), + bytes(semantic_content), + semantic_inspection_complete, + ) + + +def base_scope( + repo: Path, + base: str, + exclusions: list[str], +) -> tuple[list[str], bytes, bool]: + validate_base(repo, base) + range_spec = f"{base}...HEAD" + paths = changed_paths_without_symlinks( + repo, + ["diff", "--raw", "--no-renames", "-z", range_spec, "--"], + ) + diff, complete = run_git_bounded( + repo, + ["diff", *SEMANTIC_DIFF_FLAGS, range_spec, "--"], + MAX_SEMANTIC_TOTAL_BYTES, + ) + return ( + ordered_unique([*paths, *excluded_workspace_paths(repo, exclusions)]), + diff, + semantic_output_complete(diff, complete), + ) + + +def commit_scope( + repo: Path, + commit: str, + exclusions: list[str], +) -> tuple[list[str], bytes, bool]: + validate_commit(repo, commit) + merge_mode = "--diff-merges=first-parent" + paths = changed_paths_without_symlinks( + repo, + [ + "diff-tree", + "--root", + merge_mode, + "--raw", + "--no-renames", + "--no-commit-id", + "-r", + "-z", + commit, + "--", + ], + ) + diff, complete = run_git_bounded( + repo, + [ + "show", + merge_mode, + *SEMANTIC_DIFF_FLAGS, + "--format=", + commit, + "--", + ], + MAX_SEMANTIC_TOTAL_BYTES, + ) + return ( + ordered_unique([*paths, *excluded_workspace_paths(repo, exclusions)]), + diff, + semantic_output_complete(diff, complete), + ) + + +def semantic_text(diff: bytes) -> str: + decoded = diff.decode("utf-8", errors="replace") + changed_lines = [ + line[1:] + for line in decoded.splitlines() + if (line.startswith("+") or line.startswith("-")) + and not line.startswith("+++") + and not line.startswith("---") + ] + return "\n".join(changed_lines) diff --git a/.agents/skills/codex-review-loop/scripts/review_workspace.py b/.agents/skills/codex-review-loop/scripts/review_workspace.py new file mode 100644 index 0000000..90cc392 --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/review_workspace.py @@ -0,0 +1,143 @@ +"""Bounded workspace inventory for safe native review.""" + +from __future__ import annotations + +import os +from pathlib import Path + +from review_git_process import ( + ScopeError, + decode_git_path, + display_git_path, + run_git_bounded, +) +from review_path_matching import matching_pattern + + +MAX_PATH_INVENTORY_BYTES = 8 * 1_048_576 + + +def bounded_path_output(repo: Path, arguments: list[str]) -> bytes: + output, complete = run_git_bounded(repo, arguments, MAX_PATH_INVENTORY_BYTES) + if not complete: + raise ScopeError("Git path inventory exceeds the safe byte limit") + return output + + +def safe_symlink_target(repo: Path, path: str) -> str | None: + candidate = repo / path + if not candidate.is_symlink(): + return None + try: + target = candidate.resolve(strict=True).relative_to(repo.resolve()) + except (OSError, RuntimeError, ValueError) as error: + raise ScopeError( + "workspace symbolic link cannot be reviewed safely: " + f"{display_git_path(path)}" + ) from error + if not target.parts or target.parts[0] == ".git": + raise ScopeError( + "workspace symbolic link cannot be reviewed safely: " + f"{display_git_path(path)}" + ) + return target.as_posix() + + +def expand_workspace_paths(repo: Path, paths: list[str]) -> list[str]: + pending = list(paths) + expanded: list[str] = [] + seen: set[str] = set() + inventory_bytes = 0 + while pending: + path = pending.pop() + if path in seen: + continue + seen.add(path) + if path: + try: + inventory_bytes += len(path.encode("utf-8", errors="strict")) + 1 + except UnicodeEncodeError as error: + raise ScopeError("Git path is not valid UTF-8") from error + if inventory_bytes > MAX_PATH_INVENTORY_BYTES: + raise ScopeError("workspace path inventory exceeds the safe byte limit") + candidate = repo / path + is_symlink = candidate.is_symlink() + is_directory = candidate.is_dir() + if path and (is_symlink or not is_directory): + expanded.append(path) + if is_symlink or not is_directory: + continue + try: + entries = sorted(candidate.iterdir(), key=lambda entry: os.fsencode(entry.name)) + except OSError as error: + raise ScopeError( + f"cannot inspect workspace directory: {display_git_path(path)}" + ) from error + if path and any(entry.name == ".git" for entry in entries): + raise ScopeError( + "embedded Git repository cannot be reviewed safely: " + f"{display_git_path(path)}" + ) + pending.extend( + entry.relative_to(repo).as_posix() + for entry in entries + if entry.name != ".git" + ) + return expanded + + +def workspace_paths_without_symlinks(repo: Path) -> list[str]: + tracked_output = bounded_path_output(repo, ["ls-files", "--stage", "-z"]) + tracked_paths: list[str] = [] + symlink_aliases: list[tuple[str, str]] = [] + for record in tracked_output.split(b"\0"): + if not record: + continue + metadata, separator, encoded_path = record.partition(b"\t") + fields = metadata.split() + if not separator or len(fields) != 3: + raise ScopeError("Git tracked path inventory has invalid metadata") + path = decode_git_path(encoded_path) + if fields[0] == b"160000": + raise ScopeError( + f"workspace submodule cannot be reviewed safely: {display_git_path(path)}" + ) + if fields[0] == b"120000": + target = safe_symlink_target(repo, path) + if target is None: + raise ScopeError( + "workspace symbolic link cannot be reviewed safely: " + f"{display_git_path(path)}" + ) + symlink_aliases.append((path, target)) + tracked_paths.append(path) + + filesystem_paths = expand_workspace_paths(repo, [""]) + for path in filesystem_paths: + target = safe_symlink_target(repo, path) + if target is not None: + symlink_aliases.append((path, target)) + workspace_paths = list( + dict.fromkeys( + [ + *tracked_paths, + *filesystem_paths, + *(target for _, target in symlink_aliases), + ] + ) + ) + alias_paths = [ + f"{alias}{path.removeprefix(target)}" + for path in workspace_paths + for alias, target in symlink_aliases + if path == target or path.startswith(f"{target}/") + ] + return list(dict.fromkeys([*workspace_paths, *alias_paths])) + + +def excluded_workspace_paths(repo: Path, exclusions: list[str]) -> list[str]: + return [ + path + for path in workspace_paths_without_symlinks(repo) + if matching_pattern(path, exclusions) is not None + ] diff --git a/.agents/skills/codex-review-loop/scripts/run_review_lane.sh b/.agents/skills/codex-review-loop/scripts/run_review_lane.sh new file mode 100644 index 0000000..453cdb1 --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/run_review_lane.sh @@ -0,0 +1,243 @@ +#!/usr/bin/env bash +set -euo pipefail + +original_path="${PATH:-}" +PATH=/usr/bin:/bin +export PATH +export TOKI_REVIEW_ORIGINAL_PATH="$original_path" + +unset GIT_ALTERNATE_OBJECT_DIRECTORIES +unset GIT_COMMON_DIR +unset GIT_CONFIG_PARAMETERS +unset GIT_DIR +unset GIT_EXEC_PATH +unset GIT_INDEX_FILE +unset GIT_NAMESPACE +unset GIT_OBJECT_DIRECTORY +unset GIT_WORK_TREE + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +skill_dir="$(cd "$script_dir/.." && pwd)" +repo="." +lane="" +scope_kind="" +scope_value="" + +usage() { + printf '%s\n' \ + "Usage: run_review_lane.sh --lane ID [--repo PATH] (--uncommitted | --base BRANCH | --commit SHA)" +} + +set_scope() { + local next_kind="$1" + local next_value="$2" + if [[ -n "$scope_kind" ]]; then + printf 'run_review_lane.sh: choose exactly one review scope\n' >&2 + exit 2 + fi + scope_kind="$next_kind" + scope_value="$next_value" +} + +while [[ $# -gt 0 ]]; do + case "$1" in + --repo) + [[ $# -ge 2 ]] || { usage >&2; exit 2; } + repo="$2" + shift 2 + ;; + --lane) + [[ $# -ge 2 ]] || { usage >&2; exit 2; } + lane="$2" + shift 2 + ;; + --uncommitted) + set_scope "uncommitted" "" + shift + ;; + --base) + [[ $# -ge 2 ]] || { usage >&2; exit 2; } + set_scope "base" "$2" + shift 2 + ;; + --commit) + [[ $# -ge 2 ]] || { usage >&2; exit 2; } + set_scope "commit" "$2" + shift 2 + ;; + -h|--help) + usage + exit 0 + ;; + *) + printf 'run_review_lane.sh: unknown argument: %s\n' "$1" >&2 + usage >&2 + exit 2 + ;; + esac +done + +if [[ -z "$lane" || -z "$scope_kind" ]]; then + usage >&2 + exit 2 +fi +if [[ ! "$lane" =~ ^[a-z0-9]+(-[a-z0-9]+)*$ ]]; then + printf 'run_review_lane.sh: invalid lane id\n' >&2 + exit 2 +fi + +if ! IFS= read -r -d '' repo_root < <( + python3 "$script_dir/resolve_repo_root.py" "$repo" +); then + exit 2 +fi +temp_dir="$(mktemp -d)" +cleanup() { + rm -rf -- "$temp_dir" +} +trap cleanup EXIT + +scope_file="$temp_dir/scope.json" +prompt_file="$temp_dir/prompt.md" +result_file="$temp_dir/result.json" +resolver_args=(--repo "$repo_root") +codex_scope_args=() + +case "$scope_kind" in + uncommitted) + resolver_args+=(--uncommitted) + codex_scope_args+=(--uncommitted) + ;; + base) + resolver_args+=(--base "$scope_value") + codex_scope_args+=(--base "$scope_value") + ;; + commit) + resolver_args+=(--commit "$scope_value") + codex_scope_args+=(--commit "$scope_value") + ;; +esac + +python3 "$script_dir/resolve_review_scope.py" "${resolver_args[@]}" > "$scope_file" + +lane_prompt="$( + python3 - "$scope_file" "$lane" <<'PY' +import json +import sys + +scope_path, requested_lane = sys.argv[1:] +with open(scope_path, encoding="utf-8") as handle: + scope = json.load(handle) +if not scope.get("hasChanges"): + raise SystemExit("run_review_lane.sh: the selected scope has no changes") +if not scope.get("safeToReview"): + reasons = scope.get("blockingReasons") or ["the scope is unsafe"] + raise SystemExit("run_review_lane.sh: " + " ".join(reasons)) +for lane in scope.get("activatedLanes", []): + if lane.get("id") == requested_lane: + print(lane["prompt"]) + break +else: + raise SystemExit(f"run_review_lane.sh: lane is not active for this scope: {requested_lane}") +PY +)" + +common_prompt="$skill_dir/references/prompts/reviewer.md" +selected_prompt="$skill_dir/$lane_prompt" +schema_file="$skill_dir/references/schemas/review-findings.schema.json" +if [[ ! -f "$common_prompt" || ! -f "$selected_prompt" || ! -f "$schema_file" ]]; then + printf 'run_review_lane.sh: a required prompt or schema file is missing\n' >&2 + exit 2 +fi + +{ + printf '# Selected review lane\n\n' + printf 'Set the output lane field to: %s\n\n' "$lane" + python3 - "$scope_file" <<'PY' +import json +import sys + +with open(sys.argv[1], encoding="utf-8") as handle: + resolved = json.load(handle) +scope = { + "kind": resolved["scope"]["kind"], + "comparisonMode": { + "uncommitted": "working-tree", + "base": "merge-base", + "commit": "first-parent", + }[resolved["scope"]["kind"]], +} +print("") +print(json.dumps(scope, ensure_ascii=False, separators=(",", ":"))) +print("") +print() +PY + sed -n '1,$p' "$common_prompt" + printf '\n\n' + sed -n '1,$p' "$selected_prompt" +} > "$prompt_file" + +codex_candidate="${TOKI_REVIEW_CODEX_BIN:-codex}" +codex_path="$(PATH="$original_path" command -v "$codex_candidate" || true)" +if [[ -z "$codex_path" ]]; then + printf 'run_review_lane.sh: review executable is unavailable\n' >&2 + exit 2 +fi +codex_bin="$( + python3 - "$repo_root" "$codex_path" <<'PY' +from pathlib import Path +import sys + +repo = Path(sys.argv[1]).resolve() +candidate = Path(sys.argv[2]).resolve(strict=True) +try: + candidate.relative_to(repo) +except ValueError: + print(candidate) +else: + print("run_review_lane.sh: review executable must be outside the repository", file=sys.stderr) + raise SystemExit(2) +PY +)" +developer_config="$( + python3 - "$prompt_file" <<'PY' +import json +import sys + +with open(sys.argv[1], encoding="utf-8") as handle: + prompt = handle.read() +print(f"developer_instructions={json.dumps(prompt, ensure_ascii=False)}") +PY +)" +codex_args=( + exec + --sandbox read-only + --ephemeral + --output-schema "$schema_file" + --output-last-message "$result_file" + review + "${codex_scope_args[@]}" + -c "$developer_config" +) + +python3 "$script_dir/run_with_safe_git.py" \ + "$repo_root" \ + -- \ + "$codex_bin" \ + "${codex_args[@]}" > /dev/null + +if [[ ! -s "$result_file" ]]; then + printf 'run_review_lane.sh: Codex did not write a structured result\n' >&2 + exit 2 +fi + +python3 "$script_dir/merge_findings.py" validate --lane "$lane" "$result_file" >/dev/null +python3 - "$result_file" <<'PY' +import json +import sys + +with open(sys.argv[1], encoding="utf-8") as handle: + result = json.load(handle) +json.dump(result, sys.stdout, ensure_ascii=True, separators=(",", ":")) +sys.stdout.write("\n") +PY diff --git a/.agents/skills/codex-review-loop/scripts/run_with_safe_git.py b/.agents/skills/codex-review-loop/scripts/run_with_safe_git.py new file mode 100755 index 0000000..776cb10 --- /dev/null +++ b/.agents/skills/codex-review-loop/scripts/run_with_safe_git.py @@ -0,0 +1,37 @@ +#!/usr/bin/env python3 +"""Run a review child with repository-selected Git filters disabled.""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +from review_git_process import ScopeError, git_environment_without_filters + + +def main() -> int: + if len(sys.argv) < 4 or sys.argv[2] != "--": + print( + "run_with_safe_git.py: usage: REPO -- COMMAND [ARG ...]", + file=sys.stderr, + ) + return 2 + repo = Path(sys.argv[1]).resolve() + environment = git_environment_without_filters(repo) + environment["TOKI_REVIEW_CHILD"] = "1" + result = subprocess.run( + sys.argv[3:], + cwd=repo, + env=environment, + check=False, + ) + return result.returncode + + +if __name__ == "__main__": + try: + raise SystemExit(main()) + except (OSError, ScopeError) as error: + print(f"run_with_safe_git.py: {error}", file=sys.stderr) + raise SystemExit(2) from error diff --git a/.agents/skills/codex-review-loop/tests/test_activation_hook.py b/.agents/skills/codex-review-loop/tests/test_activation_hook.py new file mode 100644 index 0000000..f612a3e --- /dev/null +++ b/.agents/skills/codex-review-loop/tests/test_activation_hook.py @@ -0,0 +1,87 @@ +from __future__ import annotations + +import json +import os +import subprocess +import unittest +from pathlib import Path + + +AGENTS_DIR = Path(__file__).resolve().parents[3] +ACTIVATION_HOOK = AGENTS_DIR / "hooks" / "skill_activation.ts" + + +class ActivationHookTests(unittest.TestCase): + def run_hook( + self, + *, + review_child: bool, + prompt: str = "Run a Codex code review for the current diff.", + ) -> str: + environment = os.environ.copy() + if review_child: + environment["TOKI_REVIEW_CHILD"] = "1" + else: + environment.pop("TOKI_REVIEW_CHILD", None) + completed = subprocess.run( + [ + "node", + "--no-warnings", + "--experimental-strip-types", + str(ACTIVATION_HOOK), + ], + input=json.dumps( + { + "prompt": prompt, + "session_id": "activation-test", + } + ), + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=environment, + check=True, + ) + if not completed.stdout: + return "" + output = json.loads(completed.stdout) + return output["hookSpecificOutput"]["additionalContext"] + + def test_normal_request_activates_review_loop(self) -> None: + context = self.run_hook(review_child=False) + + self.assertIn("Apply `codex-review-loop`", context) + + def test_leaf_reviewer_does_not_reactivate_review_loop(self) -> None: + context = self.run_hook(review_child=True) + + self.assertNotIn("Apply `codex-review-loop`", context) + self.assertIn("project-conventions", context) + + def test_bare_commit_sha_activates_review_loop(self) -> None: + context = self.run_hook( + review_child=False, + prompt="Please review b32dfa83", + ) + + self.assertIn("Apply `codex-review-loop`", context) + + def test_commit_sha_before_korean_review_activates_review_loop(self) -> None: + context = self.run_hook( + review_child=False, + prompt="b32dfa83 리뷰해줘", + ) + + self.assertIn("Apply `codex-review-loop`", context) + + def test_plain_review_request_does_not_activate_review_loop(self) -> None: + context = self.run_hook( + review_child=False, + prompt="Please review this", + ) + + self.assertNotIn("codex-review-loop", context) + + +if __name__ == "__main__": + unittest.main() diff --git a/.agents/skills/codex-review-loop/tests/test_finding_merge.py b/.agents/skills/codex-review-loop/tests/test_finding_merge.py new file mode 100644 index 0000000..46e35d2 --- /dev/null +++ b/.agents/skills/codex-review-loop/tests/test_finding_merge.py @@ -0,0 +1,508 @@ +from __future__ import annotations + +import json +import subprocess +import tempfile +import unittest +from itertools import permutations +from pathlib import Path + + +SKILL_DIR = Path(__file__).resolve().parents[1] +MERGER = SKILL_DIR / "scripts" / "merge_findings.py" + + +def finding( + *, + priority: str = "P2", + title: str = "Reject stale snapshot", + root_cause: str = "Snapshot generation is not compared", + file: str = "Sources/TokiSyncProtocol/SnapshotValidation.swift", + start: int = 20, + end: int = 23, +) -> dict: + return { + "priority": priority, + "confidence": 0.9, + "title": title, + "file": file, + "startLine": start, + "endLine": end, + "rootCause": root_cause, + "evidence": "An older generation can reach the acceptance branch.", + "impact": "A stale snapshot can replace current state.", + "suggestedFix": "Compare generations before replacement.", + "verification": ["swift test"], + } + + +def lane_result(lane: str, findings: list[dict]) -> dict: + return { + "schemaVersion": "1.0", + "lane": lane, + "verdict": "findings" if findings else "clean", + "summary": "Actionable findings." if findings else "No actionable findings.", + "findings": findings, + } + + +class FindingMergeTests(unittest.TestCase): + def setUp(self) -> None: + self.temporary_directory = tempfile.TemporaryDirectory() + self.addCleanup(self.temporary_directory.cleanup) + self.directory = Path(self.temporary_directory.name) + + def write_result(self, name: str, result: dict) -> Path: + path = self.directory / name + path.write_text(json.dumps(result), encoding="utf-8") + return path + + def run_merger(self, *arguments: str, check: bool = True) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["python3", str(MERGER), *arguments], + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=check, + ) + + def test_merges_overlapping_root_cause_and_keeps_priority_opinions(self) -> None: + baseline = self.write_result("baseline.json", lane_result("baseline", [finding()])) + remote = self.write_result( + "remote.json", + lane_result( + "remote-sync", + [ + finding( + priority="P0", + title="Stale snapshots bypass generation checks", + root_cause="Snapshot generation comparison is missing", + start=21, + end=24, + ) + ], + ), + ) + + completed = self.run_merger("merge", str(baseline), str(remote)) + merged = json.loads(completed.stdout) + + self.assertEqual(merged["verdict"], "findings") + self.assertEqual(len(merged["findings"]), 1) + result = merged["findings"][0] + self.assertEqual(result["priority"], "P0") + self.assertEqual(result["lanes"], ["baseline", "remote-sync"]) + self.assertEqual(result["occurrences"], 2) + self.assertEqual( + result["priorityOpinions"], + {"baseline": "P2", "remote-sync": "P0"}, + ) + self.assertEqual(len(merged["conflicts"]), 1) + + def test_keeps_non_overlapping_findings_separate(self) -> None: + first = self.write_result("first.json", lane_result("baseline", [finding()])) + second = self.write_result( + "second.json", + lane_result( + "testing", + [ + finding( + title="Missing corrupt-cache coverage", + root_cause="Corrupt cache fixture is not tested", + start=80, + end=82, + ) + ], + ), + ) + + completed = self.run_merger("merge", str(first), str(second)) + merged = json.loads(completed.stdout) + + self.assertEqual(len(merged["findings"]), 2) + + def test_assigns_distinct_ids_to_same_cause_at_different_locations(self) -> None: + first = self.write_result("first.json", lane_result("baseline", [finding()])) + second = self.write_result( + "second.json", + lane_result( + "testing", + [finding(start=80, end=82)], + ), + ) + + completed = self.run_merger("merge", str(first), str(second)) + merged = json.loads(completed.stdout) + + self.assertEqual(len(merged["findings"]), 2) + self.assertEqual( + len({result["id"] for result in merged["findings"]}), + 2, + ) + + def test_keeps_same_title_findings_with_distinct_causes_separate(self) -> None: + first = self.write_result( + "first.json", + lane_result( + "baseline", + [ + finding( + title="Reject unsafe fallback", + root_cause="Authorization check is omitted", + ) + ], + ), + ) + second = self.write_result( + "second.json", + lane_result( + "testing", + [ + finding( + title="Reject unsafe fallback", + root_cause="Cache eviction uses a stale timestamp", + start=21, + end=24, + ) + ], + ), + ) + + completed = self.run_merger("merge", str(first), str(second)) + merged = json.loads(completed.stdout) + + self.assertEqual(len(merged["findings"]), 2) + self.assertEqual( + {tuple(result["lanes"]) for result in merged["findings"]}, + {("baseline",), ("testing",)}, + ) + + def test_merges_identical_non_ascii_findings(self) -> None: + korean_finding = finding( + title="권한 검증 누락", + root_cause="요청 권한을 확인하지 않음", + ) + first = self.write_result( + "first.json", + lane_result("baseline", [korean_finding]), + ) + second = self.write_result( + "second.json", + lane_result("testing", [korean_finding]), + ) + + completed = self.run_merger("merge", str(first), str(second)) + merged = json.loads(completed.stdout) + + self.assertEqual(len(merged["findings"]), 1) + self.assertEqual( + merged["findings"][0]["lanes"], + ["baseline", "testing"], + ) + + def test_non_ascii_causes_contribute_to_finding_ids(self) -> None: + first = self.write_result( + "first.json", + lane_result( + "baseline", + [ + finding( + title="권한 검증 누락", + root_cause="요청 권한을 확인하지 않음", + ) + ], + ), + ) + second = self.write_result( + "second.json", + lane_result( + "testing", + [ + finding( + title="캐시 만료 오류", + root_cause="캐시 만료 시간을 잘못 계산함", + ) + ], + ), + ) + + completed = self.run_merger("merge", str(first), str(second)) + merged = json.loads(completed.stdout) + + self.assertEqual(len(merged["findings"]), 2) + self.assertEqual( + len({result["id"] for result in merged["findings"]}), + 2, + ) + + def test_rejects_parent_relative_finding_path(self) -> None: + invalid = finding(file="../secret.txt") + path = self.write_result("invalid.json", lane_result("baseline", [invalid])) + + completed = self.run_merger("validate", str(path), check=False) + + self.assertEqual(completed.returncode, 2) + self.assertIn("repository-relative", completed.stderr) + + def test_accepts_literal_backslash_in_posix_finding_path(self) -> None: + literal_path = r"Sources/TokiUsageReaders/..\outside.txt" + result = self.write_result( + "literal-backslash.json", + lane_result("baseline", [finding(file=literal_path)]), + ) + + completed = self.run_merger("merge", str(result)) + merged = json.loads(completed.stdout) + + self.assertEqual(merged["findings"][0]["file"], literal_path) + + def test_rejects_non_utf8_finding_path_during_validation(self) -> None: + invalid = finding(file="bad-\udcff.swift") + path = self.write_result("invalid-utf8.json", lane_result("baseline", [invalid])) + + completed = self.run_merger("validate", str(path), check=False) + + self.assertEqual(completed.returncode, 2) + self.assertIn("valid UTF-8", completed.stderr) + self.assertNotIn("Traceback", completed.stderr) + + def test_rejects_non_utf8_verification_string_during_validation(self) -> None: + invalid = finding() + invalid["verification"] = ["swift test\udcff"] + path = self.write_result("invalid-verification.json", lane_result("baseline", [invalid])) + + completed = self.run_merger("validate", str(path), check=False) + + self.assertEqual(completed.returncode, 2) + self.assertIn("valid UTF-8", completed.stderr) + self.assertNotIn("Traceback", completed.stderr) + + def test_rejects_non_utf8_finding_path_during_merge(self) -> None: + valid = self.write_result("valid.json", lane_result("baseline", [finding()])) + invalid = self.write_result( + "invalid-utf8.json", + lane_result("testing", [finding(file="bad-\udcff.swift")]), + ) + + completed = self.run_merger("merge", str(valid), str(invalid), check=False) + + self.assertEqual(completed.returncode, 2) + self.assertIn("valid UTF-8", completed.stderr) + self.assertNotIn("Traceback", completed.stderr) + + def test_rejects_nul_in_finding_path_during_validation_and_merge(self) -> None: + invalid = self.write_result( + "invalid-nul.json", + lane_result("baseline", [finding(file="bad-\0name.swift")]), + ) + + validated = self.run_merger("validate", str(invalid), check=False) + merged = self.run_merger("merge", str(invalid), check=False) + + for completed in (validated, merged): + self.assertEqual(completed.returncode, 2) + self.assertIn("NUL", completed.stderr) + self.assertNotIn("Traceback", completed.stderr) + + def test_accepts_other_posix_control_characters_in_finding_paths(self) -> None: + controls = [ + chr(code_point) + for code_point in (*range(1, 32), *range(127, 160)) + ] + expected_paths = [ + expected + for character in controls + for expected in ( + f"{character}control.swift", + f"control-{character}-name.swift", + f"control.swift{character}", + ) + ] + result = self.write_result( + "control-paths.json", + lane_result( + "baseline", + [ + finding(file=path, start=index + 1, end=index + 1) + for index, path in enumerate(expected_paths) + ], + ), + ) + + completed = self.run_merger("merge", str(result)) + merged = json.loads(completed.stdout) + + self.assertEqual( + {item["file"] for item in merged["findings"]}, + set(expected_paths), + ) + + def test_rejects_noncanonical_posix_path_segments(self) -> None: + for index, invalid_path in enumerate( + ("/absolute.swift", "Sources//File.swift", "Sources/./File.swift", "Sources/../File.swift") + ): + with self.subTest(path=invalid_path): + result = self.write_result( + f"noncanonical-{index}.json", + lane_result("baseline", [finding(file=invalid_path)]), + ) + + completed = self.run_merger("validate", str(result), check=False) + + self.assertEqual(completed.returncode, 2) + self.assertIn("repository-relative", completed.stderr) + + def test_merges_transitive_bridge_for_every_input_order(self) -> None: + bridged_findings = [ + finding(start=1, end=2), + finding(start=5, end=6), + finding(start=2, end=5), + ] + for index, order in enumerate(permutations(bridged_findings)): + with self.subTest(order=index): + result = self.write_result( + f"bridge-{index}.json", + lane_result("baseline", list(order)), + ) + + completed = self.run_merger("merge", str(result)) + merged = json.loads(completed.stdout) + + self.assertEqual(len(merged["findings"]), 1) + self.assertEqual(merged["findings"][0]["startLine"], 1) + self.assertEqual(merged["findings"][0]["endLine"], 6) + self.assertEqual(merged["findings"][0]["occurrences"], 3) + + def test_merges_transitive_root_cause_similarity_for_every_order(self) -> None: + bridged_findings = [ + finding(root_cause="alpha beta gamma delta"), + finding(root_cause="alpha beta gamma delta epsilon zeta"), + finding(root_cause="gamma delta epsilon zeta"), + ] + for index, order in enumerate(permutations(bridged_findings)): + with self.subTest(order=index): + result = self.write_result( + f"root-bridge-{index}.json", + lane_result("baseline", list(order)), + ) + + completed = self.run_merger("merge", str(result)) + merged = json.loads(completed.stdout) + + self.assertEqual(len(merged["findings"]), 1) + self.assertEqual(merged["findings"][0]["occurrences"], 3) + + def test_transitive_group_output_is_stable_across_lane_order(self) -> None: + paths = [ + self.write_result( + "baseline-bridge.json", + lane_result("baseline", [finding(start=1, end=2)]), + ), + self.write_result( + "privacy-bridge.json", + lane_result("privacy-security", [finding(start=5, end=6)]), + ), + self.write_result( + "testing-bridge.json", + lane_result("testing", [finding(start=2, end=5)]), + ), + ] + outputs = [] + for order in permutations(paths): + completed = self.run_merger( + "merge", + *(str(path) for path in order), + ) + outputs.append(json.loads(completed.stdout)["findings"]) + + self.assertTrue(all(output == outputs[0] for output in outputs[1:])) + self.assertEqual(len(outputs[0]), 1) + self.assertEqual(outputs[0][0]["occurrences"], 3) + + def test_preserves_whitespace_around_finding_path(self) -> None: + padded = finding(file=" Sources/TokiSyncProtocol/SnapshotValidation.swift ") + path = self.write_result("padded.json", lane_result("baseline", [padded])) + + completed = self.run_merger("merge", str(path)) + merged = json.loads(completed.stdout) + + self.assertEqual( + merged["findings"][0]["file"], + " Sources/TokiSyncProtocol/SnapshotValidation.swift ", + ) + + def test_distinguishes_whitespace_and_unicode_normalization_in_paths(self) -> None: + paths = [ + "file.swift", + " file.swift", + "file.swift ", + "caf\u00e9.swift", + "cafe\u0301.swift", + ] + result = self.write_result( + "distinct-paths.json", + lane_result( + "baseline", + [ + finding(file=path, start=index + 1, end=index + 1) + for index, path in enumerate(paths) + ], + ), + ) + + completed = self.run_merger("merge", str(result)) + merged = json.loads(completed.stdout) + + self.assertEqual({item["file"] for item in merged["findings"]}, set(paths)) + + def test_rejects_raw_non_utf8_json_without_traceback(self) -> None: + path = self.directory / "raw-invalid-utf8.json" + path.write_bytes( + b'{"schemaVersion":"1.0","lane":"baseline","summary":"' + + bytes([0xFF]) + + b'"}' + ) + + for mode in ("validate", "merge"): + with self.subTest(mode=mode): + completed = self.run_merger(mode, str(path), check=False) + + self.assertEqual(completed.returncode, 2) + self.assertIn("valid UTF-8", completed.stderr) + self.assertNotIn("Traceback", completed.stderr) + + def test_rejects_lane_identifier_with_boundary_whitespace(self) -> None: + for index, lane in enumerate((" baseline", "baseline ", "\tbaseline")): + with self.subTest(lane=lane): + result = self.write_result( + f"invalid-lane-{index}.json", + lane_result(lane, [finding()]), + ) + + completed = self.run_merger("validate", str(result), check=False) + + self.assertEqual(completed.returncode, 2) + self.assertIn("valid lane id", completed.stderr) + + def test_rejects_clean_verdict_with_findings(self) -> None: + invalid = lane_result("baseline", [finding()]) + invalid["verdict"] = "clean" + path = self.write_result("invalid.json", invalid) + + completed = self.run_merger("validate", str(path), check=False) + + self.assertEqual(completed.returncode, 2) + self.assertIn("clean requires no findings", completed.stderr) + + def test_rejects_duplicate_lane_results(self) -> None: + first = self.write_result("first.json", lane_result("baseline", [])) + second = self.write_result("second.json", lane_result("baseline", [])) + + completed = self.run_merger("merge", str(first), str(second), check=False) + + self.assertEqual(completed.returncode, 2) + self.assertIn("each lane may appear only once", completed.stderr) + + +if __name__ == "__main__": + unittest.main() diff --git a/.agents/skills/codex-review-loop/tests/test_runner_contract.py b/.agents/skills/codex-review-loop/tests/test_runner_contract.py new file mode 100644 index 0000000..cc6654d --- /dev/null +++ b/.agents/skills/codex-review-loop/tests/test_runner_contract.py @@ -0,0 +1,529 @@ +from __future__ import annotations + +import json +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + + +SKILL_DIR = Path(__file__).resolve().parents[1] +RUNNER = SKILL_DIR / "scripts" / "run_review_lane.sh" + + +class RunnerContractTests(unittest.TestCase): + def setUp(self) -> None: + self.temporary_directory = tempfile.TemporaryDirectory() + self.addCleanup(self.temporary_directory.cleanup) + self.root = Path(self.temporary_directory.name).resolve() + self.repo = self.root / "repo" + self.repo.mkdir() + self.git("init", "-q", "-b", "main") + self.git("config", "user.email", "review-test@example.com") + self.git("config", "user.name", "Review Test") + + source = self.repo / "Sources" / "TokiSyncProtocol" / "SnapshotCipher.swift" + source.parent.mkdir(parents=True) + source.write_text("func seal() {}\n", encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "initial") + self.git("switch", "-q", "-c", "feature") + source.write_text("func seal(nonce: String) {}\n", encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "change cipher") + + self.capture = self.root / "capture.json" + self.fake_codex = self.root / "fake-codex" + self.fake_codex.write_text( + """#!/usr/bin/env python3 +import json +import os +import subprocess +import sys +from pathlib import Path + +arguments = sys.argv[1:] + +def git_config(key): + result = subprocess.run( + ["git", "config", "--get", key], + text=True, + stdout=subprocess.PIPE, + check=False, + ) + return result.stdout.strip() if result.returncode == 0 else None + +diff_output = subprocess.check_output(["git", "diff"]) +config_value = arguments[arguments.index("-c") + 1] +if not config_value.startswith("developer_instructions="): + raise SystemExit("missing developer instructions") +prompt = json.loads(config_value.split("=", 1)[1]) +output_index = arguments.index("--output-last-message") + 1 +output_path = Path(arguments[output_index]) +lane_marker = "Set the output lane field to: " +lane = prompt.split(lane_marker, 1)[1].splitlines()[0].strip() +result = { + "schemaVersion": "1.0", + "lane": lane, + "verdict": "clean", + "summary": "No actionable findings.", + "findings": [], +} +output_path.write_text(json.dumps(result), encoding="utf-8") +Path(os.environ["TOKI_REVIEW_CAPTURE_FILE"]).write_text( + json.dumps({ + "arguments": arguments, + "prompt": prompt, + "reviewChild": os.environ.get("TOKI_REVIEW_CHILD"), + "fsmonitor": git_config("core.fsmonitor"), + "hooksPath": git_config("core.hooksPath"), + "gitOptionalLocks": os.environ.get("GIT_OPTIONAL_LOCKS"), + "gitPager": os.environ.get("GIT_PAGER"), + "diffHasContent": bool(diff_output), + "gitNoLazyFetch": os.environ.get("GIT_NO_LAZY_FETCH"), + "cwd": str(Path.cwd()), + }), + encoding="utf-8", +) +""", + encoding="utf-8", + ) + self.fake_codex.chmod(0o755) + + def git(self, *arguments: str) -> str: + result = subprocess.run( + ["git", *arguments], + cwd=self.repo, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=True, + ) + return result.stdout + + def run_runner( + self, + *arguments: str, + check: bool = True, + environment_overrides: dict[str, str] | None = None, + use_default_codex: bool = False, + ) -> subprocess.CompletedProcess[str]: + environment = os.environ.copy() + if not use_default_codex: + environment["TOKI_REVIEW_CODEX_BIN"] = str(self.fake_codex) + environment["TOKI_REVIEW_CAPTURE_FILE"] = str(self.capture) + if environment_overrides is not None: + environment.update(environment_overrides) + return subprocess.run( + ["bash", str(RUNNER), "--repo", str(self.repo), *arguments], + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=environment, + check=check, + ) + + def test_rejects_repository_local_codex_from_path(self) -> None: + marker = self.repo / "codex-invoked" + repository_codex = self.repo / "codex" + repository_codex.write_text( + "#!/bin/sh\n" + f"printf invoked > {str(marker)!r}\n", + encoding="utf-8", + ) + repository_codex.chmod(0o755) + + completed = self.run_runner( + "--lane", + "baseline", + "--base", + "main", + check=False, + environment_overrides={ + "PATH": f"{self.repo}:{os.environ['PATH']}", + }, + use_default_codex=True, + ) + + self.assertEqual(completed.returncode, 2) + self.assertIn("review executable", completed.stderr) + self.assertFalse(marker.exists()) + + def test_review_child_cannot_resolve_repository_local_git(self) -> None: + marker = self.repo / "git-invoked" + repository_git = self.repo / "git" + repository_git.write_text( + "#!/bin/sh\n" + f"printf invoked > {str(marker)!r}\n", + encoding="utf-8", + ) + repository_git.chmod(0o755) + source = self.repo / "Sources" / "TokiSyncProtocol" / "SnapshotCipher.swift" + source.write_text("func seal(nonce: String, key: String) {}\n", encoding="utf-8") + + self.run_runner( + "--lane", + "baseline", + "--uncommitted", + environment_overrides={ + "PATH": f"{self.repo}:{os.environ['PATH']}", + }, + ) + + self.assertFalse(marker.exists()) + + def test_runner_tools_ignore_repository_local_path_entries(self) -> None: + marker = self.repo / "python-invoked" + repository_python = self.repo / "python3" + repository_python.write_text( + "#!/bin/sh\n" + f"printf invoked > {str(marker)!r}\n" + "exit 91\n", + encoding="utf-8", + ) + repository_python.chmod(0o755) + + self.run_runner( + "--lane", + "baseline", + "--base", + "main", + environment_overrides={ + "PATH": f"{self.repo}:{os.environ['PATH']}", + }, + ) + + self.assertFalse(marker.exists()) + + def test_review_child_disables_lazy_fetch(self) -> None: + self.run_runner("--lane", "baseline", "--base", "main") + capture = json.loads(self.capture.read_text(encoding="utf-8")) + + self.assertEqual(capture["gitNoLazyFetch"], "1") + + def test_runner_preserves_repository_root_trailing_newline(self) -> None: + newline_repo = self.root / "repo\n" + self.repo.rename(newline_repo) + self.repo = newline_repo + + self.run_runner("--lane", "baseline", "--base", "main") + capture = json.loads(self.capture.read_text(encoding="utf-8")) + + self.assertEqual(capture["cwd"], str(newline_repo)) + + def test_passes_native_review_scope_and_developer_instructions(self) -> None: + status_before = self.git("status", "--porcelain=v1", "--untracked-files=all") + + completed = self.run_runner("--lane", "baseline", "--base", "main") + + status_after = self.git("status", "--porcelain=v1", "--untracked-files=all") + output = json.loads(completed.stdout) + capture = json.loads(self.capture.read_text(encoding="utf-8")) + arguments = capture["arguments"] + self.assertEqual(output["lane"], "baseline") + self.assertEqual(status_before, status_after) + self.assertIn("--ephemeral", arguments) + self.assertIn("--output-schema", arguments) + self.assertIn("review", arguments) + self.assertIn("--base", arguments) + self.assertEqual(arguments[arguments.index("--base") + 1], "main") + self.assertEqual(arguments[arguments.index("--sandbox") + 1], "read-only") + self.assertNotIn("-", arguments) + self.assertEqual(capture["reviewChild"], "1") + scope_text = capture["prompt"].split("", 1)[1] + scope_payload = scope_text.split("", 1)[0] + scope = json.loads(scope_payload) + self.assertEqual(scope["kind"], "base") + self.assertNotIn("argument", scope) + self.assertEqual(scope["comparisonMode"], "merge-base") + self.assertNotIn("changedPaths", scope) + self.assertNotIn("untrackedReviewedPaths", scope) + self.assertIn("Common Reviewer Contract", capture["prompt"]) + self.assertIn("Baseline Lane", capture["prompt"]) + + def test_commit_scope_keeps_native_flag_and_first_parent_instruction(self) -> None: + commit = self.git("rev-parse", "HEAD").strip() + + self.run_runner("--lane", "baseline", "--commit", commit) + capture = json.loads(self.capture.read_text(encoding="utf-8")) + arguments = capture["arguments"] + scope_text = capture["prompt"].split("", 1)[1] + scope_payload = scope_text.split("", 1)[0] + scope = json.loads(scope_payload) + + self.assertIn("review", arguments) + self.assertEqual(arguments[arguments.index("--commit") + 1], commit) + self.assertEqual(scope["comparisonMode"], "first-parent") + + def test_ref_name_cannot_escape_review_scope_payload(self) -> None: + base = "topic/\u2003base" + self.git("branch", base, "main") + + self.run_runner("--lane", "baseline", "--base", base) + capture = json.loads(self.capture.read_text(encoding="utf-8")) + prompt = capture["prompt"] + scope_text = prompt.split("", 1)[1] + scope_payload = scope_text.split("", 1)[0] + scope = json.loads(scope_payload) + + self.assertEqual(prompt.count(""), 1) + self.assertEqual(prompt.count(""), 1) + self.assertNotIn("argument", scope) + + def test_instruction_payload_stays_bounded_for_large_scope(self) -> None: + notes = self.repo / "docs" + notes.mkdir() + for index in range(1_800): + (notes / f"generated-{index:04}.txt").write_text( + "ordinary text\n", + encoding="utf-8", + ) + + self.run_runner("--lane", "baseline", "--uncommitted") + capture = json.loads(self.capture.read_text(encoding="utf-8")) + arguments = capture["arguments"] + config_value = arguments[arguments.index("-c") + 1] + scope_text = capture["prompt"].split("", 1)[1] + scope_payload = scope_text.split("", 1)[0] + scope = json.loads(scope_payload) + + self.assertLess(len(config_value.encode()), 32_768) + self.assertNotIn("changedPaths", scope) + self.assertNotIn("untrackedReviewedPaths", scope) + + def test_refuses_excluded_uncommitted_scope_before_invoking_codex(self) -> None: + sensitive = self.repo / ".hermes" / "session.log" + sensitive.parent.mkdir() + sensitive.write_text("sensitive fixture\n", encoding="utf-8") + + completed = self.run_runner( + "--lane", + "baseline", + "--uncommitted", + check=False, + ) + + self.assertNotEqual(completed.returncode, 0) + self.assertIn("excluded sensitive or generated paths", completed.stderr) + self.assertFalse(self.capture.exists()) + + def test_refuses_environment_file_before_invoking_codex(self) -> None: + environment = self.repo / ".env" + environment.write_text("API_TOKEN=secret\n", encoding="utf-8") + + completed = self.run_runner( + "--lane", + "baseline", + "--uncommitted", + check=False, + ) + + self.assertNotEqual(completed.returncode, 0) + self.assertIn("excluded sensitive or generated paths", completed.stderr) + self.assertFalse(self.capture.exists()) + + def test_refuses_ignored_environment_file_before_invoking_codex(self) -> None: + ignore_file = self.repo / ".gitignore" + ignore_file.write_text(".env\n", encoding="utf-8") + self.git("add", ".gitignore") + self.git("commit", "-q", "-m", "ignore environment file") + environment = self.repo / ".env" + environment.write_text("API_TOKEN=secret\n", encoding="utf-8") + source = self.repo / "Sources" / "TokiSyncProtocol" / "SnapshotCipher.swift" + source.write_text("func seal(nonce: String, key: String) {}\n", encoding="utf-8") + + completed = self.run_runner( + "--lane", + "baseline", + "--uncommitted", + check=False, + ) + + self.assertNotEqual(completed.returncode, 0) + self.assertIn("excluded sensitive or generated paths", completed.stderr) + self.assertFalse(self.capture.exists()) + + def test_native_review_does_not_execute_clean_filter(self) -> None: + attributes = self.repo / ".gitattributes" + attributes.write_text("*.review filter=unsafe\n", encoding="utf-8") + reviewed = self.repo / "sample.review" + reviewed.write_text("before\n", encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "add clean filter fixture") + + clean_filter = self.root / "clean-filter.py" + clean_filter.write_text( + "#!/usr/bin/env python3\n" + "from pathlib import Path\n" + "import sys\n" + "Path(__file__).with_suffix('.invoked').write_text('invoked\\n')\n" + "sys.stdout.buffer.write(sys.stdin.buffer.read())\n", + encoding="utf-8", + ) + clean_filter.chmod(0o755) + marker = clean_filter.with_suffix(".invoked") + self.git("config", "filter.unsafe.clean", str(clean_filter)) + reviewed.write_text("after\n", encoding="utf-8") + + completed = self.run_runner("--lane", "baseline", "--uncommitted") + + self.assertEqual(completed.returncode, 0) + self.assertFalse(marker.exists()) + + def test_review_child_disables_fsmonitor_hooks_and_git_writes(self) -> None: + fsmonitor = self.root / "fsmonitor.py" + fsmonitor.write_text( + "#!/usr/bin/env python3\n" + "from pathlib import Path\n" + "Path(__file__).with_suffix('.invoked').write_text('invoked\\n')\n", + encoding="utf-8", + ) + fsmonitor.chmod(0o755) + marker = fsmonitor.with_suffix(".invoked") + self.git("config", "core.fsmonitor", str(fsmonitor)) + + self.run_runner("--lane", "baseline", "--base", "main") + capture = json.loads(self.capture.read_text(encoding="utf-8")) + + self.assertFalse(marker.exists()) + self.assertEqual(capture["fsmonitor"], "false") + self.assertEqual(capture["hooksPath"], os.devnull) + self.assertEqual(capture["gitOptionalLocks"], "0") + self.assertEqual(capture["gitPager"], "cat") + + def test_review_child_preserves_internal_diff_output(self) -> None: + source = self.repo / "Sources" / "TokiSyncProtocol" / "SnapshotCipher.swift" + source.write_text("func seal(nonce: String, key: String) {}\n", encoding="utf-8") + + self.run_runner("--lane", "baseline", "--uncommitted") + capture = json.loads(self.capture.read_text(encoding="utf-8")) + + self.assertTrue(capture["diffHasContent"]) + + def test_runner_emits_validated_lane_result_not_receipt(self) -> None: + completed = self.run_runner("--lane", "baseline", "--base", "main") + output = json.loads(completed.stdout) + + self.assertEqual( + set(output), + {"schemaVersion", "lane", "verdict", "summary", "findings"}, + ) + self.assertEqual(output["lane"], "baseline") + self.assertEqual(output["verdict"], "clean") + + def test_inherited_config_parameters_cannot_restore_fsmonitor(self) -> None: + fsmonitor = self.root / "fsmonitor.py" + fsmonitor.write_text( + "#!/usr/bin/env python3\n" + "from pathlib import Path\n" + "Path(__file__).with_suffix('.invoked').write_text('invoked\\n')\n", + encoding="utf-8", + ) + fsmonitor.chmod(0o755) + marker = fsmonitor.with_suffix(".invoked") + + self.run_runner( + "--lane", + "baseline", + "--base", + "main", + environment_overrides={ + "GIT_CONFIG_PARAMETERS": f"'core.fsmonitor={fsmonitor}'", + }, + ) + capture = json.loads(self.capture.read_text(encoding="utf-8")) + + self.assertFalse(marker.exists()) + self.assertEqual(capture["fsmonitor"], "false") + + def test_review_child_does_not_execute_external_diff(self) -> None: + external_diff = self.root / "external-diff.py" + external_diff.write_text( + "#!/usr/bin/env python3\n" + "from pathlib import Path\n" + "Path(__file__).with_suffix('.invoked').write_text('invoked\\n')\n", + encoding="utf-8", + ) + external_diff.chmod(0o755) + marker = external_diff.with_suffix(".invoked") + self.git("config", "diff.external", str(external_diff)) + source = self.repo / "Sources" / "TokiSyncProtocol" / "SnapshotCipher.swift" + source.write_text("func seal(nonce: String, key: String) {}\n", encoding="utf-8") + + completed = self.run_runner( + "--lane", + "baseline", + "--uncommitted", + check=False, + ) + + self.assertEqual(completed.returncode, 2) + self.assertIn("external diff configuration", completed.stderr) + self.assertFalse(marker.exists()) + + def test_review_child_does_not_execute_inherited_external_diff(self) -> None: + external_diff = self.root / "external-diff.py" + external_diff.write_text( + "#!/usr/bin/env python3\n" + "from pathlib import Path\n" + "Path(__file__).with_suffix('.invoked').write_text('invoked\\n')\n", + encoding="utf-8", + ) + external_diff.chmod(0o755) + marker = external_diff.with_suffix(".invoked") + source = self.repo / "Sources" / "TokiSyncProtocol" / "SnapshotCipher.swift" + source.write_text("func seal(nonce: String, key: String) {}\n", encoding="utf-8") + + completed = self.run_runner( + "--lane", + "baseline", + "--uncommitted", + check=False, + environment_overrides={"GIT_EXTERNAL_DIFF": str(external_diff)}, + ) + + self.assertEqual(completed.returncode, 2) + self.assertIn("external diff configuration", completed.stderr) + self.assertFalse(marker.exists()) + + def test_review_child_does_not_execute_textconv(self) -> None: + attributes = self.repo / ".gitattributes" + attributes.write_text("*.swift diff=unsafe\n", encoding="utf-8") + self.git("add", ".gitattributes") + self.git("commit", "-q", "-m", "add diff attributes") + textconv = self.root / "textconv.py" + textconv.write_text( + "#!/usr/bin/env python3\n" + "from pathlib import Path\n" + "import sys\n" + "Path(__file__).with_suffix('.invoked').write_text('invoked\\n')\n" + "sys.stdout.buffer.write(Path(sys.argv[1]).read_bytes())\n", + encoding="utf-8", + ) + textconv.chmod(0o755) + marker = textconv.with_suffix(".invoked") + self.git("config", "diff.unsafe.textconv", str(textconv)) + source = self.repo / "Sources" / "TokiSyncProtocol" / "SnapshotCipher.swift" + source.write_text("func seal(nonce: String, key: String) {}\n", encoding="utf-8") + + self.run_runner("--lane", "baseline", "--uncommitted") + + self.assertFalse(marker.exists()) + + def test_refuses_lane_that_is_not_active(self) -> None: + completed = self.run_runner( + "--lane", + "swiftui-architecture", + "--base", + "main", + check=False, + ) + + self.assertNotEqual(completed.returncode, 0) + self.assertIn("lane is not active", completed.stderr) + self.assertFalse(self.capture.exists()) + + +if __name__ == "__main__": + unittest.main() diff --git a/.agents/skills/codex-review-loop/tests/test_scope_routing.py b/.agents/skills/codex-review-loop/tests/test_scope_routing.py new file mode 100644 index 0000000..a41f0c2 --- /dev/null +++ b/.agents/skills/codex-review-loop/tests/test_scope_routing.py @@ -0,0 +1,380 @@ +from __future__ import annotations + +import json +import subprocess +import tempfile +import unittest +from pathlib import Path + + +SKILL_DIR = Path(__file__).resolve().parents[1] +RESOLVER = SKILL_DIR / "scripts" / "resolve_review_scope.py" +REGISTRY = SKILL_DIR / "references" / "lane-registry.json" + + +class ScopeRoutingTests(unittest.TestCase): + def setUp(self) -> None: + self.temporary_directory = tempfile.TemporaryDirectory() + self.addCleanup(self.temporary_directory.cleanup) + self.repo = Path(self.temporary_directory.name) / "repo" + self.repo.mkdir() + self.git("init", "-q", "-b", "main") + self.git("config", "user.email", "review-test@example.com") + self.git("config", "user.name", "Review Test") + + (self.repo / "README.md").write_text("fixture\n", encoding="utf-8") + docs = self.repo / "docs" / "notes.md" + docs.parent.mkdir(parents=True) + docs.write_text("ordinary notes\n", encoding="utf-8") + reader = self.repo / "Sources" / "TokiUsageReaders" / "Reader.swift" + reader.parent.mkdir(parents=True) + reader.write_text("struct Reader {}\n", encoding="utf-8") + app_test = self.repo / "TokiTests" / "BehaviorTests.swift" + app_test.parent.mkdir() + app_test.write_text("func verifyBehavior() {}\n", encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "initial") + + def git(self, *arguments: str) -> str: + result = subprocess.run( + ["git", *arguments], + cwd=self.repo, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=True, + ) + return result.stdout + + def resolve(self, *scope: str) -> dict: + result = subprocess.run( + ["python3", str(RESOLVER), "--repo", str(self.repo), *scope], + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=True, + ) + return json.loads(result.stdout) + + def resolve_registry( + self, + registry: dict | bytes, + name: str, + ) -> subprocess.CompletedProcess[str]: + registry_path = self.repo.parent / name + if isinstance(registry, bytes): + registry_path.write_bytes(registry) + else: + registry_path.write_text(json.dumps(registry), encoding="utf-8") + return subprocess.run( + [ + "python3", + str(RESOLVER), + "--repo", + str(self.repo), + "--registry", + str(registry_path), + "--uncommitted", + ], + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=False, + ) + + @staticmethod + def lane_ids(result: dict) -> set[str]: + return {lane["id"] for lane in result["activatedLanes"]} + + def test_base_scope_routes_protocol_change_to_remote_and_testing(self) -> None: + self.git("switch", "-q", "-c", "feature") + cipher = self.repo / "Sources" / "TokiSyncProtocol" / "SnapshotCipher.swift" + cipher.parent.mkdir(parents=True) + cipher.write_text("func seal(nonce: String) {}\n", encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "add cipher") + + result = self.resolve("--base", "main") + + self.assertTrue(result["safeToReview"]) + self.assertTrue(result["hasChanges"]) + self.assertEqual(result["scope"]["codexArgs"], ["--base", "main"]) + self.assertTrue( + {"baseline", "remote-sync", "testing"}.issubset(self.lane_ids(result)) + ) + + def test_commit_scope_preserves_exact_sha_target(self) -> None: + self.git("switch", "-q", "-c", "feature") + cipher = self.repo / "Sources" / "TokiSyncProtocol" / "SnapshotCipher.swift" + cipher.parent.mkdir(parents=True) + cipher.write_text("func open(ciphertext: String) {}\n", encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "add open") + commit = self.git("rev-parse", "HEAD").strip() + + result = self.resolve("--commit", commit) + + self.assertEqual(result["scope"]["codexArgs"], ["--commit", commit]) + self.assertIn("remote-sync", self.lane_ids(result)) + + def test_commit_scope_preserves_root_commit_handling(self) -> None: + root_commit = self.git("rev-list", "--max-parents=0", "HEAD").strip() + + result = self.resolve("--commit", root_commit) + + self.assertTrue(result["hasChanges"]) + self.assertIn("README.md", result["changedPaths"]) + self.assertIn( + "Sources/TokiUsageReaders/Reader.swift", + result["changedPaths"], + ) + self.assertEqual(result["scope"]["codexArgs"], ["--commit", root_commit]) + + def test_commit_scope_resolves_merge_against_first_parent(self) -> None: + self.git("switch", "-q", "-c", "feature") + reader = self.repo / "Sources" / "TokiUsageReaders" / "MergedReader.swift" + reader.write_text("struct MergedReader {}\n", encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "add merged reader") + self.git("switch", "-q", "main") + self.git("merge", "-q", "--no-ff", "feature", "-m", "merge feature") + merge_commit = self.git("rev-parse", "HEAD").strip() + + result = self.resolve("--commit", merge_commit) + + self.assertTrue(result["hasChanges"]) + self.assertIn( + "Sources/TokiUsageReaders/MergedReader.swift", + result["changedPaths"], + ) + self.assertIn("usage-pricing", self.lane_ids(result)) + self.assertEqual(result["scope"]["codexArgs"], ["--commit", merge_commit]) + + def test_uncommitted_reader_change_activates_usage_privacy_and_testing(self) -> None: + reader = self.repo / "Sources" / "TokiUsageReaders" / "Reader.swift" + reader.write_text("struct Reader { let tokenCount: Int }\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertTrue(result["safeToReview"]) + self.assertTrue( + {"baseline", "usage-pricing", "privacy-security", "testing"}.issubset( + self.lane_ids(result) + ) + ) + + def test_semantic_signal_activates_lane_outside_normal_path(self) -> None: + notes = self.repo / "docs" / "notes.md" + notes.write_text("Review the MainActor handoff.\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertIn("concurrency-lifecycle", self.lane_ids(result)) + + def test_binary_attribute_uses_conservative_semantic_fallback(self) -> None: + (self.repo / ".gitattributes").write_text( + "docs/*.md binary\n", + encoding="utf-8", + ) + self.git("add", ".gitattributes") + self.git("commit", "-q", "-m", "mark docs binary") + notes = self.repo / "docs" / "notes.md" + notes.write_text("MainActor\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertFalse(result["semanticInspectionComplete"]) + self.assertIn("concurrency-lifecycle", self.lane_ids(result)) + + def test_untracked_semantic_signal_activates_lane_outside_normal_path(self) -> None: + notes = self.repo / "docs" / "new.md" + notes.write_text("Review the MainActor handoff.\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertIn("docs/new.md", result["untrackedReviewedPaths"]) + self.assertIn("concurrency-lifecycle", self.lane_ids(result)) + + def test_direct_app_test_path_activates_testing_lane(self) -> None: + app_test = self.repo / "TokiTests" / "BehaviorTests.swift" + app_test.write_text("func verifyBehavior() { _ = 1 }\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertIn("testing", self.lane_ids(result)) + + def test_safe_untracked_directory_routes_individual_source_file(self) -> None: + service = self.repo / "Sources" / "TokiAgentCore" / "NewService.swift" + service.parent.mkdir() + service.write_text("actor NewService {}\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertTrue(result["safeToReview"]) + self.assertIn( + "Sources/TokiAgentCore/NewService.swift", + result["untrackedReviewedPaths"], + ) + self.assertTrue( + {"remote-sync", "concurrency-lifecycle", "testing"}.issubset( + self.lane_ids(result) + ) + ) + + def test_excluded_untracked_directory_blocks_uncommitted_review(self) -> None: + sensitive = self.repo / ".hermes" / "session.log" + sensitive.parent.mkdir() + sensitive.write_text("sensitive fixture\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + serialized = json.dumps(result) + + self.assertTrue(result["hasChanges"]) + self.assertFalse(result["safeToReview"]) + self.assertNotIn("session.log", serialized) + self.assertEqual(result["excludedChanges"], [{"pattern": ".hermes", "count": 1}]) + + def test_root_environment_file_blocks_uncommitted_review(self) -> None: + environment = self.repo / ".env" + environment.write_text("API_TOKEN=secret\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + serialized = json.dumps(result) + + self.assertTrue(result["hasChanges"]) + self.assertFalse(result["safeToReview"]) + self.assertNotIn("API_TOKEN", serialized) + self.assertEqual(result["excludedChanges"], [{"pattern": ".env", "count": 1}]) + + def test_nested_environment_file_blocks_uncommitted_review(self) -> None: + environment = self.repo / "config" / ".env.local" + environment.parent.mkdir() + environment.write_text("API_TOKEN=secret\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + serialized = json.dumps(result) + + self.assertTrue(result["hasChanges"]) + self.assertFalse(result["safeToReview"]) + self.assertNotIn("API_TOKEN", serialized) + self.assertEqual( + result["excludedChanges"], + [{"pattern": "**/.env.*", "count": 1}], + ) + + def test_private_key_file_blocks_uncommitted_review(self) -> None: + private_key = self.repo / "keys" / "service.pem" + private_key.parent.mkdir() + private_key.write_text("private key fixture\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + serialized = json.dumps(result) + + self.assertTrue(result["hasChanges"]) + self.assertFalse(result["safeToReview"]) + self.assertNotIn("private key fixture", serialized) + self.assertEqual( + result["excludedChanges"], + [{"pattern": "**/*.pem", "count": 1}], + ) + + def test_registry_requires_baseline_to_be_always_on(self) -> None: + cases = ( + ("false", False), + ("missing", None), + ("string", "true"), + ("integer", 1), + ("null", None), + ) + for index, (mode, always_value) in enumerate(cases): + with self.subTest(mode=mode): + registry = json.loads(REGISTRY.read_text(encoding="utf-8")) + baseline = next( + lane for lane in registry["lanes"] if lane["id"] == "baseline" + ) + if mode == "missing": + baseline.pop("always") + else: + baseline["always"] = always_value + + completed = self.resolve_registry( + registry, + f"registry-{index}.json", + ) + + self.assertEqual(completed.returncode, 2) + self.assertIn("baseline lane must be always-on", completed.stderr) + + def test_registry_rejects_invalid_verification_profiles(self) -> None: + invalid_profiles = ([], ["unknown"], ["common", "common"]) + for index, profiles in enumerate(invalid_profiles): + with self.subTest(profiles=profiles): + registry = json.loads(REGISTRY.read_text(encoding="utf-8")) + registry["lanes"][0]["verificationProfiles"] = profiles + + completed = self.resolve_registry( + registry, + f"profiles-{index}.json", + ) + + self.assertEqual(completed.returncode, 2) + self.assertIn("verificationProfiles", completed.stderr) + + def test_registry_rejects_unsupported_execution_settings(self) -> None: + invalid_execution = ( + {"replicas": 2, "adjudication": False}, + {"replicas": 1, "adjudication": True}, + {"replicas": 1, "adjudication": False, "unknown": True}, + ) + for index, execution in enumerate(invalid_execution): + with self.subTest(execution=execution): + registry = json.loads(REGISTRY.read_text(encoding="utf-8")) + registry["lanes"][0]["execution"] = execution + + completed = self.resolve_registry( + registry, + f"execution-{index}.json", + ) + + self.assertEqual(completed.returncode, 2) + self.assertIn("execution", completed.stderr) + + def test_registry_rejects_noncanonical_prompt_paths(self) -> None: + invalid_prompts = ( + str((SKILL_DIR / "references" / "lanes" / "baseline.md").resolve()), + "references/verification.md", + "references/lane-registry.json", + ) + for index, prompt in enumerate(invalid_prompts): + with self.subTest(prompt=prompt): + registry = json.loads(REGISTRY.read_text(encoding="utf-8")) + registry["lanes"][0]["prompt"] = prompt + + completed = self.resolve_registry( + registry, + f"prompt-{index}.json", + ) + + self.assertEqual(completed.returncode, 2) + self.assertIn("prompt", completed.stderr) + + def test_registry_rejects_raw_non_utf8_without_traceback(self) -> None: + completed = self.resolve_registry( + b'{"version":"1.0","pathExclusions":[],"lanes":["' + bytes([0xFF]) + b'"]}', + "raw-invalid.json", + ) + + self.assertEqual(completed.returncode, 2) + self.assertIn("cannot load lane registry", completed.stderr) + self.assertNotIn("Traceback", completed.stderr) + + def test_clean_scope_reports_no_changes(self) -> None: + result = self.resolve("--uncommitted") + + self.assertFalse(result["hasChanges"]) + self.assertTrue(result["safeToReview"]) + self.assertEqual(self.lane_ids(result), {"baseline"}) + + +if __name__ == "__main__": + unittest.main() diff --git a/.agents/skills/codex-review-loop/tests/test_scope_security.py b/.agents/skills/codex-review-loop/tests/test_scope_security.py new file mode 100644 index 0000000..c955e57 --- /dev/null +++ b/.agents/skills/codex-review-loop/tests/test_scope_security.py @@ -0,0 +1,835 @@ +from __future__ import annotations + +import importlib +import json +import os +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path +from unittest import mock + + +SKILL_DIR = Path(__file__).resolve().parents[1] +RESOLVER = SKILL_DIR / "scripts" / "resolve_review_scope.py" +sys.path.insert(0, str(RESOLVER.parent)) +review_scope_git = importlib.import_module("review_scope_git") + + +class ScopeSecurityTests(unittest.TestCase): + def setUp(self) -> None: + self.temporary_directory = tempfile.TemporaryDirectory() + self.addCleanup(self.temporary_directory.cleanup) + self.repo = Path(self.temporary_directory.name) / "repo" + self.repo.mkdir() + self.git("init", "-q", "-b", "main") + self.git("config", "user.email", "review-test@example.com") + self.git("config", "user.name", "Review Test") + self.git("config", "diff.renames", "true") + (self.repo / "README.md").write_text("fixture\n", encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "initial") + + def git(self, *arguments: str) -> str: + result = subprocess.run( + ["git", *arguments], + cwd=self.repo, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=True, + ) + return result.stdout + + def resolve(self, *scope: str) -> dict: + completed = subprocess.run( + ["python3", str(RESOLVER), "--repo", str(self.repo), *scope], + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=True, + ) + return json.loads(completed.stdout) + + def commit_secret(self) -> None: + content = "".join(f"SECRET_{index}=value\n" for index in range(20)) + (self.repo / ".env").write_text(content, encoding="utf-8") + self.git("add", ".env") + self.git("commit", "-q", "-m", "add secret fixture") + + def rename_secret(self) -> None: + (self.repo / ".env").rename(self.repo / "harmless.txt") + renamed = self.repo / "harmless.txt" + content = renamed.read_text(encoding="utf-8") + renamed.write_text( + content.replace("SECRET_19=value", "SECRET_19=changed"), + encoding="utf-8", + ) + + def assert_secret_is_blocked(self, result: dict) -> None: + self.assertTrue(result["hasChanges"]) + self.assertFalse(result["safeToReview"]) + self.assertEqual(result["excludedChanges"], [{"pattern": ".env", "count": 1}]) + + def test_unstaged_rename_out_of_secret_path_is_blocked(self) -> None: + self.commit_secret() + self.rename_secret() + + result = self.resolve("--uncommitted") + + self.assert_secret_is_blocked(result) + + def test_staged_rename_out_of_secret_path_is_blocked(self) -> None: + self.commit_secret() + self.rename_secret() + self.git("add", "-A") + + result = self.resolve("--uncommitted") + + self.assert_secret_is_blocked(result) + + def test_base_rename_out_of_secret_path_is_blocked(self) -> None: + self.commit_secret() + self.git("switch", "-q", "-c", "feature") + self.rename_secret() + self.git("add", "-A") + self.git("commit", "-q", "-m", "rename secret") + + result = self.resolve("--base", "main") + + self.assert_secret_is_blocked(result) + + def test_commit_rename_out_of_secret_path_is_blocked(self) -> None: + self.commit_secret() + self.rename_secret() + self.git("add", "-A") + self.git("commit", "-q", "-m", "rename secret") + commit = self.git("rev-parse", "HEAD").strip() + + result = self.resolve("--commit", commit) + + self.assert_secret_is_blocked(result) + + def test_mixed_case_root_environment_file_is_blocked(self) -> None: + environment = self.repo / ".ENV" + environment.write_text("API_TOKEN=secret\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertFalse(result["safeToReview"]) + self.assertEqual(result["excludedChanges"], [{"pattern": ".env", "count": 1}]) + + def test_mixed_case_nested_private_key_is_blocked(self) -> None: + private_key = self.repo / "keys" / "service.PEM" + private_key.parent.mkdir() + private_key.write_text("private key fixture\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertFalse(result["safeToReview"]) + self.assertEqual( + result["excludedChanges"], + [{"pattern": "**/*.pem", "count": 1}], + ) + + def test_local_usage_reader_paths_are_blocked(self) -> None: + sensitive_paths = [ + ".claude/projects/session.jsonl", + ".codex/state_5.sqlite", + ".codex/sessions/rollout.jsonl", + ".codex/archived_sessions/rollout.jsonl", + ".config/Cursor/User/globalStorage/state.vscdb", + "Library/Application Support/Cursor/User/globalStorage/state.vscdb", + ".gemini/tmp/chat.json", + ".gjc/agent/sessions/session.jsonl", + ".local/share/opencode/opencode.db", + ".openclaw/agents/main/sessions/session.jsonl", + ".local/state/toki-agent/usage-cache.json", + ".local/state/toki/usage-cache.json", + "Library/Application Support/Toki/usage-cache.json", + ] + for relative_path in sensitive_paths: + sensitive = self.repo / relative_path + sensitive.parent.mkdir(parents=True, exist_ok=True) + sensitive.write_text("sensitive fixture\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertFalse(result["safeToReview"]) + self.assertEqual(result["changedPaths"], []) + self.assertEqual( + sum(change["count"] for change in result["excludedChanges"]), + len(sensitive_paths), + ) + + def test_untracked_symlink_outside_repository_is_rejected(self) -> None: + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + (self.repo / "notes.txt").symlink_to(outside) + + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve("--uncommitted") + + self.assertIn("symbolic link cannot be reviewed safely", raised.exception.stderr) + + def test_staged_symlink_outside_repository_is_rejected(self) -> None: + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + (self.repo / "notes.txt").symlink_to(outside) + self.git("add", "notes.txt") + + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve("--uncommitted") + + self.assertIn("changed symbolic link", raised.exception.stderr) + + def test_tracked_file_replaced_by_symlink_is_rejected(self) -> None: + notes = self.repo / "notes.txt" + notes.write_text("safe fixture\n", encoding="utf-8") + self.git("add", "notes.txt") + self.git("commit", "-q", "-m", "add tracked fixture") + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + notes.unlink() + notes.symlink_to(outside) + + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve("--uncommitted") + + self.assertIn("changed symbolic link", raised.exception.stderr) + + def test_committed_regular_file_replaced_by_symlink_is_rejected_for_object_scopes( + self, + ) -> None: + self.git("switch", "-q", "-c", "feature") + reviewed = self.repo / "reviewed.txt" + reviewed.write_text("committed\n", encoding="utf-8") + self.git("add", "reviewed.txt") + self.git("commit", "-q", "-m", "add reviewed file") + commit = self.git("rev-parse", "HEAD").strip() + reviewed.unlink() + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + reviewed.symlink_to(outside) + + for scope in (("--base", "main"), ("--commit", commit)): + with self.subTest(scope=scope): + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve(*scope) + + self.assertIn("symbolic link", raised.exception.stderr) + + def test_base_scope_symlink_outside_repository_is_rejected(self) -> None: + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + self.git("switch", "-q", "-c", "feature") + (self.repo / "notes.txt").symlink_to(outside) + self.git("add", "notes.txt") + self.git("commit", "-q", "-m", "add symlink") + + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve("--base", "main") + + self.assertIn("changed symbolic link", raised.exception.stderr) + + def test_commit_scope_symlink_outside_repository_is_rejected(self) -> None: + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + (self.repo / "notes.txt").symlink_to(outside) + self.git("add", "notes.txt") + self.git("commit", "-q", "-m", "add symlink") + commit = self.git("rev-parse", "HEAD").strip() + + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve("--commit", commit) + + self.assertIn("changed symbolic link", raised.exception.stderr) + + def test_ignored_sensitive_file_blocks_base_and_commit_scopes(self) -> None: + (self.repo / ".gitignore").write_text(".env\n", encoding="utf-8") + self.git("add", ".gitignore") + self.git("commit", "-q", "-m", "ignore environment") + self.git("switch", "-q", "-c", "feature") + (self.repo / "README.md").write_text("feature\n", encoding="utf-8") + self.git("add", "README.md") + self.git("commit", "-q", "-m", "feature") + commit = self.git("rev-parse", "HEAD").strip() + (self.repo / ".env").write_text("API_TOKEN=secret\n", encoding="utf-8") + + for scope in (("--base", "main"), ("--commit", commit)): + with self.subTest(scope=scope): + result = self.resolve(*scope) + + self.assertFalse(result["safeToReview"]) + self.assertIn( + {"pattern": ".env", "count": 1}, + result["excludedChanges"], + ) + + def test_untracked_sensitive_file_blocks_base_and_commit_scopes(self) -> None: + self.git("switch", "-q", "-c", "feature") + (self.repo / "README.md").write_text("feature\n", encoding="utf-8") + self.git("add", "README.md") + self.git("commit", "-q", "-m", "feature") + commit = self.git("rev-parse", "HEAD").strip() + (self.repo / ".env").write_text("API_TOKEN=secret\n", encoding="utf-8") + + for scope in (("--base", "main"), ("--commit", commit)): + with self.subTest(scope=scope): + result = self.resolve(*scope) + + self.assertFalse(result["safeToReview"]) + + def test_embedded_repository_sensitive_file_blocks_every_scope(self) -> None: + self.git("switch", "-q", "-c", "feature") + (self.repo / "README.md").write_text("feature\n", encoding="utf-8") + self.git("add", "README.md") + self.git("commit", "-q", "-m", "feature") + commit = self.git("rev-parse", "HEAD").strip() + vendor = self.repo / "vendor" + vendor.mkdir() + subprocess.run(["git", "init", "-q"], cwd=vendor, check=True) + (vendor / ".env").write_text("API_TOKEN=secret\n", encoding="utf-8") + + for scope in ( + ("--uncommitted",), + ("--base", "main"), + ("--commit", commit), + ): + with self.subTest(scope=scope): + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve(*scope) + + self.assertIn("embedded Git repository", raised.exception.stderr) + + def test_embedded_repository_external_symlink_is_rejected_everywhere(self) -> None: + self.git("switch", "-q", "-c", "feature") + (self.repo / "README.md").write_text("feature\n", encoding="utf-8") + self.git("add", "README.md") + self.git("commit", "-q", "-m", "feature") + commit = self.git("rev-parse", "HEAD").strip() + vendor = self.repo / "vendor" + vendor.mkdir() + subprocess.run(["git", "init", "-q"], cwd=vendor, check=True) + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + (vendor / "link").symlink_to(outside) + + for scope in ( + ("--uncommitted",), + ("--base", "main"), + ("--commit", commit), + ): + with self.subTest(scope=scope): + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve(*scope) + + self.assertIn("cannot be reviewed safely", raised.exception.stderr) + + def test_tracked_submodule_is_rejected_for_every_scope(self) -> None: + submodule = self.repo / "vendor" + submodule.mkdir() + subprocess.run(["git", "init", "-q"], cwd=submodule, check=True) + subprocess.run( + ["git", "config", "user.email", "review-test@example.com"], + cwd=submodule, + check=True, + ) + subprocess.run( + ["git", "config", "user.name", "Review Test"], + cwd=submodule, + check=True, + ) + (submodule / "README.md").write_text("submodule\n", encoding="utf-8") + subprocess.run(["git", "add", "."], cwd=submodule, check=True) + subprocess.run( + ["git", "commit", "-q", "-m", "submodule"], + cwd=submodule, + check=True, + ) + submodule_commit = subprocess.check_output( + ["git", "rev-parse", "HEAD"], + cwd=submodule, + text=True, + ).strip() + self.git( + "update-index", + "--add", + "--cacheinfo", + "160000", + submodule_commit, + "vendor", + ) + self.git("commit", "-q", "-m", "track submodule") + self.git("switch", "-q", "-c", "feature") + (self.repo / "README.md").write_text("feature\n", encoding="utf-8") + self.git("add", "README.md") + self.git("commit", "-q", "-m", "feature") + commit = self.git("rev-parse", "HEAD").strip() + + for scope in ( + ("--uncommitted",), + ("--base", "main"), + ("--commit", commit), + ): + with self.subTest(scope=scope): + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve(*scope) + + self.assertIn("submodule", raised.exception.stderr) + + def test_nested_git_metadata_under_tracked_directory_is_rejected(self) -> None: + tracked_directory = self.repo / "vendor" + tracked_directory.mkdir() + (tracked_directory / "README.md").write_text("tracked\n", encoding="utf-8") + self.git("add", "vendor/README.md") + self.git("commit", "-q", "-m", "track vendor directory") + external_git = self.repo.parent / "external-git" + external_git.mkdir() + (tracked_directory / ".git").symlink_to(external_git) + + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve("--uncommitted") + + self.assertIn("embedded Git repository", raised.exception.stderr) + + def test_swift_build_cache_with_embedded_git_is_rejected(self) -> None: + checkout = self.repo / ".build" / "checkouts" / "dependency" + (checkout / ".git").mkdir(parents=True) + (checkout / "README.md").write_text("generated checkout\n", encoding="utf-8") + (self.repo / "README.md").write_text("changed\n", encoding="utf-8") + + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve("--uncommitted") + + self.assertIn("embedded Git repository", raised.exception.stderr) + + def test_internal_symlink_alias_preserves_sensitive_exclusions(self) -> None: + (self.repo / ".gitignore").write_text( + ".agents/projects/\n", + encoding="utf-8", + ) + (self.repo / ".agents").mkdir() + (self.repo / ".claude").symlink_to(".agents") + self.git("add", ".gitignore", ".claude") + self.git("commit", "-q", "-m", "add internal agent alias") + session = self.repo / ".agents" / "projects" / "session.jsonl" + session.parent.mkdir() + session.write_text("sensitive transcript\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertFalse(result["safeToReview"]) + self.assertTrue( + any( + item["pattern"] == ".claude/projects/**" + for item in result["excludedChanges"] + ) + ) + + def test_ignored_symlink_is_rejected_for_every_scope(self) -> None: + (self.repo / ".gitignore").write_text("ignored-link\n", encoding="utf-8") + self.git("add", ".gitignore") + self.git("commit", "-q", "-m", "ignore link") + self.git("switch", "-q", "-c", "feature") + (self.repo / "README.md").write_text("feature\n", encoding="utf-8") + self.git("add", "README.md") + self.git("commit", "-q", "-m", "feature") + commit = self.git("rev-parse", "HEAD").strip() + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + (self.repo / "ignored-link").symlink_to(outside) + + for scope in ( + ("--uncommitted",), + ("--base", "main"), + ("--commit", commit), + ): + with self.subTest(scope=scope): + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve(*scope) + + self.assertIn("symbolic link", raised.exception.stderr) + + def test_untracked_symlink_is_rejected_for_base_and_commit_scopes(self) -> None: + self.git("switch", "-q", "-c", "feature") + (self.repo / "README.md").write_text("feature\n", encoding="utf-8") + self.git("add", "README.md") + self.git("commit", "-q", "-m", "feature") + commit = self.git("rev-parse", "HEAD").strip() + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + (self.repo / "notes-link").symlink_to(outside) + + for scope in (("--base", "main"), ("--commit", commit)): + with self.subTest(scope=scope): + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve(*scope) + + self.assertIn("symbolic link", raised.exception.stderr) + + def test_unchanged_tracked_symlink_is_rejected_for_every_scope(self) -> None: + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + (self.repo / "tracked-link").symlink_to(outside) + self.git("add", "tracked-link") + self.git("commit", "-q", "-m", "track link") + self.git("switch", "-q", "-c", "feature") + (self.repo / "README.md").write_text("feature\n", encoding="utf-8") + self.git("add", "README.md") + self.git("commit", "-q", "-m", "feature") + commit = self.git("rev-parse", "HEAD").strip() + + for scope in ( + ("--uncommitted",), + ("--base", "main"), + ("--commit", commit), + ): + with self.subTest(scope=scope): + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve(*scope) + + self.assertIn("symbolic link", raised.exception.stderr) + + def test_unchanged_internal_tracked_symlink_is_allowed_for_every_scope(self) -> None: + target = self.repo / "Sources" + target.mkdir() + (self.repo / "tracked-link").symlink_to(target) + self.git("add", "tracked-link") + self.git("commit", "-q", "-m", "track internal link") + self.git("switch", "-q", "-c", "feature") + (self.repo / "README.md").write_text("feature\n", encoding="utf-8") + self.git("add", "README.md") + self.git("commit", "-q", "-m", "feature") + commit = self.git("rev-parse", "HEAD").strip() + + for scope in ( + ("--uncommitted",), + ("--base", "main"), + ("--commit", commit), + ): + with self.subTest(scope=scope): + result = self.resolve(*scope) + + self.assertTrue(result["safeToReview"]) + + def test_symlink_deletion_is_rejected_for_every_scope(self) -> None: + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + link = self.repo / "tracked-link" + link.symlink_to(outside) + self.git("add", "tracked-link") + self.git("commit", "-q", "-m", "track link") + self.git("switch", "-q", "-c", "feature") + link.unlink() + self.git("add", "tracked-link") + + with self.assertRaises(subprocess.CalledProcessError): + self.resolve("--uncommitted") + + self.git("commit", "-q", "-m", "delete link") + commit = self.git("rev-parse", "HEAD").strip() + for scope in (("--base", "main"), ("--commit", commit)): + with self.subTest(scope=scope): + with self.assertRaises(subprocess.CalledProcessError): + self.resolve(*scope) + + def test_untracked_non_utf8_path_is_rejected(self) -> None: + with mock.patch( + "review_scope_git.changed_paths_without_symlinks", + return_value=[], + ), mock.patch( + "review_scope_git.run_git", + return_value=b"invalid-\xff.txt\0", + ): + with self.assertRaisesRegex( + review_scope_git.ScopeError, + "Git path is not valid UTF-8", + ): + review_scope_git.uncommitted_scope(self.repo, []) + + def test_staged_non_utf8_path_is_rejected(self) -> None: + with mock.patch( + "review_scope_git.changed_paths_without_symlinks", + side_effect=[ + [], + review_scope_git.ScopeError("Git path is not valid UTF-8"), + ], + ): + with self.assertRaisesRegex( + review_scope_git.ScopeError, + "Git path is not valid UTF-8", + ): + review_scope_git.uncommitted_scope(self.repo, []) + + def test_large_untracked_file_activates_specialists_conservatively(self) -> None: + notes = self.repo / "docs" / "large.md" + notes.parent.mkdir() + notes.write_text( + ("ordinary text\n" * 87_381) + "MainActor\n", + encoding="utf-8", + ) + + result = self.resolve("--uncommitted") + lane_ids = {lane["id"] for lane in result["activatedLanes"]} + + self.assertFalse(result["semanticInspectionComplete"]) + self.assertIn("concurrency-lifecycle", lane_ids) + + def test_aggregate_untracked_content_activates_specialists_conservatively(self) -> None: + notes = self.repo / "docs" + notes.mkdir() + content = "ordinary text\n" * 69_230 + for index in range(5): + (notes / f"large-{index}.md").write_text(content, encoding="utf-8") + + result = self.resolve("--uncommitted") + lane_ids = {lane["id"] for lane in result["activatedLanes"]} + + self.assertFalse(result["semanticInspectionComplete"]) + self.assertIn("concurrency-lifecycle", lane_ids) + + def test_aggregate_untracked_binary_content_counts_toward_budget(self) -> None: + assets = self.repo / "assets" + assets.mkdir() + content = b"\0" + (b"x" * 900_000) + for index in range(5): + (assets / f"large-{index}.bin").write_bytes(content) + + result = self.resolve("--uncommitted") + lane_ids = {lane["id"] for lane in result["activatedLanes"]} + + self.assertFalse(result["semanticInspectionComplete"]) + self.assertIn("concurrency-lifecycle", lane_ids) + + def test_safe_text_rename_does_not_route_unchanged_content(self) -> None: + notes = self.repo / "docs" + notes.mkdir() + original = notes / "original.txt" + original.write_text("MainActor\n" * 50_000, encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "add rename fixture") + self.git("mv", "docs/original.txt", "docs/renamed.txt") + + result = self.resolve("--uncommitted") + lane_ids = {lane["id"] for lane in result["activatedLanes"]} + + self.assertIn("docs/original.txt", result["changedPaths"]) + self.assertIn("docs/renamed.txt", result["changedPaths"]) + self.assertNotIn("concurrency-lifecycle", lane_ids) + + def test_large_tracked_diff_activates_specialists_conservatively(self) -> None: + notes = self.repo / "docs" + notes.mkdir() + (notes / "large.txt").write_text( + "ordinary text\n" * 350_000, + encoding="utf-8", + ) + self.git("add", ".") + + result = self.resolve("--uncommitted") + lane_ids = {lane["id"] for lane in result["activatedLanes"]} + + self.assertFalse(result["semanticInspectionComplete"]) + self.assertIn("concurrency-lifecycle", lane_ids) + + def test_semantic_diff_does_not_execute_textconv_filter(self) -> None: + attributes = self.repo / ".gitattributes" + attributes.write_text("*.review diff=unsafe\n", encoding="utf-8") + reviewed = self.repo / "sample.review" + reviewed.write_text("before\n", encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "add textconv fixture") + + converter = self.repo.parent / "textconv.py" + converter.write_text( + "#!/usr/bin/env python3\n" + "from pathlib import Path\n" + "import sys\n" + "Path(__file__).with_suffix('.invoked').write_text('invoked\\n')\n" + "sys.stdout.buffer.write(Path(sys.argv[1]).read_bytes())\n", + encoding="utf-8", + ) + converter.chmod(0o755) + marker = converter.with_suffix(".invoked") + self.git("config", "diff.unsafe.textconv", str(converter)) + reviewed.write_text("after\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertTrue(result["hasChanges"]) + self.assertFalse(marker.exists()) + + def test_scope_resolution_does_not_execute_clean_filter(self) -> None: + attributes = self.repo / ".gitattributes" + attributes.write_text("*.review filter=unsafe\n", encoding="utf-8") + reviewed = self.repo / "sample.review" + reviewed.write_text("before\n", encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "add clean filter fixture") + + clean_filter = self.repo.parent / "clean-filter.py" + clean_filter.write_text( + "#!/usr/bin/env python3\n" + "from pathlib import Path\n" + "import sys\n" + "Path(__file__).with_suffix('.invoked').write_text('invoked\\n')\n" + "sys.stdout.buffer.write(sys.stdin.buffer.read())\n", + encoding="utf-8", + ) + clean_filter.chmod(0o755) + marker = clean_filter.with_suffix(".invoked") + self.git("config", "filter.unsafe.clean", str(clean_filter)) + reviewed.write_text("after\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + + self.assertTrue(result["hasChanges"]) + self.assertFalse(marker.exists()) + + def test_scope_resolution_does_not_execute_fsmonitor_hook(self) -> None: + self.git("switch", "-q", "-c", "feature") + (self.repo / "README.md").write_text("feature\n", encoding="utf-8") + self.git("add", "README.md") + self.git("commit", "-q", "-m", "feature") + commit = self.git("rev-parse", "HEAD").strip() + fsmonitor = self.repo.parent / "fsmonitor.py" + fsmonitor.write_text( + "#!/usr/bin/env python3\n" + "from pathlib import Path\n" + "Path(__file__).with_suffix('.invoked').write_text('invoked\\n')\n", + encoding="utf-8", + ) + fsmonitor.chmod(0o755) + marker = fsmonitor.with_suffix(".invoked") + self.git("config", "core.fsmonitor", str(fsmonitor)) + (self.repo / "README.md").write_text("dirty feature\n", encoding="utf-8") + + for scope in ( + ("--uncommitted",), + ("--base", "main"), + ("--commit", commit), + ): + with self.subTest(scope=scope): + marker.unlink(missing_ok=True) + result = self.resolve(*scope) + + self.assertTrue(result["hasChanges"]) + self.assertFalse(marker.exists()) + + def test_repository_selection_environment_cannot_hide_filter_config(self) -> None: + attributes = self.repo / ".gitattributes" + attributes.write_text("*.review filter=unsafe\n", encoding="utf-8") + reviewed = self.repo / "sample.review" + reviewed.write_text("before\n", encoding="utf-8") + self.git("add", ".") + self.git("commit", "-q", "-m", "add filtered file") + marker = self.repo.parent / "filter-invoked" + filter_script = self.repo.parent / "filter.py" + filter_script.write_text( + "#!/usr/bin/env python3\n" + "from pathlib import Path\n" + "import sys\n" + f"Path({str(marker)!r}).write_text('invoked\\n')\n" + "sys.stdout.buffer.write(sys.stdin.buffer.read())\n", + encoding="utf-8", + ) + filter_script.chmod(0o755) + self.git("config", "filter.unsafe.clean", str(filter_script)) + self.git("config", "filter.unsafe.required", "true") + reviewed.write_text("after\n", encoding="utf-8") + decoy = self.repo.parent / "decoy" + decoy.mkdir() + subprocess.run(["git", "init", "-q"], cwd=decoy, check=True) + environment = os.environ.copy() + environment["GIT_DIR"] = str(decoy / ".git") + + completed = subprocess.run( + [ + "python3", + str(RESOLVER), + "--repo", + str(self.repo), + "--uncommitted", + ], + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=environment, + check=True, + ) + result = json.loads(completed.stdout) + + self.assertTrue(result["hasChanges"]) + self.assertFalse(marker.exists()) + + def test_repository_root_preserves_trailing_whitespace(self) -> None: + whitespace_repo = self.repo.parent / "repo " + whitespace_repo.mkdir() + subprocess.run(["git", "init", "-q", "-b", "main"], cwd=whitespace_repo, check=True) + subprocess.run( + ["git", "config", "user.email", "review-test@example.com"], + cwd=whitespace_repo, + check=True, + ) + subprocess.run( + ["git", "config", "user.name", "Review Test"], + cwd=whitespace_repo, + check=True, + ) + (whitespace_repo / "README.md").write_text("fixture\n", encoding="utf-8") + subprocess.run(["git", "add", "README.md"], cwd=whitespace_repo, check=True) + subprocess.run( + ["git", "commit", "-q", "-m", "initial"], + cwd=whitespace_repo, + check=True, + ) + (whitespace_repo / ".env").write_text("API_TOKEN=secret\n", encoding="utf-8") + + completed = subprocess.run( + [ + "python3", + str(RESOLVER), + "--repo", + str(whitespace_repo), + "--uncommitted", + ], + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=True, + ) + result = json.loads(completed.stdout) + + self.assertFalse(result["safeToReview"]) + + def test_symlink_error_escapes_terminal_control_characters(self) -> None: + unsafe_name = "\x1b]0;PWN\x07link" + outside = self.repo.parent / "outside.txt" + outside.write_text("credential fixture\n", encoding="utf-8") + (self.repo / unsafe_name).symlink_to(outside) + + with self.assertRaises(subprocess.CalledProcessError) as raised: + self.resolve("--uncommitted") + + self.assertNotIn("\x1b", raised.exception.stderr) + self.assertNotIn("\x07", raised.exception.stderr) + self.assertIn("\\x1b", raised.exception.stderr) + + def test_literal_backslash_parent_name_does_not_read_outside_repository(self) -> None: + outside = self.repo.parent / "outside.txt" + outside.write_text("MainActor\n", encoding="utf-8") + literal_name = r"..\outside.txt" + (self.repo / literal_name).write_text("ordinary text\n", encoding="utf-8") + + result = self.resolve("--uncommitted") + lane_ids = {lane["id"] for lane in result["activatedLanes"]} + + self.assertNotIn("concurrency-lifecycle", lane_ids) + self.assertIn(literal_name, result["changedPaths"]) + self.assertIn(literal_name, result["untrackedReviewedPaths"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/.agents/skills/skill-rules.json b/.agents/skills/skill-rules.json index ea560cc..2a6ae0d 100644 --- a/.agents/skills/skill-rules.json +++ b/.agents/skills/skill-rules.json @@ -67,6 +67,58 @@ "**/DerivedData/**" ] } + }, + "codex-review-loop": { + "type": "workflow", + "enforcement": "suggest", + "priority": "high", + "description": "Toki local code-review workflow with additive specialist lanes, P0-P3 findings, explicit fix gates, targeted verification, and bounded re-review.", + "additionalContext": "This request appears to ask for a Toki code review or review-and-fix loop. Apply `codex-review-loop`. Keep review read-only until the user explicitly approves edits or commits, and keep push, PR comments, reviews, labels, merges, and other GitHub writes outside the loop.", + "promptTriggers": { + "keywords": [ + "codex review loop", + "review loop", + "review and fix", + "code review", + "리뷰 루프", + "코드 리뷰", + "코드리뷰", + "리뷰하고 수정", + "리뷰 후 수정", + "재리뷰" + ], + "intentPatterns": [ + "(review|리뷰).*(diff|change|branch|commit|수정|변경|브랜치|커밋)", + "(diff|change|branch|commit|수정|변경|브랜치|커밋).*(review|리뷰)", + "(review|리뷰).*\\b[0-9a-f]{7,64}\\b", + "\\b[0-9a-f]{7,64}\\b.*(review|리뷰)", + "(fix|address|수정|해결).*(review finding|review feedback|리뷰 지적|리뷰 결과)" + ] + }, + "fileTriggers": { + "pathPatterns": [ + "**/Toki/**/*.swift", + "**/TokiTests/**/*.swift", + "**/Sources/**/*.swift", + "**/Tests/**/*.swift", + "**/TokiHub/**/*.swift", + "**/Package.swift", + "**/Package.resolved", + "**/project.yml", + "**/.swiftformat", + "**/.swiftlint.yml", + "**/.github/workflows/**" + ], + "pathExclusions": [ + "**/.git/**", + "**/.hermes/**", + "**/.omo/**", + "**/.senpi/**", + "**/.codegraph/**", + "**/build/**", + "**/DerivedData/**" + ] + } } } } diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9fb5702..9a0dc3f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,14 @@ jobs: - name: Test Codex review label automation run: python3 -m unittest discover -s .github/scripts -p "test_*.py" + - name: Set up Node.js for review-loop hook tests + uses: actions/setup-node@v4 + with: + node-version: 22 + + - name: Test Codex review loop + run: python3 -m unittest discover -s .agents/skills/codex-review-loop/tests -p "test_*.py" + - name: Select latest Xcode uses: maxim-lobanov/setup-xcode@v1 with: diff --git a/.gitignore b/.gitignore index 328dacb..31aa62c 100644 --- a/.gitignore +++ b/.gitignore @@ -18,6 +18,10 @@ build/ *.dSYM.zip *.dSYM +# Python tooling +__pycache__/ +*.pyc + # macOS .DS_Store .AppleDouble diff --git a/AGENTS.md b/AGENTS.md index aca7faf..6f0c4f1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,6 +9,7 @@ files referenced below. | Topic | Source | When | | --- | --- | --- | | Toki Source Conventions | `/project-conventions` skill or `.agents/skills/project-conventions/conventions.md` plus task-specific files under `.agents/skills/project-conventions/references/` | Before editing `Toki/**/*.swift`, `TokiTests/**/*.swift`, `project.yml`, `.swiftformat`, `.swiftlint.yml`, or app resources | +| Local Codex Review Loop | `/codex-review-loop` skill or `.agents/skills/codex-review-loop/SKILL.md` | Local diff review, approved finding fixes, and bounded re-review | | Git Workflow | `.agents/conventions/git-workflow.md` | Branch / commit / PR work | The shared `.agents` hooks provide soft enforcement for `/project-conventions`