Skip to content

[bug fix] Harden project installation with manifests and portable checks - #351

Open
jarmo-blip wants to merge 3 commits into
buildermethods:mainfrom
jarmo-blip:agent/installer-hardening-only
Open

jarmo-blip wants to merge 3 commits into
buildermethods:mainfrom
jarmo-blip:agent/installer-hardening-only

Conversation

@jarmo-blip

@jarmo-blip jarmo-blip commented Sep 13, 2026

Copy link
Copy Markdown

Summary

The current installer searches profiles/<name>/standards even though the default profile is flat, so a default installation omits global/tech-stack.md. It also writes directly into project files without tracking ownership or protecting local edits.

This is the installer-hardening-only replacement requested in the review of #348. It installs flat profiles with inheritance, preserves supplied index.yml bytes and metadata, stages writes, commits files by atomic sibling rename with rollback, and records SHA-256 ownership in a validated project-relative manifest. Unmanaged or modified destinations require --force, which creates a unique backup. Doctor reports drift; uninstall removes only tracked files and preserves user changes unless explicitly forced.

The command destination remains .claude/commands/agent-os; --target none installs standards only. There is no generated Codex skill adapter, LLM router/optimizer, release preparation, command-content rewrite, or profile-schema migration in this PR. Existing profile indexes are opaque inputs; curated-skill installation remains a separate integration.

Linked item

Follow-up to the requested-changes review on #348: #348 (review)

Checklist

  • Linked to the related review requesting a smaller replacement PR
  • Documented steps to test below
  • Installer-specific usage documented in docs/installer.md
  • Backwards compatibility considered: existing profile/commands-only/verbose flags and Claude command paths retained; local overwrites now require explicit force with backup

Documented steps to test

  1. Run bash tests/installer.sh.
  2. Run shellcheck -x scripts/installer-common.sh scripts/project-install.sh scripts/doctor.sh scripts/uninstall.sh tests/installer.sh.
  3. On macOS, run /bin/bash tests/installer.sh; the harness invokes every installer script with the same interpreter.
  4. Install into a temporary project using scripts/project-install.sh --project-dir <dir> --yes, edit a tracked standard, and verify doctor fails and an unforced reinstall preserves the edit. Forced reinstall must preserve a backup.
  5. Run uninstall and verify unrelated files remain. The regression suite replaces a tracked parent directory with a symlink and proves doctor/uninstall abort before deleting any outside files.

Notes for reviewers

Validated head: b844c9399c518abd856ebc27a68ebd0be79691f2, based on upstream main at 475b0cac4c7c5cf2336ad5a663b691a6d3415e05.

  • Local functional tests: 39 passed, 0 failed. ShellCheck 0.11.0 and git diff --check pass.
  • Exact-head Linux/macOS CI: both jobs pass ShellCheck and all 39 tests; macOS explicitly uses system Bash 3.2 for the harness and every script under test.
  • Mandatory repo-auditor baseline of upstream main: no critical/high findings. Final diff-scoped --base <upstream SHA> --strict --sarif audit: 0 findings in the 7 changed files, gate passed. Semgrep/Gitleaks ran, target-code execution and LLM interpretation were disabled; the separate model review ran through cc-code.
  • Separate cc-code review with explicit deepseek:deepseek-chat: no remaining P1/P2 after fixes, including duplicate/malformed index structures and portable Bash 3.2/BSD awk behavior.

The workflow runs ShellCheck and functional tests on Linux and macOS. Hashing supports GNU sha256sum, macOS shasum, and openssl without realpath -m.

The manifest preserves ownership of previously installed files when they disappear from a newer profile or when only commands/standards are updated. Uninstall never recursively removes directories or follows symlink components.

@jarmo-blip

Copy link
Copy Markdown
Author

DeepSeek review

Reviewed the installer-only implementation through cc-code with the explicit router alias deepseek:deepseek-chat, then reviewed the final argument-guard/test-fixture diff again. Validated head: 273c85d4cc0fc14d4188e59e0887a2f1134ab13d.

