feat(custody): private-vault — git-tracked ciphertext custody for high-value private artifacts - #2141
feat(custody): private-vault — git-tracked ciphertext custody for high-value private artifacts#21414444J99 wants to merge 3 commits into
Conversation
…h-value private artifacts .gitignore is a secrecy mechanism, not a custody mechanism: the 2026-08-08 professional-positioning research dossier (ten research dispatches of paid model spend, the controlling input to a live strategy engagement) existed only as a gitignored file plus a session-state orphan — one host, one device, no remote, no receipts. The estate already measured this shape at the 2026-07-27 evacuation and built the CUSTODY axis for corpora roots; this is the same answer at document granularity. Mechanism (scripts/private-vault.py): - add: encrypt to the committed GPG pubkey (docs/keys/anthony-padavano-gpg.asc, Anthony's key, private half on his hardware only) → institutio/vault/<sha16>-<slug>.gpg + manifest row (public-safe metadata only). Idempotent on plaintext sha. - verify (gate 'private-vault', pr-gate): every manifest row's ciphertext exists, sha-matches, and is git-tracked; NO plaintext source tracked (leak check); no unmanifested ciphertext. Exit 0 ⟺ custodied. - restore: decrypt + plaintext-sha verification (requires his private key). - Isolated GNUPGHOME per run — no host keyring dependency; no new secret minted; ciphertext rides ordinary git replication to every clone/mirror. Vaulted now (3 rows): the positioning research dossier + the engagement's decision memo and strategy packet (previously session-state-only). Verified: encrypt→decrypt roundtrip proven with a scratch keypair; restore against the real key correctly demands the private key; verify exit 0; scoped verification cheap wave PASS incl. check-gates (113 gates). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Multi-agent review roll call (CodeRabbit and Claude review automatically. Reviewers: post substantive findings only. Authors/agents: address every thread, push fixes to this branch, reply and resolve, then re-request review.) |
|
To use Codex here, create a Codex account and connect to github. |
📝 WalkthroughWalkthroughThe change adds a GPG-encrypted private vault CLI, four manifest records, ciphertext verification, restoration and listing commands, focused tests, and a governance gate that runs vault verification. ChangesPrivate vault custody
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant private_vault_cli
participant isolated_gpg
participant manifest_jsonl
Operator->>private_vault_cli: run add
private_vault_cli->>isolated_gpg: import committed public key
private_vault_cli->>isolated_gpg: encrypt validated envelope
isolated_gpg-->>private_vault_cli: return ciphertext
private_vault_cli->>manifest_jsonl: atomically record metadata
Operator->>private_vault_cli: run verify
private_vault_cli->>manifest_jsonl: load and validate records
private_vault_cli-->>Operator: return verification status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4444J99
added
the
lifecycle:delivery
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/private-vault.py`:
- Around line 82-89: Update _read_manifest() and the cmd_verify() verification
path so a missing manifest or a manifest with no non-empty records causes
verification to fail with a nonzero result, rather than being treated as an
empty valid vault. Preserve normal parsing and successful verification for
non-empty manifests.
- Around line 176-184: The leak check must validate content hashes rather than
only manifest source paths. Update the verification logic around the
tracked_set/source_path check to collect or compute SHA-256 hashes for every
Git-tracked blob and fail when any matches a manifest plaintext_sha256,
including rows whose source_path is outside ROOT; retain the existing failure
reporting context where applicable.
- Line 138: Remove source_path from the manifest records produced by the
private-vault manifest writer, and update leak verification to validate content
rather than local filesystem paths. In institutio/vault/manifest.jsonl lines
1-3, delete source_path from every existing record and regenerate the rows
without host-specific metadata.
- Around line 69-79: Centralize all external command execution used by
_import_pubkey and the other subprocess call sites into a shared runner that
emits start and finish receipts, enforces a finite timeout, and bounds captured
stdout/stderr diagnostics. Convert FileNotFoundError and timeout failures into
the documented SKIP path without emitting OK, while preserving existing
command-specific failure handling and status reporting.
- Around line 93-148: The add and restore commands mutate the filesystem without
an --apply confirmation gate. Update cmd_add to require args.apply before
creating VAULT_DIR, writing cipher_path, or appending MANIFEST, and update
cmd_restore to require it before creating dest_dir or writing restored
plaintext; preserve non-mutating behavior when the flag is absent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3042ffe1-5d67-4be8-bc69-a7dc6360f2ee
📒 Files selected for processing (6)
institutio/governance/gates.yamlinstitutio/vault/5c1cd7bd2371ed9a-positioning-strategy-packet-2026-08-09.gpginstitutio/vault/80d1e4b1bee2e4be-positioning-research-dossier-2026-08-08.gpginstitutio/vault/c366315688fc3731-positioning-decision-memo-2026-08-09.gpginstitutio/vault/manifest.jsonlscripts/private-vault.py
| def _import_pubkey(gnupghome: str) -> None: | ||
| if not PUBKEY.exists(): | ||
| sys.exit(f"FAIL: committed public key missing: {PUBKEY}") | ||
| run = subprocess.run( | ||
| ["gpg", "--batch", "--import", str(PUBKEY)], | ||
| env=_gpg_env(gnupghome), | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if run.returncode != 0: | ||
| sys.exit(f"FAIL: pubkey import: {run.stderr.strip()}") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound external commands and handle missing executables.
Each subprocess.run() has no timeout. gpg or git can block the command indefinitely. A missing executable raises FileNotFoundError and bypasses a final receipt. capture_output=True also has no output ceiling.
Centralize command execution. Set a finite timeout, cap captured diagnostics, emit start and finish receipts, and convert missing dependencies and timeouts into the repository’s documented fail-open SKIP behavior without printing OK.
Also applies to: 111-128, 157-161, 207-211
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 71-76: Command coming from incoming request
Context: subprocess.run(
["gpg", "--batch", "--import", str(PUBKEY)],
env=_gpg_env(gnupghome),
capture_output=True,
text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/private-vault.py` around lines 69 - 79, Centralize all external
command execution used by _import_pubkey and the other subprocess call sites
into a shared runner that emits start and finish receipts, enforces a finite
timeout, and bounds captured stdout/stderr diagnostics. Convert
FileNotFoundError and timeout failures into the documented SKIP path without
emitting OK, while preserving existing command-specific failure handling and
status reporting.
Sources: Coding guidelines, Path instructions
| def _read_manifest() -> list[dict]: | ||
| if not MANIFEST.exists(): | ||
| return [] | ||
| rows = [] | ||
| for line in MANIFEST.read_text().splitlines(): | ||
| line = line.strip() | ||
| if line: | ||
| rows.append(json.loads(line)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail when the required vault manifest is absent or empty.
If institutio/vault/ and manifest.jsonl are deleted together, _read_manifest() returns an empty list and cmd_verify() returns 0. The private-vault gate then reports successful custody after all three required artifacts are removed.
Make a missing or empty manifest a failure for this repository.
Proposed fix
def cmd_verify(_args: argparse.Namespace) -> int:
+ if not MANIFEST.is_file():
+ print("FAIL: private-vault manifest missing")
+ return 1
rows = _read_manifest()
if not rows:
- print("OK: vault empty (0 rows)")
- return 0
+ print("FAIL: private-vault manifest has no rows")
+ return 1Also applies to: 151-155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/private-vault.py` around lines 82 - 89, Update _read_manifest() and
the cmd_verify() verification path so a missing manifest or a manifest with no
non-empty records causes verification to fail with a nonzero result, rather than
being treated as an empty valid vault. Preserve normal parsing and successful
verification for non-empty manifests.
| def cmd_add(args: argparse.Namespace) -> int: | ||
| src = Path(args.file).expanduser().resolve() | ||
| if not src.is_file(): | ||
| sys.exit(f"FAIL: not a file: {src}") | ||
| plain_sha = _sha256(src) | ||
| rows = _read_manifest() | ||
| for row in rows: | ||
| if row["plaintext_sha256"] == plain_sha: | ||
| print(f"OK: already vaulted as {row['ciphertext']} ({row['slug']})") | ||
| return 0 | ||
| slug = args.slug or src.stem | ||
| cipher_name = f"{plain_sha[:16]}-{slug}.gpg" | ||
| VAULT_DIR.mkdir(parents=True, exist_ok=True) | ||
| cipher_path = VAULT_DIR / cipher_name | ||
|
|
||
| with tempfile.TemporaryDirectory() as gnupghome: | ||
| os.chmod(gnupghome, 0o700) | ||
| _import_pubkey(gnupghome) | ||
| run = subprocess.run( | ||
| [ | ||
| "gpg", | ||
| "--batch", | ||
| "--yes", | ||
| "--trust-model", | ||
| "always", | ||
| "--recipient", | ||
| FINGERPRINT, | ||
| "--output", | ||
| str(cipher_path), | ||
| "--encrypt", | ||
| str(src), | ||
| ], | ||
| env=_gpg_env(gnupghome), | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if run.returncode != 0 or not cipher_path.exists(): | ||
| sys.exit(f"FAIL: encrypt: {run.stderr.strip()}") | ||
|
|
||
| row = { | ||
| "slug": slug, | ||
| "ciphertext": cipher_name, | ||
| "ciphertext_sha256": _sha256(cipher_path), | ||
| "plaintext_sha256": plain_sha, | ||
| "plaintext_bytes": src.stat().st_size, | ||
| "source_path": str(src), | ||
| "description": args.description or "", | ||
| "recipient_fpr": FINGERPRINT, | ||
| "vaulted_at": datetime.now(timezone.utc).isoformat(timespec="seconds"), | ||
| } | ||
| with MANIFEST.open("a") as fh: | ||
| fh.write(json.dumps(row, sort_keys=True) + "\n") | ||
| print(f"OK: vaulted {src.name} -> institutio/vault/{cipher_name}") | ||
| print(f" plaintext sha256 {plain_sha}") | ||
| print(" next: git add the ciphertext + manifest; plaintext stays untracked") | ||
| return 0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require --apply before writing ciphertext or plaintext.
add writes ciphertext and the manifest immediately. restore creates a destination directory and writes plaintext immediately. The scripts/** policy requires an --apply gate before mutation.
Require --apply before cmd_add() writes cipher_path or MANIFEST. Require it before cmd_restore() creates dest_dir or writes restored plaintext.
Also applies to: 197-217
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 110-127: Command coming from incoming request
Context: subprocess.run(
[
"gpg",
"--batch",
"--yes",
"--trust-model",
"always",
"--recipient",
FINGERPRINT,
"--output",
str(cipher_path),
"--encrypt",
str(src),
],
env=_gpg_env(gnupghome),
capture_output=True,
text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[info] 143-143: use jsonify instead of json.dumps for JSON output
Context: json.dumps(row, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/private-vault.py` around lines 93 - 148, The add and restore commands
mutate the filesystem without an --apply confirmation gate. Update cmd_add to
require args.apply before creating VAULT_DIR, writing cipher_path, or appending
MANIFEST, and update cmd_restore to require it before creating dest_dir or
writing restored plaintext; preserve non-mutating behavior when the flag is
absent.
Sources: Coding guidelines, Path instructions
| # leak check: the plaintext source must never be tracked | ||
| src = row.get("source_path", "") | ||
| if src: | ||
| try: | ||
| rel_src = Path(src).resolve().relative_to(ROOT).as_posix() | ||
| if rel_src in tracked_set: | ||
| failures.append(f"PLAINTEXT TRACKED (leak): {rel_src}") | ||
| except ValueError: | ||
| pass # plaintext outside repo — fine |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Check tracked content hashes, not only the recorded source path.
The leak check only tests source_path when that path is under ROOT. Manifest rows 2 and 3 use external source paths, so this branch skips them. A tracked copy of any vaulted plaintext at another repository path also passes verification.
Compare each manifest plaintext_sha256 with every Git-tracked blob. Fail when any tracked blob matches a vaulted plaintext hash.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/private-vault.py` around lines 176 - 184, The leak check must
validate content hashes rather than only manifest source paths. Update the
verification logic around the tracked_set/source_path check to collect or
compute SHA-256 hashes for every Git-tracked blob and fail when any matches a
manifest plaintext_sha256, including rows whose source_path is outside ROOT;
retain the existing failure reporting context where applicable.
There was a problem hiding this comment.
Pull request overview
Introduces a “private vault” mechanism to keep high-value private artifacts under durable custody by committing only ciphertext to the repo (plus a public-safe manifest), while keeping plaintext out of git. This adds a dedicated verification gate to ensure ciphertext/manifest coherence and detect plaintext tracking leaks.
Changes:
- Added
scripts/private-vault.pyCLI withadd,verify,restore, andlistcommands for ciphertext custody. - Added an initial
institutio/vault/manifest.jsonlwith 3 vaulted entries’ metadata. - Registered a new
private-vaultgate ininstitutio/governance/gates.yamlto enforce custody/leak checks inpr-gate.
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| scripts/private-vault.py | New CLI tool implementing encryption, verification (custody + leak check), restore, and listing for vaulted artifacts. |
| institutio/vault/manifest.jsonl | Adds initial vault manifest rows for three artifacts (ciphertext metadata). |
| institutio/governance/gates.yaml | Adds private-vault gate to run private-vault verify on relevant path changes. |
Suppressed comments (1)
scripts/private-vault.py:201
restoreallows running with neither--slugnor--all, which currently produces a confusing error (slug 'None'). Alsorestore --allagainst an empty manifest reports the same misleading message. Fail closed with a clear error when the vault is empty or when no target is specified.
def cmd_restore(args: argparse.Namespace) -> int:
rows = _read_manifest()
targets = rows if args.all else [r for r in rows if r["slug"] == args.slug]
if not targets:
sys.exit(f"FAIL: no vault entry with slug '{args.slug}' (use list)")
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| slug = args.slug or src.stem | ||
| cipher_name = f"{plain_sha[:16]}-{slug}.gpg" | ||
| VAULT_DIR.mkdir(parents=True, exist_ok=True) | ||
| cipher_path = VAULT_DIR / cipher_name | ||
|
|
| row = { | ||
| "slug": slug, | ||
| "ciphertext": cipher_name, | ||
| "ciphertext_sha256": _sha256(cipher_path), | ||
| "plaintext_sha256": plain_sha, | ||
| "plaintext_bytes": src.stat().st_size, | ||
| "source_path": str(src), | ||
| "description": args.description or "", | ||
| "recipient_fpr": FINGERPRINT, |
| # leak check: the plaintext source must never be tracked | ||
| src = row.get("source_path", "") | ||
| if src: | ||
| try: | ||
| rel_src = Path(src).resolve().relative_to(ROOT).as_posix() | ||
| if rel_src in tracked_set: | ||
| failures.append(f"PLAINTEXT TRACKED (leak): {rel_src}") | ||
| except ValueError: | ||
| pass # plaintext outside repo — fine |
| {"ciphertext": "80d1e4b1bee2e4be-positioning-research-dossier-2026-08-08.gpg", "ciphertext_sha256": "730d8c976d924ae4c34fe35e22ad16ff87f09ec8b5a857c579d7b5539238562a", "description": "10-dispatch professional-positioning research dossier (paid research spend); controlling input to the 2026-08 positioning engagement", "plaintext_bytes": 54036, "plaintext_sha256": "80d1e4b1bee2e4be746ef1dfe06887c2ed71f3b7e621eeeb6ec46ca19ab1cfae", "recipient_fpr": "205A566A5FFE43D2E28E05A4C5B98FFAF8ED000E", "slug": "positioning-research-dossier-2026-08-08", "source_path": "/Users/4jp/Workspace/limen/.limen-private/reports/positioning-research-dossier-2026-08-08.md", "vaulted_at": "2026-08-09T09:56:47+00:00"} | ||
| {"ciphertext": "c366315688fc3731-positioning-decision-memo-2026-08-09.gpg", "ciphertext_sha256": "3e83ad5983f0ec8779e8b4adf40bef86b86c3758119b4558f4d3695466c401f0", "description": "Strategic positioning decision memo: chosen category, 9-candidate scoring, doors kept open", "plaintext_bytes": 4712, "plaintext_sha256": "c366315688fc373171b69efe44f2e42a3df061d03bcae94ae3d2b070ebfc3a25", "recipient_fpr": "205A566A5FFE43D2E28E05A4C5B98FFAF8ED000E", "slug": "positioning-decision-memo-2026-08-09", "source_path": "/Users/4jp/.copilot/session-state/d53ec957-c8eb-4e9f-a345-6b30699bc263/files/decision-memo.md", "vaulted_at": "2026-08-09T09:56:47+00:00"} | ||
| {"ciphertext": "5c1cd7bd2371ed9a-positioning-strategy-packet-2026-08-09.gpg", "ciphertext_sha256": "74d8ffd50b9d069af1a679b8b9a6109f89cb4ab8930baa152040d4aa60a05439", "description": "Private strategy packet: copy system, offer ladder w/ internal anchors, 90-day inbound, handoff scorecard, conversation playbook, 12-mo roadmap", "plaintext_bytes": 15097, "plaintext_sha256": "5c1cd7bd2371ed9afa743c521aadf20db300685a36018cf721d60c7cd3fd14d3", "recipient_fpr": "205A566A5FFE43D2E28E05A4C5B98FFAF8ED000E", "slug": "positioning-strategy-packet-2026-08-09", "source_path": "/Users/4jp/.copilot/session-state/d53ec957-c8eb-4e9f-a345-6b30699bc263/files/strategy-packet.md", "vaulted_at": "2026-08-09T09:56:47+00:00"} |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2cc5ae07f
ℹ️ 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".
| note: "A governed measurement document is written only by its keeper: every commit touching it is a keeper ship (A), every production toucher is declared (B), every committed row is a distinct correctly-ordered census (C). tasks.yaml had this via task-writer-audit.py; the debt ledger had a convention and nothing enforcing it — five of its six observations rode in as passengers on unrelated feature PRs. Registry: institutio/governance/ledger-custody.yaml; adding a governed document is one entry." | ||
| private-vault: | ||
| command: "python3 scripts/private-vault.py verify" | ||
| paths: ["institutio/vault/**", "scripts/private-vault.py", "docs/keys/anthony-padavano-gpg.asc"] |
There was a problem hiding this comment.
When a PR deletes the manifest and all .gpg files, this path rule does not select the new gate: scripts/verify.py:179 removes deleted paths from changed_set() because they neither exist nor remain in git ls-files, while both the PR and merge-group workflows invoke verify.py --changed. Consequently, complete removal of the vault can merge without running its custody predicate; deletion paths must remain eligible for gate selection.
AGENTS.md reference: AGENTS.md:L160-L165
Useful? React with 👍 / 👎.
| if not rows: | ||
| print("OK: vault empty (0 rows)") | ||
| return 0 |
There was a problem hiding this comment.
If the manifest and ciphertexts are deleted together, a direct or whole-matrix invocation reaches this branch and reports success with zero rows and no stray ciphertext. Thus even after deletion-trigger selection is fixed, the predicate still certifies loss of all three custody artifacts; retain a monotonic baseline or require an explicit governed removal receipt instead of accepting an empty vault.
AGENTS.md reference: AGENTS.md:L160-L165
Useful? React with 👍 / 👎.
| "ciphertext_sha256": _sha256(cipher_path), | ||
| "plaintext_sha256": plain_sha, | ||
| "plaintext_bytes": src.stat().st_size, | ||
| "source_path": str(src), |
There was a problem hiding this comment.
For repository-local plaintext, persisting the resolved absolute path binds the leak check to the machine that ran add. The first committed row records /Users/4jp/Workspace/limen/...; in CI or another clone, relative_to(ROOT) raises ValueError and lines 183–184 treat the source as safely outside the repository, so force-adding that exact plaintext at .limen-private/reports/... passes the advertised no-plaintext gate. Store an in-repo relative path, or otherwise remap it against the current checkout before checking tracked files.
AGENTS.md reference: AGENTS.md:L481-L484
Useful? React with 👍 / 👎.
| "slug": slug, | ||
| "ciphertext": cipher_name, | ||
| "ciphertext_sha256": _sha256(cipher_path), | ||
| "plaintext_sha256": plain_sha, |
There was a problem hiding this comment.
Every vault row commits the complete SHA-256 of its private plaintext, so the tracked ledger exposes a durable confirmation oracle for anyone who obtains or guesses a candidate document, even though the repository protocol explicitly requires full hashes to remain in the private corpus. Keep the verification digest inside encrypted metadata or another private owner rather than replicating it to every clone.
AGENTS.md reference: AGENTS.md:L262-L264
Useful? React with 👍 / 👎.
| if _sha256(path) != row["ciphertext_sha256"]: | ||
| failures.append(f"ciphertext sha mismatch: {name}") |
There was a problem hiding this comment.
If a ciphertext is replaced with random bytes or content encrypted to another key and its manifest hash is updated, this verifier still reports custody because it checks only self-consistency against the same editable row; it never validates the OpenPGP packet recipient, uses recipient_fpr, or proves that the actual private key can restore the plaintext. Require recipient validation plus a content-pinned restore receipt from the real key before treating these irreplaceable artifacts as custodied.
AGENTS.md reference: AGENTS.md:L284-L287
Useful? React with 👍 / 👎.
| cipher_path = VAULT_DIR / row["ciphertext"] | ||
| out = dest_dir / row["ciphertext"].removesuffix(".gpg") | ||
| run = subprocess.run( | ||
| ["gpg", "--batch", "--yes", "--output", str(out), "--decrypt", str(cipher_path)], |
There was a problem hiding this comment.
On a multi-user host, this command exposes the restored private artifacts to other local accounts because neither the destination directory nor the output file receives a restrictive mode. In a focused probe with the installed GnuPG 2.4.4, whose help describes --output FILE as “write output to FILE,” the default umask 022 produced a 0644 plaintext file, while mkdir likewise creates a traversable 0755 destination; create the directory as 0700 and force each restored file to 0600.
Useful? React with 👍 / 👎.
| src = Path(args.file).expanduser().resolve() | ||
| if not src.is_file(): | ||
| sys.exit(f"FAIL: not a file: {src}") | ||
| plain_sha = _sha256(src) |
There was a problem hiding this comment.
When another process modifies a source file after this hash is computed but before or during the later GPG invocation, the manifest records the old plaintext hash while the ciphertext contains the new bytes (and the size is sampled yet again afterward). The ciphertext hash remains internally consistent, so verify passes, but every subsequent restore fails its plaintext check; copy the source to an immutable temporary snapshot and derive the hash, size, and ciphertext from that single snapshot.
Useful? React with 👍 / 👎.
| if _sha256(out) != row["plaintext_sha256"]: | ||
| sys.exit(f"FAIL: restored plaintext sha mismatch for {row['slug']}") |
There was a problem hiding this comment.
When decryption succeeds but the expected plaintext hash is wrong, this failure exits after GPG has already written the unverified bytes directly to the final destination, leaving a file behind despite reporting that restoration failed. Decrypt into a restricted temporary file, verify its digest, and atomically move it to out only on success so failed restores cannot contaminate the destination.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e60320c9f3
ℹ️ 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".
| envelope = _temporary_file(destination_root, artifact_id, ".envelope") | ||
| plaintext = _temporary_file(destination_root, artifact_id, ".plaintext") | ||
| try: | ||
| _decrypt_file(cipher_path, envelope) |
There was a problem hiding this comment.
When restore is run without a separate prior verify, this path decrypts the file without comparing it to the manifest's ciphertext_sha256 or ciphertext_bytes. Because the encryption key is public, an attacker can replace a ciphertext with a newly encrypted envelope containing the same artifact ID and a self-consistent inner hash; restoration will then publish arbitrary substituted content while reporting that the encrypted hash was verified. Enforce the manifest's pinned ciphertext digest and size before decryption.
AGENTS.md reference: AGENTS.md:L284-L287
Useful? React with 👍 / 👎.
| if row["artifact_id"] == artifact_id: | ||
| print(f"OK: {artifact_id} is already vaulted as {row['ciphertext']}") | ||
| return 0 |
There was a problem hiding this comment.
When an operator edits a dossier and reruns add with its existing artifact ID, this branch prints OK and exits without establishing that the supplied file matches the vaulted plaintext. Since v2 deliberately keeps the plaintext digest out of the public manifest, even a completely different input receives this success result and remains uncustodied; reject reused IDs unless content equality can actually be proven.
Useful? React with 👍 / 👎.
| relative = path.relative_to(ROOT).as_posix() | ||
| if relative not in tracked: | ||
| failures.append(f"ciphertext not git-tracked (custody gap): {relative}") |
There was a problem hiding this comment.
If a plaintext source is force-added after it has been vaulted, verify still passes because the tracked-file set is used only to check the manifest and ciphertext paths. The fresh v2 implementation removes source_path from the manifest and contains no replacement plaintext check, so the earlier path-remapping issue has become a complete absence of the gate advertised by this registry row; the exact private source can now be committed alongside its ciphertext without failing custody verification.
Useful? React with 👍 / 👎.
| command: "python3 scripts/private-vault.py verify" | ||
| paths: ["institutio/vault/**", "scripts/private-vault.py", "docs/keys/anthony-padavano-gpg.asc"] |
There was a problem hiding this comment.
In the checked gate registry, this command only runs private-vault.py verify against the committed vault; it never executes cli/tests/test_private_vault.py. The current PR happens to select pytest-cli because it adds a file under cli/**, but a future change limited to scripts/private-vault.py will select this gate without exercising the add, restore, traversal, cleanup, or permission contracts, allowing those regressions through scoped verification. Add the focused test file to this gate command or include the script path in a test gate.
AGENTS.md reference: AGENTS.md:L142-L149
Useful? React with 👍 / 👎.
| for stray in VAULT_DIR.iterdir() if VAULT_DIR.exists() else []: | ||
| if stray.is_file() and stray != MANIFEST and stray.suffix != ".gpg": | ||
| failures.append(f"unsupported vault file: {stray.name}") |
There was a problem hiding this comment.
If a ciphertext is placed below a nested directory such as institutio/vault/import/artifact.gpg, verification reports success: glob("*.gpg") scans only direct children, while this second pass silently skips the containing directory because it checks only stray.is_file(). The advertised no-unmanifested-ciphertext invariant therefore misses an entire class of vault contents; scan recursively or reject every directory under the vault root.
Useful? React with 👍 / 👎.
| try: | ||
| _write_manifest([*rows, row]) |
There was a problem hiding this comment.
When two add commands run concurrently with different artifact IDs, both can read the same original rows before either reaches this replacement. Each then writes its own stale [*rows, row] snapshot and reports success, but the last replacement drops the other new row while both ciphertext files remain, leaving one supposedly vaulted artifact unmanifested. Hold a lock across the manifest read, ciphertext publication, and manifest update, or use a compare-and-swap retry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cli/tests/test_private_vault.py (1)
99-113: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest the private-key failure path.
corrupt_decryptsucceeds and then causes an envelope integrity failure. It does not test a failure from_decrypt_filebefore envelope extraction. Add a test where_decrypt_fileraisesVaultError, then assert that no temporary or restored plaintext remains. This verifies the private-key failure behavior stated in the PR objective.Proposed test
+def test_failed_restore_cleans_up_after_decrypt_failure(vault, monkeypatch: pytest.MonkeyPatch, tmp_path: Path): + source = tmp_path / "private.md" + source.write_text("secret", encoding="utf-8") + _add(vault, source) + destination = tmp_path / "restore" + + def reject_decrypt(_ciphertext: Path, _envelope: Path) -> None: + raise vault.VaultError("gpg decryption failed: no secret key") + + monkeypatch.setattr(vault, "_decrypt_file", reject_decrypt) + with pytest.raises(vault.VaultError): + vault.cmd_restore(SimpleNamespace(all=False, artifact_id="artifact-001", dest=str(destination))) + assert list(destination.iterdir()) == []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/tests/test_private_vault.py` around lines 99 - 113, Add a separate test alongside test_failed_restore_removes_all_plaintext_temporaries that monkeypatches vault._decrypt_file to raise VaultError before extraction, invokes vault.cmd_restore with the same restore setup, and asserts the command raises VaultError and leaves the destination empty with no temporary or restored plaintext files.scripts/private-vault.py (1)
133-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider excluding manifest temporaries from the stray-file rule.
_write_manifestcreates.manifest.*temporaries insideVAULT_DIR. If the process is killed betweenmkstempandos.replace, the temporary stays.cmd_verifyat Line 360-362 then reportsunsupported vault file, and theprivate-vaultgate fails until an operator removes the file by hand.Either write the temporary outside
VAULT_DIRon the same filesystem, or letcmd_verifyignore names that start with.manifest..🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/private-vault.py` around lines 133 - 146, Update _write_manifest and cmd_verify so interrupted .manifest.* temporary files do not cause private-vault verification to fail: either create the temporary outside VAULT_DIR on the same filesystem or explicitly exclude filenames beginning with .manifest. from stray-file validation. Preserve verification for all other unsupported vault files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/private-vault.py`:
- Around line 393-394: Update the restore-directory creation in the destination
setup to enforce owner-only permissions (0o700), rather than relying on the
process umask. Ensure this applies when creating the default ~/.limen-restore
directory while preserving the existing parent creation and idempotent behavior.
---
Nitpick comments:
In `@cli/tests/test_private_vault.py`:
- Around line 99-113: Add a separate test alongside
test_failed_restore_removes_all_plaintext_temporaries that monkeypatches
vault._decrypt_file to raise VaultError before extraction, invokes
vault.cmd_restore with the same restore setup, and asserts the command raises
VaultError and leaves the destination empty with no temporary or restored
plaintext files.
In `@scripts/private-vault.py`:
- Around line 133-146: Update _write_manifest and cmd_verify so interrupted
.manifest.* temporary files do not cause private-vault verification to fail:
either create the temporary outside VAULT_DIR on the same filesystem or
explicitly exclude filenames beginning with .manifest. from stray-file
validation. Preserve verification for all other unsupported vault files.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85b89dfd-3f01-4a66-820d-5bbd2ff1c16a
📒 Files selected for processing (7)
cli/tests/test_private_vault.pyinstitutio/vault/artifact-001.gpginstitutio/vault/artifact-002.gpginstitutio/vault/artifact-003.gpginstitutio/vault/manifest.jsonlinstitutio/vault/styx-effort-brief-20260809.gpgscripts/private-vault.py
🚧 Files skipped from review as they are similar to previous changes (1)
- institutio/vault/manifest.jsonl
| destination_root = Path(args.dest).expanduser().resolve() | ||
| destination_root.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Create the restore directory with owner-only permissions.
mkdir(parents=True, exist_ok=True) uses the process umask, so the default ~/.limen-restore is typically 0o755. Restored files are 0o600, but the directory listing exposes artifact_id--original_name pairs to other local accounts. The envelope design keeps original filenames private, so this leaks the metadata the vault protects.
🔒 Proposed fix
- destination_root.mkdir(parents=True, exist_ok=True)
+ destination_root.mkdir(parents=True, exist_ok=True, mode=0o700)
+ os.chmod(destination_root, 0o700)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| destination_root = Path(args.dest).expanduser().resolve() | |
| destination_root.mkdir(parents=True, exist_ok=True) | |
| destination_root = Path(args.dest).expanduser().resolve() | |
| destination_root.mkdir(parents=True, exist_ok=True, mode=0o700) | |
| os.chmod(destination_root, 0o700) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/private-vault.py` around lines 393 - 394, Update the
restore-directory creation in the destination setup to enforce owner-only
permissions (0o700), rather than relying on the process umask. Ensure this
applies when creating the default ~/.limen-restore directory while preserving
the existing parent creation and idempotent behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccc87f0f47
ℹ️ 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".
| restore.add_argument("--artifact-id", help="neutral id to restore") | ||
| restore.add_argument("--all", action="store_true") |
There was a problem hiding this comment.
When a caller supplies both restore selectors, argparse accepts them and cmd_restore takes the args.all branch, silently ignoring the requested artifact ID and decrypting every vault entry into the destination. The generated restore --help confirms these are currently independent options; make them a mutually exclusive argument group so a contradictory invocation cannot publish more private artifacts than intended.
Useful? React with 👍 / 👎.
| _write_envelope(source, artifact_id, envelope) | ||
| _encrypt_file(envelope, temporary_cipher) | ||
| os.chmod(temporary_cipher, 0o644) | ||
| os.replace(temporary_cipher, cipher_path) |
There was a problem hiding this comment.
When the checkout is on an external SSD or other mount while the default temporary directory is on the system filesystem, this os.replace raises EXDEV (Invalid cross-device link), so every add attempt terminates with an uncaught traceback and creates no custody record. Stage the ciphertext inside VAULT_DIR before performing the atomic replacement, rather than creating it in the system temporary directory.
AGENTS.md reference: AGENTS.md:L284-L288
Useful? React with 👍 / 👎.
| try: | ||
| _decrypt_file(cipher_path, envelope) | ||
| original_name = _extract_envelope(envelope, artifact_id, plaintext) | ||
| final_name = f"{artifact_id}--{original_name}" |
There was a problem hiding this comment.
When a valid source basename is near the filesystem's NAME_MAX (255 bytes on the checked filesystem), prefixing it with a potentially 64-character artifact ID produces an invalid destination basename. A focused restore with a 240-character source name and a 64-character ID raises an uncaught OSError: [Errno 36] File name too long, so an artifact that add accepted cannot be restored through this tool; use a bounded or fallback output name while preserving the original name inside the encrypted metadata.
AGENTS.md reference: AGENTS.md:L286-L288
Useful? React with 👍 / 👎.
Why
The 2026-08-08 positioning research dossier — ten research dispatches of paid model spend, the controlling input to a live strategy engagement — existed only as a gitignored file plus a Copilot session-state orphan. One host, one device, no remote, no custody receipts.
.gitignoreis a secrecy mechanism, not a custody mechanism: an ignored file is exactly the file every clone, mirror, and evacuation sweep skips. The estate measured this shape at the 2026-07-27 evacuation and built the CUSTODY axis for corpora roots; this PR is the same answer at document granularity — too small for an archive-class root, too valuable for /tmp.Mechanism
scripts/private-vault.py— ciphertext tracked, plaintext ignored:docs/keys/anthony-padavano-gpg.asc; private half on Anthony's hardware only) →institutio/vault/<sha16>-<slug>.gpg+ a manifest row carrying only public-safe metadata. Idempotent on plaintext sha. Isolated GNUPGHOME per run — no host-keyring dependency, no new secret minted.private-vault(pr-gate): every row's ciphertext exists, sha-matches, is git-tracked; no plaintext source tracked (leak check); no unmanifested ciphertext.Ciphertext rides ordinary git replication — every clone/mirror/evacuation copy carries it for free; decryption stays gated on the private key.
Vaulted now (3 rows)
The research dossier + the engagement's decision memo and strategy packet (both previously session-state-only, produced by the PR #2136 engagement).
Verification
private-vault verifyexit 0; leak check activescripts/verify-scoped.shcheap wave PASS incl.check-gates(113 gates, registry parity; thecli/**UNJUSTIFIED deploy-trigger note is pre-existing)Summary by CodeRabbit
New Features
Documentation
Bug Fixes