feat: audit-driven hardening pass (Critical/High/Medium) + v0.3.0 - #8
Merged
Conversation
…backlog Full 8-domain/24-criteria enterprise audit of the ADR Toolkit, plus a priority-ordered backlog in improvements.md (scoped to exclude domains 1/5 and work happening in other worktrees: agy adapter, auto version sync, README prose). handoff.md points to the implementation plan (docs/superpowers/plans/2026-09-01-critical-hardening.md, gitignored by convention) and lists per-task commit messages so a resumed or different session can tell what's already done from git log alone.
New core/atomic_io.py: atomic_write_text() (temp-file + os.replace, so a mid-write crash never leaves a torn file) and adr_directory_lock() (a cross-process advisory lock -- fcntl on POSIX, msvcrt on Windows) for serializing ID allocation + writes. Not yet wired into any command.
Wraps ID allocation + existence check + write in atomic_io.adr_directory_lock and replaces the direct write_text with atomic_io.atomic_write_text. The dry-run path stays outside the lock entirely (it must not create adr_dir or a lock file, per existing dry-run tests) and does an unprotected preview instead -- a race there is harmless since nothing is persisted. Reproduced the race first: 20 concurrent `create` calls produced only 13 unique ADR IDs before this fix.
Same shape as the create.py fix, plus one refinement: schema validation runs once against a preview ID before ever touching disk (not even to create exceptions_dir or a lock file), since validity never depends on which sequential number gets assigned. This satisfies the existing "SCHEMA_ERROR must not create exceptions_dir" test as well as dry-run's "must not create anything" test. The lock then wraps only the final ID allocation + atomic write. Reproduced the race first: 20 concurrent `exception` calls produced only 18 unique EXC IDs before this fix.
Wraps the whole two-file update in atomic_io.adr_directory_lock and replaces both write_text calls (and the rollback write) with atomic_write_text, so a crash never leaves either file torn. Two existing tests (test_supersede_rolls_back_old_file_when_new_file_write_fails, test_supersede_double_write_failure_reports_inconsistent_state_not_silent) monkeypatched Path.write_text directly to simulate a write failure; that seam no longer exists once writes go through atomic_io, so both were retargeted to monkeypatch supersede.atomic_io.atomic_write_text instead, preserving their original intent and assertions unchanged.
conflict._guarded_search wraps regex.search with a 0.25s SIGALRM-based timeout on POSIX (Windows has no SIGALRM and runs unguarded there -- tracked as a known gap, not a regression). RegexTimeout subclasses re.error so commands/check.py's existing `except re.error` handling downgrades a timeout to a BAD_CONSTRAINTS warning without any change needed there. Verified against a classic catastrophic-backtracking pattern ((a+)+$) that would otherwise hang; the guard interrupts it in well under 1s.
core/rendering.py gains safe_md_link_text(), which escapes \, [, ], ( and ) and collapses embedded newlines. commands/index.py's four [id — title](filename) link sites now route the title through it, so an ADR title like "foo](http://evil.example)[bar" can no longer split the generated docs/decisions/README.md into two links, one pointing off-repo. This is a fix to the README *generator code*, unrelated to hand-authored README prose being worked on elsewhere.
New core/telemetry.get_logger(operation) returns a LoggerAdapter that writes JSON Lines to stderr (level, operation, correlation_id, message, and exception_type on exceptions). adr.py's global exception handler logs via logger.exception() and includes the same correlation_id in the stdout JSON error response, so a failure reported by an agent/CI can be matched back to its stderr log line. Default level is WARNING (quiet on success); ADR_TOOLKIT_LOG_LEVEL overrides it. stdout's pure-JSON contract is unchanged except for the additive correlation_id field.
improvements.md's Critical section is cleared (resolved work now lives in changelog.md's Unreleased section + git history, per this file's own convention). handoff.md records the 7 commits that made up this pass, the 2 real regressions TDD caught and how they were fixed, and the remaining unscheduled High/Medium backlog for a future session to pick up only with explicit owner direction.
resolve_from_root now raises PathEscapesRootError when a relative path (e.g. --dir ../../etc/cron.d) would resolve outside the given root. An absolute path is left unchanged, as before -- it's the caller's own explicit choice, with no relative containment to check.
Measured baseline before adding the gate: 93.32% branch+statement coverage across skills/adr-toolkit/scripts. --cov-fail-under=85 leaves real headroom rather than being a guessed threshold. release.yml is untouched.
New core/contracts.py defines TypedDicts (CommandError, BaseResult, ErrorResult, CreateResult) for command JSON output shapes. A new type-check CI job runs `mypy --strict` over the fully-typed core modules (atomic_io, telemetry, contracts) -- fixed 3 real errors mypy found in atomic_io.py/telemetry.py to get there (missing return type on the contextmanager generator, an unnarrowed Optional in exc_info[0], and an unparameterized generic LoggerAdapter). Command *arguments* stay untyped (argparse.Namespace resists TypedDict without a larger refactor) -- extending strict typing into the 16 command modules is deliberately out of scope for this pass.
adr.py --diagnostic <operation> adds an elapsed_ms field to the JSON result via time.perf_counter(). Must precede the operation name (argparse subparsers can't see a flag registered only on the parent parser). Omitted by default -- stdout's JSON shape is unchanged unless requested.
Forks a child that pauses right before os.replace() (after the temp file is written, before the rename), SIGKILLs it there, and asserts the target file still holds its original content -- an OS-level proof of the guarantee atomic_write_text already provides, not a simulated exception. Skipped on Windows (os.fork is POSIX-only).
New scripts/adapter_sdk.py (repo-root tooling, same category as sync_version.py) validates the two fields every manifest-based adapter shares: name and description, both required non-empty strings. All 4 manifest-based adapter test files (Claude, Codex, Gemini CLI, Antigravity) now assert their real manifest passes it, loaded via importlib.util the same way test_sync_version.py already works around the scripts/ vs skills/adr-toolkit/scripts/ naming collision. adapters/generic/ has no manifest and is unaffected.
improvements.md's Critical section is gone and High now holds only the 2 items deferred to another worktree. handoff.md records all 13 commits across both the Critical and High-priority passes, the naming-collision and dry-run-side-effect gotchas discovered mid-session, and the remaining unscheduled Medium backlog for a future session to pick up only with explicit owner direction.
…ape errors New core/errors.py's AdrToolkitError(Exception) is now the base for ConfigError, FrontmatterError, ConstraintsError, InvalidTransitionError, GitPathsError, and PathEscapesRootError, each carrying a stable error_code matching the string already used at its call sites. No call site anywhere catches these by their old ValueError/RuntimeError base, only by name, so this is a safe change (verified via grep before starting). Bundled fix: PathEscapesRootError (added in the prior High-priority pass) was never actually caught anywhere -- a rejected path escape fell through to adr.py's generic INTERNAL_ERROR instead of a specific code, unlike every other domain exception in this codebase. New resolve_from_root_or_error() wraps resolve_from_root() and is now used at all 7 call sites (create, exception, index, validate, check, init, and graph's two sites), returning a proper PATH_ESCAPES_ROOT error.
Cross-checks schemas/adr.schema.json and schemas/exception.schema.json's required-field lists and enum values against core/schema.py, core/exceptions.py, core/lifecycle.py, and core/locale.py. Deliberately stdlib-only -- adopting the `jsonschema` library, as the audit report originally sketched, would trade away this project's zero-dependency design for a moderate documentation-drift risk. Verified the test has teeth by temporarily removing a required field from the schema file and confirming it fails (reverted before committing).
Adds CheckFinding and CheckResult TypedDicts, using Dict[str, Any] for the genuinely heterogeneous evidence/exception/warning payloads (bare `dict` fails mypy --strict's type-arg check). Only CreateResult and CheckResult are covered so far -- the other 14 commands remain future work per the module's existing docstring.
200 generated ADRs, search+index complete in ~0.07s (bound: 5s). Proves no catastrophic (e.g. quadratic) blowup rather than building a benchmarking system -- the audit's original "2,000 fixtures + CI regression tracking" needs historical-baseline infrastructure this project doesn't have.
adr.py prints one dim "-> <operation> ok|FAILED" line to stderr when stderr is a real terminal, suppressible via ADR_TOOLKIT_NO_COLOR=1. stdout's JSON contract is completely unaffected, and the common piped/redirected case (e.g. `adr.py check ... | jq`) sees no extra output since capsys/pipes never report isatty() as true.
improvements.md's Medium section now records the parsing-cache decline with its rationale and the output-contract item's partial (2/16) status; everything else in Medium is removed as done. handoff.md summarizes all 3 hardening passes (Critical, High, Medium) completed this session -- 19 implementation commits total -- plus the discovered gaps and their fixes, for a future session or different harness to resume from cold.
Adds TypedDicts for preflight, discover, init, index, related, significance, validate, status, supersede, diff, exception, graph, and search, alongside the existing CreateResult/CheckResult -- closing out improvements.md's "출력 계약 스키마 고정" item (was 2/16, now 16/16). Each shape was determined by reading every command's actual return statements (not guessed), and verified against real run() calls including at least one error-path branch for status and supersede. All errors/ warnings/nested-payload fields use Dict[str, Any] rather than the shared CommandError type where a command's real error dicts carry extra fields (file, id, ids, cycle, ...) that CommandError doesn't declare, to avoid overclaiming structure that isn't true.
improvements.md's Medium section is now down to exactly one item (the declined parsing-cache) -- nothing else remains open in this worktree's scope. handoff.md records the full-coverage extension and corrects its stale "future work" note about contracts.py.
improvements.md's Done section is filled in (deviating from its usual "stays empty" convention) with a full summary of every Critical/High/ Medium item shipped this session, at the owner's explicit request ahead of moving to a new session. handoff.md's Next step section is rewritten as an explicit checklist for a cold-start session: no queued work exists, the 2 remaining Open items belong to a different worktree, and prior deferred decisions (branch finish, parsing-cache decline) should be re-asked rather than assumed.
…adr-toolkit # Conflicts: # changelog.md # handoff.md # tests/unit/test_antigravity_adapter.py
…on.md Sourced from docs/enterprise-adoption.md §4/§6-9 (a separate governance/ adoption-maturity report, distinct from the code/architecture audit). Three of the four items are precondition-gated on real-world facts (repo going public, 2+ qualified maintainers, 2+ repositories existing) rather than blocked by missing code -- flagged accordingly so a future session doesn't try to "implement" a GitHub ruleset change or multi-repo tooling against a single private repo. The fourth (adoption-metrics collection from existing ADR/exception frontmatter) has no such precondition and is flagged as the one actually startable item in this tier. Also updated the Done section's branch/test-count notes to reflect the origin/develop merge completed this session.
…rlier miss) The prior "Low priority" pass only pulled from docs/enterprise-adoption.md; the actual ask was to also mine docs/adr-toolkit-audit-report.md's own Low-risk findings. Reviewed all 8 of that report's Low badges: 4 were "no action needed" or already resolved (6.1, 8.3, 8.4's PR-title-check is now the merged-in pr-title-check job, 5.3's README-escaping inconsistency was fixed by this session's safe_md_link_text work) and are noted as such; the remaining 4 real gaps are now in improvements.md's ### Low section alongside (separately sourced and labeled) the enterprise-adoption.md items: CODEOWNERS doc for constraints: blocks, a trivial proposed->deprecated lifecycle transition, moving CHECK's constraint lint earlier to CREATE/STATUS time, and folding Antigravity into harness-parity once agy gets a public registry (blocked, same as the other-worktree agy work).
…ock review - core/lifecycle.py: ALLOWED_TRANSITIONS["proposed"] now includes "deprecated", supporting withdrawing a proposal that never reached consensus (previously only accepted/rejected were reachable from proposed). docs/adr-toolkit-audit-report.md §2.5 5.1. - CONTRIBUTING.md: documents that a constraints: block change needs sign-off from someone with authority over its affected_paths, since it's enforced policy text, not prose -- a process control in place of code sandboxing, per §2.2 2.1's reasoning (no third-party code executes here, so isolation isn't the applicable defense).
…time New core/constraints.lint(body) wraps extract_constraints() and returns a BAD_CONSTRAINTS warning instead of raising, so a typo in a constraints: block surfaces as soon as the ADR is authored or accepted, rather than silently going unenforced until CHECK happens to run against it later. - create.py: lints the draft body on every path (dry-run and real write), added to a new "warnings" field alongside the existing response keys. - status.py: lints only on transition to "accepted", since that's the status CHECK actually enforces constraints against -- other transitions return warnings: [] without parsing the body. - core/contracts.py: CreateResult and StatusResult gain the warnings field to match. docs/adr-toolkit-audit-report.md §2.5 5.2.
improvements.md's audit-report Low sub-group is down to 1 item (Antigravity/harness-parity, blocked on an external precondition). handoff.md records the new commits, the parallel Codex session working on the adoption-metrics item in this same worktree/branch, and updates the Next-step checklist to reflect that almost nothing remains open in this worktree's own scope.
…tatically
New _reject_if_redos_prone() in core/constraints.py rejects a `pattern`
value at parse time if it contains a quantified group whose own body ends
in a quantifier (e.g. (a+)+, (a*)*, (x{1,3})+) -- the classic catastrophic-
backtracking shape. This is a static, string-level check, not a runtime
guard, so it works identically on every platform and closes the Windows
gap left by rules/conflict.py's SIGALRM-based timeout (POSIX-only,
confirmed unguarded on Windows per docs/adr-toolkit-audit-report.md
§2.2 2.3's Open Risk).
Scoped to forbidden_import/dependency_forbidden only -- required_path/
forbidden_path treat `pattern` as glob syntax via core/globs.py, which
can't produce catastrophic backtracking, so flagging it there would be a
false positive. Verified the real dogfooded ADR-0011 constraints block
(genuine regex patterns) still validates and checks cleanly, and that a
rejected pattern never reaches re.compile() (monkeypatched re.compile to
assert it's never called for a dangerous pattern).
This is a heuristic covering the single most common ReDoS shape, not a
full detector -- alternation-based patterns like (a|a)* are a different
dangerous shape and remain uncaught, noted in the code comment.
…xists improvements.md and handoff.md updated for this session's work only (the Windows static-complexity-linter item promoted from Open Risks). Codex's already-committed adoption-metrics commits are acknowledged as present in this branch but not itemized here, per the owner's explicit request not to reflect that in-progress work from this session's docs.
…sync work PR #6 (feature/agy-plugin-implements-2) and PR #7 (feature/add-githooks) -- the "다른 워크트리" that improvements.md's 2 High items were deferred to -- are both already merged into origin/develop and pulled into this branch (0a0db8a). Re-checked both items against actual current code: - 8.4 auto-version-direction review: genuinely done. sync_version.py and release.yml still do manual-only version bumps, no conflict with the audit's recommendation. Closed, moved to Done. - Supply-chain signing: genuinely still unimplemented (release.yml has no checksum/signing step). Stays Open, stale "다른 워크트리 확인" framing removed since that worktree's work is already merged -- now startable here, though it touches the release pipeline so needs owner confirmation before starting. - Antigravity/harness-parity Low item: re-verified still correctly blocked (agy still has no public registry per its README) -- confirms not everything tied to that merged worktree is automatically resolved. Does not touch or itemize the parallel Codex session's adoption-metrics work, per explicit instruction.
.github/workflows/release.yml now packages skills/adr-toolkit/ into a version-named tarball, checksums it (SHA-256), and generates a Sigstore- backed GitHub Artifact Attestation for it via actions/attest-build- provenance@v2 -- keyless (OIDC-based), no private key to manage or rotate. Both the tarball and its checksum are attached to the GitHub Release. Chose this over signing the git tag itself: every adapter (Claude Code marketplace source:"./", Codex/Gemini CLI plugin installs, generic copy/ symlink) references the repo or skill folder directly rather than downloading a packaged release, and tags in this project are created locally by a human before the push that triggers this workflow -- CI can't retroactively sign a tag that already exists. Attestation instead ties provenance to the exact commit the tag points to, verifiable via `gh attestation verify`, which covers the one real gap: someone grabbing the archive off the GitHub Releases page instead of cloning. SECURITY.md documents the verification commands (sha256sum -c + gh attestation verify) and is explicit that the git-clone/adapter-install paths verify via Git/GitHub history already, not this archive. Verified locally: the tar+sha256sum packaging commands run correctly against the real skills/adr-toolkit/ tree, and the modified YAML parses without syntax errors. The actual OIDC/attestation exchange can only be exercised by a real tag push through GitHub Actions, which this session did not do (release pipeline; requires explicit owner action to trigger). docs/adr-toolkit-audit-report.md §2.2 2.2.
Marks the release-pipeline attestation work (18d4662) done in improvements.md/handoff.md and records the option analysis in a troubleshooting worklog, without touching the concurrent adoption- metrics entries already present in both files.
Documents atomic writes + directory locking, the two-stage ReDoS guard, typed result contracts + mypy --strict, and structured JSON logging with correlation IDs -- each grounded in the actual commits and test evidence from this session, per the troubleshooting-worklog skill format.
Uses the ADR toolkit itself to document the Critical/High-priority hardening work: atomic writes + directory locking, the two-stage ReDoS guard, typed result contracts + mypy --strict, structured JSON logging with correlation IDs, and release artifact attestation. Each ADR passed significance scoring (recommended band) and `related` conflict checks before creation; all 16 ADRs now validate and the index/graph are regenerated. Also removes docs/worklogs/ (superseded by these ADRs and backed up externally) and gitignores the .adr-toolkit.lock runtime mutex file, which was never meant to be committed.
Rolls changelog.md's Unreleased section into a v0.3.0 entry and propagates VERSION to every manifest via scripts/sync_version.py. README.md needed no changes (no version strings, and this release's hardening work doesn't add new user-facing operations); examples/ verified clean with no drift via scripts/verify_examples.py.
CI on PR #8 caught a real, Windows/Python-3.9-only failure: `adr.py init --dir docs/decisions` (and every other resolve_from_root call site) rejected a plainly-under-root path with PATH_ESCAPES_ROOT, only on windows-latest with Python 3.9 -- not 3.12, and not on ubuntu/macOS at any version. `docs/decisions` doesn't exist on disk yet when INIT scaffolds it, and Path.resolve() on a non-existent path has inconsistent cross-version behavior on Windows. Fix: check containment by lexically normalizing the joined path (os.path.normpath, no filesystem access) against the already-resolved root, instead of calling .resolve() on a path that may not exist yet. The threat model here is lexical `..` traversal, which normpath handles without ever touching the filesystem. All 13 repository_paths/path-escape tests plus the full suite (541) still pass locally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enterprise audit-driven hardening pass across Critical/High/Medium priority findings from
docs/adr-toolkit-audit-report.md, plus supply-chain release attestation and this session's own ADR documentation (ADR-0012..0016).create/exception/supersede(race-condition fix, verified with concurrency + fork/SIGKILL chaos tests)core/contracts.py, 16/16 commands) + a scopedmypy --strictCI gate--dir/--rootpath-escape guard, consistently caught at all 7 call sitesscripts/sync_version.pyTest plan
python3 -m pytest tests/unit tests/integration -q— 541 passedmypy --strictoveratomic_io/telemetry/contracts— cleanpython3 scripts/sync_version.py --check— cleanpython3 skills/adr-toolkit/scripts/adr.py validate --dir docs/decisions— 16/16 cleanpython3 scripts/verify_examples.py— no drift🤖 Generated with Claude Code