No remaining identified P1/P2 findings.

Findings addressed before publication:

  • Doctor/uninstall preflight the manifest and all managed parent paths before any deletion, preventing a symlinked standards directory from redirecting operations outside the project. A real outside-tree regression was reproduced before the fix and passes afterward.
  • Commit files use unique mktemp siblings and atomic rename; rollback tracks created directories with counted arrays so spaces and literal pipes remain safe.
  • Profile inheritance matches profile keys exactly, including dotted names, rather than interpolating names into a regular expression.
  • SHA-256 hashes are computed on stdin and validated, preserving backslash filenames and failing closed on hash-tool errors. Empty, duplicate, malformed, and unterminated manifests are rejected before changes.
  • Generated index scalars quote YAML reserved words and numeric/date-like names. Supplied profile indexes remain byte-for-byte opaque inputs.
  • Tests invoke every script with the harness's $BASH, so the macOS job genuinely exercises system Bash 3.2. The test harness self-check proves intermediate assertion failures cannot be masked.

The colon-space backup parsing suspicion was dismissed after checking the real code and reproducing a: b.md forced backup: ${line#*: } strips the first kind delimiter correctly. That regression also passes.

Non-blocking maintainability suggestion: repeated argument guards are retained beside each option for clarity. Generated-index description reuse is explicitly limited to supported plain folder/name keys; arbitrary structured/quoted profile indexes are preserved rather than reinterpreted.

Validation: 37/37 functional tests, ShellCheck and whitespace checks pass locally. Exact-head CI passes on Linux Bash 5.2.21 and macOS system Bash 3.2.57, including ShellCheck and all 37 tests on each. Mandatory diff-scoped repo-auditor --base <upstream main SHA> --strict --sarif gate passes with 0 findings touching the seven changed files.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

https://github.com/buildermethods/agent-os/blob/273c85d4cc0fc14d4188e59e0887a2f1134ab13d/scripts/project-project-install.sh#L339-L343
P2 Badge Preserve descriptions for non-alphanumeric index keys

On a subsequent forced install after /index-standards has supplied custom descriptions, this parser recognizes only unquoted folder and standard keys containing [A-Za-z0-9_./-]. Keys with spaces, colons, hashes, reserved words, or numeric prefixes are emitted by this installer either as valid plain scalars or quoted scalars, but are not read back here, so their curated descriptions are silently replaced with the default placeholder.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +385 to +387
if [ "$first" -eq 1 ]; then
ico_die "empty manifest (missing header): $file"
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject manifests with no managed rows

When a manifest has the correct header but no data rows—for example, after truncation at a newline boundary—first is cleared and validation succeeds. Consequently, doctor.sh exits successfully with “all 0 managed file(s) match” even if installed files remain, and uninstall.sh removes the manifest while leaving those files behind. Track whether at least one valid data row was read and reject a header-only manifest.

Useful? React with 👍 / 👎.

@jarmo-blip

Copy link
Copy Markdown
Author

DeepSeek review — follow-up implementation

DeepSeek reviewed the follow-up diff at b844c9399c518abd856ebc27a68ebd0be79691f2 through cc-code.

The review found no remaining P1/P2 findings. The follow-up preserves descriptions for quoted and escaped standard-index keys and now fails closed before project mutation for duplicate folder/name keys, missing descriptions, unsupported YAML indicators, and tab/carriage-return corruption. The literal Bash lookup also preserves backslashes on Bash 3.2/macOS awk paths.

Validation:

  • bash tests/installer.sh: 39 passed, 0 failed
  • ShellCheck 0.11.0: clean
  • Exact-head Linux/macOS CI: both jobs passed; macOS used system Bash 3.2
  • Strict diff-scoped repo-auditor audit against upstream main: 0 findings in changed files; no critical/high findings

The PR remains installer-hardening-only. No adapters, router, optimizer, release workflow, or credentials are included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant