Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
e45b18f to
3accb60
Compare
michiosw
left a comment
There was a problem hiding this comment.
Internal review pass complete. The docs now match the shipped behavior: generated local .env.kontext, hosted connect only when needed, and next-start semantics for newly connected providers. No blocking documentation issues remain.
39e1f2a to
1469fdc
Compare
e7faf4e to
f66c534
Compare
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
⚠️ 2 issues in files not directly in the diff
⚠️ Quoted managed placeholders are misclassified as sync collisions (internal/credential/template_file.go:174)
LoadTemplateFile normalizes quoted placeholder values for parsing but stores the unnormalized raw value in ExistingValues (internal/credential/template_file.go:83-85). During sync, EnsureManagedTemplate compares that raw stored value directly to the canonical managed placeholder (internal/credential/template_file.go:172-175), so values like "{{kontext:github}}" are incorrectly treated as collisions instead of already-synced entries. This causes incorrect collision warnings and inaccurate sync behavior for valid quoted placeholders.
⚠️ Credential retry backoff ignores context cancellation (internal/run/run.go:355-357)
retryConnectableCredentials receives a context but uses unconditional time.Sleep for retry delays (internal/run/run.go:355-357). If the command context is canceled (for example by interrupt or timeout), the function still blocks through the backoff sleeps before returning, which delays shutdown and makes cancellation handling unresponsive.
View 6 additional findings in Devin Review.
97f1a87 to
4754121
Compare
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
⚠️ 2 issues in files not directly in the diff
⚠️ NormalizeEnvValue does not strip inline comments, injecting comment text into environment variables (internal/credential/credential.go:23-26)
The new buildEnv function at internal/run/run.go:733 injects literal (non-placeholder) values from the template file into the agent's environment using credential.NormalizeEnvValue. However, NormalizeEnvValue (internal/credential/credential.go:24-26) only strips surrounding quotes — it does not strip inline comments. In contrast, normalizePlaceholderValue (internal/credential/credential.go:13-20) correctly strips both inline comments and quotes before matching placeholders.
This means a literal value like DATABASE_URL=postgres://localhost:5432/mydb # local dev would be injected as DATABASE_URL=postgres://localhost:5432/mydb # local dev, with # local dev becoming part of the environment variable value. Similarly, a quoted value with a comment like MY_KEY="my value" # comment would not have the comment stripped either, since the outer quotes don't match (first ", last t).
⚠️ buf.gen.yaml proto source references a feature branch that may become unreachable (buf.gen.yaml:9-10)
The proto input in buf.gen.yaml was changed from branch: main to branch: feat/cli-bootstrap-proto. The ref field pins a specific commit SHA (8c96817...), but if the proto feature branch is squash-merged into main and then deleted, that commit SHA becomes unreachable (it won't be in main's history after a squash merge). This would cause buf generate to fail for any contributor trying to regenerate protobuf code. The CONTRIBUTING.md states "Keep generated code, docs, and release notes in sync with the code" — the proto source should point to main (or a tag) before this PR is merged.
View 11 additional findings in Devin Review.
4754121 to
f6e74ad
Compare
f6e74ad to
5d26787
Compare
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
⚠️ 2 issues in files not directly in the diff
⚠️ Typo in exported struct field: HasManagedPlaceholds should be HasManagedPlaceholders (internal/credential/template_file.go:28)
The TemplateFile struct at internal/credential/template_file.go:28 defines an exported field HasManagedPlaceholds which is missing the er suffix — it should be HasManagedPlaceholders. The field is set at internal/credential/template_file.go:98 but never read anywhere in the codebase. Since this is an exported field on a public struct, any future consumer trying to check HasManagedPlaceholders (the correct English spelling) will not find it, and the misspelled field will persist as a confusing API surface.
⚠️ NormalizeEnvValue godoc comment does not mention inline comment stripping (internal/credential/credential.go:22-23)
The NormalizeEnvValue function now delegates to normalizePlaceholderValue which strips inline comments (e.g., value # comment → value) in addition to trimming quotes. However, the godoc comment at internal/credential/credential.go:22-23 still only mentions quote trimming: "trims surrounding quotes from dotenv-style values so the launched process receives the literal token, not the quote characters." This violates the CONTRIBUTING.md rule "Keep generated code, docs, and release notes in sync with the code."
View 13 additional findings in Devin Review.
b03fd61 to
1a094b8
Compare
eae6490 to
ccd60be
Compare
1a094b8 to
0a52fc2
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
0a52fc2 to
a3aebad
Compare

Summary
Dependencies
Testing