Skip to content

fix: support native user config probe seeds - #22

Merged
rianjs merged 3 commits into
mainfrom
fix/83-native-user-config-seed
Jun 3, 2026
Merged

fix: support native user config probe seeds#22
rianjs merged 3 commits into
mainfrom
fix/83-native-user-config-seed

Conversation

@rianjs

@rianjs rianjs commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add seed_config.base: native_user_config to darwin-gate probe seeding
  • reject absolute and traversing seed paths before writing probe config
  • validate keychain probe seed base/path in identity-check

Verification

  • bash actions/darwin-gate/test_darwin_gate.sh
  • uv run --with pytest>=8,<9 --with PyYAML==6.0.2 --with defusedxml==0.7.1 pytest -q

Supports open-cli-collective/codereview-cli#83; codereview-cli cannot remove its traversal seed until this shared action is available through the v1 action ref.

@rianjs

rianjs commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Coverage is directionally good, but there are two gaps that keep it from fully proving the new behavior.

  • Major: actions/darwin-gate/test_darwin_gate.sh and actions/darwin-gate/test_darwin_gate.sh are still over-mocked. stub-good never inspects the seeded file at all, and stub-native-seed only checks that a file exists under HOME/Library/Application Support. That means these tests would still pass if probe() wrote the seed to the wrong location for the default xdg_config branch, or wrote the wrong/empty content. A small stub that reads the actual file contents would make the native-user-config behavior real instead of just syntactic.

  • Minor: The new absolute-path rejection is only covered in actions/identity-check/test_identity.py, not in the shell action itself. actions/darwin-gate/darwin-gate.sh has its own runtime guard, but there is no shell-level negative test for /tmp/..., so a regression there could slip through if the manifest validator is bypassed or if the action is reused independently.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR Review

Reviewed commit: b513596

Summary

Reviewer Findings
harness-engineering:harness-enforcement-reviewer 1
harness-engineering:harness-knowledge-reviewer 1
harness-engineering:harness-self-documenting-code-reviewer 2
harness-engineering:harness-enforcement-reviewer (1 findings)

💡 Suggestion - actions/darwin-gate/test_darwin_gate.sh:42

stub-good and stub-native-seed are over-mocked: stub-good never inspects seeded file contents, and stub-native-seed only checks file existence. If probe() wrote the seed to the wrong location or with wrong content, these tests would still pass. The test passes for the right path but does not explicitly assert file contents were written correctly to $HOME/Library/Application Support/x/config.yml.

harness-engineering:harness-knowledge-reviewer (1 findings)

💡 Suggestion - actions/darwin-gate/test_darwin_gate.sh:50

NSPEC for the native_user_config probe test omits env_unset: ["X_KEYRING_BACKEND"], which the existing SPEC includes. This means the test passes even if X_KEYRING_BACKEND is set in the CI environment and does not accurately model real auto-detection behavior.

harness-engineering:harness-self-documenting-code-reviewer (2 findings)

💡 Suggestion - actions/identity-check/identity.py:111

The empty-string guard path == "" does not reject whitespace-only paths such as " ". A caller supplying path: ' ' would pass this check, producing a PurePosixPath whose .parts is (' ',) — not absolute, no .. — and silently write a malformed seed path. Consider not path.strip() instead.

⚠️ Should Fix - actions/identity-check/identity.py:88

No pytest tests for _validate_keychain_probe are present in this diff. The new validation function — including the base allowlist, absolute-path rejection, and traversal rejection — has no visible unit-test coverage for its error paths, leaving the path-security logic untested.

1 PR discussion thread considered.


Completed in 2m 27s | $0.55 | sonnet | daemon 0.2.121 | Glorfindel
Field Value
Model sonnet
Reviewers hybrid-synthesis, database:database-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine claude · sonnet
Reviewed by pr-review-daemon · monit-pr-reviewer
Duration 2m 27s wall · 6m 03s compute (Reviewers: 1m 54s · Synthesis: 31s)
Cost $0.55
Tokens 98.1k in / 23.2k out
Turns 7

Per-workstream usage

Workstream Model In Out Cache read Cache create Cost
hybrid-synthesis sonnet 31.8k 1.8k 18.6k 13.1k (1h) $0.09
database:database-reviewer sonnet 9.5k 530 2.1k 7.4k (1h) $0.04
harness-engineering:harness-architecture-reviewer sonnet 12.2k 5.0k 2.1k 10.0k (1h) $0.12
harness-engineering:harness-enforcement-reviewer sonnet 12.2k 579 2.1k 10.0k (1h) $0.05
harness-engineering:harness-knowledge-reviewer sonnet 12.2k 6.1k 2.1k 10.0k (1h) $0.14
harness-engineering:harness-self-documenting-code-reviewer sonnet 9.6k 2.3k 2.1k 7.4k (1h) $0.07
security:security-code-auditor haiku 10.7k 6.8k 0 10.7k (1h) $0.05

Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.

Comment thread actions/identity-check/identity.py
Comment thread actions/identity-check/identity.py
Comment thread actions/darwin-gate/test_darwin_gate.sh
Comment thread actions/darwin-gate/test_darwin_gate.sh
@monit-reviewer
monit-reviewer dismissed their stale review June 3, 2026 01:30

Superseded by updated review

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR Review

Reviewed commit: 7532cbb | Previous: b513596 (incremental)

Summary

No issues found.

5 PR discussion threads considered.


Completed in 1m 01s | $0.69 | sonnet | daemon 0.2.121 | Glorfindel
Field Value
Model sonnet
Mode Re-review · Cycle 2 · Session resumed
Reviewers hybrid-synthesis, database:database-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine claude · sonnet
Reviewed by pr-review-daemon · monit-pr-reviewer
Duration 1m 01s wall · 1m 10s compute (Reviewers: 37s · Synthesis: 10s)
Cost $0.69
Tokens 190.2k in / 5.9k out
Turns 7

Per-workstream usage

Workstream Model In Out Cache read Cache create Cost
hybrid-synthesis sonnet 36.1k 430 18.6k 17.5k (1h) $0.08
database:database-reviewer sonnet 13.0k 55 2.8k 10.2k (1h) $0.04
harness-engineering:harness-architecture-reviewer sonnet 23.1k 57 2.8k 20.2k (1h) $0.08
harness-engineering:harness-enforcement-reviewer sonnet 18.6k 67 2.8k 15.8k (1h) $0.06
harness-engineering:harness-knowledge-reviewer sonnet 24.2k 65 2.8k 21.3k (1h) $0.08
harness-engineering:harness-self-documenting-code-reviewer sonnet 14.8k 245 2.8k 12.0k (1h) $0.05
security:security-code-auditor haiku 21.8k 4.5k 0 21.8k (1h) $0.05
discussion-summarizer 38.5k 470 0 38.5k (1h) $0.25

Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.

@rianjs
rianjs merged commit 2497046 into main Jun 3, 2026
17 checks passed
@rianjs
rianjs deleted the fix/83-native-user-config-seed branch June 3, 2026 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants