Repository navigation
feat: the credential gate, the credential store, and the commands around them - #2
Merged
Merged
Conversation
The approved strategy document argues for the design and sequences the rollout; it does not decompose the work. This is that decomposition, with the exact values each task must use and the tests each must carry. Global constraints are stated once at the top because they bind every task and are the things most easily lost in a single-task view: the dependency floor, the gate resolving no credentials, failing closed, printing locations rather than values, and holding no environment-specific knowledge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
One representation for a Pulumi `--path`-style config path, used by findings, manifest keys and the matcher alike: dotted segments, `[n]` for list indices (int), and `["..."]` for a key containing `.`, `[`, `]`, `"`, or the empty string. A literal `"` inside a bracketed key is escaped as `\"`. `parse` only ever produces an `int` from an unquoted `[n]` — a dotted or quoted segment is always `str`, which is what keeps `["1"]` (the string key "1") and `[1]` (index 1) from collapsing into each other on either side of the round trip. `render` rejects `bool` and negative int segments and an empty segment list, since letting any of those through would make the stated round-trip property false rather than merely undocumented. Standard library only, per the dependency constraint. ValueErrors from `parse` name the offending position, never the surrounding text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
render(["a." + "\\"]) round-tripped through a broken text: escaping only `"` left a trailing literal `\` indistinguishable, on decode, from the backslash that starts a `\"` escape. Any key quoted for an unrelated reason (a dot, a bracket, being empty) that also ended in one or more literal backslashes failed parse(render(y)) == y. - render: escape `\` as `\\` before escaping `"` as `\"` (order matters — escaping `"` first would double-escape a backslash just inserted). - parse: decode both `\\` and `\"` as one two-character escape pair; a `\` followed by anything else (including end of input) is now a named ValueError, not a passthrough — the previous 1-char lookahead couldn't otherwise tell a real escape from a lone trailing backslash. - `\` joins `.`, `[`, `]`, `"` in `_NEEDS_QUOTING`: once backslash is an escape prefix inside brackets, a bare/dotted key containing one has to go through the same quoted-and-escaped regime rather than meaning itself literally in one context and an escape prefix in another. - Also: reject a leading zero in an unquoted list index (`a[01]`). `render` never produces one; tolerating it on parse would give the same index two silently-equivalent spellings. Added regression coverage: a key with only a mid-string backslash and no other reason to quote, a key ending in one literal backslash, a key ending in two, an invalid-escape-sequence malformed-input case, and the leading-zero case. Fixed a SyntaxWarning from an unescaped backslash in the module docstring along the way. uv run pytest -q: 61 passed (50 in tests/test_paths.py). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Add src/stackward/config.py: parse and validate .stackward.toml with tomllib (stdlib only, no new dependency). Recognises profile, python, min_version, [check] and [secrets.*]/[secrets.source]. Unknown keys at the top level and within [check] are errors naming the key; [secrets.*] content is validated only as far as "a table of tables" since Task 8 owns its semantics. A present-but-invalid config always raises ConfigError, never falls back to defaults; an absent config is None, left for the caller to decide. [check].sensitive_keys EXTENDS the built-in sensitive-key set; [check].sensitive_parents REPLACES it. This asymmetry is deliberate, documented at _extend/_replace: a repo can only add key names to flag, but may need to fully opt a whole parent category back out of the built-ins, which extension alone can never express. The built-in sensitive_parents default is empty on purpose — the brief supplies a built-in list for sensitive_keys but not for parents, and inventing one would be exactly the environment-specific guessing this module exists to avoid. Flagged in the task report for the controller. min_version enforcement (enforce_min_version) compares versions as numeric tuples, not strings, so "0.10.0" > "0.9.0" compares correctly. It honours cli.VERSION_CHECK_EXEMPT (doctor, self-update) via a function-local import, which also breaks the import cycle: cli now imports find_repo_config from config, and config needs VERSION_CHECK_EXEMPT from cli. Move find_repo_config (and CONFIG_FILENAME) out of cli.py into config.py verbatim; cli.py now imports it. config_home() stays in cli.py per the task brief (Task 6 owns the credential store's paths). Move the three tests that exercise find_repo_config from tests/test_cli.py to the new tests/test_config.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Two Important findings from review of af6291d: 1. The extend-vs-replace asymmetry test coverage proved _extend/_replace differ in isolation, but nothing exercised the wiring at _build_check's call site. Because the shipped BUILTIN_SENSITIVE_PARENTS is empty, swapping _replace for _extend there would not have failed any existing test (replacing or extending an empty set is the same set). Added a load_config-level test that monkeypatches BUILTIN_SENSITIVE_PARENTS to a non-empty synthetic value and asserts a declared sensitive_parents list fully replaces it. Verified the fix actually catches the mutation it targets by temporarily swapping the call site to _extend, confirming the new test fails, then reverting. 2. stack_models values ("<ns>" = "module:Class") were accepted as any string; the brief states this shape explicitly (unlike [secrets.*]'s interior, which it leaves unspecified and Task 8 owns), so a typo like a dot instead of a colon loaded silently and would only have surfaced later as a confusing ImportError deep in Task 10. Added _require_module_class: exactly one ':' with non-empty module and class segments, raised as ConfigError naming the key. Four new tests: no colon, two colons, empty module, empty class. Also: dropped the dead isinstance(key, str) check on stack_models keys (TOML table keys are always strings); replaced "environment_variables" as an example sensitive_parents value in tests with a neutral placeholder, so no test reads as documenting an intended-but-shipped-empty default; corrected the test-count arithmetic in the task report. uv run pytest tests/test_config.py -v: 41 passed. uv run pytest -q (full suite): 99 passed, no warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Implements Task 3: the gate itself. src/stackward/nets/heuristic.py walks
an already-parsed YAML document (never a rendered path string) and reports
plaintext credentials as canonical paths via paths.render — a leaf is a
finding when its own key matches sensitive_keys or a strict ancestor key is
in sensitive_parents, unless its parent mapping is exactly {"secure": ...}
(tested by shape, never by a ".secure" path suffix), the value is None,
"", True or False (tested by identity/exact type, never a containment test
that would also swallow the integers 0 and 1), or its path is in
allowed_references.
find_plaintext_credentials(document, check) is content-level: it takes
parsed data plus policy, does no I/O, and imports nothing beyond ..config
and ..paths. src/stackward/commands/check_config.py adds scan_file as the
thin file-path wrapper for the CLI, so a later staged-blob caller (git show
":<path>") can call the net directly without round-tripping through a temp
file. check-config registered in cli.py; dispatch itself is untouched, so
doctor still runs with a malformed .stackward.toml (regression test added).
Exit codes: 0 clean, 1 a credential was found, 2 the check could not run
(bad path, invalid YAML, non-mapping document, unusable repo config) —
never conflated. Findings print as "<file>: plaintext credential at
'<path>'", sorted, never the value.
Tests: tests/test_heuristic.py covers every rule individually plus the
scan_file/check-config CLI contract; two additions to test_cli.py (wiring
smoke test, doctor-survives-malformed-config regression). 125 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Controller ruling: under the shipped default policy (sensitive_parents
ships empty by design), a sensitive key's own {"secure": ...} envelope
renamed the actual leaf's key to "secure"/"other", which matches no
built-in pattern. With no sensitive_parents ancestor declared either, the
leaf was never even considered sensitive, so _is_encrypted's careful
parent-shape check never ran — the malformed-envelope protection
(apiToken: {secure: X, other: Y}, other a plaintext leak) was unreachable
in exactly the policy every repository starts with.
Fix: a leaf is now a finding when its own key matches sensitive_keys, OR
any ancestor key matched sensitive_keys, OR any ancestor key is in
sensitive_parents. _walk threads a single ancestor_sensitive boolean down
the recursion (replacing the frozenset[str] ancestor-keys tracking used
only for sensitive_parents) that turns true once and is never cleared, so
a sensitive key's whole subtree stays sensitive without leaking sideways
to a sibling. _is_encrypted and _is_empty are unchanged.
Accepted cost, stated in the module docstring: a sensitive-keyed mapping
holding structured non-credential data now reports every leaf beneath it
(token_settings: {retries: 3} flags token_settings.retries) — a deliberate
false-positive-over-false-negative trade for a commit gate.
Five new covering tests in tests/test_heuristic.py, including the
regression test (apiToken: {secure: X, other: Y} under the default policy,
previously []) verified against a reproduction of the pre-ruling logic to
confirm it fails for the right reason. Full suite: 130 passed (125 + 5).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
exception exiting 1 Review findings, both verified empirically against the real CLI. Important 1: scan_file built its CheckError message from str(exc) on the raw yaml.YAMLError. MarkedYAMLError.__str__ calls Mark.__str__, which calls Mark.get_snippet() -- a slice of the actual source line around the error. A file containing "myproject:dbPassword: hunter2: not-valid-yaml" (an unescaped colon in a value) printed hunter2 to stderr. This is the one file in the whole system expected to hold a credential. Fixed with _describe_yaml_error, which reports only exc.problem plus exc.problem_mark.line/.column for a MarkedYAMLError, and a generic message otherwise -- never str(exc) or str(mark). Important 2: cmd_check_config's per-file loop caught only CheckError. Python's default exit code for an uncaught exception is 1, the code reserved for "a credential was found", so a file the checker never evaluated could report as a file with a credential in it. Two confirmed triggers: a self-referential YAML alias (_walk had no cycle guard and recursed forever -- confirmed RecursionError against the pre-fix logic) and a non-UTF-8 file (path.read_text() raises UnicodeDecodeError, not an OSError, uncaught). Fixed with (1) a visiting: frozenset[int] cycle guard threaded through _walk, tracking container id() along the current descent path (not a global seen-set, so two branches legitimately sharing one non-cyclic object via separate YAML anchors are each still scanned), and (2) a catch-all except Exception in cmd_check_config's per-file loop mapping anything else to exit 2, reporting only type(exc).__name__, never str(exc). Also fixed: a real finding was suppressed when a later file errored (cmd_check_config returned 2 before the findings-print loop) -- moved the print above the errored check, verified the old logic really did print nothing for good.yaml+broken.yaml. And the sideways-leak propagation test only discriminated by dict insertion order -- added a reversed-order variant. _is_encrypted, _is_empty, and the propagation logic are unchanged; the review confirmed them correct. 8 new tests in tests/test_heuristic.py. Full suite: 138 passed (130 + 8). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…ages Round-2 review finding: the round-1 fix's premise that exc.problem and problem_mark "carry no source text" was wrong. Several PyYAML scanner/parser messages interpolate one raw character or a user-chosen name via %r -- "found character %r that cannot start any token", "found unknown escape character %r", the "expected ... but found %r" family, and anchor/alias/tag-handle names in duplicate/undefined-alias errors. Reproduced: password: "hunter\2ok" yielded "found unknown escape character '2' (line 1, column 19)" -- one character of the credential. Fixed by redacting every '...'-quoted run in exc.problem (_QUOTED_FRAGMENT = re.compile(r"'[^']*'")) before interpolating it, per the reviewer's instruction not to whitelist specific PyYAML message shapes since they vary by version. Diagnostic value (error kind, line, column) survives; the credential character does not. Corrected _describe_yaml_error's docstring, which had claimed exc.problem carries no source text -- restated as the guarantee the code actually provides (built from PyYAML's description and position only, with quoted fragments redacted) and why str(exc) must never be used here. Two new tests in tests/test_heuristic.py, one per named message shape (bad escape, reserved leading character), each with a credential-shaped value, asserting the leaked character is absent while error kind and line/column survive -- verified against a reproduction of the pre-fix _describe_yaml_error to confirm they fail for the right reason. Cycle guard, catch-all, finding-print ordering, and the order-independent propagation test are unchanged, per the reviewer's explicit instruction -- all confirmed correct in this round. Full suite: 140 passed (138 + 2). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Round-3 review finding: both round-2 findings not addressed.
_QUOTED_FRAGMENT matched '[^']*' only. Python's repr() switches to double
quotes whenever the value contains ' and no " -- so when the character
PyYAML interpolates via %r is itself an apostrophe, the fragment is "'"',
not ''', and the single-quote-only pattern doesn't touch it. Reproduced:
password: "s3cr3t\'value" produced "found unknown escape character "'""
in the output -- an apostrophe from the credential's own content.
Fixed: _QUOTED_FRAGMENT = re.compile(r"'[^']*'|\"[^\"]*\"") -- redacts
both delimiter forms, per the reviewer's instruction not to whitelist
message shapes.
The deeper finding: the round-2 tests pinned one character each ('2',
'`') and both passed with the hole wide open, because neither ever
supplied an apostrophe. Replaced them with two tests parameterized over
five trigger characters (apostrophe, double quote, backslash, newline,
non-printable), one per PyYAML message family that interpolates a raw
character via %r, each constructing a real yaml.MarkedYAMLError and
calling the actual _describe_yaml_error directly -- testing double-quote
and backslash this way because both are themselves valid YAML escapes and
so cannot trigger "unknown escape character" through real parsing at all.
Kept a new real end-to-end regression test reproducing the reviewer's
exact repro through scan_file, alongside the round-2 real tests.
Verified the parameterized tests actually discriminate the two
implementations: reproduced the pre-round-3 (single-quote-only) regex
against all five characters -- only apostrophe leaks, confirming the
round-2 tests could never have caught this regardless of how many times
run.
Corrected _describe_yaml_error's docstring, which claimed the message
"never carr[ies] document content, current or future PyYAML wording
notwithstanding" -- an unbounded promise a quote-shaped regex cannot
license. Restated as the mechanism and its boundary: removes every
character PyYAML is currently known to interpolate via %r, by shape, and
the parameterized test (not the docstring) is what would catch a future
wording change that escapes it.
Cycle guard, catch-all, finding-print ordering, and the order-independent
propagation test unchanged, per the reviewer's explicit instruction.
Full suite: 151 passed (140 + 11 new).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…ename Implements Task 4: `stackward pre-commit`, the git-hook-body subcommand that scans *staged* content for plaintext credentials before a commit object exists -- the layer that actually prevents a leak, since CI can only ever report one that is already public. - `git diff --cached --name-only -z --diff-filter=ACMR` selects staged files (`R` included, so a rename plus an edit cannot bypass the gate). - A dedicated `--diff-filter=U` check runs first: an unmerged index entry is invisible to the ACMR listing above, so without this it would silently pass rather than error. Documented in `cmd_pre_commit` as a deliberate deviation from the brief's "state export refused before any other check" ordering -- see the report for the full reasoning. - Pulumi state exports (`state.json`, `*.stack-export.json`) are refused outright, before the credential scan or `.stackward.toml` is even loaded. - `Pulumi.<stack>.yaml` matched by prefix/suffix with a required non-empty stack name, so the `Pulumi.yaml` project file is excluded. - Every staged blob is read via `git show ":<path>"` and handed straight to `find_plaintext_credentials` -- never the working tree, never a temp-file round-trip through the file-path wrapper. - Exit 0 clean / 1 blocked (finding or state export) / 2 could-not-run (unmerged entry, git failure, undecodable/unparseable content, non-mapping document, bad config, or any unanticipated exception -- every scan step is wrapped so Python's uncaught-exception default of exit 1 can never misreport "credential found"). Also keeps the uncommitted rename from the previous, interrupted attempt at this task: `check_config._describe_yaml_error` -> the public `describe_yaml_error`, so `pre_commit.py` can reuse its redaction of PyYAML's `%r`-interpolated error fragments for staged-blob YAML errors too, rather than a second, independently-written copy of it. 38 new tests in tests/test_pre_commit.py, all against real temporary git repositories (git is never mocked) -- covering a rename+edit staged as `R`, a staged state export, staged-vs-working-tree divergence in both directions, filenames with a space and with non-ASCII characters, and a real merge conflict. 189 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Addresses review findings on Task 4 (`pre-commit`):
1. Important: `cmd_check_config` and `cmd_pre_commit` each had call sites
outside their per-file scan loop -- policy loading in both, plus the
staged-file listing in `cmd_pre_commit` -- guarded only by `except
ConfigError`/`except CheckError`. A different, unanticipated exception
from either (concretely: `find_repo_config` walks parent directories
with bare `.is_file()`/`.exists()` calls, so a `PermissionError` there
is a plain `OSError`) would propagate out of `main()` uncaught and
exit 1 by Python's default -- misreporting "credential found" for an
invocation that never got far enough to check anything.
Fixed with one shared `fail_closed` decorator (in `check_config.py`,
applied to both `cmd_check_config` and `cmd_pre_commit`) rather than
patching three call sites individually and leaving the next command
built this way exposed by default. It is the outer boundary the
per-file loops' own catch-alls don't reach.
2. Ruling: `--diff-filter=ACMR` (the brief's own literal text) excludes
`T` (type-change). Converting an already-tracked `Pulumi.<stack>.yaml`
into a symlink stages as `T`, not `A`/`M`/`R` -- verified empirically
-- so it vanished from the staged-file listing exactly the way an
unmerged path vanishes without `U`, and the commit exited 0. Fixed:
`_staged_paths("ACMR")` -> `_staged_paths("ACMRT")`.
Both fixes reproduced as real regressions before being fixed (decorator
temporarily removed / filter temporarily reverted, confirming the new
tests fail for the exact reason described) and restored once confirmed.
6 new tests: 3 for the fail_closed boundary (a policy-load failure for
each command, plus 2 direct unit tests of the decorator itself), 3 for
the diff-filter gap (a tracked config converted to a symlink exits 2,
plus a direct proof that ACMR alone misses it while ACMRT does not).
195 passed, 0 failed (`uv run pytest`).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…oks dir Adds `stackward hooks install`, a logic-free installer for the `pre-commit` gate built in Task 4. The hook body only locates the `stackward` executable and execs `stackward pre-commit`; every actual rule stays in that command, so a fix or a new check reaches an already-installed hook through a normal stackward upgrade rather than going stale in every clone that installed it once. - Resolves the hooks directory with `git rev-parse --git-path hooks`, never `.git/hooks` hardcoded — `core.hooksPath` redirection (husky, the `pre-commit` framework, org config) makes that hardcoding a silent no-op. - Refuses when the resolved hooks directory is tracked by git (`git ls-files --error-unmatch <dir>`), printing the reason and suggesting chaining from the tracked hook instead of overwriting it. - Embeds the absolute path to the running executable (handling the frozen build, ordinary console-script, and `python -m stackward` cases differently — the last has a non-executable `sys.argv[0]`, so it falls back to a `PATH` lookup at install time too), with the hook itself falling back to `PATH` again at runtime if that embedded path stops working. - A fixed marker line identifies a hook this tool wrote; an existing hook without it is backed up to `<hook>.backup.<UTC timestamp>` rather than refused or overwritten. Installing twice leaves one hook and no second backup. - `chmod 0o755` explicitly on every write. Tests (tests/test_install_hooks.py, 32 tests) use real temporary git repositories only — git is never mocked. Beyond the brief's required coverage, several tests drive a real `git commit` through the actual installed hook (not just asserting a file landed at the expected path), which is what actually proves the hooks-directory resolution works rather than merely appearing to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…kipping quietly in CI
Review of Task 5 found _is_tracked was the one place in this module that
inverted its own fail-closed pattern: any non-zero `git ls-files
--error-unmatch` exit was read as "not tracked", but exit 128 is also what
git reports for a broad class of unrelated fatal errors (a corrupt index, a
permissions problem) where whether the directory is tracked is genuinely
unanswered. `_hooks_dir` resolving symlinks gives a real route to this: it
can make the resolved path diverge from git's own repository root and turn a
genuinely-tracked directory into one git reports as outside the repository.
`_is_tracked` now keys off git's own reported text rather than the exit code
alone: only "did not match any file" (the ordinary untracked case) and "is
outside repository" (an absolute core.hooksPath outside the worktree) count
as "not tracked". Anything else raises `InstallError`, mapped by
`cmd_install_hooks` to the same exit 2 every other could-not-determine
failure in this command already uses — consistent with `_run_git`'s own
any-non-zero-means-cannot-proceed rule, which this was the sole exception to.
Also: the real-git-commit end-to-end tests skip when no `stackward` console
script is found for the test interpreter. That's fine locally, but would let
CI go quietly green having lost exactly the coverage the rest of the review
leaned on. The skip now fails loudly instead when the `CI` environment
variable is set.
Tests added (tests/test_install_hooks.py):
- test_unrecognised_ls_files_failure_exits_2_rather_than_installing
- test_is_tracked_raises_install_error_for_an_unrecognised_failure
Both sabotage-verified against the pre-fix `return proc.returncode == 0`
before this commit; the existing outside-worktree redirect test continues
to pass unchanged.
$ uv run pytest -q
........................................................................ [ 31%]
........................................................................ [ 62%]
........................................................................ [ 94%]
............. [100%]
229 passed in 2.18s
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
The gate built so far reads files and resolves nothing. This is the store those credentials live in: one envelope per profile, Argon2id + AES-256-GCM, with the profile name bound as AAD so an envelope cannot be moved between profiles undetected. crypto.py is the envelope and nothing else — no file I/O, no profile knowledge — so the part that is hard to get right and easy to get subtly wrong reads in one sitting. store.py owns profiles, the two files, and the writes. Argon2id at m=64 MiB, t=3, p=4 (RFC 9106's second recommended option, ~60 ms here). Deliberately NOT the PBKDF2/1,000,000 that check-passphrase will need: that code must reproduce Pulumi's own envelope to verify a stack passphrase, which is a different job with an externally-imposed format. Do not unify them. Three properties worth naming, because each is a silent failure otherwise: - A store-level verifier envelope is opened before any profile envelope, so a mistyped password fails as a wrong password immediately rather than weeks later at first use. - Rotation builds the whole re-sealed document in memory and performs exactly one atomic write, so it re-seals every envelope or none. It iterates the credentials file rather than config's profile list: an envelope whose [profile.<name>] table was deleted is unreachable but not gone, and rotating past it would destroy it. - Passwords are NFC-normalised. The same characters typed on two platforms can arrive as different UTF-8 bytes and derive different keys, giving a store that opens only on the machine that created it. This had to be settled now: adding normalisation after any store exists breaks every password that worked without it. Writes are tmp + fsync + fchmod + rename, with the mode set before the rename and not after — "after" is a window in which the finished file exists at its real path with the wrong permissions. Selection is --profile, STACKWARD_PROFILE, the repo config's profile, then default_profile, then an error. A source that names a profile which does not exist errors rather than falling through: running against a backend nobody asked for is the worst thing this tool could do quietly. backend_url is passed through byte-for-byte and never parsed, which is what lets s3://, gs://, azblob:// and file:// all work without this module knowing anything about any of them. The component form is carried as structured data; composing it into a URL belongs with the command that runs pulumi login. Tests are named for the property each proves, and the ones that matter were sabotage-verified rather than trusted for passing. Three of my own tests failed that check and were rewritten: the mode-ordering test was vacuous (mkstemp already creates at 0600), the all-or-nothing rotation test missed an incremental-write implementation, and my stated rationale for the KDF parameter bounds was empirically wrong — the load-bearing case is iterations, which raises nothing and simply runs, not memory_kib, which _derive already maps. Error messages are hardened against the file they read. The version and kdf fields are hand-editable and live in the one file expected to hold secrets, so crypto.describe_value shows numbers and None and reports anything else by type rather than echoing it back — the same rule check_config.describe_yaml_error follows, and public for the same reason: store.py reuses it instead of growing a second copy of the judgement. _require_credentials is the only branch where decrypted values are in scope when an exception is raised. It interpolates neither half of an entry: the payload is whatever was sealed, and a mapping built the wrong way round puts a credential in the key position, so "keys are just variable names" is not an assumption this branch can make. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Six findings from task review. Two mattered.
The dependency floor predated this work. pyproject pinned cryptography>=42.0,
but Argon2id lives in cryptography.hazmat.primitives.kdf.argon2, which does not
exist before 44.0 — established by installing 42.0.0, 43.0.0, 43.0.3, 44.0.0
and 45.0.0 into throwaway environments and importing, not from the changelog.
The lock resolves 50.0.1, which is exactly why the suite passed and the diff
hid it: any install not using the lock, onto an environment already carrying an
older cryptography, gets an ImportError on `import stackward.crypto` — the
module does not load at all.
UnsupportedAlgorithm now joins the tuple _derive catches. Argon2id needs an
OpenSSL 3.2+ backend and cryptography signals its absence with an exception
inheriting straight from Exception, so on such a build it sailed past every
`except CryptoError` in store.py as a raw traceback — on the one class of
machine where nothing about the store works. _normalise also moves outside that
try, so an absent password reads as this module refusing rather than as the
library rejecting an envelope.
An empty password sealed a real store, silently. Argon2id derives a perfectly
good 32-byte key from b"" — verified against the pinned library — so
init_store("") wrote a well-formed store that opens with no password at all,
and rotate_password(old, "") did the same to an existing one, irreversibly.
Strength is the prompting command's business; emptiness is not, because "" is
indistinguishable from "nothing was supplied" — the same reasoning this module
already applies one field over, where an empty STACKWARD_PROFILE is an error
rather than a miss. Guarded in crypto._normalise, which sits under every seal
and every open so no path can miss it, and again in store._require_password,
which supplies the error type and says which of two passwords was missing.
Sabotage shows both are load-bearing: without the store guard, rotation
propagates a raw CryptoError instead of PasswordError. Whitespace-only is
refused; nothing is stripped, since trimming would silently change what was
typed.
Also: `home` renamed to `directory` across store.py, because config_home()
already appends /stackward and a caller passing an actual config home would
have got <home>/credentials; rotation now replaces only the two fields it owns
instead of rebuilding the document from literals, so it no longer disagrees
with set_credentials about what a document is; and two comments corrected that
overclaimed — the name charset is a strict subset of TOML's bare-key charset,
not exactly it, and the memory_kib bound has a fallback where the iterations
ceiling has none.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
`pulumi login` persists only the backend URL; AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY and PULUMI_CONFIG_PASSPHRASE are read from the environment of every subsequent `pulumi` process. `exec`/`shell` decrypt a profile and inject those three plus PULUMI_BACKEND_URL into a child process only -- inherited environment, never argv, never disk, never logged. `login` resolves a profile's backend URL (composing the component form into an S3 URL when given, passing `backend_url` through byte-for-byte otherwise) and runs `pulumi login <url>`. Wires `select_profile`'s `repo_profile` parameter to the repository's own `.stackward.toml`, the precedence tier Task 6's report flagged as dead code until something supplied it. An explicit `--profile` skips parsing `.stackward.toml` entirely, so a broken repo config elsewhere cannot block an unambiguous explicit choice. Backend guard: before `exec`/`shell` (never `login`, which is what sets the state the guard checks against) runs a child, compares the resolved profile's backend URL against Pulumi's own persisted backend and refuses on mismatch, naming both. Read directly from `<PULUMI_HOME>/credentials.json`'s `current` field rather than by shelling out to `pulumi` -- `pulumi whoami`, `whoami --json` and `about` were all verified, empirically, to contact the backend itself even for a read-only status query, which a pre-flight safety check must never depend on. The store password comes from `STACKWARD_PASSWORD` or an interactive `getpass` prompt, and never from `sys.stdin`: verified that without a `/dev/tty` pre-check, `getpass` falls back to reading (and echoing) `sys.stdin`, which is the child's stdin for `exec`/`shell`. Exit status is translated (`128 + signal` for a signal-terminated child, since `subprocess` represents that as a negative return code `sys.exit` cannot carry through intact) and propagated exactly, including through a real subprocess for both a non-zero exit and a signal kill. Introduces no new concurrent-write path over the credential store: this module only ever calls the read-only `load_store_config`/`resolve_credentials`, never `set_credentials`/`rotate_password`. 456 tests pass (368 before, 88 new), output pristine. Several properties verified by sabotage: mutating the backend guard away, the exit-status translation, the environment-overlay behavior, the repo-profile wiring, the tty pre-check, and the usage-error/profile-resolution ordering each broke a specific, named test; two of those sweeps found and fixed a genuinely vacuous test before being trusted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…nd URL
Two Important review findings, both closed.
1. STACKWARD_PASSWORD was inherited by exec/shell's child (and login's)
via plain `dict(os.environ)`. The store password unlocks every profile,
while what a child receives is scoped to one -- in the single-command
form `STACKWARD_PASSWORD=x stackward exec -- pulumi up`, the user's
intent is unambiguously "stackward only". New
`_environ_without_store_password` scrubs it everywhere a child
environment is built. The env-diff tests could not have caught this on
their own: a set-difference assertion is satisfied by an inherited
value already present on both sides, only by an added one -- fixed by
asserting the removal directly (`parent_keys - child_keys ==
{ENV_STORE_PASSWORD}`, and its absence from the dumped child
environment).
2. `login` put the backend URL in `pulumi login`'s argv (visible in `ps`
for the call's lifetime), and the backend guard's mismatch message
printed it unredacted. `pulumi login --help` documents
`postgres://user:password@host/db` as a supported backend form, so a
supported profile shape put a password in both places. Fixed both
parts of the ruling: `login` now sets `PULUMI_BACKEND_URL` in the
child's environment and invokes `pulumi login` with no positional URL
argument -- verified, empirically, that this logs in identically for a
`file://` backend with no network access. `_redact_url` replaces any
`user:pass@` userinfo with `<redacted>@` everywhere a backend URL is
printed.
Also fixed, per review:
- `_current_backend` now checks this process's own `PULUMI_BACKEND_URL`
before the persisted `credentials.json` -- Pulumi's CLI honours that
variable over a stored login, so a value already exported into the
caller's shell is what a stray `pulumi` command would actually use, and
is what the guard's own stated purpose requires it to catch.
- `_run_child` now catches `KeyboardInterrupt` (CPython's re-raised
`SIGINT`, which `fail_closed` cannot catch -- it is a `BaseException`)
and returns `128 + SIGINT`, the same exit status Python's own default
handling already produced, but without the traceback that made
`Ctrl-C` on `stackward exec -- pulumi up` read as a crash.
- The guard's real-file/real-comparison joint now has an end-to-end test
that writes a genuinely disagreeing `credentials.json`, not only each
half mocking the other.
- `test_guard_allows_a_match` deleted: its own comment admitted it tested
the never-logged-in case, duplicating a test three lines below it under
a name that promised something else.
- Triple-copied `print(f"error: {exc}", ...); return 2` replaced by a
`_report` helper.
Not changed, per review: stale `AWS_SESSION_TOKEN`/`AWS_PROFILE` in the
parent are not scrubbed (fails loudly as the wrong identity rather than
silently; the brief mandates exactly four injected names, and removing a
fifth ambient one is beyond this task).
Carry-forward, recorded rather than fixed: every `_compose_backend_url`
composition test uses a scheme-less endpoint, so percent-encoding a
realistic `https://host:9000` endpoint is never exercised, and the
composed URL was never verified against a live S3-compatible backend.
470 tests pass (456 before, 14 new/changed), output pristine. Every fix
sabotage-verified: reverting each one independently breaks a specific,
named test (one of them -- removing the KeyboardInterrupt catch -- kills
the pytest run outright, the strongest possible signal).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
1. test_child_env_overlays_exactly_four_names_on_a_copy_of_the_parent never
injected STACKWARD_PASSWORD into the parent environment, so its
difference-based assertions could not distinguish "scrubbed" from "was
never there" -- the same shape of gap already fixed in the end-to-end
exec test, missed here. Worse than stale: for anyone running the suite
with STACKWARD_PASSWORD exported (this feature's own documented
non-interactive path), the real difference becomes {STACKWARD_PASSWORD},
the equality assertion fails, and the following per-key value-equality
loop raises KeyError on the popped key. Fixed the same way as the
end-to-end test: inject the password, assert the difference is exactly
{ENV_STORE_PASSWORD}, assert its absence from the child, skip it in the
value-equality loop, and assert the password value itself is absent from
every child value.
2. The autouse no_real_pulumi_state fixture isolated PULUMI_HOME only.
Since _current_backend() now consults PULUMI_BACKEND_URL first (this
task's own earlier fix), every end-to-end test exercising the real
_current_backend() silently depended on the invoking shell's ambient
PULUMI_BACKEND_URL being unset -- reopening the isolation guarantee the
fixture's own docstring states. Reproduced concretely: with
PULUMI_BACKEND_URL exported in the real shell and the old fixture
restored, 25 tests in this file fail. Fixed by clearing
PULUMI_BACKEND_URL in the autouse fixture itself; dropped the one
now-redundant per-test delenv.
Both sabotage-verified: reverting either fix independently reproduces the
described failure (the second reproduced against the real ambient
environment, not merely a simulated one).
470 tests pass, output pristine, no new tests added -- both fixes correct
existing tests' blind spots rather than adding coverage.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Reads `[secrets."<dir>".secret]` from `.stackward.toml` (config path ->
logical name), resolves each name against the process environment and
`[secrets.source].files` (env first, later files override earlier ones,
`{stack}` substituted), and publishes resolved values with
`pulumi config set --secret --path <path>`, value on stdin -- never in
argv. An unresolved name is skipped, never cleared; a path listed under
`[required]` turns an unresolved skip into a failure without aborting the
rest of the run. `[[secrets."<dir>".drift_pairs]]` compares a local
bootstrap value against the published value at whichever path a "managed"
logical name resolves to (read with `pulumi config get`, which decrypts),
warning only on a genuine, readable difference.
Only `secret` is published; `plaintext` and `unmanaged` share the same
manifest shape but are read and left untouched, per the brief's own
`--secret`-only wording -- see the task report for the full reasoning and
the spec document that corroborates it.
`LocalSecretSource.resolve(name) -> str | None` is written to the shape
Task 11's `SecretSource` interface will register providers against, so
that task adapts this module rather than rewriting it.
58 new tests (528 total), including sabotage-verified coverage of: the
value reaching a real child process via stdin and never argv; skip-if-
unset vs. required-and-unset; a timeout in the middle of three entries not
aborting the other two; drift warning only on a genuine, readable
difference (and the trailing-newline-from-`config get` bug that a naive
test stub would otherwise hide); and a `pulumi` failure never echoing the
value it was given.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Two requirements changes from the coordinator, applied before review.
Ruling 1 -- `[secrets."<dir>"].plaintext` entries are now published, not
just parsed and left inert. `ProjectManifest` gains a `plaintext` table;
`_pulumi_config_set` takes a `mode` ("secret" | "plaintext") instead of a
bare `secret: bool` and emits `--secret` or `--plaintext` accordingly, one
code path and one stdin delivery mechanism for both. `_publish_entries`
walks `secret` then `plaintext`, each in its own declared order; `required`
now checks membership across both tables. `unmanaged` is unchanged --
still documentation-only.
Ruling 2 -- `[required]` now has exactly one accepted shape:
[secrets."<dir>".required]
paths = ["a.b", "c.d"]
`_normalize_required` (which accepted a bare list or a keys-as-paths table)
is replaced by `_parse_required`, which rejects both of those former shapes
with a named error instead of silently reinterpreting either as valid --
consistent with `config.py`'s own "invalid is never treated as absent" and
"unknown key is named and rejected" rules.
9 new tests (537 total): plaintext published with --plaintext and never
--secret; secret never published with --plaintext; both tables live in one
run; the new [required] shape's unit-level acceptance/rejection cases; and
an end-to-end test proving the old bare-list [required] shape is now
rejected at load time rather than silently tolerated -- the first version
of that last test was itself vacuous (the tolerated old shape happened to
produce the same exit code for its chosen manifest) and was strengthened,
after sabotage exposed the gap, to assert on the actual distinguishing
signal: a load-time rejection never reaches the publish loop's closing
summary line at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…ix drift edges
Task review findings (Spec fail, quality needs-fixes), all five addressed.
Important 1 -- a path declared in both `secret` and `plaintext` was
unvalidated. `_publish_entries` builds secret items then plaintext items,
so an overlapping path published twice with the `--plaintext` write
landing last and winning: a declared credential ending up unencrypted.
`parse_project_manifest` now raises `SetSecretsError` naming the path when
`set(secret) & set(plaintext)` is non-empty, mirroring the existing
`required` cross-table check.
Important 2 -- `resolve_source_files` had zero direct test coverage; the
brief's own "precedence across the environment and two files" requirement
was only exercised through hand-built `LocalSecretSource` dicts, never
through the manifest-to-disk path that actually implements it. Added seven
unit tests plus one end-to-end test through `main()` that reads two real
files (the second overriding a name from the first) with `--stack`
substitution wired in and a third file genuinely absent, verified via the
stub `pulumi`'s own stdin -- and a separate test for the absolute-path
branch.
Also fixed: `_check_drift` now inverts over `secret ∪ plaintext` (both are
published, so a `managed` name in `plaintext` no longer wrongly hits the
"ambiguous" branch); `_pulumi_config_get` uses `.removesuffix("\n")`
instead of `.rstrip("\n")`, which was eating a published value's own
trailing newline and firing spurious drift warnings; `ProjectManifest`'s
docstring no longer claims `config.py` validates `unmanaged`'s shape (it
doesn't -- this module defers that, and simply chooses not to look at it).
13 new tests (550 total), each sabotage-verified against the defect it
covers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Re-review regression: `_check_drift`'s pre-loop `published` (the merged `secret ∪ plaintext` dict, added when Finding 3 was fixed) shared its name with the loop's own `published` (`_pulumi_config_get`'s `str | None` result, pre-existing and untouched by that fix). The first drift pair evaluated correctly; the second and every later pair called `_config_path_for_name` with the previous iteration's `str` instead of the dict and raised `AttributeError`. User-visible effect: `_check_drift` runs after `_publish_entries` has already made every `pulumi config set` call and printed every outcome, so a run where everything published successfully still exited 2 via `fail_closed`'s generic catch, with the closing `summary:` line -- exactly what the operator needs at that moment -- never printed. Fix: rename the pre-loop binding to `published_entries`; the loop's own `published = _pulumi_config_get(...)` is untouched. Also corrects `DriftPair`'s docstring, which still named `ProjectManifest.secret` alone as the inversion source after Finding 3 changed it to `secret ∪ plaintext`. Regression test: two drift pairs, both unambiguously resolvable and both genuinely differing (so a pass requires both warnings to actually fire, not merely "no crash"), reproduced against the pre-fix source with the exact reported failure shape before being fixed. 551 tests pass (550 before, 1 new). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Prove a passphrase actually decrypts a stack, which no ordinary `pulumi` command can answer -- `pulumi config` renders secrets as `[secret]` without decrypting and exits 0 under any passphrase, including a wrong one. `stackward check-passphrase STACK...` reads the candidate passphrase from stdin (falling back to an echo-free `getpass` prompt when stdin is a terminal, never as a CLI argument), runs `pulumi stack export --stack <name>` for each named stack, and reproduces Pulumi's own passphrase-based secrets-provider check against `deployment.secrets_providers.state.salt`: PBKDF2-HMAC-SHA256 at 1,000,000 iterations deriving a 32-byte key (Pulumi's own fixed scheme, deliberately not crypto.py's Argon2id), then AES-GCM, expecting the literal plaintext `b"pulumi"`. Reports accepted/rejected per stack and a SHA-256 fingerprint of the passphrase (never the passphrase itself); exits 0 only if every named stack accepted. Fails closed: a malformed salt string, an unparseable export, or a missing `deployment`/`secrets_providers`/`state`/`salt` field (e.g. a stack using a non-passphrase secrets provider) is reported as an error, never "rejected" or "accepted". Explicit byte-length checks on the salt/nonce/ciphertext guard against a subtle misclassification: `cryptography`'s `AESGCM.decrypt` raises `InvalidTag` -- not a distinguishable error -- for a too-short ciphertext or an unexpected-but-still-valid nonce length, which without these checks would be misreported as "rejected" instead of the malformed input it actually is. The format (v1:<b64 salt>:v1:<b64 nonce>:<b64 ciphertext>, standard/padded base64, 8-byte salt, 12-byte nonce) was confirmed empirically against a real `pulumi stack export` from a throwaway local-backend stack, not assumed -- that exact vector is frozen as a test fixture, since a round trip against this module's own encryption would prove only self-consistency, never interoperability with Pulumi's actual format. 38 new tests in tests/test_check_passphrase.py; 589 passed, up from 551. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…he index
The gate now unions the key-name heuristic with a second net built from a
repository's own `json_schema_extra={"secret": True}` marks. A shipped binary
cannot import the consuming repo's classes, so the declarations cross that
boundary as data.
The artifact is a graph, not a list of path patterns. The counterexample is
concrete: a model with `token` marked and `children: list[Node]` yields the
pattern list `['token']` -- any generator needs a cycle guard -- while the
data it describes is unbounded. The flat form fails open exactly where it
must fail closed. `nets/model.py` walks the graph and the document together,
so recursion and mutual recursion work by construction.
Coverage is prefix-based (a marked field covers everything beneath it,
annotation irrelevant) and structural (a dict key may contain `.` or `[`, so
the walk never builds or re-parses a path string to decide anything).
`bootstrap/regen.py` runs under the repository's interpreter -- shipped as
data, never imported, which is what keeps pydantic test-only. It refuses any
unmarked field whose annotation could hold a model but is not walkable rather
than emitting nothing for it; the refusal names the field and both remedies.
Freshness: `check-config` reads the artifact from the git index, not the
worktree, and compares recorded git blob ids in one `git ls-files -s`.
Sources come from an MRO walk (an inherited mark in another file) and from an
identity scan for `Annotated` aliases (a mark whose alias is defined
elsewhere); an out-of-repo contributor is pinned by the dependency lockfile.
The boundary is stated rather than overclaimed: blob ids catch modification
of a recorded source, and CI regeneration is what catches addition.
Any failure to load exits 2, never 1 -- a fail-closed refusal must not be
filed under "a credential was found". `model_net = "none"` is a declared mode
and is never reached as a fallback.
Also: `is_encrypted`/`is_empty` become public in `nets/heuristic.py` so both
nets share one leaf decision; `check.declared_paths_fn` lands in config as the
escape hatch for a non-standard marking convention.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…gate path Four things the diff review found: - `sync-declared-secrets` wrote the artifact without ever reading it back. The generator and the matcher are two halves of one schema running in two processes under two interpreters, so a drift between them would have surfaced as a refusal in someone else's repository at their next commit. `build_artifact` now parses its own output with the matcher's parser and refuses on anything it cannot read. - `repo_toplevel` did not resolve its path, while every caller compares it against a resolved module path. Under a symlinked parent directory those are two strings for one directory and every source file would have read as "outside the repository". - No test asserted that the gate path reaches no credential code -- and this change added an import to it. Added one, in a subprocess. - `nets/__init__.py` still described the model net as a later addition. Also two tests that passed for the wrong reason, both found by sabotage rather than by the suite: the mapping-key test's value had no leaf beneath it, so matching keys against field names changed nothing observable; and regenerating twice on one machine cannot detect unsorted keys, since one interpreter's dict order is already stable. Both now discriminate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
The brief mandates recording the dependency lockfile's hash for modules outside the repository. That was implemented and then had no test, and none of the sabotage mutations touched it either -- so the whole branch was asserted by nothing. Five tests through `build_artifact`, which is already directly callable: - a tracked lockfile is recorded by blob id, compared against `git rev-parse` - every tracked lockfile is recorded, not the first match - a lockfile on disk but not in the index pins nothing and still refuses - no tracked lockfile with an out-of-repo contributor refuses, naming both the module and the filenames looked for - no lockfile is recorded when every mark came from inside the repository Four mutations confirm they discriminate: no lockfile ever recorded, only the first recorded, the out-of-repo refusal removed, and an untracked lockfile treated as though it pinned something. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Four items from coordinator review; two are correctness. 1. `pre-commit` now runs the model net. It is the layer that actually prevents publication -- `check-config` invoked by hand reports a leak that may already be committed -- so a model net running only on manual invocation was absent at the one moment it counts. Same index-read and exit-2-on-stale semantics as `check-config`, loaded before the scan loop so a stale artifact refuses having reported nothing. Four of the five new tests drive a real `git commit`, with a companion proving the same commit succeeds under `model_net = "none"` so the blocking test cannot pass because the hook blocks everything. 2. `RootModel` is refused at generation. It appears in a document as its inner value with no `root` key, so walking it as an ordinary model looks up a field name that can never occur, produces no coverage, and says nothing about it -- unknowable coverage, which by R1's own principle refuses. Not walked: a mark on `root` covers the whole value at the *parent* position, and the schema cannot say that when the model is reached through a container node, which carries no `secret` of its own. 3. The refusal message names the field, its class and the annotation as written, then three ways forward. A refusal a reader cannot act on is one that gets silenced with `model_net = "none"`, losing the whole net. 4. `doctor` now reads `regen.py` back through the shipped accessor and exits non-zero if it is missing or is not the generator; the release workflow greps for the line. Verified against real frozen bundles both ways: with `--add-data`, doctor exits 0 and no pydantic is in the bundle; without it, doctor exits 1 and the grep fails. Also moves the real-console-script fixture into `tests/conftest.py`: two test files now need its "skip locally, fail under CI" rule, and a second copy that forgot the CI branch would let CI go green having lost this coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Each reproduced end-to-end in a clean, fully-staged repository before fixing, and confirmed closed after against the same repository. 1. `_may_hold_model`'s last branch answered "no" for any unrecognised class, so a pydantic dataclass, `TypedDict` or `NamedTuple` holding a marked field left the graph with no refusal -- R1 inverted for a whole category. A bare class is now a leaf only when no class in its MRO declares an annotation of its own whose name does not start with an underscore: pydantic's own field-collection rule, which separates a structure from a scalar without enumerating either, and which keeps `AnyUrl` and `SecretStr` (private annotations only) as leaves. Fixing the generator does nothing for an artifact already committed by the broken one -- it is fresh against every source it records and internally consistent while covering less than the repository declares. So ARTIFACT_VERSION is bumped 1 -> 2, and every existing artifact now refuses with the regeneration command named. 2. The MRO source walk filtered on `issubclass(base, BaseModel)`, so pydantic collected a marked field from a plain mixin while the mixin's file never reached `sources` -- a tracked, already-contributing file, not the "newly added" boundary. Now: skip `object`/`BaseModel`, record repo-local bases unconditionally (a blob id already in the index costs nothing), and out-of-repo bases only when they declare fields. Not "every base except BaseModel": that would record `abc` for `class Config(BaseModel, ABC)` and reimpose the universal lockfile precondition. A test with no lockfile in the repository pins it. 3. `.stackward.toml` was not a recorded source, though it supplies `roots` and `declared_paths_fn` -- so a namespace could be added to the policy, staged and committed with the artifact untouched, and the namespace was simply skipped. Its blob id is now recorded, and `_verify_roots` adds a second guard needing no blob id at all: the artifact's roots must be exactly the namespaces the policy declares, compared as key sets since `module:QualName` diverges from `module:Class` for a nested class. Also: `scan_file`'s `net` loses its default (asserted as a signature, over both scanners); the duplicate-data-key refusal tracks claimed keys separately from emitted entries so it no longer depends on declaration order; `run_git`'s dead `cwd` keyword dropped; refusals name the annotation as written plus the offending part; `getsourcefile` guarded against OSError as well as TypeError; and CI now asserts pydantic is absent from `dist/`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Two roles, one method each, both `typing.Protocol`s so a caller depends on
the method shape rather than a module: `CredentialStore.resolve(profile)`
returns a profile's bootstrap set (`file`, `env`); `SecretSource.resolve
(name)` returns one manifest logical name's value or `None` (`dotenv`,
`env`). `file` and `dotenv` adapt Task 6's `store.resolve_credentials` and
Task 8's `LocalSecretSource` respectively, unchanged, per the brief. A
generic `command` provider (for both roles) shells out to a configured CLI
-- the vehicle for "adding a remote provider must not add a Python
dependency" and for a fake remote provider the contract test can exercise
with a real subprocess rather than a Python object standing in for "remote".
Selection is `provider = "..."` in `[profile.<name>.credentials]`
(store.py's Profile, defaulting to "file") and in `[secrets.source]`
(set_secrets.py, defaulting to "dotenv") -- both wired into the real
`exec`/`shell`/`login` and `set-secrets` entry points, not only into a
factory nothing calls.
Gate-path invariant: no `stackward.providers*` module is ever imported by
check-config/pre-commit. session.py and set_secrets.py import from
`providers` only inside the function that is about to resolve a credential
(never at module scope), since cli.py imports both unconditionally.
Verified both ways: `test_the_check_config_entry_point_never_loads_a_
provider_module` (literal, per the brief) and a stronger sibling that runs
the real `cli.main(["check-config", ...])` dispatch, since cli.py's own
module-scope imports are what a real commit's hook process actually
exercises. Both were confirmed to fail under a deliberate sabotage import
before being reverted.
A provider failure is `ProviderError`, reported, never a silent empty
value: `_publish_entries` records it as `failed` (never `skipped`), `_check_
drift` warns rather than folding it into its existing silent states, and
`session._prepare_env` turns it into `SessionError` rather than running a
child with partial credentials. `file`'s `StoreError` propagates unchanged
instead of being rewrapped, since existing session.py tests already assert
on it and rewrapping would be a rewrite, not an adaptation.
Carry-forward fix from Task 6's review: `crypto.seal_json` was reported to
raise a bare `UnicodeEncodeError` on a payload value containing a lone
surrogate. Empirically false as stated -- `json.dumps(...,
ensure_ascii=True)` escapes a lone surrogate to plain ASCII before the
`.encode("utf-8")` that follows, confirmed by test. The real, reproduced
site is `_normalise`'s raw `str.encode("utf-8")` on an NFC-normalised
password (reachable via `STACKWARD_PASSWORD` from `os.environ`, which
Python decodes with `surrogateescape`), plus the identical `aad.encode
("utf-8")` in both `seal` and `unseal` -- so this also fail-closes the
unseal path, not only seal. All three, plus `seal_json`'s own call as a
second line of defence, now raise the new `EncodingError(CryptoError)`
instead of a bare exception.
Contract tests (tests/test_providers.py) prove the "swapping the provider
changes nothing for the caller" guarantee rather than merely asserting it:
`set-secrets --dry-run` produces byte-identical printed outcomes under
`dotenv` and the fake `command` remote seeded with the same values;
`set-secrets` without `--dry-run` produces byte-identical `pulumi` argv
*and* stdin against a real stub (`--dry-run` alone can't prove this --
`_publish_entries` never reaches `_pulumi_config_set` under it, so that
comparison would be `[] == []`); and `exec` injects byte-identical child
environments across `file`, `command` and `env` `CredentialStore`s seeded
with the same credentials.
Scope note: `providers/command.py` is not one of the four implementations
the brief names (`file`, `env`, `dotenv`, `env`) -- it exists because "a
remote provider shells out to its CLI" is otherwise untestable without a
Python object standing in for "remote", and the contract test needs a real
fork/exec.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…ation Five defects two branch reviews found in the gate itself, each of which let a command exit for a reason it does not mean. **`pre-commit` read content from the index and policy from the working tree.** Every path it scanned came from `git show ":<path>"`, but `_load_check_policy` called `find_repo_config()` + `load_config(path)` — plain filesystem reads. That is a fail-open with a working exploit: stage a credential, then edit the working-tree `.stackward.toml` to add an `allowed_references` entry for it, staging nothing, and the commit goes through. The credential enters history under a relaxation that is not itself committed, so the next clone has the leak and not the excuse. Three knobs weaken from that direction — `allowed_references` adds a suppression, `model_net = "none"` drops a whole net, and `sensitive_parents` *replaces* rather than extends, so the worktree can shrink what the index declared. `nets.model.verify_sources` cannot close any of them: it compares an index blob against a recorded blob, a worktree-only edit touches neither, and under `model_net = "none"` it does not run at all. `config.py` now separates parsing from file reading (`load_config_text(text, origin)`, with `load_config(path)` as a thin reader in front of it), which is what `pre-commit` needs to load the policy the index holds. `_index_config_path` reproduces `find_repo_config`'s upward walk in repository-relative terms via `git rev-parse --show-prefix`, and the two `_load_check_policy` helpers are now genuinely different rather than redundant — `check-config` answers "is this file clean right now?" and so reads its policy from disk; `pre-commit` answers "is what I am about to commit clean?" and so reads both from the index. Both docstrings say which they are and why. **A missing `.stackward.toml` was a silent skip.** Both commands returned `CheckConfig()` defaults, which carry `model_net = "none"` — turning the declared heuristic-only mode into exactly the silent fallback the plan says it must never be, and Global Constraint 3 says a missing policy file is a refusal. Both now exit 2 naming the minimal file to create, `model_net` included, following `sync_declared._load_repo_config`'s precedent. `cmd_doctor` still never loads a config, so it keeps working with none. Many existing gate tests ran in repositories with no policy file and asserted exit 0 — they were pinning the forbidden behaviour. They now declare a minimal policy and still check what they were checking. Three branches that survived all 746 tests are pinned for the first time: `check-config`'s and `pre-commit`'s policy guards (`return 2` mutated to `return 0` passed), and `config.py`'s `_require_str_list`, `_build_check` and `load_config` error paths (each mutated into a silent default passed). **`cryptography` and the credential store were on the real gate path.** `cli.py` imported `commands.check_passphrase` (-> `cryptography`) and `commands.session` (-> `store` -> `crypto`) at module scope, so every invocation loaded them. With `cryptography` unimportable, `stackward pre-commit` died on a traceback and exited 1 — the code reserved exclusively for "a credential was found". Those four command modules are now imported by a dispatch thunk at call time; `--help` still works for every subcommand, and the thunk resolves through `getattr` so a `monkeypatch.setattr` on the command module still takes effect. The test that should have caught this was split so that neither half could: `test_model_net.py` checked `cryptography`/`store` but imported the command modules directly, bypassing `cli.py`, while `test_providers.py` went through `cli.main` but grepped only for `stackward.providers*`. Both are replaced by one test in `tests/test_gate_isolation.py` asserting the union through `cli.main`. That file also adds the first tests for guarantee 1's "never prompt" and "never touch the network" clauses, over both gate commands — inserting a real `getpass` call into `cmd_check_config` previously survived the whole suite. **`min_version` was built and never wired.** `enforce_min_version` had zero production call sites; `cli.main` dispatched straight to `args.func`. It is now called between parsing and dispatch, from one broadly-guarded helper so a malformed config cannot take `doctor` down with it (Task 2's carry-forward ruling) — deliberately one guard and not two, because with two neither is falsifiable. The floor runs before the missing-policy refusal, which costs nothing: with no config there is no floor to enforce. Its message pointed at `stackward self-update`, a subcommand `build_parser` never registered; it now points at `install.sh`. `self-update` stays in `VERSION_CHECK_EXEMPT` with a comment saying it is reserved, not registered. **Public-repo hygiene.** The comment in `config.py` justifying an empty `sensitive_parents` default described a gate script in the private environment this repository was extracted from, and named its sensitive-parent category; the rule is kept and the provenance dropped. `tests/test_heuristic.py`'s fixture corroborated that same string and is renamed. A fixture literal naming a vendor, a region and a namespace in one value is replaced with the `placeholder-*` convention the rest of the suite already uses. **Deferred, now fixed:** `pre_commit._run_git` sets `LC_ALL=C`, with the environment merged rather than replaced — git sets `GIT_INDEX_FILE` and `GIT_DIR` for a hook process, and dropping them would make the gate read a different index than the commit it is gating. Every test added or changed here was verified by breaking the source and confirming it goes red; where a mutation left a test green, the test's claim was corrected rather than the result explained away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
`test_git_runs_under_a_fixed_locale` loops over every git invocation the command makes, which holds only because the `repo` fixture declares `model_net = "none"`. Under `model_net = "artifact"`, `nets.model.run_git` runs too and sets no locale -- a second call site with the same gap, in a file this wave does not own. Left as a whole-command assertion on purpose (it is the property that matters), but a fixture change would otherwise produce a failure in this test pointing at a file nobody was told about. `install_hooks._is_tracked` is a third call site, and there the missing locale is a live bug rather than hardening: it branches on git's translated strings, so under a non-English locale `hooks install` exits 2 everywhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
Every leak test on this branch used `assert value not in text`. The final
review showed the boundary precisely: {value!r} in a message is caught,
because repr embeds the whole string, but value[:8] in an error message
survives all 746 tests -- as does the value base64-encoded. A whole-string
containment check cannot see a partial disclosure, and a partial disclosure
of a credential is still a disclosure.
assert_no_leak checks the value, every 8-character run of it, and the
encodings this codebase actually emits. It reports the encoding and offset
and never the value, so the check cannot itself become the leak.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…cale Six ValueError branches in `paths.py` had never run in any test, and `set_secrets.parse_project_manifest` wraps `paths.parse` in its own `try`, so none of them could surface as a traceback either. A refusal that has never executed is where a fail-open hides. Each of the six now has a case asserting the position and the wording, and that the offending text stays out of the message. `nets.model.run_git` was the last git call site with no `LC_ALL=C`. The environment is merged rather than replaced: git exports `GIT_DIR` and `GIT_INDEX_FILE` to a hook process, and this helper is what reads the index the commit is being gated on. `tests/test_providers.py` derived its three-provider contract `expected` from one of the three captured environments, so a uniform mutation in the shared `_child_env` satisfied it as well as correct behaviour did; it is now anchored to the seeded constants. Its `import subprocess` went unused when the gate-path test moved to `tests/test_gate_isolation.py`. The plan's adversarial "a marked map whose only key is literally `secure`" case is now named as an accepted false negative rather than left looking untested. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…leaks
"Values reach the CLI on stdin, never argv" was proved for exactly one
invocation shape: one `secret` entry, no `--stack`. Appending the value to
argv under `--stack` -- the production shape for any rotation -- or under
`plaintext`, which shares `_pulumi_config_set`, passed all 781 tests. The
assertion is now a helper every argv-dumping test reads its calls through,
and the test is parametrised over both tables and both stack shapes.
Every "no value printed" assertion was a whole-string `not in`, which sees
`{value!r}` but not `value[:8]` and not a base64 encoding. They now go
through `leakcheck.assert_no_leak`. The markers had to be respelled: the old
`marker-db-password-...` shares the eight-character run `password` with the
config path `db.password` those same tests print, so the stricter check
would have fired on output that disclosed nothing.
`parse_env_file` tolerated an inline-`#` truncation, a split at the last
`=`, and the loss of either half of the quote anchoring, because no test
anywhere wrote a value containing `#` or `=` -- both ordinary in a
credential. A scalar where the `secret` table belongs produced `0 set, 0
skipped, 0 failed` and exit 0 when its raise was removed. The summary line's
counts were asserted nowhere.
`resolve_source_files` re-checked that `[secrets.source]` is a table after
its only caller had already refused a non-table; the copy could never
execute. Removed, with the reason recorded so it is not helpfully re-added.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
`build_artifact` removes the artifact from its own `sources` map, so a hand
edit or a bad merge in the committed JSON passes every freshness check. That
makes the parse-time guards the only integrity check the file has, and four
of them had never executed: a non-boolean `secret` mark (`0` reads as
"unmarked" and is a one-character diff), a null container interior (drops
the whole subtree), a recorded source deleted from the index rather than
changed, and an empty `check.stack_models`.
The last two needed the right assertion, not just the right exit code. Both
`verify_sources` branches exit 2 saying "stale", so removing the
missing-source branch merely reports a deleted file as *changed*; the test
asserts the wording. And `sync` with the empty-`stack_models` guard removed
exits 0 having written an artifact whose `roots` is `{}`, which
`_verify_roots` then passes as `set() == set()` — so the test asserts the
artifact was never written, and that `check-config` afterwards refuses on a
credential only the model net could name.
`_verify_roots` is kept and now exercised end to end, against exactly the
edit it is the last defence for: drop `.stackward.toml` from `sources` and a
namespace from `roots`, and every remaining source still matches the index.
Its stated rationale was stale — nested-class roots cannot arise, because
`regen._import_object` resolves targets with a flat `getattr` and refuses
`declared:Outer.Inner` at sync time. The reachable divergence is a
module-level alias or a re-export, which now has its own test; comparing
`.items()` instead of key sets fails it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
"No storage vendor is named anywhere in this codebase" is not true: `commands/session.py` defines `AWS_ACCESS_KEY_ID`/`AWS_SECRET_ACCESS_KEY`, composes `s3://` for the component backend form, and sets `s3ForcePathStyle`. All three are Pulumi's own vocabulary for a DIY backend rather than a choice about anyone's provider, so the code is right and the sentence was wrong. The claim it replaces says what is actually true: no bucket, region, endpoint, account, domain, namespace or model class appears here, and the two de-facto-standard names that do are protocol. "No backend assumptions" holds for a passed-through `backend_url` and not for the component form, which can only ever produce an `s3://` URL. Split and qualified. The status banner still described `v0.1.0` as shipping `--version` and `doctor` only, and the table was headed "What it will do". Every row now exists — except one: `credentials` was never a subcommand. The store is implemented and `login`/`exec`/`shell` read it, but nothing on the command line writes to it, so that row is removed from the table and the gap stated plainly instead of being listed as though it shipped. `src/stackward/cli.py:3` carries the same stale v0.1.0 claim; it is not in this wave's files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
`tests/test_providers.py` kept its own copy of `MARKER_DB_PASSWORD` at the old spelling, so the two suites disagreed about the constant and the next leak assertion added here would have hit the same `password`-run false positive against `db.password` -- with the explanation sitting in the other file. Both suites now spell their markers the same opaque way, and this one says why in place. Its three whole-string absence assertions are upgraded to `assert_no_leak`: a dry run that helpfully printed the first few characters of what it would set, so an operator could confirm the right value, passed the old form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
A mutation review found nine assertions in the session, store, passphrase
and hook-install layers that no implementation could fail. Each one is
replaced by an assertion that a specific defective implementation breaks,
and the defects the vacuous ones were hiding are fixed.
The passphrase fingerprint was pinned to a length and to nothing else, so
`passphrase.encode("utf-8").hex()[:16]` -- the first eight bytes of the
passphrase -- passed every test while `check-passphrase` printed it on
stdout. The digest is pinned against known values now. Note that
`assert_no_leak` alone does not catch this: it checks whole encodings and
runs of the *raw* value, and a truncated hex encoding is neither. The tests
hand it the hex encoding as the value, which is what closes it.
`exec`'s most likely failure -- a mistyped command -- had no test at all, so
adding the child environment to that error message passed the suite while
printing every injected credential. `shell` had no exactly-these-keys test
either. And no test ever put a credential name in the *parent* environment,
which left `env.setdefault(name, ...)` indistinguishable from
`env[name] = ...`: a developer with `AWS_ACCESS_KEY_ID` exported would have
got their own key paired with another profile's backend. The autouse fixture
now seeds all three names, which fixes the blindness and the reverse problem
in one place -- those same tests failed outright for anyone who had the
variables exported.
`_tty_available` had never executed: pointing it at `/dev/null` passed the
suite, and that is the fail-open where `getpass` falls back to the child's
stdin and echoes the typed password. It is now exercised in a real process
with no controlling terminal.
`config` was blessed at 0644 on the stated grounds that it "holds backend
identity and never a credential", while `postgres://user:password@host/db`
is a documented Pulumi backend form that `backend_url` passes through
verbatim. Same false premise justified quoting tomllib's message back;
tomllib does echo document text (`Cannot declare ('...',) twice`). The mode
is 0600, the claim is corrected, and only the position is quoted. The store's
own mode tests compared the module against itself -- `CREDENTIALS_MODE =
0o606` and `DIR_MODE = 0o777` passed all 781 -- and now use literals.
`hooks install` matched git's translated strings, so it exited 2 in every
repository under a non-English locale; every git call in that module now
goes through one runner that sets `LC_ALL=C`. Its git fixtures ran `git
init` with no config isolation, so a global `core.hooksPath` -- what husky
and lefthook set, and what the command exists to handle -- errored 62 tests.
Also: the missing-policy warning the `TODO(final-review)` in `check_config`
described, and the store, timestamp and encoding paths that had never run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
The helper checked whole encodings and runs of the raw value only. Wave 2 showed the gap while fixing the passphrase fingerprint: hex()[:16] contains neither the raw value nor the complete hex encoding, so assert_no_leak passed on text that bytes.fromhex turns straight back into the first eight characters of the credential -- the exact defect the tautological test hid. Runs are now checked for the byte-aligned encodings too, at lengths that each recover about as much plaintext as the raw run length does. The URL-quoted forms are deliberately left whole-string only: percent-encoding is variable-width, so a run boundary can land mid-escape and the recovered text is not what the run implies. Verified by mutation: with fingerprint() returning the passphrase's own hex, the leak assertion now fails on its own, independently of the digest pin added alongside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…mmand
`credentials` was named in the design (§4, migration step 2) and in the
README table, but no task owned the CLI surface, so the store could be read
by login/exec/shell and populated by nothing. The only working path for a
fresh user was the env provider, which is what the encrypted store exists
to replace. init / set / list / show / rotate now exist, built on the
store API that was already there: values arrive as KEY=VALUE on stdin and
never as arguments, `show` prints names and never values, and a wrong
master password fails on the store verifier rather than weeks later.
The larger find came out of building it. Making the gate import-clean had
turned dispatch into `importlib.import_module(f".commands.{name}")`, and
PyInstaller resolves imports by reading literal import statements out of
bytecode -- it cannot see a name assembled at runtime. So in the shipped
binary, *every* lazily dispatched command was absent: `stackward exec` died
on ModuleNotFoundError with a traceback and exit code 1, the code this tool
reserves for "a credential was found". Verified by building both ways: at
the previous commit set-secrets, sync-declared-secrets and login are all
missing from the bundle, while --version and doctor -- the only two things
the release job checked, and both registered directly -- pass throughout.
Dispatch is now an if-chain of literal imports, still function-local so
laziness and gate isolation hold. The release job gained a check that runs
two lazily dispatched commands from two modules against the bundle; it
rejects "invalid choice" as well as ModuleNotFoundError, because an
unregistered subcommand also exits 2 having loaded nothing, and it runs
from an empty directory because min_version enforcement precedes dispatch.
Confirmed discriminating: the check fails the old bundle and passes the new.
Also: config.py no longer quotes tomllib's message, which echoes document
text (the fix store.py already had, now shared rather than duplicated);
describe_value is pinned in both directions; AWS_SESSION_TOKEN and
AWS_PROFILE are scrubbed from the child, seeded in the fixture so the
assertion tests a removal rather than an absence; and isolated_git moved to
conftest, after a global core.hooksPath was found not just to break 65
tests but to make one of them write a hook into that global directory,
outside tmp_path, where it persisted after the run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
…evert
M1 A repo declaring `min_version = "9.9.9"` *and* a key this binary does
not know -- forward-compatibility, the case the floor exists for --
reported `unknown key 'future_key'` and ran anyway, because
`_build_config` rejects the unknown key before `min_version` is read
and `_min_version_refusal`'s broad guard then returns None. Every
command still exited 2 for its own reasons; `hooks install`, which
reads no policy, exited 0 and installed the gate. `config.read_min_version`
now reads the floor alone, past any key this binary cannot validate,
and the refusal uses it.
Not fixed by re-ordering `_build_config`, as the item proposed: that
function has neither `command` nor `installed`, so it cannot raise
`MinVersionError`, and comparing against `__version__` there would
break `doctor`, which is exempt. Doing both would leave two guards and
neither falsifiable -- the failure `_min_version_refusal`'s own
docstring warns against. One mechanism, so that reverting it is red.
The broad guard is unchanged: it is what keeps `doctor` working on the
broken config you run `doctor` to diagnose.
M2 `hooks install`'s missing-policy warning read the worktree while the
gate it installs reads the index, so a `.stackward.toml` that existed
but was unstaged got no warning and then blocked every commit. It now
asks the index, through the existing `_is_tracked`, and says
"not staged" for that state rather than "declares none", which is
visibly false to someone looking at the file. A git failure here stays
silent: this runs after the hook is written, inside a `@fail_closed`
command. One bounded gap (in the index, deleted from the worktree) is
recorded in the docstring rather than closed with a second copy of
`pre_commit._index_config_path`.
M3 A vacuous test from Task 9: its control raised `AssertionError` inside
`@fail_closed`, which swallows it into `return 2` -- the exact code the
test asserts. Replacing the early refusal with `except
CheckPassphraseError: passphrase = ""` left the file at 40 passed. The
control is now a private `BaseException`, as two other suites already
do, and that mutation fails it.
M4 `check_config.describe_yaml_error` still said `config.load_config`
"safely echoes tomllib's message in full because `.stackward.toml`
holds only path and key names" -- the false premise a later wave
corrected, left standing where it would authorise reverting the fix.
Rewritten to what both modules now do.
M5 `test_pre_commit.py`'s locale docstring called `nets.model.run_git`
locale-less and `install_hooks._is_tracked` a live bug. Both were fixed
afterwards and both are mutation-killed; it now says so and cites the
tests.
M6 The publish job smoke-tested `--version` and `doctor` -- the two
commands registered directly, and so the only two that kept passing
through the bundle regression that removed every lazily dispatched one.
Both jobs now run `scripts/check-bundle.sh` against their own binary,
one definition rather than two that can drift. Every `mktemp` in it
passes an explicit template, which GNU and BSD both accept: the matrix
includes a macOS runner and this workflow only fires on a tag, so a
bare `mktemp` would fail first during a release.
M7 `credentials set` seals every name in a piped `.env` while `exec`
injects three, and nothing said so. Sealing the extras is intended --
refusing them would break the documented `< .env` workflow and
contradict `_require_values` and `_prompt_for_values`, which both
already say an envelope may hold more -- so `show` now names them.
M8 The plan's Task 6 still required `config` to be "mode 0o644 ... no
credentials". The code deliberately changed both, because `backend_url`
is passed through verbatim and may carry userinfo. Recorded as a
revision with its reason.
897 -> 902 tests. Each new test was confirmed red against a mutation of the
source it covers; M6 is a workflow change and carries none, checked instead
by running the script against a stub that passes `--version` and `doctor`
and fails on `credentials list`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
CI caught what three local runs could not. pydantic refuses `typing.TypedDict` below 3.12 and demands the typing_extensions one, so on 3.11 the generated module failed to import and sync-declared-secrets refused for pydantic's reason rather than for the walkability reason this test exists to pin. The exit code is 2 either way. Only the assertion on the message told them apart -- had the test stopped at `== 2`, it would have passed on 3.11 while proving nothing, which is the failure mode this branch has been hunting all along. Verified on 3.11, 3.12 and 3.13: 902 passed on each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
.superpowers/ held the review ledger, eleven task reports and twenty-eight review diffs for this branch — prose about how the work was done, written for a private audience. On a public repository that is a worse exposure than a stray artefact: the neutrality gate greps for known terms and cannot catch a novel detail described in a sentence. Nothing was ever published; the only thing that prevented it was an ignore file the tooling drops inside its own directory. That file is untracked, so it protected this checkout and no other, and covered only the one subdirectory it sat in. A tracked rule protects every clone, and covers the sibling paths the nested one never did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14
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
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.
Implements the eleven-task plan in
docs/implementation-plan.md: the credential gate, the credential store and the commands around them. 42 commits, 902 tests.What ships
check-configpre-commithooks installcore.hooksPathcredentialsinit/set/list/show/rotateover one encrypted, profile-scoped storelogin/exec/shellpulumiwith those credentials injected into the childset-secretssync-declared-secretscheck-passphrasedoctorTwo nets: a key-name heuristic, and a model net carried across the interpreter boundary as a graph rather than a flat pattern list — walked alongside the data, so self- and mutual recursion work by construction. A flat pattern list fails open exactly where a recursive model needs it to fail closed.
Design commitments, each with a test
check-configandpre-commitread files. A subprocess test asserts that no provider, credential-store orcryptographymodule reachessys.modulesthroughcli.main, so a commit cannot be blocked by an expired session or a missing native extension..stackward.tomlsupplies every environment-specific fact; the tool ships no defaults about anyone's infrastructure.argv, so they appear in neitherpsnor shell history.Notes for review
Two whole-branch reviews ran after the eleven tasks, and both found real defects. The ones worth knowing about:
pre-commitread policy from the working tree while reading content from the index, so an unstagedallowed_referencesedit suppressed a staged credential — a relaxation that was not itself being committed. Policy now comes from the index too.importlib.import_module(f".commands.{name}"), which PyInstaller cannot resolve;stackward execdied onModuleNotFoundErrorwith exit code 1 — the code reserved for "a credential was found".--versionanddoctorare registered directly, which is why the release job's only two checks kept passing. Dispatch is now literal imports, andscripts/check-bundle.shruns two lazily dispatched commands against the built artifact in both the build and publish jobs. Verified discriminating against a bundle built the old way.credentialswas specified in the design and never built, so the encrypted store could be read but not populated. It exists now, which is what makes the store usable at all.tests/leakcheck.pyreplaces the whole-stringnot inidiom that letvalue[:8]and re-encoded forms through.The suite runs clean under a hostile global
core.hooksPathand withHOME,XDG_CONFIG_HOMEandGIT_CONFIG_GLOBALredirected.🤖 Generated with Claude Code
https://claude.ai/code/session_014GNW1Z8CmJgdKvBpgXfJ14